fix: apply HideSecretData to server-side diff results for Secrets (cherry-pick argoproj/argo-cd#27598) (#804)

Signed-off-by: Peter Jiang <peterjiang823@gmail.com>

Author: pjiang-dev

pkg/diff/diff.go

@@ -189,13 +189,23 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D
 	Normalize(predictedLive, opts...)
 	unstructured.RemoveNestedField(predictedLive.Object, "metadata", "managedFields")
 
+	Normalize(live, opts...)
+	unstructured.RemoveNestedField(live.Object, "metadata", "managedFields")
+
+	if isCoreSecret(config) {
+		// Mask Secret data symmetrically before comparison.
+		// Equal values get equal placeholders, different values get different placeholders.
+		predictedLive, live, err = HideSecretData(predictedLive, live, nil)
+		if err != nil {
+			return nil, fmt.Errorf("error hiding secret data for resource %s/%s: %w", config.GetKind(), config.GetName(), err)
+		}
+	}
+
 	predictedLiveBytes, err := json.Marshal(predictedLive)
 	if err != nil {
 		return nil, fmt.Errorf("error marshaling predicted live for resource %s/%s: %w", config.GetKind(), config.GetName(), err)
 	}
 
-	Normalize(live, opts...)
-	unstructured.RemoveNestedField(live.Object, "metadata", "managedFields")
 	liveBytes, err := json.Marshal(live)
 	if err != nil {
 		return nil, fmt.Errorf("error marshaling live resource %s/%s: %w", config.GetKind(), config.GetName(), err)
@@ -355,6 +365,15 @@ func jsonStrToUnstructured(jsonString string) (*unstructured.Unstructured, error
 	return &unstructured.Unstructured{Object: res}, nil
 }
 
+// isCoreSecret reports whether obj is a core/v1 Secret (Group="" and Kind="Secret").
+func isCoreSecret(obj *unstructured.Unstructured) bool {
+	if obj == nil {
+		return false
+	}
+	gvk := obj.GroupVersionKind()
+	return gvk.Group == "" && gvk.Kind == "Secret"
+}
+
 // StructuredMergeDiff will calculate the diff using the structured-merge-diff
 // k8s library (https://github.com/kubernetes-sigs/structured-merge-diff).
 func StructuredMergeDiff(config, live *unstructured.Unstructured, gvkParser *managedfields.GvkParser, manager string) (*DiffResult, error) {

pkg/diff/diff_test.go

@@ -1166,6 +1166,126 @@ func TestServerSideDiff(t *testing.T) {
 		assert.Empty(t, predictedDeploy.Annotations[AnnotationLastAppliedConfig])
 		assert.Empty(t, liveDeploy.Annotations[AnnotationLastAppliedConfig])
 	})
+
+	t.Run("will mask Secret data symmetrically so identical values do not produce a spurious diff", func(t *testing.T) {
+		t.Parallel()
+
+		desired := buildSecret("test-secret", "default", map[string]string{"password": "vault:secret/foo"}, nil)
+		live := buildSecret("test-secret", "default", map[string]string{"password": "injected-by-webhook"}, nil)
+		predictedLiveJSON := mustMarshalJSON(t, buildSecret("test-secret", "default", map[string]string{"password": "injected-by-webhook"}, nil))
+
+		opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false))
+		result, err := serverSideDiff(desired, live, opts...)
+		require.NoError(t, err)
+		require.NotNil(t, result)
+
+		assert.False(t, result.Modified, "identical secret values on both sides must not be flagged as modified after masking")
+
+		predictedData := mustGetSecretData(t, result.PredictedLive)
+		liveData := mustGetSecretData(t, result.NormalizedLive)
+		assert.Equal(t, "++++++++", predictedData["password"], "predicted data must be masked, not raw")
+		assert.Equal(t, "++++++++", liveData["password"], "live data must be masked, not raw")
+	})
+
+	t.Run("will keep Secret data masked but still detect genuine value differences", func(t *testing.T) {
+		t.Parallel()
+
+		desired := buildSecret("test-secret", "default", map[string]string{"password": "vault:secret/foo"}, nil)
+		live := buildSecret("test-secret", "default", map[string]string{"password": "old-value"}, nil)
+		predictedLiveJSON := mustMarshalJSON(t, buildSecret("test-secret", "default", map[string]string{"password": "new-value"}, nil))
+
+		opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false))
+		result, err := serverSideDiff(desired, live, opts...)
+		require.NoError(t, err)
+		require.NotNil(t, result)
+
+		assert.True(t, result.Modified, "different secret values must still be flagged as modified")
+
+		predictedData := mustGetSecretData(t, result.PredictedLive)
+		liveData := mustGetSecretData(t, result.NormalizedLive)
+		// HideSecretData yields different placeholder lengths for different values, so the
+		// data field is masked on both sides and the two placeholders differ.
+		assert.NotEqual(t, "new-value", predictedData["password"], "raw new value must not leak into PredictedLive")
+		assert.NotEqual(t, "old-value", liveData["password"], "raw old value must not leak into NormalizedLive")
+		assert.NotEqual(t, predictedData["password"], liveData["password"], "differing values must yield differing placeholders")
+	})
+
+	t.Run("will detect Secret key additions and removals", func(t *testing.T) {
+		t.Parallel()
+
+		desired := buildSecret("test-secret", "default", map[string]string{"password": "x", "token": "y"}, nil)
+		live := buildSecret("test-secret", "default", map[string]string{"password": "x"}, nil)
+		predictedLiveJSON := mustMarshalJSON(t, buildSecret("test-secret", "default", map[string]string{"password": "x", "token": "y"}, nil))
+
+		opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false))
+		result, err := serverSideDiff(desired, live, opts...)
+		require.NoError(t, err)
+		require.NotNil(t, result)
+
+		assert.True(t, result.Modified, "added Secret keys must still be flagged as modified after masking")
+	})
+
+	t.Run("will not mask non-core Secret resources", func(t *testing.T) {
+		// Resources whose Kind is "Secret" but whose Group is non-empty (e.g. CRDs)
+		// must not be touched by the core/v1 Secret masking path.
+		t.Parallel()
+
+		desired := buildSecret("test-secret", "default", map[string]string{"password": "raw-value"}, nil)
+		desired.SetAPIVersion("custom.io/v1")
+		live := buildSecret("test-secret", "default", map[string]string{"password": "raw-value"}, nil)
+		live.SetAPIVersion("custom.io/v1")
+		predictedLiveJSON := mustMarshalJSON(t, desired)
+
+		opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false))
+		result, err := serverSideDiff(desired, live, opts...)
+		require.NoError(t, err)
+		require.NotNil(t, result)
+
+		predictedData := mustGetSecretData(t, result.PredictedLive)
+		assert.Equal(t, "raw-value", predictedData["password"], "non-core Secret data must be left untouched")
+	})
+}
+
+// buildSecret returns a core/v1 Secret as an *unstructured.Unstructured.
+func buildSecret(name, namespace string, data map[string]string, annotations map[string]string) *unstructured.Unstructured {
+	dataField := make(map[string]any, len(data))
+	for k, v := range data {
+		dataField[k] = v
+	}
+	metadata := map[string]any{
+		"name":      name,
+		"namespace": namespace,
+	}
+	if len(annotations) > 0 {
+		annField := make(map[string]any, len(annotations))
+		for k, v := range annotations {
+			annField[k] = v
+		}
+		metadata["annotations"] = annField
+	}
+	return &unstructured.Unstructured{Object: map[string]any{
+		"apiVersion": "v1",
+		"kind":       "Secret",
+		"metadata":   metadata,
+		"type":       "Opaque",
+		"data":       dataField,
+	}}
+}
+
+func mustMarshalJSON(t *testing.T, obj *unstructured.Unstructured) string {
+	t.Helper()
+	bytes, err := json.Marshal(obj)
+	require.NoError(t, err)
+	return string(bytes)
+}
+
+func mustGetSecretData(t *testing.T, secretBytes []byte) map[string]any {
+	t.Helper()
+	var obj map[string]any
+	require.NoError(t, json.Unmarshal(secretBytes, &obj))
+	data, ok := obj["data"].(map[string]any)
+	require.True(t, ok, "expected data field to be a map")
+	return data
 }
 
 // testIgnoreDifferencesNormalizer implements a simple normalizer that removes specified fields