diff: fix secret redaction for secrets with stringData

Fixes #302.

redactSecrets() uses `stringData` as a behind-the-scenes crutch for
holding redacted values from `data`. To ensure that secrets using
`secretData` diff correctly, this change moves all `secretData` values
into `data` before performing the redaction.

Moving `secretData` into `data` is the same rewrite that the k8s
apiserver does [1]. We follow the same precedence rules as k8s: A value
in `stringData` takes precedence over a value with the same key in
`data`.

[1] https://kubernetes.io/docs/concepts/configuration/secret/#restriction-names-data

Author: majewsky

diff/diff.go

@@ -163,12 +163,29 @@ func redactSecrets(old, new *manifest.MappingResult) {
 		if err := yaml.NewYAMLToJSONDecoder(bytes.NewBufferString(old.Content)).Decode(&oldSecret); err != nil {
 			old.Content = fmt.Sprintf("Error parsing old secret: %s", err)
 		}
+		//if we have a Secret containing `stringData`, apply the same
+		//transformation that the apiserver would do with it (this protects
+		//stringData keys from being overwritten down below)
+		if len(oldSecret.StringData) > 0 && oldSecret.Data == nil {
+			oldSecret.Data = make(map[string][]byte, len(oldSecret.StringData))
+		}
+		for k, v := range oldSecret.StringData {
+			oldSecret.Data[k] = []byte(v)
+		}
 	}
 	if new != nil {
 		if err := yaml.NewYAMLToJSONDecoder(bytes.NewBufferString(new.Content)).Decode(&newSecret); err != nil {
 			new.Content = fmt.Sprintf("Error parsing new secret: %s", err)
 		}
+		//same as above
+		if len(newSecret.StringData) > 0 && newSecret.Data == nil {
+			newSecret.Data = make(map[string][]byte, len(newSecret.StringData))
+		}
+		for k, v := range newSecret.StringData {
+			newSecret.Data[k] = []byte(v)
+		}
 	}
+
 	if old != nil {
 		oldSecret.StringData = make(map[string]string, len(oldSecret.Data))
 		for k, v := range oldSecret.Data {
@@ -189,6 +206,7 @@ func redactSecrets(old, new *manifest.MappingResult) {
 			}
 		}
 	}
+
 	// remove Data field now that we are using StringData for serialization
 	var buf bytes.Buffer
 	if old != nil {

diff/diff_test.go

@@ -463,3 +463,124 @@ Plan: 0 to add, 1 to change, 0 to destroy.
 		require.Equal(t, "Resource name: nginx\n", buf1.String())
 	})
 }
+
+func TestManifestsWithRedactedSecrets(t *testing.T) {
+	ansi.DisableColors(true)
+
+	specSecretWithByteData := map[string]*manifest.MappingResult{
+		"default, foobar, Secret (v1)": {
+			Name: "default, foobar, Secret (v1)",
+			Kind: "Secret",
+			Content: `
+apiVersion: v1
+kind: Secret
+metadata:
+  name: foobar
+type: Opaque
+data:
+  key1: dmFsdWUx
+  key2: dmFsdWUy
+  key3: dmFsdWUz
+`,
+		}}
+
+	specSecretWithByteDataChanged := map[string]*manifest.MappingResult{
+		"default, foobar, Secret (v1)": {
+			Name: "default, foobar, Secret (v1)",
+			Kind: "Secret",
+			Content: `
+apiVersion: v1
+kind: Secret
+metadata:
+  name: foobar
+type: Opaque
+data:
+  key1: dmFsdWUxY2hhbmdlZA==
+  key2: dmFsdWUy
+  key4: dmFsdWU0
+`,
+		}}
+
+	specSecretWithStringData := map[string]*manifest.MappingResult{
+		"default, foobar, Secret (v1)": {
+			Name: "default, foobar, Secret (v1)",
+			Kind: "Secret",
+			Content: `
+apiVersion: v1
+kind: Secret
+metadata:
+  name: foobar
+type: Opaque
+stringData:
+  key1: value1
+  key2: value2
+  key3: value3
+`,
+		}}
+
+	specSecretWithStringDataChanged := map[string]*manifest.MappingResult{
+		"default, foobar, Secret (v1)": {
+			Name: "default, foobar, Secret (v1)",
+			Kind: "Secret",
+			Content: `
+apiVersion: v1
+kind: Secret
+metadata:
+  name: foobar
+type: Opaque
+stringData:
+  key1: value1changed
+  key2: value2
+  key4: value4
+`,
+		}}
+
+	t.Run("OnChangeSecretWithByteData", func(t *testing.T) {
+		var buf1 bytes.Buffer
+		diffOptions := Options{"diff", 10, false, false, []string{}, 0.5} //NOTE: ShowSecrets = false
+
+		if changesSeen := Manifests(specSecretWithByteData, specSecretWithByteDataChanged, &diffOptions, &buf1); !changesSeen {
+			t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
+		}
+
+		//TODO: Why is there no empty line between the header and the start of the diff, like in the other diffs?
+		require.Equal(t, `default, foobar, Secret (v1) has changed:
+  apiVersion: v1
+  kind: Secret
+  metadata:
+    name: foobar
+  data:
+-   key1: '-------- # (6 bytes)'
++   key1: '++++++++ # (13 bytes)'
+    key2: 'REDACTED # (6 bytes)'
+-   key3: '-------- # (6 bytes)'
++   key4: '++++++++ # (6 bytes)'
+  type: Opaque
+
+`, buf1.String())
+	})
+
+	t.Run("OnChangeSecretWithStringData", func(t *testing.T) {
+		var buf1 bytes.Buffer
+		diffOptions := Options{"diff", 10, false, false, []string{}, 0.5} //NOTE: ShowSecrets = false
+
+		if changesSeen := Manifests(specSecretWithStringData, specSecretWithStringDataChanged, &diffOptions, &buf1); !changesSeen {
+			t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
+		}
+
+		require.Equal(t, `default, foobar, Secret (v1) has changed:
+  apiVersion: v1
+  kind: Secret
+  metadata:
+    name: foobar
+  data:
+-   key1: '-------- # (6 bytes)'
++   key1: '++++++++ # (13 bytes)'
+    key2: 'REDACTED # (6 bytes)'
+-   key3: '-------- # (6 bytes)'
++   key4: '++++++++ # (6 bytes)'
+  type: Opaque
+
+`, buf1.String())
+	})
+}