From c4d899f7f3abd3b114f6fa7418d9afcfca4d59f8 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Mon, 8 Jul 2019 01:56:59 -0500 Subject: [PATCH] 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 | 140 +++++--- pkg/transformers/namereference_test.go | 307 +++++++++++++++++- 6 files changed, 418 insertions(+), 48 deletions(-) diff --git a/pkg/resmaptest/rmbuilder.go b/pkg/resmaptest/rmbuilder.go index 638267bc1..87ef5f231 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 eb8bbbba7..a409f2dd8 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 2d8c0a4d0..e7fb08d35 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 a27c86d0f..8ed0879ef 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 f0f789c79..aa30007fe 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 { + 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 + } + 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 + } + l[idx] = newMap + default: 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 _, index := range indexes { - l[index] = res.GetName() - } - res.AppendRefBy(referrer.CurId()) - return l, nil + "%#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 9fc8e4daa..3f3464978 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) + } +}