From 2aaa36009d983fa6a569309e04b14f1a53775124 Mon Sep 17 00:00:00 2001 From: Josh Hudson <382062+itmustbejj@users.noreply.github.com> Date: Tue, 21 Nov 2023 17:31:35 -0800 Subject: [PATCH 1/7] Fix edge case in diff.NormalizeSecret that silently breaks diff.HideSecretData Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com> --- pkg/diff/diff.go | 12 ++++++++++++ pkg/diff/diff_test.go | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 2581bd2c5..778fb558c 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -859,6 +859,18 @@ func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) { return } o := applyOptions(opts) + if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil { + for k, v := range stringData { + switch v := v.(type) { + case int64: + stringData[k] = toString(v) + } + } + err := unstructured.SetNestedField(un.Object, stringData, "stringData") + if err != nil { + return + } + } var secret corev1.Secret err := runtime.DefaultUnstructuredConverter.FromUnstructured(un.Object, &secret) if err != nil { diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index d2e3736ea..b79484db1 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -983,6 +983,24 @@ var ( replacement3 = strings.Repeat("+", 16) ) + +func TestHideSecretStringDataInvalidValues(t *testing.T) { + var err error + var configUn unstructured.Unstructured + err = yaml.Unmarshal([]byte(secretInvalidConfig), &configUn) + require.NoError(t, err) + + var liveUn unstructured.Unstructured + err = yaml.Unmarshal([]byte(secretInvalidLive), &liveUn) + require.NoError(t, err) + + target, live, err := HideSecretData(&configUn, &liveUn) + require.NoError(t, err) + + assert.Equal(t, map[string]interface{}{"foo": replacement1}, secretData(target)) + assert.Equal(t, map[string]interface{}{"foo": replacement2}, secretData(live)) +} + func TestHideSecretDataSameKeysDifferentValues(t *testing.T) { target, live, err := HideSecretData( createSecret(map[string]string{"key1": "test", "key2": "test"}), From 4de0eab85bad25dbac9b08062a74d3e4cf597503 Mon Sep 17 00:00:00 2001 From: Josh Hudson <382062+itmustbejj@users.noreply.github.com> Date: Tue, 21 Nov 2023 18:49:41 -0800 Subject: [PATCH 2/7] Return an error from diff.NormalizeSecret to handle HideSecretData calls that would fail silently Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com> --- pkg/diff/diff.go | 55 ++++++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 778fb558c..6c57add0d 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -835,7 +835,10 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) { gvk := un.GroupVersionKind() if gvk.Group == "" && gvk.Kind == "Secret" { - NormalizeSecret(un, opts...) + err := NormalizeSecret(obj) + if err != nil { + return nil, nil, err + } } else if gvk.Group == "rbac.authorization.k8s.io" && (gvk.Kind == "ClusterRole" || gvk.Kind == "Role") { normalizeRole(un, o) } else if gvk.Group == "" && gvk.Kind == "Endpoints" { @@ -849,33 +852,32 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) { } // NormalizeSecret mutates the supplied object and encodes stringData to data, and converts nils to -// empty strings. If the object is not a secret, or is an invalid secret, then returns the same object. -func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) { +// empty strings. Invalid secrets return appropriate errors. If the object is not a secret, or is +// an invalid secret, then returns the same object. +func NormalizeSecret(un *unstructured.Unstructured) (error) { if un == nil { - return + return nil } gvk := un.GroupVersionKind() if gvk.Group != "" || gvk.Kind != "Secret" { - return + return nil + } + if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil { + for k, v := range stringData { + switch v := v.(type) { + case int64: + stringData[k] = toString(v) + } + } + err := unstructured.SetNestedField(un.Object, stringData, "stringData") + if err != nil { + return fmt.Errorf("unstructured.SetNestedField error: %s", err) + } } - o := applyOptions(opts) - if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil { - for k, v := range stringData { - switch v := v.(type) { - case int64: - stringData[k] = toString(v) - } - } - err := unstructured.SetNestedField(un.Object, stringData, "stringData") - if err != nil { - return - } - } var secret corev1.Secret err := runtime.DefaultUnstructuredConverter.FromUnstructured(un.Object, &secret) if err != nil { - o.log.Error(err, "Failed to convert from unstructured into Secret") - return + return fmt.Errorf("Failed to convert from unstructured into Secret: %s", err) } // We normalize nils to empty string to handle: https://github.com/argoproj/argo-cd/issues/943 for k, v := range secret.Data { @@ -894,16 +896,16 @@ func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) { } newObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&secret) if err != nil { - o.log.Error(err, "object unable to convert from secret") - return + return fmt.Errorf("Object unable to convert from Secret: %s", err) } if secret.Data != nil { err = unstructured.SetNestedField(un.Object, newObj["data"], "data") if err != nil { - o.log.Error(err, "failed to set secret.data") - return + return fmt.Errorf("Failed to set Secret data: %s", err) } } + + return nil } // normalizeEndpoint normalizes endpoint meaning that EndpointSubsets are sorted lexicographically @@ -1015,7 +1017,10 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru if obj == nil { continue } - NormalizeSecret(obj) + err := NormalizeSecret(obj) + if err != nil { + return nil, nil, err + } if data, found, err := unstructured.NestedMap(obj.Object, "data"); found && err == nil { for k := range data { keys[k] = true From c97a8600d6f89e106f75ea508812f19c0f88e49c Mon Sep 17 00:00:00 2001 From: Josh Hudson <382062+itmustbejj@users.noreply.github.com> Date: Tue, 21 Nov 2023 18:51:15 -0800 Subject: [PATCH 3/7] Fix bad text fixture data in getLiveSecretJsonBytes Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com> --- pkg/diff/diff_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index b79484db1..ed17e2caa 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -1073,7 +1073,7 @@ func getLiveSecretJsonBytes() []byte { }, "data": { "namespace": "ZGVmYXVsdA==", - "token": "ZGVmYXVsdAcb==" + "token": "ZGVmYXVsdA==" } }`) } From 6888b280fce4a75ec933e09aeb73810102691f36 Mon Sep 17 00:00:00 2001 From: Josh Hudson <382062+itmustbejj@users.noreply.github.com> Date: Tue, 21 Nov 2023 19:44:38 -0800 Subject: [PATCH 4/7] Add error return value for diff.Normalize Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com> --- pkg/diff/diff.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 6c57add0d..8fb3c6039 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -823,9 +823,9 @@ func DiffArray(configArray, liveArray []*unstructured.Unstructured, opts ...Opti return &diffResultList, nil } -func Normalize(un *unstructured.Unstructured, opts ...Option) { +func Normalize(un *unstructured.Unstructured, opts ...Option) (error) { if un == nil { - return + return nil } o := applyOptions(opts) @@ -835,9 +835,9 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) { gvk := un.GroupVersionKind() if gvk.Group == "" && gvk.Kind == "Secret" { - err := NormalizeSecret(obj) + err := NormalizeSecret(un) if err != nil { - return nil, nil, err + return err } } else if gvk.Group == "rbac.authorization.k8s.io" && (gvk.Kind == "ClusterRole" || gvk.Kind == "Role") { normalizeRole(un, o) @@ -849,6 +849,8 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) { if err != nil { o.log.Error(err, fmt.Sprintf("Failed to normalize %s/%s/%s", un.GroupVersionKind(), un.GetNamespace(), un.GetName())) } + + return nil } // NormalizeSecret mutates the supplied object and encodes stringData to data, and converts nils to From 73c233fcecc12637bb75ac8588a58308af18e133 Mon Sep 17 00:00:00 2001 From: Josh Hudson <382062+itmustbejj@users.noreply.github.com> Date: Tue, 21 Nov 2023 20:14:56 -0800 Subject: [PATCH 5/7] Formatting Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com> --- pkg/diff/diff.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 8fb3c6039..e694772a8 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -823,7 +823,7 @@ func DiffArray(configArray, liveArray []*unstructured.Unstructured, opts ...Opti return &diffResultList, nil } -func Normalize(un *unstructured.Unstructured, opts ...Option) (error) { +func Normalize(un *unstructured.Unstructured, opts ...Option) error { if un == nil { return nil } @@ -837,7 +837,7 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) (error) { if gvk.Group == "" && gvk.Kind == "Secret" { err := NormalizeSecret(un) if err != nil { - return err + return err } } else if gvk.Group == "rbac.authorization.k8s.io" && (gvk.Kind == "ClusterRole" || gvk.Kind == "Role") { normalizeRole(un, o) @@ -854,9 +854,9 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) (error) { } // NormalizeSecret mutates the supplied object and encodes stringData to data, and converts nils to -// empty strings. Invalid secrets return appropriate errors. If the object is not a secret, or is +// empty strings. Invalid secrets return appropriate errors. If the object is not a secret, or is // an invalid secret, then returns the same object. -func NormalizeSecret(un *unstructured.Unstructured) (error) { +func NormalizeSecret(un *unstructured.Unstructured) error { if un == nil { return nil } @@ -865,16 +865,16 @@ func NormalizeSecret(un *unstructured.Unstructured) (error) { return nil } if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil { - for k, v := range stringData { - switch v := v.(type) { - case int64: - stringData[k] = toString(v) - } - } - err := unstructured.SetNestedField(un.Object, stringData, "stringData") - if err != nil { - return fmt.Errorf("unstructured.SetNestedField error: %s", err) - } + for k, v := range stringData { + switch v := v.(type) { + case int64: + stringData[k] = toString(v) + } + } + err := unstructured.SetNestedField(un.Object, stringData, "stringData") + if err != nil { + return fmt.Errorf("unstructured.SetNestedField error: %s", err) + } } var secret corev1.Secret err := runtime.DefaultUnstructuredConverter.FromUnstructured(un.Object, &secret) @@ -1021,7 +1021,7 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru } err := NormalizeSecret(obj) if err != nil { - return nil, nil, err + return nil, nil, err } if data, found, err := unstructured.NestedMap(obj.Object, "data"); found && err == nil { for k := range data { From f72530c55fb90f9d6074ef8742146d8cd5a28528 Mon Sep 17 00:00:00 2001 From: Josh Hudson <382062+itmustbejj@users.noreply.github.com> Date: Tue, 21 Nov 2023 20:22:07 -0800 Subject: [PATCH 6/7] Improve cognitive complexity of diff.NormalizeSecret Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com> --- pkg/diff/diff.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index e694772a8..29b1b71e1 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -865,15 +865,9 @@ func NormalizeSecret(un *unstructured.Unstructured) error { return nil } if stringData, found, err := unstructured.NestedMap(un.Object, "stringData"); found && err == nil { - for k, v := range stringData { - switch v := v.(type) { - case int64: - stringData[k] = toString(v) - } - } - err := unstructured.SetNestedField(un.Object, stringData, "stringData") + err := stringifyNestedIntKeys(un, stringData) if err != nil { - return fmt.Errorf("unstructured.SetNestedField error: %s", err) + return err } } var secret corev1.Secret @@ -1133,3 +1127,18 @@ func remarshal(obj *unstructured.Unstructured, o options) *unstructured.Unstruct unstrBody = jsonutil.RemoveMapFields(obj.Object, unstrBody) return &unstructured.Unstructured{Object: unstrBody} } + +func stringifyNestedIntKeys(un *unstructured.Unstructured, stringData map[string]interface{}) error { + for k, v := range stringData { + switch v := v.(type) { + case int64: + stringData[k] = toString(v) + } + } + err := unstructured.SetNestedField(un.Object, stringData, "stringData") + if err != nil { + return fmt.Errorf("unstructured.SetNestedField error: %s", err) + } else { + return nil + } +} From f6aaa0e1d1d465cca2df3421c0c5ba4352dbf403 Mon Sep 17 00:00:00 2001 From: Josh Hudson <382062+itmustbejj@users.noreply.github.com> Date: Tue, 21 Nov 2023 20:49:49 -0800 Subject: [PATCH 7/7] Handle diff.Normalize() retval in function calls Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com> --- pkg/diff/diff.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 29b1b71e1..dd484bf28 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -77,11 +77,17 @@ func Diff(config, live *unstructured.Unstructured, opts ...Option) (*DiffResult, o := applyOptions(opts) if config != nil { config = remarshal(config, o) - Normalize(config, opts...) + err := Normalize(config, opts...) + if err != nil { + return nil, err + } } if live != nil { live = remarshal(live, o) - Normalize(live, opts...) + err := Normalize(live, opts...) + if err != nil { + return nil, err + } } if o.serverSideDiff { @@ -118,7 +124,10 @@ func Diff(config, live *unstructured.Unstructured, opts ...Option) (*DiffResult, o.log.V(1).Info(fmt.Sprintf("Failed to get last applied configuration: %v", err)) } else { if orig != nil && config != nil { - Normalize(orig, opts...) + err := Normalize(orig, opts...) + if err != nil { + return nil, err + } dr, err := ThreeWayDiff(orig, config, live) if err == nil { return dr, nil