From f3c54dbb4f733b6df2b842668fe882b69a084b8e Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Tue, 14 May 2024 18:43:18 +0530 Subject: [PATCH 1/7] Add option to hide annotations on secrets Signed-off-by: Siddhesh Ghadi --- pkg/diff/diff.go | 35 +++++++++++- pkg/diff/diff_test.go | 126 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 2 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index a45618a14..4ea777b3a 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -969,10 +969,10 @@ func CreateTwoWayMergePatch(orig, new, dataStruct interface{}) ([]byte, bool, er return patch, string(patch) != "{}", nil } -// HideSecretData replaces secret data values in specified target, live secrets and in last applied configuration of live secret with stars. Also preserves differences between +// HideSecretData replaces secret data & optional annotations values in specified target, live secrets and in last applied configuration of live secret with stars. Also preserves differences between // target, live and last applied config values. E.g. if all three are equal the values would be replaced with same number of stars. If all the are different then number of stars // in replacement should be different. -func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured) (*unstructured.Unstructured, *unstructured.Unstructured, error) { +func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured, hideAnnots ...string) (*unstructured.Unstructured, *unstructured.Unstructured, error) { var orig *unstructured.Unstructured if live != nil { orig, _ = GetLastAppliedConfigAnnotation(live) @@ -995,6 +995,7 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru } } + // hide data for k := range keys { // we use "+" rather than the more common "*" nextReplacement := "++++++++" @@ -1037,6 +1038,36 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru } } } + + // hide annotations + for _, k := range hideAnnots { + nextReplacement := "++++++++" + valToReplacement := make(map[string]string) + for _, obj := range []*unstructured.Unstructured{target, live, orig} { + var annots map[string]interface{} + if obj != nil { + annots, _, _ = unstructured.NestedMap(obj.Object, "metadata", "annotations") + } + if annots == nil { + annots = make(map[string]interface{}) + } + val, ok := annots[k] + if !ok { + continue + } + strVal := toString(val) + replacement, ok := valToReplacement[strVal] + if !ok { + replacement = nextReplacement + nextReplacement = nextReplacement + "++++" + valToReplacement[strVal] = replacement + } + annots[k] = replacement + unstructured.SetNestedMap(obj.Object, annots, "metadata", "annotations") + } + } + + // hide last-applied-config annotation if live != nil && orig != nil { annotations := live.GetAnnotations() if annotations == nil { diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index fd2fd754e..b6440b20f 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -1013,6 +1013,132 @@ func TestHideSecretDataDifferentKeysDifferentValues(t *testing.T) { assert.Equal(t, map[string]interface{}{"key2": replacement2, "key3": replacement1}, secretData(live)) } +func TestHideSecretAnnotations(t *testing.T) { + tests := []struct { + name string + hideAnnots []string + annots map[string]interface{} + expectedAnnots map[string]interface{} + targetNil bool + }{ + { + name: "no hidden annotations", + hideAnnots: []string{""}, + annots: map[string]interface{}{"token/value": "secret", "key": "secret-key", "app": "test"}, + expectedAnnots: map[string]interface{}{"token/value": "secret", "key": "secret-key", "app": "test"}, + }, + { + name: "hide annotations", + hideAnnots: []string{"token/value", "key"}, + annots: map[string]interface{}{"token/value": "secret", "key": "secret-key", "app": "test"}, + expectedAnnots: map[string]interface{}{"token/value": replacement1, "key": replacement1, "app": "test"}, + }, + { + name: "hide annotations in last-applied-config", + hideAnnots: []string{"token/value", "key"}, + annots: map[string]interface{}{ + "token/value": "secret", + "app": "test", + "kubectl.kubernetes.io/last-applied-configuration": `{"apiVersion":"v1","kind":"Secret","metadata":{"annotations":{"app":"test","token/value":"secret","key":"secret-key"},"labels":{"app.kubernetes.io/instance":"test"},"name":"my-secret","namespace":"default"},"type":"Opaque"}`, + }, + expectedAnnots: map[string]interface{}{ + "token/value": replacement1, + "app": "test", + "kubectl.kubernetes.io/last-applied-configuration": `{"apiVersion":"v1","kind":"Secret","metadata":{"annotations":{"app":"test","key":"++++++++","token/value":"++++++++"},"labels":{"app.kubernetes.io/instance":"test"},"name":"my-secret","namespace":"default"},"type":"Opaque"}`, + }, + targetNil: true, + }, + { + name: "hide annotations for malformed annotations", + hideAnnots: []string{"token/value", "key"}, + annots: map[string]interface{}{"token/value": 0, "key": "secret", "app": true}, + expectedAnnots: map[string]interface{}{"token/value": replacement1, "key": replacement1, "app": true}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + unSecret := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "test-secret", + "annotations": tt.annots, + }, + "type": "Opaque", + }, + } + + liveUn := remarshal(unSecret, applyOptions(diffOptionsForTest())) + targetUn := remarshal(unSecret, applyOptions(diffOptionsForTest())) + + if tt.targetNil { + targetUn = nil + } + + target, live, err := HideSecretData(targetUn, liveUn, tt.hideAnnots...) + require.NoError(t, err) + + // verify configured annotations are hidden + for _, obj := range []*unstructured.Unstructured{target, live} { + if obj != nil { + annots, _, _ := unstructured.NestedMap(obj.Object, "metadata", "annotations") + for ek, ev := range tt.expectedAnnots { + v, found := annots[ek] + assert.True(t, found) + assert.Equal(t, ev, v) + } + } + } + }) + } +} + +func TestHideSecretAnnotationsPreserveDifference(t *testing.T) { + hideAnnots := []string{"token/value"} + + liveUn := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "test-secret", + "annotations": map[string]interface{}{"token/value": "secret", "app": "test"}, + }, + "type": "Opaque", + }, + } + targetUn := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "test-secret", + "annotations": map[string]interface{}{"token/value": "new-secret", "app": "test"}, + }, + "type": "Opaque", + }, + } + + liveUn = remarshal(liveUn, applyOptions(diffOptionsForTest())) + targetUn = remarshal(targetUn, applyOptions(diffOptionsForTest())) + + target, live, err := HideSecretData(targetUn, liveUn, hideAnnots...) + require.NoError(t, err) + + liveAnnots := live.GetAnnotations() + v, found := liveAnnots["token/value"] + assert.True(t, found) + assert.Equal(t, replacement2, v) + + targetAnnots := target.GetAnnotations() + v, found = targetAnnots["token/value"] + assert.True(t, found) + assert.Equal(t, replacement1, v) +} + func getTargetSecretJsonBytes() []byte { return []byte(` { From acd76f2dc041532535eb15a5613662cdabcde869 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Tue, 14 May 2024 22:57:06 +0530 Subject: [PATCH 2/7] Handle err Signed-off-by: Siddhesh Ghadi --- pkg/diff/diff.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 4ea777b3a..eb66d3a0c 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -1045,8 +1045,12 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru valToReplacement := make(map[string]string) for _, obj := range []*unstructured.Unstructured{target, live, orig} { var annots map[string]interface{} + var err error if obj != nil { - annots, _, _ = unstructured.NestedMap(obj.Object, "metadata", "annotations") + annots, _, err = unstructured.NestedMap(obj.Object, "metadata", "annotations") + if err != nil { + return nil, nil, fmt.Errorf("unstructured.NestedMap error: %s", err) + } } if annots == nil { annots = make(map[string]interface{}) @@ -1063,7 +1067,10 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru valToReplacement[strVal] = replacement } annots[k] = replacement - unstructured.SetNestedMap(obj.Object, annots, "metadata", "annotations") + err = unstructured.SetNestedMap(obj.Object, annots, "metadata", "annotations") + if err != nil { + return nil, nil, fmt.Errorf("unstructured.SetNestedField error: %s", err) + } } } From 6395c8993e4d922273a294888b7a015ac6680d76 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Mon, 8 Jul 2024 17:52:00 +0530 Subject: [PATCH 3/7] Move hide logic to a generic func Signed-off-by: Siddhesh Ghadi --- pkg/diff/diff.go | 106 ++++++++++++++------------------- pkg/diff/diff_test.go | 32 ++++++---- pkg/utils/kube/resource_ops.go | 2 +- 3 files changed, 66 insertions(+), 74 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index eb66d3a0c..1cf6027a1 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -972,7 +972,7 @@ func CreateTwoWayMergePatch(orig, new, dataStruct interface{}) ([]byte, bool, er // HideSecretData replaces secret data & optional annotations values in specified target, live secrets and in last applied configuration of live secret with stars. Also preserves differences between // target, live and last applied config values. E.g. if all three are equal the values would be replaced with same number of stars. If all the are different then number of stars // in replacement should be different. -func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured, hideAnnots ...string) (*unstructured.Unstructured, *unstructured.Unstructured, error) { +func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured, hideAnnots map[string]bool) (*unstructured.Unstructured, *unstructured.Unstructured, error) { var orig *unstructured.Unstructured if live != nil { orig, _ = GetLastAppliedConfigAnnotation(live) @@ -995,7 +995,36 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru } } + var err error // hide data + target, live, orig, err = hide(target, live, orig, keys, "data") + if err != nil { + return nil, nil, err + } + + // hide annotations + target, live, orig, err = hide(target, live, orig, hideAnnots, "metadata", "annotations") + if err != nil { + return nil, nil, err + } + + // hide last-applied-config annotation + if live != nil && orig != nil { + annotations := live.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + } + lastAppliedData, err := json.Marshal(orig) + if err != nil { + return nil, nil, fmt.Errorf("error marshaling json: %s", err) + } + annotations[corev1.LastAppliedConfigAnnotation] = string(lastAppliedData) + live.SetAnnotations(annotations) + } + return target, live, nil +} + +func hide(target, live, orig *unstructured.Unstructured, keys map[string]bool, fields ...string) (*unstructured.Unstructured, *unstructured.Unstructured, *unstructured.Unstructured, error) { for k := range keys { // we use "+" rather than the more common "*" nextReplacement := "++++++++" @@ -1005,16 +1034,14 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru if obj != nil { // handles an edge case when secret data has nil value // https://github.com/argoproj/argo-cd/issues/5584 - dataValue, ok := obj.Object["data"] - if ok { - if dataValue == nil { - continue - } + dataValue, ok, _ := unstructured.NestedFieldCopy(obj.Object, fields...) + if !ok || dataValue == nil { + continue } var err error - data, _, err = unstructured.NestedMap(obj.Object, "data") + data, _, err = unstructured.NestedMap(obj.Object, fields...) if err != nil { - return nil, nil, fmt.Errorf("unstructured.NestedMap error: %s", err) + return nil, nil, nil, fmt.Errorf("unstructured.NestedMap error: %s", err) } } if data == nil { @@ -1032,62 +1059,13 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru valToReplacement[val] = replacement } data[k] = replacement - err := unstructured.SetNestedField(obj.Object, data, "data") - if err != nil { - return nil, nil, fmt.Errorf("unstructured.SetNestedField error: %s", err) - } - } - } - - // hide annotations - for _, k := range hideAnnots { - nextReplacement := "++++++++" - valToReplacement := make(map[string]string) - for _, obj := range []*unstructured.Unstructured{target, live, orig} { - var annots map[string]interface{} - var err error - if obj != nil { - annots, _, err = unstructured.NestedMap(obj.Object, "metadata", "annotations") - if err != nil { - return nil, nil, fmt.Errorf("unstructured.NestedMap error: %s", err) - } - } - if annots == nil { - annots = make(map[string]interface{}) - } - val, ok := annots[k] - if !ok { - continue - } - strVal := toString(val) - replacement, ok := valToReplacement[strVal] - if !ok { - replacement = nextReplacement - nextReplacement = nextReplacement + "++++" - valToReplacement[strVal] = replacement - } - annots[k] = replacement - err = unstructured.SetNestedMap(obj.Object, annots, "metadata", "annotations") + err := unstructured.SetNestedField(obj.Object, data, fields...) if err != nil { - return nil, nil, fmt.Errorf("unstructured.SetNestedField error: %s", err) + return nil, nil, nil, fmt.Errorf("unstructured.SetNestedField error: %s", err) } } } - - // hide last-applied-config annotation - if live != nil && orig != nil { - annotations := live.GetAnnotations() - if annotations == nil { - annotations = make(map[string]string) - } - lastAppliedData, err := json.Marshal(orig) - if err != nil { - return nil, nil, fmt.Errorf("error marshaling json: %s", err) - } - annotations[corev1.LastAppliedConfigAnnotation] = string(lastAppliedData) - live.SetAnnotations(annotations) - } - return target, live, nil + return target, live, orig, nil } func toString(val interface{}) string { @@ -1097,6 +1075,14 @@ func toString(val interface{}) string { return fmt.Sprintf("%s", val) } +func convertSliceToMap(s []string) map[string]bool { + m := make(map[string]bool) + for _, k := range s { + m[k] = true + } + return m +} + // remarshal checks resource kind and version and re-marshal using corresponding struct custom marshaller. // This ensures that expected resource state is formatter same as actual resource state in kubernetes // and allows to find differences between actual and target states more accurately. diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index b6440b20f..542c6cef4 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -986,7 +986,9 @@ var ( func TestHideSecretDataSameKeysDifferentValues(t *testing.T) { target, live, err := HideSecretData( createSecret(map[string]string{"key1": "test", "key2": "test"}), - createSecret(map[string]string{"key1": "test-1", "key2": "test-1"})) + createSecret(map[string]string{"key1": "test-1", "key2": "test-1"}), + nil, + ) require.NoError(t, err) assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1}, secretData(target)) @@ -996,7 +998,9 @@ func TestHideSecretDataSameKeysDifferentValues(t *testing.T) { func TestHideSecretDataSameKeysSameValues(t *testing.T) { target, live, err := HideSecretData( createSecret(map[string]string{"key1": "test", "key2": "test"}), - createSecret(map[string]string{"key1": "test", "key2": "test"})) + createSecret(map[string]string{"key1": "test", "key2": "test"}), + nil, + ) require.NoError(t, err) assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1}, secretData(target)) @@ -1006,7 +1010,9 @@ func TestHideSecretDataSameKeysSameValues(t *testing.T) { func TestHideSecretDataDifferentKeysDifferentValues(t *testing.T) { target, live, err := HideSecretData( createSecret(map[string]string{"key1": "test", "key2": "test"}), - createSecret(map[string]string{"key2": "test-1", "key3": "test-1"})) + createSecret(map[string]string{"key2": "test-1", "key3": "test-1"}), + nil, + ) require.NoError(t, err) assert.Equal(t, map[string]interface{}{"key1": replacement1, "key2": replacement1}, secretData(target)) @@ -1016,26 +1022,26 @@ func TestHideSecretDataDifferentKeysDifferentValues(t *testing.T) { func TestHideSecretAnnotations(t *testing.T) { tests := []struct { name string - hideAnnots []string + hideAnnots map[string]bool annots map[string]interface{} expectedAnnots map[string]interface{} targetNil bool }{ { name: "no hidden annotations", - hideAnnots: []string{""}, + hideAnnots: nil, annots: map[string]interface{}{"token/value": "secret", "key": "secret-key", "app": "test"}, expectedAnnots: map[string]interface{}{"token/value": "secret", "key": "secret-key", "app": "test"}, }, { name: "hide annotations", - hideAnnots: []string{"token/value", "key"}, + hideAnnots: map[string]bool{"token/value": true, "key": true}, annots: map[string]interface{}{"token/value": "secret", "key": "secret-key", "app": "test"}, expectedAnnots: map[string]interface{}{"token/value": replacement1, "key": replacement1, "app": "test"}, }, { name: "hide annotations in last-applied-config", - hideAnnots: []string{"token/value", "key"}, + hideAnnots: map[string]bool{"token/value": true, "key": true}, annots: map[string]interface{}{ "token/value": "secret", "app": "test", @@ -1050,7 +1056,7 @@ func TestHideSecretAnnotations(t *testing.T) { }, { name: "hide annotations for malformed annotations", - hideAnnots: []string{"token/value", "key"}, + hideAnnots: map[string]bool{"token/value": true, "key": true}, annots: map[string]interface{}{"token/value": 0, "key": "secret", "app": true}, expectedAnnots: map[string]interface{}{"token/value": replacement1, "key": replacement1, "app": true}, }, @@ -1078,7 +1084,7 @@ func TestHideSecretAnnotations(t *testing.T) { targetUn = nil } - target, live, err := HideSecretData(targetUn, liveUn, tt.hideAnnots...) + target, live, err := HideSecretData(targetUn, liveUn, tt.hideAnnots) require.NoError(t, err) // verify configured annotations are hidden @@ -1097,7 +1103,7 @@ func TestHideSecretAnnotations(t *testing.T) { } func TestHideSecretAnnotationsPreserveDifference(t *testing.T) { - hideAnnots := []string{"token/value"} + hideAnnots := map[string]bool{"token/value": true} liveUn := &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -1125,7 +1131,7 @@ func TestHideSecretAnnotationsPreserveDifference(t *testing.T) { liveUn = remarshal(liveUn, applyOptions(diffOptionsForTest())) targetUn = remarshal(targetUn, applyOptions(diffOptionsForTest())) - target, live, err := HideSecretData(targetUn, liveUn, hideAnnots...) + target, live, err := HideSecretData(targetUn, liveUn, hideAnnots) require.NoError(t, err) liveAnnots := live.GetAnnotations() @@ -1204,7 +1210,7 @@ func TestHideSecretDataHandleEmptySecret(t *testing.T) { liveSecret := bytesToUnstructured(t, getLiveSecretJsonBytes()) // when - target, live, err := HideSecretData(targetSecret, liveSecret) + target, live, err := HideSecretData(targetSecret, liveSecret, nil) // then assert.NoError(t, err) @@ -1222,7 +1228,7 @@ func TestHideSecretDataLastAppliedConfig(t *testing.T) { require.NoError(t, err) liveSecret.SetAnnotations(map[string]string{corev1.LastAppliedConfigAnnotation: string(lastAppliedStr)}) - target, live, err := HideSecretData(targetSecret, liveSecret) + target, live, err := HideSecretData(targetSecret, liveSecret, nil) require.NoError(t, err) err = json.Unmarshal([]byte(live.GetAnnotations()[corev1.LastAppliedConfigAnnotation]), &lastAppliedSecret) require.NoError(t, err) diff --git a/pkg/utils/kube/resource_ops.go b/pkg/utils/kube/resource_ops.go index 00d8a05fa..a7a0c419d 100644 --- a/pkg/utils/kube/resource_ops.go +++ b/pkg/utils/kube/resource_ops.go @@ -80,7 +80,7 @@ func (k *kubectlResourceOperations) runResourceCommand(ctx context.Context, obj if err != nil { return "", err } - redacted, _, err := diff.HideSecretData(&obj, nil) + redacted, _, err := diff.HideSecretData(&obj, nil, nil) if err != nil { return "", err } From 2dd3ac24b0a92e08175da09297603e61be47d69a Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Mon, 8 Jul 2024 18:09:51 +0530 Subject: [PATCH 4/7] Remove test code Signed-off-by: Siddhesh Ghadi --- pkg/diff/diff.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 1cf6027a1..1769bdef5 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -1075,14 +1075,6 @@ func toString(val interface{}) string { return fmt.Sprintf("%s", val) } -func convertSliceToMap(s []string) map[string]bool { - m := make(map[string]bool) - for _, k := range s { - m[k] = true - } - return m -} - // remarshal checks resource kind and version and re-marshal using corresponding struct custom marshaller. // This ensures that expected resource state is formatter same as actual resource state in kubernetes // and allows to find differences between actual and target states more accurately. From e3e911ab003cf8429426ae261921727c50e1cca7 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Fri, 20 Sep 2024 09:13:17 +0530 Subject: [PATCH 5/7] Address review comments Signed-off-by: Siddhesh Ghadi --- pkg/diff/diff.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 1769bdef5..762061930 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -969,10 +969,10 @@ func CreateTwoWayMergePatch(orig, new, dataStruct interface{}) ([]byte, bool, er return patch, string(patch) != "{}", nil } -// HideSecretData replaces secret data & optional annotations values in specified target, live secrets and in last applied configuration of live secret with stars. Also preserves differences between -// target, live and last applied config values. E.g. if all three are equal the values would be replaced with same number of stars. If all the are different then number of stars +// HideSecretData replaces secret data & optional annotations values in specified target, live secrets and in last applied configuration of live secret with plus(+). Also preserves differences between +// target, live and last applied config values. E.g. if all three are equal the values would be replaced with same number of plus(+). If all are different then number of plus(+) // in replacement should be different. -func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured, hideAnnots map[string]bool) (*unstructured.Unstructured, *unstructured.Unstructured, error) { +func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured, hideAnnotations map[string]bool) (*unstructured.Unstructured, *unstructured.Unstructured, error) { var orig *unstructured.Unstructured if live != nil { orig, _ = GetLastAppliedConfigAnnotation(live) @@ -1003,7 +1003,7 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru } // hide annotations - target, live, orig, err = hide(target, live, orig, hideAnnots, "metadata", "annotations") + target, live, orig, err = hide(target, live, orig, hideAnnotations, "metadata", "annotations") if err != nil { return nil, nil, err } From 916f64e5c4458653614c7548c7507c11221ad8dc Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Tue, 24 Sep 2024 16:45:55 +0530 Subject: [PATCH 6/7] Handle lastAppliedConfig special case Signed-off-by: Siddhesh Ghadi --- pkg/diff/diff.go | 34 ++++++++++++++++++++-------------- pkg/diff/diff_test.go | 14 ++++++++++++++ 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 762061930..a9ab609fc 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -35,6 +35,7 @@ import ( const ( couldNotMarshalErrMsg = "Could not unmarshal to object of type %s: %v" AnnotationLastAppliedConfig = "kubectl.kubernetes.io/last-applied-configuration" + replacement = "++++++++" ) // Holds diffing result of two resources @@ -973,9 +974,9 @@ func CreateTwoWayMergePatch(orig, new, dataStruct interface{}) ([]byte, bool, er // target, live and last applied config values. E.g. if all three are equal the values would be replaced with same number of plus(+). If all are different then number of plus(+) // in replacement should be different. func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured, hideAnnotations map[string]bool) (*unstructured.Unstructured, *unstructured.Unstructured, error) { - var orig *unstructured.Unstructured + var liveLastAppliedAnnotation *unstructured.Unstructured if live != nil { - orig, _ = GetLastAppliedConfigAnnotation(live) + liveLastAppliedAnnotation, _ = GetLastAppliedConfigAnnotation(live) live = live.DeepCopy() } if target != nil { @@ -983,7 +984,7 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru } keys := map[string]bool{} - for _, obj := range []*unstructured.Unstructured{target, live, orig} { + for _, obj := range []*unstructured.Unstructured{target, live, liveLastAppliedAnnotation} { if obj == nil { continue } @@ -997,39 +998,44 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru var err error // hide data - target, live, orig, err = hide(target, live, orig, keys, "data") + target, live, liveLastAppliedAnnotation, err = hide(target, live, liveLastAppliedAnnotation, keys, "data") if err != nil { return nil, nil, err } // hide annotations - target, live, orig, err = hide(target, live, orig, hideAnnotations, "metadata", "annotations") + target, live, liveLastAppliedAnnotation, err = hide(target, live, liveLastAppliedAnnotation, hideAnnotations, "metadata", "annotations") if err != nil { return nil, nil, err } // hide last-applied-config annotation - if live != nil && orig != nil { + if live != nil && liveLastAppliedAnnotation != nil { annotations := live.GetAnnotations() if annotations == nil { annotations = make(map[string]string) } - lastAppliedData, err := json.Marshal(orig) - if err != nil { - return nil, nil, fmt.Errorf("error marshaling json: %s", err) + // special case: hide "kubectl.kubernetes.io/last-applied-configuration" annotation + if _, ok := hideAnnotations[corev1.LastAppliedConfigAnnotation]; ok { + annotations[corev1.LastAppliedConfigAnnotation] = replacement + } else { + lastAppliedData, err := json.Marshal(liveLastAppliedAnnotation) + if err != nil { + return nil, nil, fmt.Errorf("error marshaling json: %s", err) + } + annotations[corev1.LastAppliedConfigAnnotation] = string(lastAppliedData) } - annotations[corev1.LastAppliedConfigAnnotation] = string(lastAppliedData) live.SetAnnotations(annotations) } return target, live, nil } -func hide(target, live, orig *unstructured.Unstructured, keys map[string]bool, fields ...string) (*unstructured.Unstructured, *unstructured.Unstructured, *unstructured.Unstructured, error) { +func hide(target, live, liveLastAppliedAnnotation *unstructured.Unstructured, keys map[string]bool, fields ...string) (*unstructured.Unstructured, *unstructured.Unstructured, *unstructured.Unstructured, error) { for k := range keys { // we use "+" rather than the more common "*" - nextReplacement := "++++++++" + nextReplacement := replacement valToReplacement := make(map[string]string) - for _, obj := range []*unstructured.Unstructured{target, live, orig} { + for _, obj := range []*unstructured.Unstructured{target, live, liveLastAppliedAnnotation} { var data map[string]interface{} if obj != nil { // handles an edge case when secret data has nil value @@ -1065,7 +1071,7 @@ func hide(target, live, orig *unstructured.Unstructured, keys map[string]bool, f } } } - return target, live, orig, nil + return target, live, liveLastAppliedAnnotation, nil } func toString(val interface{}) string { diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index 542c6cef4..806919a1c 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -1054,6 +1054,20 @@ func TestHideSecretAnnotations(t *testing.T) { }, targetNil: true, }, + { + name: "special case: hide last-applied-config annotation", + hideAnnots: map[string]bool{"kubectl.kubernetes.io/last-applied-configuration": true}, + annots: map[string]interface{}{ + "token/value": replacement1, + "app": "test", + "kubectl.kubernetes.io/last-applied-configuration": `{"apiVersion":"v1","kind":"Secret","metadata":{"annotations":{"app":"test","token/value":"secret","key":"secret-key"},"labels":{"app.kubernetes.io/instance":"test"},"name":"my-secret","namespace":"default"},"type":"Opaque"}`, + }, + expectedAnnots: map[string]interface{}{ + "app": "test", + "kubectl.kubernetes.io/last-applied-configuration": replacement1, + }, + targetNil: true, + }, { name: "hide annotations for malformed annotations", hideAnnots: map[string]bool{"token/value": true, "key": true}, From 5c87732b878dbaca73ac4161841ae93a4b88469f Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Wed, 23 Oct 2024 20:49:12 +0530 Subject: [PATCH 7/7] Fix if logic and remove comments Signed-off-by: Siddhesh Ghadi --- pkg/diff/diff.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index a9ab609fc..1b43ea0ce 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -997,19 +997,16 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru } var err error - // hide data target, live, liveLastAppliedAnnotation, err = hide(target, live, liveLastAppliedAnnotation, keys, "data") if err != nil { return nil, nil, err } - // hide annotations target, live, liveLastAppliedAnnotation, err = hide(target, live, liveLastAppliedAnnotation, hideAnnotations, "metadata", "annotations") if err != nil { return nil, nil, err } - // hide last-applied-config annotation if live != nil && liveLastAppliedAnnotation != nil { annotations := live.GetAnnotations() if annotations == nil { @@ -1041,8 +1038,10 @@ func hide(target, live, liveLastAppliedAnnotation *unstructured.Unstructured, ke // handles an edge case when secret data has nil value // https://github.com/argoproj/argo-cd/issues/5584 dataValue, ok, _ := unstructured.NestedFieldCopy(obj.Object, fields...) - if !ok || dataValue == nil { - continue + if ok { + if dataValue == nil { + continue + } } var err error data, _, err = unstructured.NestedMap(obj.Object, fields...)