From 77814ac12be95df6f993e7019246ef6026999503 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Fri, 8 Jul 2022 21:15:42 -0400 Subject: [PATCH 1/3] NameReferenceTransformer misses self references in annotations --- api/krusty/namereference_test.go | 83 ++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/api/krusty/namereference_test.go b/api/krusty/namereference_test.go index 3fe26090a..806cad1b3 100644 --- a/api/krusty/namereference_test.go +++ b/api/krusty/namereference_test.go @@ -589,3 +589,86 @@ metadata: namespace: kube-system `) } + +func TestIssue4682_NameReferencesToSelfInAnnotations(t *testing.T) { + th := kusttest_test.MakeHarness(t) + th.WriteK(".", ` +namespace: newNs +resources: + - resources.yaml + +nameSuffix: -updated + +configurations: + - kustomize-nameref.yaml +`) + th.WriteF("kustomize-nameref.yaml", ` +nameReference: + - kind: Namespace + fieldSpecs: + - path: data/theNamespace + kind: ConfigMap + version: v1 + - path: metadata/annotations/theNamespace + kind: ConfigMap + version: v1 + - path: metadata/annotations/theNamespace + kind: Namespace + version: v1 + - kind: ConfigMap + fieldSpecs: + - path: data/theConfigMap + kind: ConfigMap + version: v1 + - path: metadata/annotations/theConfigMap + kind: ConfigMap + version: v1 + - path: metadata/annotations/theConfigMap + kind: Namespace + version: v1 +`) + th.WriteF("resources.yaml", ` +apiVersion: v1 +kind: ConfigMap +metadata: + annotations: + theConfigMap: cm + theNamespace: oldNs + name: cm + namespace: oldNs +data: + theConfigMap: cm + theNamespace: oldNs +--- +apiVersion: v1 +kind: Namespace +metadata: + annotations: + theConfigMap: cm + theNamespace: oldNs + name: oldNs +`) + m := th.Run(".", th.MakeDefaultOptions()) + // Incorrect: the self-reference annotations in both resources were not updated! + th.AssertActualEqualsExpected(m, ` +apiVersion: v1 +data: + theConfigMap: cm-updated + theNamespace: newNs +kind: ConfigMap +metadata: + annotations: + theConfigMap: cm + theNamespace: newNs + name: cm-updated + namespace: newNs +--- +apiVersion: v1 +kind: Namespace +metadata: + annotations: + theConfigMap: cm-updated + theNamespace: oldNs + name: newNs +`) +} From 04e1663b7001b6529a71f379efb8c6af6aff8219 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Fri, 8 Jul 2022 22:20:47 -0400 Subject: [PATCH 2/3] appendCsVAnnotation must mutate the annotations RNode in place Otherwise, it is incompatible with filters like fieldSpec.Filter that recurse through a tree of RNodes. In the case of the bug this commit fixes, the leaf RNode is a metadata.annotation value field, and appendCsvAnnotation is called immediately before updating that leaf's value. If appendCsvAnnotation replaces the entire metadata.annotation RNode on the resource, the leaf RNode that gets updated is no longer attached to the resource. Alternatively, `func (f Filter) setScalar` could be updated to change the value of the leaf RNode BEFORE calling appendCsvAnnotation. However, the modification of the entire metadata segment of the RNode in a function that nominally just edits an annotation feels like a side-effect prone to creating other difficult-to-debug situations, beyond this ordering dependency. --- api/krusty/namereference_test.go | 5 ++--- api/resource/resource.go | 10 +++------- api/resource/resource_test.go | 16 ++++++++-------- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/api/krusty/namereference_test.go b/api/krusty/namereference_test.go index 806cad1b3..293746571 100644 --- a/api/krusty/namereference_test.go +++ b/api/krusty/namereference_test.go @@ -649,7 +649,6 @@ metadata: name: oldNs `) m := th.Run(".", th.MakeDefaultOptions()) - // Incorrect: the self-reference annotations in both resources were not updated! th.AssertActualEqualsExpected(m, ` apiVersion: v1 data: @@ -658,7 +657,7 @@ data: kind: ConfigMap metadata: annotations: - theConfigMap: cm + theConfigMap: cm-updated theNamespace: newNs name: cm-updated namespace: newNs @@ -668,7 +667,7 @@ kind: Namespace metadata: annotations: theConfigMap: cm-updated - theNamespace: oldNs + theNamespace: newNs name: newNs `) } diff --git a/api/resource/resource.go b/api/resource/resource.go index 9997e408b..a88ce31d2 100644 --- a/api/resource/resource.go +++ b/api/resource/resource.go @@ -255,13 +255,9 @@ func (r *Resource) appendCsvAnnotation(name, value string) { if value == "" { return } - annotations := r.GetAnnotations() - if existing, ok := annotations[name]; ok { - annotations[name] = existing + "," + value - } else { - annotations[name] = value - } - if err := r.SetAnnotations(annotations); err != nil { + currentValue := r.getCsvAnnotation(name) + newValue := strings.Join(append(currentValue, value), ",") + if err := r.RNode.PipeE(kyaml.SetAnnotation(name, newValue)); err != nil { panic(err) } } diff --git a/api/resource/resource_test.go b/api/resource/resource_test.go index 106c827ae..8e2b692ed 100644 --- a/api/resource/resource_test.go +++ b/api/resource/resource_test.go @@ -1402,31 +1402,31 @@ spec: `)) assert.NoError(t, err) r.AppendRefBy(resid.FromString("knd1.ver1.gr1/name1.ns1")) - assert.Equal(t, r.RNode.MustString(), `apiVersion: v1 + assert.Equal(t, `apiVersion: v1 kind: Deployment metadata: name: clown annotations: - internal.config.kubernetes.io/refBy: knd1.ver1.gr1/name1.ns1 + internal.config.kubernetes.io/refBy: 'knd1.ver1.gr1/name1.ns1' spec: numReplicas: 1 -`) +`, r.RNode.MustString()) assert.Equal(t, r.GetRefBy(), []resid.ResId{resid.FromString("knd1.ver1.gr1/name1.ns1")}) r.AppendRefBy(resid.FromString("knd2.ver2.gr2/name2.ns2")) - assert.Equal(t, r.RNode.MustString(), `apiVersion: v1 + assert.Equal(t, `apiVersion: v1 kind: Deployment metadata: name: clown annotations: - internal.config.kubernetes.io/refBy: knd1.ver1.gr1/name1.ns1,knd2.ver2.gr2/name2.ns2 + internal.config.kubernetes.io/refBy: 'knd1.ver1.gr1/name1.ns1,knd2.ver2.gr2/name2.ns2' spec: numReplicas: 1 -`) - assert.Equal(t, r.GetRefBy(), []resid.ResId{ +`, r.RNode.MustString()) + assert.Equal(t, []resid.ResId{ resid.FromString("knd1.ver1.gr1/name1.ns1"), resid.FromString("knd2.ver2.gr2/name2.ns2"), - }) + }, r.GetRefBy()) } func TestOrigin(t *testing.T) { From 68780b4c0c51ed4627f303aacacc90cadbfa8ca6 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Mon, 11 Jul 2022 20:03:13 -0400 Subject: [PATCH 3/3] Fix flakey test --- api/krusty/namedspacedserviceaccounts_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/krusty/namedspacedserviceaccounts_test.go b/api/krusty/namedspacedserviceaccounts_test.go index 5a4b5e0ac..791c95074 100644 --- a/api/krusty/namedspacedserviceaccounts_test.go +++ b/api/krusty/namedspacedserviceaccounts_test.go @@ -70,8 +70,5 @@ resources: `) 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") + assert.Contains(t, err.Error(), "found multiple possible referrals: ServiceAccount.v1.[noGrp]/sa.a, ServiceAccount.v1.[noGrp]/sa.b") }