diff --git a/api/filters/nameref/nameref.go b/api/filters/nameref/nameref.go index 121b243ec..860ff0973 100644 --- a/api/filters/nameref/nameref.go +++ b/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 { diff --git a/api/internal/accumulator/namereferencetransformer_test.go b/api/internal/accumulator/namereferencetransformer_test.go index 49169db91..c964c5f9e 100644 --- a/api/internal/accumulator/namereferencetransformer_test.go +++ b/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) + } +} diff --git a/api/krusty/namedspacedserviceaccounts_test.go b/api/krusty/namedspacedserviceaccounts_test.go new file mode 100644 index 000000000..5a4b5e0ac --- /dev/null +++ b/api/krusty/namedspacedserviceaccounts_test.go @@ -0,0 +1,77 @@ +// Copyright 2022 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package krusty_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" +) + +func TestNamedspacedServiceAccountsWithOverlap(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("a/a.yaml", ` +apiVersion: v1 +kind: ServiceAccount +metadata: + name: sa +--- +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 +`) + + th.WriteK("a/", ` +namespace: a +resources: +- a.yaml +`) + + th.WriteF("b/b.yaml", ` +apiVersion: v1 +kind: ServiceAccount +metadata: + name: sa +--- +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 +`) + + th.WriteK("b/", ` +namespace: b +resources: +- b.yaml +`) + + th.WriteK(".", ` +resources: +- a +- b +`) + + err := th.RunWithErr(".", th.MakeDefaultOptions()) + assert.EqualError(t, err, + "updating name reference in 'subjects' field of 'ClusterRoleBinding.v1.rbac.authorization.k8s.io/crb-a.[noNs]': "+ + "considering field 'subjects' of object ClusterRoleBinding.v1.rbac.authorization.k8s.io/crb-a.[noNs]: "+ + "found multiple possible referrals: ServiceAccount.v1.[noGrp]/sa.a, ServiceAccount.v1.[noGrp]/sa.b") +}