Correctly detect ambiguity between potential referrers when targeting a name+namespace reference

api/filters/nameref/nameref.go

@@ -118,7 +118,9 @@ func (f Filter) setMapping(node *yaml.RNode) error {
 		return err
 	}
 	oldName := nameNode.YNode().Value
-	referral, err := f.selectReferral(oldName, candidates)
+	// use allNamesAndNamespacesAreTheSame to compare referral candidates for functional identity,
+	// because we source both name and namespace values from the referral in this case.
+	referral, err := f.selectReferral(oldName, candidates, allNamesAndNamespacesAreTheSame)
 	if err != nil || referral == nil {
 		// Nil referral means nothing to do.
 		return err
@@ -167,8 +169,10 @@ func (f Filter) filterMapCandidatesByNamespace(
 }
 
 func (f Filter) setScalar(node *yaml.RNode) error {
+	// use allNamesAreTheSame to compare referral candidates for functional identity,
+	// because we only source the name from the referral in this case.
 	referral, err := f.selectReferral(
-		node.YNode().Value, f.ReferralCandidates.Resources())
+		node.YNode().Value, f.ReferralCandidates.Resources(), allNamesAreTheSame)
 	if err != nil || referral == nil {
 		// Nil referral means nothing to do.
 		return err
@@ -311,7 +315,9 @@ func (f Filter) sameCurrentNamespaceAsReferrer() sieveFunc {
 func (f Filter) selectReferral(
 	// The name referral that may need to be updated.
 	oldName string,
-	candidates []*resource.Resource) (*resource.Resource, error) {
+	candidates []*resource.Resource,
+	// function that returns whether two referrals are identical for the purposes of the transformation
+	candidatesIdentical func(resources []*resource.Resource) bool) (*resource.Resource, error) {
 	candidates = doSieve(candidates, previousNameMatches(oldName))
 	candidates = doSieve(candidates, previousIdSelectedByGvk(&f.ReferralTarget))
 	candidates = doSieve(candidates, f.roleRefFilter())
@@ -326,7 +332,7 @@ func (f Filter) selectReferral(
 	if len(candidates) == 0 {
 		return nil, nil
 	}
-	if allNamesAreTheSame(candidates) {
+	if candidatesIdentical(candidates) {
 		// Just take the first one.
 		return candidates[0], nil
 	}
@@ -356,6 +362,17 @@ func allNamesAreTheSame(resources []*resource.Resource) bool {
 	return true
 }
 
+func allNamesAndNamespacesAreTheSame(resources []*resource.Resource) bool {
+	name := resources[0].GetName()
+	namespace := resources[0].GetNamespace()
+	for i := 1; i < len(resources); i++ {
+		if name != resources[i].GetName() || namespace != resources[i].GetNamespace() {
+			return false
+		}
+	}
+	return true
+}
+
 func getIds(rs []*resource.Resource) string {
 	var result []string
 	for _, r := range rs {

api/internal/accumulator/namereferencetransformer_test.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig"
 	"sigs.k8s.io/kustomize/api/provider"
 	"sigs.k8s.io/kustomize/api/resmap"
@@ -524,6 +525,52 @@ func TestNameReferenceUnhappyRun(t *testing.T) {
 				`considering field 'rules/resourceNames' of object ClusterRole.v1.rbac.authorization.k8s.io/cr.[noNs]: visit traversal on ` +
 				`path: [resourceNames]: path config error; no 'name' field in node`,
 		},
+		{
+			// When targeting a map reference, we need to update both name and namespace, so multiple
+			// possible referral targets with different namespaces should not be considered identical.
+			// This test covers a bug where the difference in namespace was ignored and one candidate was chosen at random.
+			resMap: resmaptest_test.NewRmBuilderDefault(t).AddWithNsAndName(
+				"", orgname,
+				map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "ServiceAccount",
+					"metadata": map[string]interface{}{
+						"name":      orgname,
+						"namespace": ns1,
+					},
+				},
+			).AddWithNsAndName(
+				"", orgname,
+				map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "ServiceAccount",
+					"metadata": map[string]interface{}{
+						"name":      orgname,
+						"namespace": ns2,
+					},
+				},
+			).Add(
+				map[string]interface{}{
+					"apiVersion": "rbac.authorization.k8s.io/v1",
+					"kind":       "ClusterRoleBinding",
+					"metadata": map[string]interface{}{
+						"name": orgname,
+					},
+					"roleRef": map[string]interface{}{
+						"apiGroup": "rbac.authorization.k8s.io",
+						"kind":     "ClusterRole",
+						"name":     orgname,
+					},
+					"subjects": []interface{}{
+						map[string]interface{}{
+							"kind": "ServiceAccount",
+							"name": orgname,
+						},
+					},
+				},
+			).ResMap(),
+			expectedErr: "found multiple possible referrals: ServiceAccount.v1.[noGrp]/uniquename.ns1, ServiceAccount.v1.[noGrp]/uniquename.ns2",
+		},
 	}
 
 	nrt := newNameReferenceTransformer(builtinconfig.MakeDefaultConfig().NameReference)
@@ -1061,3 +1108,76 @@ func TestNameReferenceCandidateSelection(t *testing.T) {
 		t.Fatalf(notEqualErrFmt, err)
 	}
 }
+
+func TestNameReferenceCandidateDisambiguationByNamespace(t *testing.T) {
+	// The ClusterRole refers to both configmaps, since it is not namespace-specific.
+	// Since both names are updated consistently, the transformer should be able to
+	// silently update the ClusterRole as well.
+	// This test guards against a regression where allNamesAndNamespacesAreTheSame would be
+	// used to detect referral candidate identity instead of allNamesAreTheSame.
+	m := resmaptest_test.NewRmBuilderDefault(t).AddWithName(orgname,
+		map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]interface{}{
+				"name":      suffixedname,
+				"namespace": ns1,
+			},
+		},
+	).AddWithName(orgname,
+		map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]interface{}{
+				"name":      suffixedname,
+				"namespace": ns2,
+			},
+		},
+	).Add(
+		map[string]interface{}{
+			"apiVersion": "rbac.authorization.k8s.io/v1",
+			"kind":       "ClusterRole",
+			"metadata": map[string]interface{}{
+				"name": orgname,
+			},
+			"rules": []interface{}{
+				map[string]interface{}{
+					"resources": []interface{}{
+						"configmaps",
+					},
+					"resourceNames": []interface{}{
+						orgname,
+					},
+				},
+			},
+		},
+	).ResMap()
+
+	expected := resmaptest_test.NewSeededRmBuilderDefault(t, m.ShallowCopy()).
+		ReplaceResource(map[string]interface{}{
+			"apiVersion": "rbac.authorization.k8s.io/v1",
+			"kind":       "ClusterRole",
+			"metadata": map[string]interface{}{
+				"name": orgname,
+			},
+			"rules": []interface{}{
+				map[string]interface{}{
+					"resources": []interface{}{
+						"configmaps",
+					},
+					"resourceNames": []interface{}{
+						suffixedname,
+					},
+				},
+			},
+		},
+		).ResMap()
+
+	nrt := newNameReferenceTransformer(builtinconfig.MakeDefaultConfig().NameReference)
+	require.NoError(t, nrt.Transform(m))
+
+	m.RemoveBuildAnnotations()
+	if err := expected.ErrorIfNotEqualLists(m); err != nil {
+		t.Fatalf(notEqualErrFmt, err)
+	}
+}

api/krusty/namedspacedserviceaccounts_test.go

@@ -6,113 +6,10 @@ package krusty_test
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 )
 
-func TestNamedspacedServiceAccountsWithoutOverlap(t *testing.T) {
-	th := kusttest_test.MakeHarness(t)
-
-	th.WriteF("a/a.yaml", `
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: sa-a
----
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
-metadata:
-  name: crb-a
-roleRef:
-  apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: cr-a
-subjects:
-  - kind: ServiceAccount
-    name: sa-a
-`)
-
-	th.WriteK("a/", `
-namespace: a
-resources:
-- a.yaml
-`)
-
-	th.WriteF("b/b.yaml", `
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: sa-b
----
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
-metadata:
-  name: crb-b
-roleRef:
-  apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: cr-b
-subjects:
-  - kind: ServiceAccount
-    name: sa-b
-`)
-
-	th.WriteK("b/", `
-namespace: b
-resources:
-- b.yaml
-`)
-
-	th.WriteK(".", `
-resources:
-- a
-- b
-`)
-
-	m := th.Run(".", th.MakeDefaultOptions())
-
-	// Everything is as expected: each CRB gets updated to reference the SA in the appropriate namespace
-	// Changing order in the root kustomization.yaml does not change the result, as expected
-	th.AssertActualEqualsExpected(m, `
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: sa-a
-  namespace: a
----
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
-metadata:
-  name: crb-a
-roleRef:
-  apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: cr-a
-subjects:
-- kind: ServiceAccount
-  name: sa-a
-  namespace: a
----
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: sa-b
-  namespace: b
----
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
-metadata:
-  name: crb-b
-roleRef:
-  apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: cr-b
-subjects:
-- kind: ServiceAccount
-  name: sa-b
-  namespace: b
-`)
-}
-
 func TestNamedspacedServiceAccountsWithOverlap(t *testing.T) {
 	th := kusttest_test.MakeHarness(t)
 
@@ -172,47 +69,9 @@ resources:
 - b
 `)
 
-	m := th.Run(".", th.MakeDefaultOptions())
+	err := th.RunWithErr(".", th.MakeDefaultOptions())
-
+	assert.EqualError(t, err,
-	// Unexpected result: crb-b's subject obtains the wrong namespace, having "namespace: a"
+		"updating name reference in 'subjects' field of 'ClusterRoleBinding.v1.rbac.authorization.k8s.io/crb-a.[noNs]': "+
-	// If the order is swapped in the kustomization.yaml then it's crb-a that gets "namespace: b"
+			"considering field 'subjects' of object ClusterRoleBinding.v1.rbac.authorization.k8s.io/crb-a.[noNs]:  "+
-	th.AssertActualEqualsExpected(m, `
+			"found multiple possible referrals: ServiceAccount.v1.[noGrp]/sa.a, ServiceAccount.v1.[noGrp]/sa.b")
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: sa
-  namespace: a
----
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
-metadata:
-  name: crb-a
-roleRef:
-  apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: cr-a
-subjects:
-- kind: ServiceAccount
-  name: sa
-  namespace: a
----
-apiVersion: v1
-kind: ServiceAccount
-metadata:
-  name: sa
-  namespace: b
----
-apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
-metadata:
-  name: crb-b
-roleRef:
-  apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: cr-b
-subjects:
-- kind: ServiceAccount
-  name: sa
-  namespace: a
-`)
 }