Fix NameReference transformer handling of self-references in annotations #4703
Conversation
|
@KnVerey: This PR has multiple commits, and the default merge method is: merge. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KnVerey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
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.
KnVerey
force-pushed
the
name-ref-annotations
branch
from
e623817
to
04e1663
Compare
k8s-ci-robot
added
size/L
| @@ -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 | |||
There was a problem hiding this comment.
supposed to be expected, actual and was reversed
| 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' |
There was a problem hiding this comment.
Are we ok with this changing? It's an effect of using AnnotationSetter, which is doing this on purpose. It has this inline comment where it forces the style: // some tools get confused about the type if annotations are not quoted. I'm thinking this is acceptable, since appendCsvAnnotation is only used by build annotations AFAICT.
There was a problem hiding this comment.
Yeah, I'm fine with this, since it only affects our kustomize-internal build annotations.
|
The fix seems straightforward and makes sense to me, and the test looks good. /lgtm |
k8s-ci-robot
added
the
lgtm
Fixes #4682
Closes #4686
The test I added in this PR shows that the bug is even more specific than reported: it only affects self-references in annotations.
The solution I chose is to make
appendCsVAnnotationmutate the annotations RNode in place. Otherwise, that function is incompatible with filters likefieldSpec.Filterthat 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 appendCsvAnnotationis called immediately before updating that leaf's value. IfappendCsvAnnotationreplaces 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) setScalarcould be updated to change the value of the leaf RNode BEFORE callingappendCsvAnnotation. 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.