From 69c90e3427f02127dc72a60309e9efeabb1d7e0d Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sat, 20 Jul 2019 12:11:46 -0500 Subject: [PATCH 1/3] Fix namereference and stacked kustomization contexts (1/3) - Update PrefixSuffixTransfomer to add empty prefix and suffix --- plugin/builtin/PrefixSuffixTransformer.go | 24 ++++++++++++++++--- .../PrefixSuffixTransformer.go | 24 ++++++++++++++++--- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/plugin/builtin/PrefixSuffixTransformer.go b/plugin/builtin/PrefixSuffixTransformer.go index 3ea0f159a..6e4e2e6d6 100644 --- a/plugin/builtin/PrefixSuffixTransformer.go +++ b/plugin/builtin/PrefixSuffixTransformer.go @@ -49,22 +49,40 @@ func (p *PrefixSuffixTransformerPlugin) Config( } func (p *PrefixSuffixTransformerPlugin) Transform(m resmap.ResMap) error { - if len(p.Prefix) == 0 && len(p.Suffix) == 0 { - return nil - } + + // Even if both the Prefix and Suffix are empty we want + // to proceed with the transformation. This allows to add contextual + // information to the resources (AddNamePrefix and AddNameSuffix). + for _, r := range m.Resources() { if p.shouldSkip(r.OrgId()) { + // Don't change the actual definition + // of a CRD. continue } id := r.OrgId() + // current default configuration contains + // only one entry: "metadata/name" with no GVK for _, path := range p.FieldSpecs { if !id.IsSelected(&path.Gvk) { + // With the currrent default configuration, + // because no Gvk is specified, so a wild + // card continue } + if smellsLikeANameChange(&path) { + // "metadata/name" is the only field. + // this will add a prefix and a suffix + // to the resource even if those are + // empty r.AddNamePrefix(p.Prefix) r.AddNameSuffix(p.Suffix) } + + // the addPrefixSuffix method will not + // change the name if both the prefix and suffix + // are empty. err := transformers.MutateField( r.Map(), path.PathSlice(), diff --git a/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer.go b/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer.go index 20a9ac9ce..80c57462e 100644 --- a/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer.go +++ b/plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer.go @@ -50,22 +50,40 @@ func (p *plugin) Config( } func (p *plugin) Transform(m resmap.ResMap) error { - if len(p.Prefix) == 0 && len(p.Suffix) == 0 { - return nil - } + + // Even if both the Prefix and Suffix are empty we want + // to proceed with the transformation. This allows to add contextual + // information to the resources (AddNamePrefix and AddNameSuffix). + for _, r := range m.Resources() { if p.shouldSkip(r.OrgId()) { + // Don't change the actual definition + // of a CRD. continue } id := r.OrgId() + // current default configuration contains + // only one entry: "metadata/name" with no GVK for _, path := range p.FieldSpecs { if !id.IsSelected(&path.Gvk) { + // With the currrent default configuration, + // because no Gvk is specified, so a wild + // card continue } + if smellsLikeANameChange(&path) { + // "metadata/name" is the only field. + // this will add a prefix and a suffix + // to the resource even if those are + // empty r.AddNamePrefix(p.Prefix) r.AddNameSuffix(p.Suffix) } + + // the addPrefixSuffix method will not + // change the name if both the prefix and suffix + // are empty. err := transformers.MutateField( r.Map(), path.PathSlice(), From 8fa3861ba31ef7e55e565e373671b18d51204611 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sat, 20 Jul 2019 15:45:58 -0500 Subject: [PATCH 2/3] Fix namereference and stacked kustomization contexts (2/3) - Leverage nameprefix and namesuffix contextual data --- pkg/resmap/resmap.go | 27 ++++++---------- pkg/resource/resource.go | 69 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 22 deletions(-) diff --git a/pkg/resmap/resmap.go b/pkg/resmap/resmap.go index 41cc29934..ca1e213f4 100644 --- a/pkg/resmap/resmap.go +++ b/pkg/resmap/resmap.go @@ -552,26 +552,17 @@ func (m *resWrangler) makeCopy(copier resCopier) ResMap { // SubsetThatCouldBeReferencedByResource implements ResMap. func (m *resWrangler) SubsetThatCouldBeReferencedByResource( inputRes *resource.Resource) ResMap { - inputId := inputRes.OrgId() - if !inputId.IsNamespaceableKind() { - if inputRes.GetOutermostNamePrefix() == "" { - return m - } - result := New() - for _, r := range m.Resources() { - if r.GetOutermostNamePrefix() == inputRes.GetOutermostNamePrefix() && - r.GetOutermostNameSuffix() == inputRes.GetOutermostNameSuffix() { - err := result.Append(r) - if err != nil { - panic(err) - } - } - } - return result - } result := New() + inputId := inputRes.CurId() + isInputIdNamespaceable := inputId.IsNamespaceableKind() + rctxm := inputRes.PrefixesSuffixesEquals for _, r := range m.Resources() { - if !r.OrgId().IsNamespaceableKind() || inputRes.InSameFuzzyNamespace(r) { + // Need to match more accuratly both at the time of selection and transformation. + // OutmostPrefixSuffixEquals is not accurate enough since it is only using + // the outer most suffix and the last prefix. Use PrefixedSuffixesEquals instead. + resId := r.CurId() + if (!isInputIdNamespaceable || !resId.IsNamespaceableKind() || resId.IsNsEquals(inputId)) && + r.InSameKustomizeCtx(rctxm) { err := result.Append(r) if err != nil { panic(err) diff --git a/pkg/resource/resource.go b/pkg/resource/resource.go index e7fb08d35..1b2f96e47 100644 --- a/pkg/resource/resource.go +++ b/pkg/resource/resource.go @@ -27,6 +27,22 @@ type Resource struct { nameSuffixes []string } +// ResCtx is an interface describing the contextual added +// kept kustomize in the context of each Resource object. +// Currently mainly the name prefix and name suffix are added. +type ResCtx interface { + AddNamePrefix(p string) + AddNameSuffix(s string) + GetOutermostNamePrefix() string + GetOutermostNameSuffix() string + GetNamePrefixes() []string + GetNameSuffixes() []string +} + +// ResCtxMatcher returns true if two Resources are being +// modified in the same kustomize context. +type ResCtxMatcher func(ResCtx) bool + // DeepCopy returns a new copy of resource func (r *Resource) DeepCopy() *Resource { rc := &Resource{ @@ -104,14 +120,17 @@ func copyStringSlice(s []string) []string { return c } +// Implements ResCtx AddNamePrefix func (r *Resource) AddNamePrefix(p string) { r.namePrefixes = append(r.namePrefixes, p) } +// Implements ResCtx AddNameSuffix func (r *Resource) AddNameSuffix(s string) { r.nameSuffixes = append(r.nameSuffixes, s) } +// Implements ResCtx GetOutermostNamePrefix func (r *Resource) GetOutermostNamePrefix() string { if len(r.namePrefixes) == 0 { return "" @@ -119,6 +138,7 @@ func (r *Resource) GetOutermostNamePrefix() string { return r.namePrefixes[len(r.namePrefixes)-1] } +// Implements ResCtx GetOutermostNameSuffix func (r *Resource) GetOutermostNameSuffix() string { if len(r.nameSuffixes) == 0 { return "" @@ -126,10 +146,51 @@ func (r *Resource) GetOutermostNameSuffix() string { return r.nameSuffixes[len(r.nameSuffixes)-1] } -func (r *Resource) InSameFuzzyNamespace(o *Resource) bool { - return r.CurId().IsNsEquals(o.CurId()) && - r.GetOutermostNamePrefix() == o.GetOutermostNamePrefix() && - r.GetOutermostNameSuffix() == o.GetOutermostNameSuffix() +func sameBeginningSubarray(a, b []string) bool { + maxlen := len(b) + if len(a) < len(b) { + maxlen = len(a) + } + + if maxlen == 0 { + return true + } + + for i, v := range a[0 : maxlen-1] { + if v != b[i] { + return false + } + } + return true +} + +// Implements ResCtx GetNamePrefixes +func (r *Resource) GetNamePrefixes() []string { + return r.namePrefixes +} + +// Implements ResCtx GetNameSuffixes +func (r *Resource) GetNameSuffixes() []string { + return r.nameSuffixes +} + +// OutermostPrefixSuffixEquals returns true if both resources +// outer suffix and prefix matches. +func (r *Resource) OutermostPrefixSuffixEquals(o ResCtx) bool { + return (r.GetOutermostNamePrefix() == o.GetOutermostNamePrefix()) && (r.GetOutermostNameSuffix() == o.GetOutermostNameSuffix()) +} + +// PrefixesSuffixesEquals is conceptually doing the same task +// as OutermostPrefixSuffix but performs a deeper comparison +// of the the list of suffix and prefix. +func (r *Resource) PrefixesSuffixesEquals(o ResCtx) bool { + return sameBeginningSubarray(r.GetNamePrefixes(), o.GetNamePrefixes()) && sameBeginningSubarray(r.GetNameSuffixes(), o.GetNameSuffixes()) +} + +// This is used to compute if a referrer could potentially be impacted +// by the change of name of a referral. +func (r *Resource) InSameKustomizeCtx(rctxm ResCtxMatcher) bool { + return rctxm(r) } func (r *Resource) GetOriginalName() string { From 230090d790bc6dc0be26406918bab7e1f3a0b2ad Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sat, 20 Jul 2019 15:52:00 -0500 Subject: [PATCH 3/3] Fix namereference and stacked kustomization contexts (3/3) - Update unit and integration tests. --- pkg/resmap/resmap_test.go | 8 +- pkg/target/resourceconflict_test.go | 226 ++++++++++++++++++++++++---- 2 files changed, 201 insertions(+), 33 deletions(-) diff --git a/pkg/resmap/resmap_test.go b/pkg/resmap/resmap_test.go index d607d8485..fa6e31261 100644 --- a/pkg/resmap/resmap_test.go +++ b/pkg/resmap/resmap_test.go @@ -356,7 +356,7 @@ func TestSubsetThatCouldBeReferencedByResource(t *testing.T) { r4 := rf.FromMap( map[string]interface{}{ "apiVersion": "v1", - "kind": "ConfigMap", + "kind": "Deployment", "metadata": map[string]interface{}{ "name": "charlie", "namespace": "happy", @@ -365,7 +365,7 @@ func TestSubsetThatCouldBeReferencedByResource(t *testing.T) { r5 := rf.FromMap( map[string]interface{}{ "apiVersion": "v1", - "kind": "Deployment", + "kind": "ConfigMap", "metadata": map[string]interface{}{ "name": "charlie", "namespace": "happy", @@ -408,12 +408,12 @@ func TestSubsetThatCouldBeReferencedByResource(t *testing.T) { "happy namespace no prefix": { filter: r3, expected: resmaptest_test.NewRmBuilder(t, rf). - AddR(r3).AddR(r4).AddR(r7).ResMap(), + AddR(r3).AddR(r4).AddR(r5).AddR(r6).AddR(r7).ResMap(), }, "happy namespace with prefix": { filter: r5, expected: resmaptest_test.NewRmBuilder(t, rf). - AddR(r5).AddR(r6).AddR(r7).ResMap(), + AddR(r3).AddR(r4).AddR(r5).AddR(r6).AddR(r7).ResMap(), }, "cluster level": { filter: r7, diff --git a/pkg/target/resourceconflict_test.go b/pkg/target/resourceconflict_test.go index 676eab44c..96574c645 100644 --- a/pkg/target/resourceconflict_test.go +++ b/pkg/target/resourceconflict_test.go @@ -4,7 +4,6 @@ package target_test import ( - "strings" "testing" "sigs.k8s.io/kustomize/v3/pkg/kusttest" @@ -16,6 +15,7 @@ resources: - serviceaccount.yaml - rolebinding.yaml - clusterrolebinding.yaml +- clusterrole.yaml namePrefix: pfx- nameSuffix: -sfx `) @@ -32,7 +32,7 @@ metadata: name: rolebinding roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role + kind: ClusterRole name: role subjects: - kind: ServiceAccount @@ -45,11 +45,21 @@ metadata: name: rolebinding roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role + kind: ClusterRole name: role subjects: - kind: ServiceAccount name: serviceaccount +`) + th.WriteF("/app/base/clusterrole.yaml", ` +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: role +rules: +- apiGroups: [""] + resources: ["secrets"] + verbs: ["get", "watch", "list"] `) } @@ -97,8 +107,8 @@ metadata: name: pfx-rolebinding-sfx roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: pfx-role-sfx subjects: - kind: ServiceAccount name: pfx-serviceaccount-sfx @@ -109,11 +119,25 @@ metadata: name: pfx-rolebinding-sfx roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: pfx-role-sfx subjects: - kind: ServiceAccount name: pfx-serviceaccount-sfx +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: pfx-role-sfx +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - watch + - list `) } @@ -137,8 +161,8 @@ metadata: name: a-pfx-rolebinding-sfx-suffixA roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: a-pfx-role-sfx-suffixA subjects: - kind: ServiceAccount name: a-pfx-serviceaccount-sfx-suffixA @@ -149,11 +173,25 @@ metadata: name: a-pfx-rolebinding-sfx-suffixA roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: a-pfx-role-sfx-suffixA subjects: - kind: ServiceAccount name: a-pfx-serviceaccount-sfx-suffixA +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: a-pfx-role-sfx-suffixA +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - watch + - list `) } @@ -177,8 +215,8 @@ metadata: name: b-pfx-rolebinding-sfx-suffixB roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: b-pfx-role-sfx-suffixB subjects: - kind: ServiceAccount name: b-pfx-serviceaccount-sfx-suffixB @@ -189,11 +227,25 @@ metadata: name: b-pfx-rolebinding-sfx-suffixB roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: b-pfx-role-sfx-suffixB subjects: - kind: ServiceAccount name: b-pfx-serviceaccount-sfx-suffixB +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: b-pfx-role-sfx-suffixB +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - watch + - list `) } @@ -218,8 +270,8 @@ metadata: name: a-pfx-rolebinding-sfx-suffixA roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: a-pfx-role-sfx-suffixA subjects: - kind: ServiceAccount name: a-pfx-serviceaccount-sfx-suffixA @@ -230,12 +282,26 @@ metadata: name: a-pfx-rolebinding-sfx-suffixA roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: a-pfx-role-sfx-suffixA subjects: - kind: ServiceAccount name: a-pfx-serviceaccount-sfx-suffixA --- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: a-pfx-role-sfx-suffixA +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - watch + - list +--- apiVersion: v1 kind: ServiceAccount metadata: @@ -247,8 +313,8 @@ metadata: name: b-pfx-rolebinding-sfx-suffixB roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: b-pfx-role-sfx-suffixB subjects: - kind: ServiceAccount name: b-pfx-serviceaccount-sfx-suffixB @@ -259,11 +325,25 @@ metadata: name: b-pfx-rolebinding-sfx-suffixB roleRef: apiGroup: rbac.authorization.k8s.io - kind: Role - name: role + kind: ClusterRole + name: b-pfx-role-sfx-suffixB subjects: - kind: ServiceAccount name: b-pfx-serviceaccount-sfx-suffixB +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: b-pfx-role-sfx-suffixB +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - watch + - list `) } @@ -289,12 +369,100 @@ metadata: name: serviceaccount `) - _, err := th.MakeKustTarget().MakeCustomizedResMap() - if err == nil { - t.Fatalf("Expected resource conflict.") - } - if !strings.Contains( - err.Error(), "multiple matches for ~G_v1_ServiceAccount") { + m, err := th.MakeKustTarget().MakeCustomizedResMap() + if err != nil { t.Fatalf("Unexpected err: %v", err) } + th.AssertActualEqualsExpected(m, ` +apiVersion: v1 +kind: ServiceAccount +metadata: + name: a-serviceaccount-suffixA +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: a-pfx-serviceaccount-sfx-suffixA +--- +apiVersion: rbac.authorization.k8s.io/v1beta1 +kind: RoleBinding +metadata: + name: a-pfx-rolebinding-sfx-suffixA +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: a-pfx-role-sfx-suffixA +subjects: +- kind: ServiceAccount + name: a-pfx-serviceaccount-sfx-suffixA +--- +apiVersion: rbac.authorization.k8s.io/v1beta1 +kind: ClusterRoleBinding +metadata: + name: a-pfx-rolebinding-sfx-suffixA +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: a-pfx-role-sfx-suffixA +subjects: +- kind: ServiceAccount + name: a-pfx-serviceaccount-sfx-suffixA +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: a-pfx-role-sfx-suffixA +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - watch + - list +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: b-pfx-serviceaccount-sfx-suffixB +--- +apiVersion: rbac.authorization.k8s.io/v1beta1 +kind: RoleBinding +metadata: + name: b-pfx-rolebinding-sfx-suffixB +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: b-pfx-role-sfx-suffixB +subjects: +- kind: ServiceAccount + name: b-pfx-serviceaccount-sfx-suffixB +--- +apiVersion: rbac.authorization.k8s.io/v1beta1 +kind: ClusterRoleBinding +metadata: + name: b-pfx-rolebinding-sfx-suffixB +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: b-pfx-role-sfx-suffixB +subjects: +- kind: ServiceAccount + name: b-pfx-serviceaccount-sfx-suffixB +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: b-pfx-role-sfx-suffixB +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - watch + - list +`) }