From 86d48b2a952bd64811be3ec30fd97efcdc38e2b4 Mon Sep 17 00:00:00 2001 From: Nuno Anselmo Date: Tue, 21 Jun 2022 23:07:54 +0100 Subject: [PATCH 1/3] Implements TestNamedspacedServiceAccounts with and without overlap --- api/krusty/namedspacedserviceaccounts_test.go | 212 ++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 api/krusty/namedspacedserviceaccounts_test.go diff --git a/api/krusty/namedspacedserviceaccounts_test.go b/api/krusty/namedspacedserviceaccounts_test.go new file mode 100644 index 000000000..d78ba403a --- /dev/null +++ b/api/krusty/namedspacedserviceaccounts_test.go @@ -0,0 +1,212 @@ +// Copyright 2022 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package krusty_test + +import ( + "testing" + + 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()) + 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) + + 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 +`) + + m := th.Run(".", th.MakeDefaultOptions()) + th.AssertActualEqualsExpected(m, ` +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 +`) +} From d8efc15169ae1fcb725c89dadb4db709d6ca4ea0 Mon Sep 17 00:00:00 2001 From: Nuno Anselmo Date: Tue, 21 Jun 2022 23:52:39 +0100 Subject: [PATCH 2/3] Adds commentary on expected/unexpected test output --- api/krusty/namedspacedserviceaccounts_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/krusty/namedspacedserviceaccounts_test.go b/api/krusty/namedspacedserviceaccounts_test.go index d78ba403a..4674c6dd6 100644 --- a/api/krusty/namedspacedserviceaccounts_test.go +++ b/api/krusty/namedspacedserviceaccounts_test.go @@ -69,6 +69,9 @@ resources: `) 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 @@ -170,6 +173,9 @@ resources: `) m := th.Run(".", th.MakeDefaultOptions()) + + // Unexpected result: crb-b's subject obtains the wrong namespace, having "namespace: a" + // If the order is swapped in the kustomization.yaml then it's crb-a that gets "namespace: b" th.AssertActualEqualsExpected(m, ` apiVersion: v1 kind: ServiceAccount From 387c95be1f80982ce18aa10870485e04a3655fd9 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Thu, 7 Jul 2022 18:33:48 -0400 Subject: [PATCH 3/3] Correctly detect ambiguity between potential referrers when targeting a name+namespace reference --- api/filters/nameref/nameref.go | 25 ++- .../namereferencetransformer_test.go | 120 ++++++++++++++ api/krusty/namedspacedserviceaccounts_test.go | 153 +----------------- 3 files changed, 147 insertions(+), 151 deletions(-) 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 index 4674c6dd6..5a4b5e0ac 100644 --- a/api/krusty/namedspacedserviceaccounts_test.go +++ b/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()) - - // Unexpected result: crb-b's subject obtains the wrong namespace, having "namespace: a" - // If the order is swapped in the kustomization.yaml then it's crb-a that gets "namespace: b" - th.AssertActualEqualsExpected(m, ` -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 -`) + 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") }