Merge pull request #4227 from natasha41575/MigrateIndexPathIdAnnotations

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

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)
 

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
 `,
 		},
 

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
 		}

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())
+			}
+
 		})
 	}
 }

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

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())
+	}
+}