Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix bug with migrating kyaml reader path, index, and id annotations #4227

Merged
merged 1 commit into from Oct 13, 2021
Merged

fix bug with migrating kyaml reader path, index, and id annotations #4227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions kyaml/kio/byteio_reader.go
View file
Open in desktop
Expand Up @@ -316,6 +316,10 @@ func (r *ByteReader) decode(originalYAML string, index int, decoder *yaml.Decode
r.SetAnnotations = map[string]string{}
}
if !r.OmitReaderAnnotations {
err := kioutil.CopyLegacyAnnotations(n)
if err != nil {
return nil, err
}
r.SetAnnotations[kioutil.IndexAnnotation] = fmt.Sprintf("%d", index)
r.SetAnnotations[kioutil.LegacyIndexAnnotation] = fmt.Sprintf("%d", index)

Expand Down
14 changes: 7 additions & 7 deletions kyaml/kio/byteio_writer_test.go
View file
Open in desktop
Expand Up @@ -299,7 +299,13 @@ g:
`,
},

expectedOutput: `a: b #first
expectedOutput: `e: f
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reordering is undoing the reordering done by the original PR https://github.com/kubernetes-sigs/kustomize/pull/4190/files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this considered as a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't considered a breaking change in #4190 when the reordering occurred, so I don't think it would be considered a breaking change to reverse it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these annotations cause an ordering change, and under what circumstances? Any reordering that surfaces in Kustomize itself should be considered breaking. But judging by this being the only test where it surfaces, I take it only direct kyaml use is affected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reordering that surfaces in Kustomize itself should be considered breaking.

I agree and this change only surfaced directly in kyaml. The reordering occurred in the test case where the nodes were being ordered - specifically in this case, some of the resources had the path/index annotations while this one did not. My guess (though I haven't done any investigation to back this up) is that the bug (introduced in #4190) which caused the legacy annotations to be sometimes overwritten or ignored caused the reordering, and this PR fixed it along with its other fixes.

g:
h:
- i # has a list
- j
---
a: b #first
metadata:
annotations:
internal.config.kubernetes.io/path: "a/b/a_test.yaml"
Expand All @@ -310,12 +316,6 @@ metadata:
annotations:
internal.config.kubernetes.io/path: "a/b/a_test.yaml"
config.kubernetes.io/path: 'a/b/a_test.yaml'
---
e: f
g:
h:
- i # has a list
- j
`,
},

Expand Down
36 changes: 33 additions & 3 deletions kyaml/kio/kio.go
View file
Open in desktop
Expand Up @@ -181,6 +181,10 @@ func storeInternalAnnotations(result []*yaml.RNode) (map[string]map[string]strin
if err != nil {
return nil, err
}
if err := checkMismatchedAnnos(meta); err != nil {
return nil, err
}

path := meta.Annotations[kioutil.PathAnnotation]
index := meta.Annotations[kioutil.IndexAnnotation]
id := meta.Annotations[kioutil.IdAnnotation]
Expand All @@ -193,6 +197,29 @@ func storeInternalAnnotations(result []*yaml.RNode) (map[string]map[string]strin
return nodeAnnosMap, nil
}

func checkMismatchedAnnos(meta yaml.ResourceMeta) error {
path := meta.Annotations[kioutil.PathAnnotation]
index := meta.Annotations[kioutil.IndexAnnotation]
id := meta.Annotations[kioutil.IdAnnotation]

legacyPath := meta.Annotations[kioutil.LegacyPathAnnotation]
legacyIndex := meta.Annotations[kioutil.LegacyIndexAnnotation]
legacyId := meta.Annotations[kioutil.LegacyIdAnnotation]

// if prior to running the functions, the legacy and internal annotations differ,
// throw an error as we cannot infer the user's intent.
if path != legacyPath {
return fmt.Errorf("resource input to function has mismatched legacy and internal path annotations")
}
if index != legacyIndex {
return fmt.Errorf("resource input to function has mismatched legacy and internal index annotations")
}
if id != legacyId {
return fmt.Errorf("resource input to function has mismatched legacy and internal id annotations")
}
return nil
}

type nodeAnnotations struct {
path string
index string
Expand All @@ -217,9 +244,12 @@ func reconcileInternalAnnotations(result []*yaml.RNode, nodeAnnosMap map[string]
if err != nil {
return err
}
// if the annotations are still somehow out of sync, prefer the internal annotations
// and copy them to the legacy ones
err = kioutil.CopyLegacyAnnotations(node)
// if the annotations are still somehow out of sync, throw an error
meta, err = node.GetMeta()
if err != nil {
return err
}
err = checkMismatchedAnnos(meta)
if err != nil {
return err
}
Expand Down
57 changes: 52 additions & 5 deletions kyaml/kio/kio_test.go
View file
Open in desktop
Expand Up @@ -233,6 +233,17 @@ func TestLegacyAnnotationReconciliation(t *testing.T) {
}
return nodes, nil
}
changeBothPathAnnos := func(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
for _, rn := range nodes {
if err := rn.PipeE(yaml.SetAnnotation(kioutil.LegacyPathAnnotation, "legacy")); err != nil {
return nil, err
}
if err := rn.PipeE(yaml.SetAnnotation(kioutil.PathAnnotation, "new")); err != nil {
return nil, err
}
}
return nodes, nil
}

noops := []Filter{
FilterFunc(noopFilter1),
Expand All @@ -242,11 +253,13 @@ func TestLegacyAnnotationReconciliation(t *testing.T) {
legacy := []Filter{FilterFunc(changeLegacyAnnos)}
legacyId := []Filter{FilterFunc(changeLegacyId)}
internalId := []Filter{FilterFunc(changeInternalId)}
changeBoth := []Filter{FilterFunc(changeBothPathAnnos), FilterFunc(noopFilter1)}

testCases := map[string]struct {
input string
filters []Filter
expected string
input string
filters []Filter
expected string
expectedErr string
}{
// the orchestrator should copy the legacy annotations to the new
// annotations
Expand Down Expand Up @@ -510,6 +523,32 @@ data:
grpcPort: 8080
`,
},
// the function changes both the legacy and new path annotation,
// so we should get an error
"change both": {
input: `apiVersion: v1
kind: ConfigMap
metadata:
name: ports-from
annotations:
config.kubernetes.io/path: 'configmap.yaml'
internal.kubernetes.io/path: 'configmap.yaml'
data:
grpcPort: 8080
---
apiVersion: v1
kind: ConfigMap
metadata:
name: ports-to
annotations:
config.kubernetes.io/path: "configmap.yaml"
config.kubernetes.io/index: '1'
data:
grpcPort: 8081
`,
filters: changeBoth,
expectedErr: "resource input to function has mismatched legacy and internal path annotations",
},
}

for tn, tc := range testCases {
Expand All @@ -526,8 +565,16 @@ data:
Filters: tc.filters,
Outputs: []Writer{&input},
}
assert.NoError(t, p.Execute())
assert.Equal(t, tc.expected, out.String())

err := p.Execute()
if tc.expectedErr == "" {
assert.NoError(t, err)
assert.Equal(t, tc.expected, out.String())
} else {
assert.Error(t, err)
assert.Equal(t, tc.expectedErr, err.Error())
}

})
}
}
12 changes: 9 additions & 3 deletions kyaml/kio/kioutil/kioutil.go
View file
Open in desktop
Expand Up @@ -55,6 +55,10 @@ func GetFileAnnotations(rn *yaml.RNode) (string, string, error) {
func CopyLegacyAnnotations(rn *yaml.RNode) error {
meta, err := rn.GetMeta()
if err != nil {
if err == yaml.ErrMissingMetadata {
// resource has no metadata, this should be a no-op
return nil
}
return err
}
if err := copyAnnotations(meta, rn, LegacyPathAnnotation, PathAnnotation); err != nil {
Expand All @@ -71,12 +75,14 @@ func CopyLegacyAnnotations(rn *yaml.RNode) error {

func copyAnnotations(meta yaml.ResourceMeta, rn *yaml.RNode, legacyKey string, newKey string) error {
newValue := meta.Annotations[newKey]
legacyValue := meta.Annotations[legacyKey]
if newValue != "" {
if err := rn.PipeE(yaml.SetAnnotation(legacyKey, newValue)); err != nil {
return err
if legacyValue == "" {
if err := rn.PipeE(yaml.SetAnnotation(legacyKey, newValue)); err != nil {
return err
}
}
} else {
legacyValue := meta.Annotations[legacyKey]
if legacyValue != "" {
if err := rn.PipeE(yaml.SetAnnotation(newKey, legacyValue)); err != nil {
return err
Expand Down
77 changes: 77 additions & 0 deletions kyaml/kio/kioutil/kioutil_test.go
View file
Open in desktop
Expand Up @@ -374,3 +374,80 @@ func TestCreatePathAnnotationValue(t *testing.T) {
}
}
}

func TestCopyLegacyAnnotations(t *testing.T) {
var tests = []struct {
input string
expected string
}{
{
input: `apiVersion: v1
kind: Foo
metadata:
name: foobar
annotations:
config.kubernetes.io/path: 'a/b.yaml'
config.kubernetes.io/index: '5'
`,
expected: `apiVersion: v1
kind: Foo
metadata:
name: foobar
annotations:
config.kubernetes.io/path: 'a/b.yaml'
config.kubernetes.io/index: '5'
internal.config.kubernetes.io/path: 'a/b.yaml'
internal.config.kubernetes.io/index: '5'
`,
},
{
input: `apiVersion: v1
kind: Foo
metadata:
name: foobar
annotations:
internal.config.kubernetes.io/path: 'a/b.yaml'
internal.config.kubernetes.io/index: '5'
`,
expected: `apiVersion: v1
kind: Foo
metadata:
name: foobar
annotations:
internal.config.kubernetes.io/path: 'a/b.yaml'
internal.config.kubernetes.io/index: '5'
config.kubernetes.io/path: 'a/b.yaml'
config.kubernetes.io/index: '5'
`,
},
{
input: `apiVersion: v1
kind: Foo
metadata:
name: foobar
annotations:
internal.config.kubernetes.io/path: 'a/b.yaml'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When both exist and don't match, should we error it out somewhere?
If we are handling it in another place, can you please help me recall it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is handled in reconcileInternalAnnotations, which detects when both exist and don't match and reconciles them accordingly. However this is only checked after running a function to account for functions that mutate one but not the other.

If the input resource to an orchestrator has both set but they differ, the orchestrator will prefer the internal annotation over the legacy one. Do you believe it should throw an error instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should error when the input to the function has both new and legacy annotations, the function modify both annotations and they mismatch. In this case, we can't infer the user's intend, we should error. It should be fine in other cases as long as we can infer user's intend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. I will update it to throw an error instead.

config.kubernetes.io/path: 'c/d.yaml'
`,
expected: `apiVersion: v1
kind: Foo
metadata:
name: foobar
annotations:
internal.config.kubernetes.io/path: 'a/b.yaml'
config.kubernetes.io/path: 'c/d.yaml'
`,
},
}

for _, tc := range tests {
rw := kio.ByteReadWriter{
Reader: bytes.NewBufferString(tc.input),
OmitReaderAnnotations: true,
}
nodes, err := rw.Read()
assert.NoError(t, err)
assert.NoError(t, kioutil.CopyLegacyAnnotations(nodes[0]))
assert.Equal(t, tc.expected, nodes[0].MustString())
}
}