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

@@ -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)

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

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

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,20 +121,34 @@ 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
 			// by the referrer.
 			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
 
 }

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,16 +811,16 @@ 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",
 				"kind":       "ClusterRole",
 				"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.