From 579995dc8a1e3e1df267f5c57f9d54377200f6c2 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sun, 14 Jul 2019 11:35:26 -0500 Subject: [PATCH] Address simultaneous transformation of name and namespace Namereference handler needs to address simulatenous change of name and namespace in ClusterRoleBinding for instance. --- pkg/commands/build/build.go | 2 +- pkg/resmap/resmap.go | 38 ++++- .../config/defaultconfig/namereference.go | 2 + pkg/transformers/namereference.go | 36 +++- pkg/transformers/namereference_test.go | 160 ++++++++++++++++-- 5 files changed, 206 insertions(+), 32 deletions(-) diff --git a/pkg/commands/build/build.go b/pkg/commands/build/build.go index 66938bcc5..1fd5d75b6 100644 --- a/pkg/commands/build/build.go +++ b/pkg/commands/build/build.go @@ -201,7 +201,7 @@ func NewCmdBuildPrune( func writeIndividualFiles( fSys fs.FileSystem, folderPath string, m resmap.ResMap) error { - byNamespace := m.GroupedByNamespace() + byNamespace := m.GroupedByCurrentNamespace() for namespace, resList := range byNamespace { for _, res := range resList { fName := fileName(res) diff --git a/pkg/resmap/resmap.go b/pkg/resmap/resmap.go index c2aab9a2f..41cc29934 100644 --- a/pkg/resmap/resmap.go +++ b/pkg/resmap/resmap.go @@ -112,13 +112,18 @@ type ResMap interface { // match. GetById(resid.ResId) (*resource.Resource, error) - // GroupedByNamespace returns a map of namespace + // GroupedByCurrentNamespace returns a map of namespace // to a slice of *Resource in that namespace. // Resources for whom IsNamespaceableKind is false are // are not included at all (see NonNamespaceable). // Resources with an empty namespace are placed // in the resid.DefaultNamespace entry. - GroupedByNamespace() map[string][]*resource.Resource + GroupedByCurrentNamespace() map[string][]*resource.Resource + + // GroupByOrginalNamespace performs as GroupByNamespace + // but use the original namespace instead of the current + // one to perform the grouping. + GroupedByOriginalNamespace() map[string][]*resource.Resource // NonNamespaceable returns a slice of resources that // cannot be placed in a namespace, e.g. @@ -390,19 +395,19 @@ func demandOneMatch( return nil, fmt.Errorf("no matches for %sId %s", s, id) } -// GroupedByNamespace implements ResMap.GroupByNamespace -func (m *resWrangler) GroupedByNamespace() map[string][]*resource.Resource { - items := m.groupedByNamespace() +// GroupedByCurrentNamespace implements ResMap.GroupByCurrentNamespace +func (m *resWrangler) GroupedByCurrentNamespace() map[string][]*resource.Resource { + items := m.groupedByCurrentNamespace() delete(items, resid.TotallyNotANamespace) return items } // NonNamespaceable implements ResMap.NonNamespaceable func (m *resWrangler) NonNamespaceable() []*resource.Resource { - return m.groupedByNamespace()[resid.TotallyNotANamespace] + return m.groupedByCurrentNamespace()[resid.TotallyNotANamespace] } -func (m *resWrangler) groupedByNamespace() map[string][]*resource.Resource { +func (m *resWrangler) groupedByCurrentNamespace() map[string][]*resource.Resource { byNamespace := make(map[string][]*resource.Resource) for _, res := range m.rList { namespace := res.CurId().EffectiveNamespace() @@ -414,6 +419,25 @@ func (m *resWrangler) groupedByNamespace() map[string][]*resource.Resource { return byNamespace } +// GroupedByNamespace implements ResMap.GroupByOrginalNamespace +func (m *resWrangler) GroupedByOriginalNamespace() map[string][]*resource.Resource { + items := m.groupedByOriginalNamespace() + delete(items, resid.TotallyNotANamespace) + return items +} + +func (m *resWrangler) groupedByOriginalNamespace() map[string][]*resource.Resource { + byNamespace := make(map[string][]*resource.Resource) + for _, res := range m.rList { + namespace := res.OrgId().EffectiveNamespace() + if _, found := byNamespace[namespace]; !found { + byNamespace[namespace] = []*resource.Resource{} + } + byNamespace[namespace] = append(byNamespace[namespace], res) + } + return byNamespace +} + // AsYaml implements ResMap. func (m *resWrangler) AsYaml() ([]byte, error) { firstObj := true diff --git a/pkg/transformers/config/defaultconfig/namereference.go b/pkg/transformers/config/defaultconfig/namereference.go index 8ed0879ef..1b4c8d1eb 100644 --- a/pkg/transformers/config/defaultconfig/namereference.go +++ b/pkg/transformers/config/defaultconfig/namereference.go @@ -343,6 +343,8 @@ nameReference: fieldSpecs: - path: spec/volumeName kind: PersistentVolumeClaim + - path: rules/resourceNames + kind: ClusterRole - kind: StorageClass version: v1 diff --git a/pkg/transformers/namereference.go b/pkg/transformers/namereference.go index aa30007fe..a3292e650 100644 --- a/pkg/transformers/namereference.go +++ b/pkg/transformers/namereference.go @@ -107,12 +107,12 @@ func (o *nameReferenceTransformer) Transform(m resmap.ResMap) error { } // utility function to replace a simple string by the new name -func (o *nameReferenceTransformer) mapSimpleNameField( +func (o *nameReferenceTransformer) selectReferral( oldName string, referrer *resource.Resource, target gvk.Gvk, referralCandidates resmap.ResMap, - referralCandidateSubset []*resource.Resource) (interface{}, error) { + referralCandidateSubset []*resource.Resource) (interface{}, interface{}, error) { for _, res := range referralCandidateSubset { id := res.OrgId() @@ -121,8 +121,8 @@ func (o *nameReferenceTransformer) mapSimpleNameField( // If there's more than one match, there's no way // to know which one to pick, so emit error. if len(matches) > 1 { - return nil, fmt.Errorf( - "string case - multiple matches for %s:\n %v", + return nil, nil, fmt.Errorf( + "multiple matches for %s:\n %v", id, getIds(matches)) } // In the resource, note that it is referenced @@ -130,11 +130,25 @@ func (o *nameReferenceTransformer) mapSimpleNameField( res.AppendRefBy(referrer.CurId()) // Return transformed name of the object, // complete with prefixes, hashes, etc. - return res.GetName(), nil + return res.GetName(), res.GetNamespace(), nil } } - return oldName, nil + return oldName, nil, nil +} + +// utility function to replace a simple string by the new name +func (o *nameReferenceTransformer) mapSimpleNameField( + oldName string, + referrer *resource.Resource, + target gvk.Gvk, + referralCandidates resmap.ResMap, + referralCandidateSubset []*resource.Resource) (interface{}, error) { + + newName, _, err := o.selectReferral(oldName, referrer, target, + referralCandidates, referralCandidateSubset) + + return newName, err } // utility function to replace name field within a map[string]interface{} @@ -159,20 +173,26 @@ func (o *nameReferenceTransformer) mapNameAndNsStruct( subset := referralCandidates.Resources() if namespacevalue, ok := inMap["namespace"]; ok { namespace := namespacevalue.(string) - bynamespace := referralCandidates.GroupedByNamespace() + bynamespace := referralCandidates.GroupedByOriginalNamespace() if _, ok := bynamespace[namespace]; !ok { return inMap, nil } subset = bynamespace[namespace] } - newname, err := o.mapSimpleNameField(oldName, referrer, target, + newname, newnamespace, err := o.selectReferral(oldName, referrer, target, referralCandidates, subset) if err != nil { return nil, err } inMap["name"] = newname + if newnamespace != "" { + // We don't want value "" to replace value "default" since + // the empty string is handled as a wild card here not default namespace + // by kubernetes. + inMap["namespace"] = newnamespace + } return inMap, nil } diff --git a/pkg/transformers/namereference_test.go b/pkg/transformers/namereference_test.go index 3f3464978..7ae02cc4f 100644 --- a/pkg/transformers/namereference_test.go +++ b/pkg/transformers/namereference_test.go @@ -647,19 +647,22 @@ func deploymentMap(metadatanamespace string, metadataname string, return deployment } -// TestNameReferenceNamespace creates configmap, secret, deployment -// serviceAccount and clusterRoleBinding object with the same original -// names (uniquename) in different namespaces and with different current Id. -func TestNameReferenceyNamespace(t *testing.T) { - defaultNs := "default" - ns1 := "ns1" - ns2 := "ns2" +const ( + defaultNs = "default" + ns1 = "ns1" + ns2 = "ns2" + ns3 = "ns3" - orgname := "uniquename" - prefixedname := "prefix-uniquename" - suffixedname := "uniquename-suffix" - modifiedname := "modifiedname" + orgname = "uniquename" + prefixedname = "prefix-uniquename" + suffixedname = "uniquename-suffix" + modifiedname = "modifiedname" +) +// TestNameReferenceNamespace creates serviceAccount and clusterRoleBinding +// object with the same original names (uniquename) in different namespaces +// and with different current Id. +func TestNameReferenceNamespace(t *testing.T) { rf := resource.NewFactory( kunstruct.NewKunstructuredFactoryImpl()) m := resmaptest_test.NewRmBuilder(t, rf). @@ -709,7 +712,31 @@ func TestNameReferenceyNamespace(t *testing.T) { // Add Deployment with the same org name in noNs, "ns1" and "ns2" namespaces AddWithNsAndName(defaultNs, orgname, deploymentMap(defaultNs, modifiedname, modifiedname, modifiedname)). AddWithNsAndName(ns1, orgname, deploymentMap(ns1, prefixedname, orgname, orgname)). - AddWithNsAndName(ns2, orgname, deploymentMap(ns2, suffixedname, orgname, orgname)). + AddWithNsAndName(ns2, orgname, deploymentMap(ns2, suffixedname, orgname, orgname)).ResMap() + + expected := resmaptest_test.NewSeededRmBuilder(t, rf, m.ShallowCopy()). + ReplaceResource(deploymentMap(defaultNs, modifiedname, modifiedname, modifiedname)). + ReplaceResource(deploymentMap(ns1, prefixedname, prefixedname, prefixedname)). + ReplaceResource(deploymentMap(ns2, suffixedname, suffixedname, suffixedname)).ResMap() + + nrt := NewNameReferenceTransformer(defaultTransformerConfig.NameReference) + err := nrt.Transform(m) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if err = expected.ErrorIfNotEqualLists(m); err != nil { + t.Fatalf("actual doesn't match expected: %v", err) + } +} + +// TestNameReferenceNamespace creates serviceAccount and clusterRoleBinding +// object with the same original names (uniquename) in different namespaces +// and with different current Id. +func TestNameReferenceClusterWide(t *testing.T) { + rf := resource.NewFactory( + kunstruct.NewKunstructuredFactoryImpl()) + m := resmaptest_test.NewRmBuilder(t, rf). // Add ServiceAccount with the same org name in noNs, "ns1" and "ns2" namespaces AddWithName(orgname, map[string]interface{}{ "apiVersion": "v1", @@ -731,7 +758,7 @@ func TestNameReferenceyNamespace(t *testing.T) { "name": suffixedname, "namespace": ns2, }}). - // Add a PersisitentVolume to have a clusterwide resources + // Add a PersistentVolume to have a clusterwide resource AddWithName(orgname, map[string]interface{}{ "apiVersion": "v1", "kind": "PersistentVolume", @@ -784,9 +811,6 @@ func TestNameReferenceyNamespace(t *testing.T) { }}).ResMap() expected := resmaptest_test.NewSeededRmBuilder(t, rf, m.ShallowCopy()). - ReplaceResource(deploymentMap(defaultNs, modifiedname, modifiedname, modifiedname)). - ReplaceResource(deploymentMap(ns1, prefixedname, prefixedname, prefixedname)). - ReplaceResource(deploymentMap(ns2, suffixedname, suffixedname, suffixedname)). ReplaceResource( map[string]interface{}{ "apiVersion": "rbac.authorization.k8s.io/v1", @@ -794,6 +818,9 @@ func TestNameReferenceyNamespace(t *testing.T) { "metadata": map[string]interface{}{ "name": modifiedname, }, + // Behavior of the transformer is still imperfect + // It should use the (resources,apigroup,resourceNames) as + // combination to select the candidates. "rules": []interface{}{ map[string]interface{}{ "resources": []interface{}{ @@ -856,6 +883,107 @@ func TestNameReferenceyNamespace(t *testing.T) { } } +// TestNameReferenceNamespaceTransformation creates serviceAccount and clusterRoleBinding +// object with the same original names (uniquename) in different namespaces +// and with different current Id. +func TestNameReferenceNamespaceTransformation(t *testing.T) { + rf := resource.NewFactory( + kunstruct.NewKunstructuredFactoryImpl()) + m := resmaptest_test.NewRmBuilder(t, rf). + // Add ServiceAccount with the same org name in "ns1" namespaces + AddWithNsAndName(ns1, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]interface{}{ + "name": prefixedname, + "namespace": ns1, + }}). + // Simulate NamespaceTransformer effect (ns3 transformed in ns2) + AddWithNsAndName(ns3, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]interface{}{ + "name": suffixedname, + "namespace": ns2, + }}). + AddWithName(orgname, map[string]interface{}{ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "ClusterRole", + "metadata": map[string]interface{}{ + "name": modifiedname, + }}). + AddWithName(orgname, map[string]interface{}{ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "ClusterRoleBinding", + "metadata": map[string]interface{}{ + "name": modifiedname, + }, + "roleRef": map[string]interface{}{ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "ClusterRole", + "name": orgname, + }, + "subjects": []interface{}{ + map[string]interface{}{ + "kind": "ServiceAccount", + "name": orgname, + "namespace": ns1, + }, + map[string]interface{}{ + "kind": "ServiceAccount", + "name": orgname, + "namespace": ns3, + }, + }}).ResMap() + + expected := resmaptest_test.NewSeededRmBuilder(t, rf, m.ShallowCopy()). + ReplaceResource( + map[string]interface{}{ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "ClusterRoleBinding", + "metadata": map[string]interface{}{ + "name": modifiedname, + }, + "roleRef": map[string]interface{}{ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "ClusterRole", + "name": modifiedname, + }, + // The following tests required a change in + // getNameFunc implementation in order to leverage + // the namespace field. + "subjects": []interface{}{ + map[string]interface{}{ + "kind": "ServiceAccount", + "name": prefixedname, + "namespace": ns1, + }, + map[string]interface{}{ + "kind": "ServiceAccount", + "name": suffixedname, + "namespace": ns2, + }, + }, + }).ResMap() + + clusterRoleId := resid.NewResId( + gvk.Gvk{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRole"}, modifiedname) + clusterRoleBindingId := resid.NewResId( + gvk.Gvk{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRoleBinding"}, modifiedname) + clusterRole, _ := expected.GetByCurrentId(clusterRoleId) + clusterRole.AppendRefBy(clusterRoleBindingId) + + nrt := NewNameReferenceTransformer(defaultTransformerConfig.NameReference) + err := nrt.Transform(m) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if err = expected.ErrorIfNotEqualLists(m); err != nil { + t.Fatalf("actual doesn't match expected: %v", err) + } +} + // TestNameReferenceNamespace creates configmap, secret, deployment // It validates the change done is IsSameFuzzyNamespace which // uses the IsNsEquals method instead of the simple == operator.