feat: Add ability to hide certain annotations on secret resources (#577)

* Add option to hide annotations on secrets Signed-off-by: Siddhesh Ghadi <[email protected]> * Handle err Signed-off-by: Siddhesh Ghadi <[email protected]> * Move hide logic to a generic func Signed-off-by: Siddhesh Ghadi <[email protected]> * Remove test code Signed-off-by: Siddhesh Ghadi <[email protected]> * Address review comments Signed-off-by: Siddhesh Ghadi <[email protected]> * Handle lastAppliedConfig special case Signed-off-by: Siddhesh Ghadi <[email protected]> * Fix if logic and remove comments Signed-off-by: Siddhesh Ghadi <[email protected]> --------- Signed-off-by: Siddhesh Ghadi <[email protected]>
argoproj · Oct 29, 2024 · 9ab0b2e · 9ab0b2e
1 parent 09e5225
commit 9ab0b2e
Show file tree

Hide file tree

Showing 3 changed files with 199 additions and 32 deletions.
diff --git 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
@@ -969,21 +970,21 @@ 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
-// 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) (*unstructured.Unstructured, *unstructured.Unstructured, error) {
-	var orig *unstructured.Unstructured
+func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstructured, hideAnnotations map[string]bool) (*unstructured.Unstructured, *unstructured.Unstructured, error) {
+	var liveLastAppliedAnnotation *unstructured.Unstructured
 	if live != nil {
-		orig, _ = GetLastAppliedConfigAnnotation(live)
+		liveLastAppliedAnnotation, _ = GetLastAppliedConfigAnnotation(live)
 		live = live.DeepCopy()
 	}
 	if target != nil {
 		target = target.DeepCopy()
 	}
 
 	keys := map[string]bool{}
-	for _, obj := range []*unstructured.Unstructured{target, live, orig} {
+	for _, obj := range []*unstructured.Unstructured{target, live, liveLastAppliedAnnotation} {
 		if obj == nil {
 			continue
 		}
@@ -995,25 +996,57 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru
 		}
 	}
 
+	var err error
+	target, live, liveLastAppliedAnnotation, err = hide(target, live, liveLastAppliedAnnotation, keys, "data")
+	if err != nil {
+		return nil, nil, err
+	}
+
+	target, live, liveLastAppliedAnnotation, err = hide(target, live, liveLastAppliedAnnotation, hideAnnotations, "metadata", "annotations")
+	if err != nil {
+		return nil, nil, err
+	}
+
+	if live != nil && liveLastAppliedAnnotation != nil {
+		annotations := live.GetAnnotations()
+		if annotations == nil {
+			annotations = make(map[string]string)
+		}
+		// 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)
+		}
+		live.SetAnnotations(annotations)
+	}
+	return target, live, nil
+}
+
+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
 				// https://github.com/argoproj/argo-cd/issues/5584
-				dataValue, ok := obj.Object["data"]
+				dataValue, ok, _ := unstructured.NestedFieldCopy(obj.Object, fields...)
 				if ok {
 					if 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 {
@@ -1031,25 +1064,13 @@ func HideSecretData(target *unstructured.Unstructured, live *unstructured.Unstru
 				valToReplacement[val] = replacement
 			}
 			data[k] = replacement
-			err := unstructured.SetNestedField(obj.Object, data, "data")
+			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)
 			}
 		}
 	}
-	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, liveLastAppliedAnnotation, nil
 }
 
 func toString(val interface{}) string {

diff --git 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,13 +1010,155 @@ 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))
 	assert.Equal(t, map[string]interface{}{"key2": replacement2, "key3": replacement1}, secretData(live))
 }
 
+func TestHideSecretAnnotations(t *testing.T) {
+	tests := []struct {
+		name           string
+		hideAnnots     map[string]bool
+		annots         map[string]interface{}
+		expectedAnnots map[string]interface{}
+		targetNil      bool
+	}{
+		{
+			name:           "no hidden annotations",
+			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:     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: map[string]bool{"token/value": true, "key": true},
+			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:       "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},
+			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 := map[string]bool{"token/value": true}
+
+	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(`
 {
@@ -1078,7 +1224,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)
@@ -1096,7 +1242,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
@@ -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
 		}