From 7998ee703616c8569b05899e867055d314a4e7b7 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Wed, 26 Jun 2019 22:41:19 -0500 Subject: [PATCH 1/5] Addresses slice case with notNamespaceable objects --- pkg/transformers/namereference.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/transformers/namereference.go b/pkg/transformers/namereference.go index 3982423e44..f0f789c798 100644 --- a/pkg/transformers/namereference.go +++ b/pkg/transformers/namereference.go @@ -117,7 +117,7 @@ func (o *nameReferenceTransformer) getNewNameFunc( for _, res := range referralCandidates.Resources() { id := res.OrgId() if id.IsSelected(&target) && res.GetOriginalName() == oldName { - matches := referralCandidates.GetMatchingResourcesByOriginalId(id.GvknEquals) + matches := referralCandidates.GetMatchingResourcesByOriginalId(id.Equals) // If there's more than one match, there's no way // to know which one to pick, so emit error. if len(matches) > 1 { @@ -149,7 +149,7 @@ func (o *nameReferenceTransformer) getNewNameFunc( indexes := indexOf(res.GetOriginalName(), names) id := res.OrgId() if id.IsSelected(&target) && len(indexes) > 0 { - matches := referralCandidates.GetMatchingResourcesByOriginalId(id.GvknEquals) + matches := referralCandidates.GetMatchingResourcesByOriginalId(id.Equals) if len(matches) > 1 { return nil, fmt.Errorf( "slice case - multiple matches for %s:\n %v", From c4d899f7f3abd3b114f6fa7418d9afcfca4d59f8 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Mon, 8 Jul 2019 01:56:59 -0500 Subject: [PATCH 2/5] Improve NameReference Test cases - Add more NameReference Namespace tests - Address issue when mixing empty/no namespace and default namespace. - Address ClusterRoleBinding subjects field pointing at multiple namespaces. --- pkg/resmaptest/rmbuilder.go | 8 + pkg/resource/factory.go | 5 + pkg/resource/resource.go | 2 +- .../config/defaultconfig/namereference.go | 4 +- pkg/transformers/namereference.go | 138 +++++--- pkg/transformers/namereference_test.go | 307 +++++++++++++++++- 6 files changed, 417 insertions(+), 47 deletions(-) diff --git a/pkg/resmaptest/rmbuilder.go b/pkg/resmaptest/rmbuilder.go index 638267bc12..87ef5f2312 100644 --- a/pkg/resmaptest/rmbuilder.go +++ b/pkg/resmaptest/rmbuilder.go @@ -62,6 +62,14 @@ func (rm *rmBuilder) AddWithNs(ns string, m map[string]interface{}) *rmBuilder { return rm } +func (rm *rmBuilder) AddWithNsAndName(ns string, n string, m map[string]interface{}) *rmBuilder { + err := rm.m.Append(rm.rf.FromMapWithNamespaceAndName(ns, n, m)) + if err != nil { + rm.t.Fatalf("test setup failure: %v", err) + } + return rm +} + func (rm *rmBuilder) ReplaceResource(m map[string]interface{}) *rmBuilder { r := rm.rf.FromMap(m) _, err := rm.m.Replace(r) diff --git a/pkg/resource/factory.go b/pkg/resource/factory.go index eb8bbbba7f..a409f2dd8b 100644 --- a/pkg/resource/factory.go +++ b/pkg/resource/factory.go @@ -43,6 +43,11 @@ func (rf *Factory) FromMapWithNamespace(n string, m map[string]interface{}) *Res return rf.makeOne(rf.kf.FromMap(m), nil).setOriginalNs(n) } +// FromMapWithNamespaceAndName returns a new instance with the given "original" namespace. +func (rf *Factory) FromMapWithNamespaceAndName(ns string, n string, m map[string]interface{}) *Resource { + return rf.makeOne(rf.kf.FromMap(m), nil).setOriginalNs(ns).setOriginalName(n) +} + // FromMapAndOption returns a new instance of Resource with given options. func (rf *Factory) FromMapAndOption( m map[string]interface{}, args *types.GeneratorArgs, option *types.GeneratorOptions) *Resource { diff --git a/pkg/resource/resource.go b/pkg/resource/resource.go index 2d8c0a4d02..e7fb08d356 100644 --- a/pkg/resource/resource.go +++ b/pkg/resource/resource.go @@ -127,7 +127,7 @@ func (r *Resource) GetOutermostNameSuffix() string { } func (r *Resource) InSameFuzzyNamespace(o *Resource) bool { - return r.GetNamespace() == o.GetNamespace() && + return r.CurId().IsNsEquals(o.CurId()) && r.GetOutermostNamePrefix() == o.GetOutermostNamePrefix() && r.GetOutermostNameSuffix() == o.GetOutermostNameSuffix() } diff --git a/pkg/transformers/config/defaultconfig/namereference.go b/pkg/transformers/config/defaultconfig/namereference.go index a27c86d0fa..8ed0879ef2 100644 --- a/pkg/transformers/config/defaultconfig/namereference.go +++ b/pkg/transformers/config/defaultconfig/namereference.go @@ -299,10 +299,10 @@ nameReference: - kind: ServiceAccount version: v1 fieldSpecs: - - path: subjects/name + - path: subjects kind: RoleBinding group: rbac.authorization.k8s.io - - path: subjects/name + - path: subjects kind: ClusterRoleBinding group: rbac.authorization.k8s.io - path: spec/serviceAccountName diff --git a/pkg/transformers/namereference.go b/pkg/transformers/namereference.go index f0f789c798..aa30007fe8 100644 --- a/pkg/transformers/namereference.go +++ b/pkg/transformers/namereference.go @@ -106,6 +106,77 @@ func (o *nameReferenceTransformer) Transform(m resmap.ResMap) error { return 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) { + + for _, res := range referralCandidateSubset { + id := res.OrgId() + if id.IsSelected(&target) && res.GetOriginalName() == oldName { + matches := referralCandidates.GetMatchingResourcesByOriginalId(id.Equals) + // 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", + id, getIds(matches)) + } + // In the resource, note that it is referenced + // by the referrer. + res.AppendRefBy(referrer.CurId()) + // Return transformed name of the object, + // complete with prefixes, hashes, etc. + return res.GetName(), nil + } + } + + return oldName, nil +} + +// utility function to replace name field within a map[string]interface{} +// and leverage the namespace field. +func (o *nameReferenceTransformer) mapNameAndNsStruct( + inMap map[string]interface{}, + referrer *resource.Resource, + target gvk.Gvk, + referralCandidates resmap.ResMap) (interface{}, error) { + + // Example: + if _, ok := inMap["name"]; !ok { + return nil, fmt.Errorf( + "%#v is expected to contain a name field", inMap) + } + oldName, ok := inMap["name"].(string) + if !ok { + return nil, fmt.Errorf( + "%#v is expected to contain a name field of type string", oldName) + } + + subset := referralCandidates.Resources() + if namespacevalue, ok := inMap["namespace"]; ok { + namespace := namespacevalue.(string) + bynamespace := referralCandidates.GroupedByNamespace() + if _, ok := bynamespace[namespace]; !ok { + return inMap, nil + } + subset = bynamespace[namespace] + } + + newname, err := o.mapSimpleNameField(oldName, referrer, target, + referralCandidates, subset) + if err != nil { + return nil, err + } + + inMap["name"] = newname + return inMap, nil + +} + func (o *nameReferenceTransformer) getNewNameFunc( referrer *resource.Resource, target gvk.Gvk, @@ -114,52 +185,35 @@ func (o *nameReferenceTransformer) getNewNameFunc( switch in.(type) { case string: oldName, _ := in.(string) - for _, res := range referralCandidates.Resources() { - id := res.OrgId() - if id.IsSelected(&target) && res.GetOriginalName() == oldName { - matches := referralCandidates.GetMatchingResourcesByOriginalId(id.Equals) - // 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", - id, getIds(matches)) - } - // In the resource, note that it is referenced - // by the referrer. - res.AppendRefBy(referrer.CurId()) - // Return transformed name of the object, - // complete with prefixes, hashes, etc. - return res.GetName(), nil - } - } - return in, nil + return o.mapSimpleNameField(oldName, referrer, target, + referralCandidates, referralCandidates.Resources()) case []interface{}: l, _ := in.([]interface{}) - var names []string - for _, item := range l { - name, ok := item.(string) - if !ok { - return nil, fmt.Errorf( - "%#v is expected to be %T", item, name) - } - names = append(names, name) - } - for _, res := range referralCandidates.Resources() { - indexes := indexOf(res.GetOriginalName(), names) - id := res.OrgId() - if id.IsSelected(&target) && len(indexes) > 0 { - matches := referralCandidates.GetMatchingResourcesByOriginalId(id.Equals) - if len(matches) > 1 { - return nil, fmt.Errorf( - "slice case - multiple matches for %s:\n %v", - id, getIds(matches)) + for idx, item := range l { + switch item.(type) { + case string: + // Kind: Role/ClusterRole + // FieldSpec is rules.resourceNames + oldName, _ := item.(string) + newName, err := o.mapSimpleNameField(oldName, referrer, target, + referralCandidates, referralCandidates.Resources()) + if err != nil { + return nil, err } - for _, index := range indexes { - l[index] = res.GetName() + l[idx] = newName + case map[string]interface{}: + // Kind: RoleBinding/ClusterRoleBinding + // FieldSpec is subjects + oldMap, _ := item.(map[string]interface{}) + newMap, err := o.mapNameAndNsStruct(oldMap, referrer, target, + referralCandidates) + if err != nil { + return nil, err } - res.AppendRefBy(referrer.CurId()) - return l, nil + l[idx] = newMap + default: + return nil, fmt.Errorf( + "%#v is expected to be either a []string or a []map[string]interface{}", in) } } return in, nil diff --git a/pkg/transformers/namereference_test.go b/pkg/transformers/namereference_test.go index 9fc8e4daaf..3f3464978d 100644 --- a/pkg/transformers/namereference_test.go +++ b/pkg/transformers/namereference_test.go @@ -8,6 +8,8 @@ import ( "testing" "sigs.k8s.io/kustomize/v3/k8sdeps/kunstruct" + "sigs.k8s.io/kustomize/v3/pkg/gvk" + "sigs.k8s.io/kustomize/v3/pkg/resid" "sigs.k8s.io/kustomize/v3/pkg/resmap" "sigs.k8s.io/kustomize/v3/pkg/resmaptest" "sigs.k8s.io/kustomize/v3/pkg/resource" @@ -466,6 +468,7 @@ func TestNameReferenceHappyRun(t *testing.T) { 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) } @@ -497,7 +500,7 @@ func TestNameReferenceUnhappyRun(t *testing.T) { }, }, }).ResMap(), - expectedErr: "is expected to be string"}, + expectedErr: "is expected to be"}, { resMap: resmaptest_test.NewRmBuilder(t, rf).Add( map[string]interface{}{ @@ -517,7 +520,7 @@ func TestNameReferenceUnhappyRun(t *testing.T) { }, }, }).ResMap(), - expectedErr: "is expected to be either a string or a []interface{}"}, + expectedErr: "is expected to be"}, } nrt := NewNameReferenceTransformer(defaultTransformerConfig.NameReference) @@ -590,3 +593,303 @@ func TestNameReferencePersistentVolumeHappyRun(t *testing.T) { t.Fatalf("actual doesn't match expected: %v", err) } } + +// utility map to create a deployment object +// with (metadatanamespace, metadataname) as key +// and pointing to "refname" secret and configmap +func deploymentMap(metadatanamespace string, metadataname string, + configmapref string, secretref string) map[string]interface{} { + deployment := map[string]interface{}{ + "group": "apps", + "apiVersion": "v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": metadataname, + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "nginx", + "image": "nginx:1.7.9", + "env": []interface{}{ + map[string]interface{}{ + "name": "CM_FOO", + "valueFrom": map[string]interface{}{ + "configMapKeyRef": map[string]interface{}{ + "name": configmapref, + "key": "somekey", + }, + }, + }, + map[string]interface{}{ + "name": "SECRET_FOO", + "valueFrom": map[string]interface{}{ + "secretKeyRef": map[string]interface{}{ + "name": secretref, + "key": "somekey", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + if metadatanamespace != "" { + metadata := deployment["metadata"].(map[string]interface{}) + metadata["namespace"] = metadatanamespace + } + 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" + + orgname := "uniquename" + prefixedname := "prefix-uniquename" + suffixedname := "uniquename-suffix" + modifiedname := "modifiedname" + + rf := resource.NewFactory( + kunstruct.NewKunstructuredFactoryImpl()) + m := resmaptest_test.NewRmBuilder(t, rf). + // Add ConfigMap with the same org name in noNs, "ns1" and "ns2" namespaces + AddWithName(orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": modifiedname, + }}). + AddWithNsAndName(ns1, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": prefixedname, + "namespace": ns1, + }}). + AddWithNsAndName(ns2, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": suffixedname, + "namespace": ns2, + }}). + // Add Secret with the same org name in noNs, "ns1" and "ns2" namespaces + AddWithNsAndName(defaultNs, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": modifiedname, + "namespace": defaultNs, + }}). + AddWithNsAndName(ns1, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": prefixedname, + "namespace": ns1, + }}). + AddWithNsAndName(ns2, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": suffixedname, + "namespace": ns2, + }}). + // 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)). + // Add ServiceAccount with the same org name in noNs, "ns1" and "ns2" namespaces + AddWithName(orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]interface{}{ + "name": modifiedname, + }}). + AddWithNsAndName(ns1, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]interface{}{ + "name": prefixedname, + "namespace": ns1, + }}). + AddWithNsAndName(ns2, orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]interface{}{ + "name": suffixedname, + "namespace": ns2, + }}). + // Add a PersisitentVolume to have a clusterwide resources + AddWithName(orgname, map[string]interface{}{ + "apiVersion": "v1", + "kind": "PersistentVolume", + "metadata": map[string]interface{}{ + "name": modifiedname, + }}). + AddWithName(orgname, map[string]interface{}{ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "ClusterRole", + "metadata": map[string]interface{}{ + "name": modifiedname, + }, + "rules": []interface{}{ + map[string]interface{}{ + "resources": []interface{}{ + "persistentvolumes", + }, + "resourceNames": []interface{}{ + orgname, + }, + }, + }}). + 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": defaultNs, + }, + map[string]interface{}{ + "kind": "ServiceAccount", + "name": orgname, + "namespace": ns1, + }, + map[string]interface{}{ + "kind": "ServiceAccount", + "name": orgname, + "namespace": ns2, + }, + }}).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", + "kind": "ClusterRole", + "metadata": map[string]interface{}{ + "name": modifiedname, + }, + "rules": []interface{}{ + map[string]interface{}{ + "resources": []interface{}{ + "persistentvolumes", + }, + "resourceNames": []interface{}{ + modifiedname, + }, + }, + }}). + 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": modifiedname, + "namespace": defaultNs, + }, + 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. +func TestNameReferenceCandidateSelection(t *testing.T) { + rf := resource.NewFactory( + kunstruct.NewKunstructuredFactoryImpl()) + m := resmaptest_test.NewRmBuilder(t, rf). + AddWithName("cm1", map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "p1-cm1-hash", + }}). + AddWithNsAndName("default", "secret1", map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "p1-secret1-hash", + "namespace": "default", + }}). + AddWithName("deploy1", deploymentMap("", "p1-deploy1", "cm1", "secret1")). + ResMap() + + expected := resmaptest_test.NewSeededRmBuilder(t, rf, m.ShallowCopy()). + ReplaceResource(deploymentMap("", "p1-deploy1", "p1-cm1-hash", "p1-secret1-hash")). + 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) + } +} From b43bd5440d6384a8d26cd92155bc509006124bd8 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Thu, 27 Jun 2019 14:43:20 -0500 Subject: [PATCH 3/5] Update Issue 1264 Reproduction Test --- pkg/target/namespaces_test.go | 48 +++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/pkg/target/namespaces_test.go b/pkg/target/namespaces_test.go index 084221947b..0ce86eea1d 100644 --- a/pkg/target/namespaces_test.go +++ b/pkg/target/namespaces_test.go @@ -5,7 +5,6 @@ package target_test import ( "sigs.k8s.io/kustomize/v3/pkg/kusttest" - "strings" "testing" ) @@ -51,19 +50,46 @@ resources: - role.yaml `) - _, err := th.MakeKustTarget().MakeCustomizedResMap() - // TODO: Fix #1044 - // This should not be an error - + m, err := th.MakeKustTarget().MakeCustomizedResMap() + // This validates Fix #1444. This should not be an error anymore - // the secrets have the same name but are in different namespaces. // The ClusterRole (by def) is not in a namespace, // an in this case applies to *any* Secret resource // named "dummy" - if err == nil { - t.Fatalf("unexpected lack of error") - } - if !strings.Contains( - err.Error(), - "slice case - multiple matches for ~G_v1_Secret|default|dummy") { - t.Fatalf("unexpected error: %s", err) + if err != nil { + t.Fatalf("Err: %v", err) } + th.AssertActualEqualsExpected(m, ` +apiVersion: v1 +data: + dummy: "" +kind: Secret +metadata: + name: dummy + namespace: default +type: Opaque +--- +apiVersion: v1 +data: + dummy: "" +kind: Secret +metadata: + name: dummy + namespace: kube-system +type: Opaque +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: dummy +rules: +- apiGroups: + - "" + resourceNames: + - dummy + resources: + - secrets + verbs: + - get +`) } From 579995dc8a1e3e1df267f5c57f9d54377200f6c2 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sun, 14 Jul 2019 11:35:26 -0500 Subject: [PATCH 4/5] 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 | 162 ++++++++++++++++-- 5 files changed, 207 insertions(+), 33 deletions(-) diff --git a/pkg/commands/build/build.go b/pkg/commands/build/build.go index 66938bcc50..1fd5d75b6a 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 c2aab9a2f9..41cc299344 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 8ed0879ef2..1b4c8d1eb3 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 aa30007fe8..a3292e6505 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 3f3464978d..7ae02cc4f3 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" - - orgname := "uniquename" - prefixedname := "prefix-uniquename" - suffixedname := "uniquename-suffix" - modifiedname := "modifiedname" +const ( + defaultNs = "default" + ns1 = "ns1" + ns2 = "ns2" + ns3 = "ns3" + + 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. From 9b40f8ab474007a4f2358586bc82460b4bcac577 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Wed, 17 Jul 2019 14:10:01 -0500 Subject: [PATCH 5/5] Implement code review comments to NameReferenceTransformer changes. - Add comments where code with potentially misleading. - Rename functions according to comments --- pkg/transformers/namereference.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/pkg/transformers/namereference.go b/pkg/transformers/namereference.go index a3292e6505..7aaf00b51e 100644 --- a/pkg/transformers/namereference.go +++ b/pkg/transformers/namereference.go @@ -106,7 +106,12 @@ func (o *nameReferenceTransformer) Transform(m resmap.ResMap) error { return nil } -// utility function to replace a simple string by the new name +// selectReferral picks the referral among a subset of candidates. +// It returns the current name and namespace of the selected candidate. +// Note that the content of the referricalCandidateSubset slice is most of the time +// identical to the referralCandidates resmap. Still in some cases, such +// as ClusterRoleBinding, the subset only contains the resources of a specific +// namespace. func (o *nameReferenceTransformer) selectReferral( oldName string, referrer *resource.Resource, @@ -138,7 +143,7 @@ func (o *nameReferenceTransformer) selectReferral( } // utility function to replace a simple string by the new name -func (o *nameReferenceTransformer) mapSimpleNameField( +func (o *nameReferenceTransformer) getSimpleNameField( oldName string, referrer *resource.Resource, target gvk.Gvk, @@ -153,7 +158,7 @@ func (o *nameReferenceTransformer) mapSimpleNameField( // utility function to replace name field within a map[string]interface{} // and leverage the namespace field. -func (o *nameReferenceTransformer) mapNameAndNsStruct( +func (o *nameReferenceTransformer) getNameAndNsStruct( inMap map[string]interface{}, referrer *resource.Resource, target gvk.Gvk, @@ -205,7 +210,7 @@ func (o *nameReferenceTransformer) getNewNameFunc( switch in.(type) { case string: oldName, _ := in.(string) - return o.mapSimpleNameField(oldName, referrer, target, + return o.getSimpleNameField(oldName, referrer, target, referralCandidates, referralCandidates.Resources()) case []interface{}: l, _ := in.([]interface{}) @@ -215,7 +220,7 @@ func (o *nameReferenceTransformer) getNewNameFunc( // Kind: Role/ClusterRole // FieldSpec is rules.resourceNames oldName, _ := item.(string) - newName, err := o.mapSimpleNameField(oldName, referrer, target, + newName, err := o.getSimpleNameField(oldName, referrer, target, referralCandidates, referralCandidates.Resources()) if err != nil { return nil, err @@ -224,8 +229,13 @@ func (o *nameReferenceTransformer) getNewNameFunc( case map[string]interface{}: // Kind: RoleBinding/ClusterRoleBinding // FieldSpec is subjects + // Note: The corresponding fieldSpec had been changed from + // from path: subjects/name to just path: subjects. This is + // what get mutatefield to request the mapping of the whole + // map containing namespace and name instead of just a simple + // string field containing the name oldMap, _ := item.(map[string]interface{}) - newMap, err := o.mapNameAndNsStruct(oldMap, referrer, target, + newMap, err := o.getNameAndNsStruct(oldMap, referrer, target, referralCandidates) if err != nil { return nil, err