From 12ae316ac4005a753d2712d56f65d8e2b8760431 Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Thu, 22 Sep 2022 13:48:20 +0200 Subject: [PATCH 1/2] fix individual run of TestManifests I observed that `go test ./diff` works, but `go test .diff -run TestManifests` fails because TestManifests silently relies on TestPrintDiffWithContext to call ansi.DisableColors(). --- diff/diff_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/diff/diff_test.go b/diff/diff_test.go index 667e4a4d..c711fd32 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -169,6 +169,8 @@ func assertDiff(t *testing.T, before, after string, context int, stripTrailingCR } func TestManifests(t *testing.T) { + ansi.DisableColors(true) + specBeta := map[string]*manifest.MappingResult{ "default, nginx, Deployment (apps)": { From 6dc2b078d0f1233a4f85e382a1b6b2a0eeef5519 Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Thu, 22 Sep 2022 13:49:33 +0200 Subject: [PATCH 2/2] 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 --- diff/diff.go | 18 +++++++ diff/diff_test.go | 121 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) diff --git a/diff/diff.go b/diff/diff.go index 9ed30bbb..42bb5d21 100644 --- a/diff/diff.go +++ b/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 --git a/diff/diff_test.go b/diff/diff_test.go index c711fd32..8df93823 100644 --- a/diff/diff_test.go +++ b/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()) + }) +}