From 55ac9ca88db95da0d8c46ee4f73ebca85897a659 Mon Sep 17 00:00:00 2001 From: natasha41575 Date: Tue, 12 Oct 2021 10:59:41 -0700 Subject: [PATCH] fix bug with migrating annotations --- kyaml/kio/byteio_reader.go | 4 ++ kyaml/kio/byteio_writer_test.go | 14 +++--- kyaml/kio/kio.go | 36 +++++++++++++-- kyaml/kio/kio_test.go | 57 +++++++++++++++++++++-- kyaml/kio/kioutil/kioutil.go | 12 +++-- kyaml/kio/kioutil/kioutil_test.go | 77 +++++++++++++++++++++++++++++++ 6 files changed, 182 insertions(+), 18 deletions(-) diff --git a/kyaml/kio/byteio_reader.go b/kyaml/kio/byteio_reader.go index 9036b1739c..fd0c45c6a2 100644 --- a/kyaml/kio/byteio_reader.go +++ b/kyaml/kio/byteio_reader.go @@ -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) diff --git a/kyaml/kio/byteio_writer_test.go b/kyaml/kio/byteio_writer_test.go index ce3c373218..60bebf7165 100644 --- a/kyaml/kio/byteio_writer_test.go +++ b/kyaml/kio/byteio_writer_test.go @@ -299,7 +299,13 @@ g: `, }, - expectedOutput: `a: b #first + expectedOutput: `e: f +g: + h: + - i # has a list + - j +--- +a: b #first metadata: annotations: internal.config.kubernetes.io/path: "a/b/a_test.yaml" @@ -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 `, }, diff --git a/kyaml/kio/kio.go b/kyaml/kio/kio.go index 7b39438399..410c0a23fe 100644 --- a/kyaml/kio/kio.go +++ b/kyaml/kio/kio.go @@ -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] @@ -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 @@ -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 } diff --git a/kyaml/kio/kio_test.go b/kyaml/kio/kio_test.go index 4c73ecd349..60027108eb 100644 --- a/kyaml/kio/kio_test.go +++ b/kyaml/kio/kio_test.go @@ -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), @@ -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 @@ -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 { @@ -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()) + } + }) } } diff --git a/kyaml/kio/kioutil/kioutil.go b/kyaml/kio/kioutil/kioutil.go index bcd9730300..bfe2e9334c 100644 --- a/kyaml/kio/kioutil/kioutil.go +++ b/kyaml/kio/kioutil/kioutil.go @@ -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 { @@ -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 diff --git a/kyaml/kio/kioutil/kioutil_test.go b/kyaml/kio/kioutil/kioutil_test.go index 6fedf21fb5..bf6c07b4ea 100644 --- a/kyaml/kio/kioutil/kioutil_test.go +++ b/kyaml/kio/kioutil/kioutil_test.go @@ -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' + 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()) + } +}