From c890fb5ce61474bf4bc5e2bc64a7823f366bcd78 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Wed, 12 Nov 2025 12:28:16 +0900 Subject: [PATCH 01/18] fix: normalize secrets in server-side diff Signed-off-by: downfa11 --- controller/sync.go | 45 ++++++++++- controller/sync_test.go | 166 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 1 deletion(-) diff --git a/controller/sync.go b/controller/sync.go index 9afbbe6ba83af..575102eff623d 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -2,6 +2,7 @@ package controller import ( "context" + "encoding/json" stderrors "errors" "fmt" "os" @@ -13,6 +14,8 @@ import ( cdcommon "github.com/argoproj/argo-cd/v3/common" gitopsDiff "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" + cmdutil "k8s.io/kubectl/pkg/cmd/util" + "github.com/argoproj/argo-cd/gitops-engine/pkg/sync" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync/common" "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube" @@ -78,7 +81,47 @@ func (m *appStateManager) getServerSideDiffDryRunApplier(cluster *v1alpha1.Clust if err != nil { return nil, nil, fmt.Errorf("error creating kubectl ResourceOperations: %w", err) } - return ops, cleanup, nil + + wrappedOps := &secretNormalizingApplier{ + inner: ops, + settingsMgr: m.settingsMgr, + } + + return wrappedOps, cleanup, nil +} + +// secretNormalizingApplier wraps a KubeApplier to normalize Secret data +type secretNormalizingApplier struct { + inner gitopsDiff.KubeApplier + settingsMgr *settings.SettingsManager +} + +func (s *secretNormalizingApplier) ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { + result, err := s.inner.ApplyResource(ctx, obj, dryRunStrategy, force, validate, serverSideApply, manager) + if err != nil { + return result, err + } + + resultObj := &unstructured.Unstructured{} + if err := json.Unmarshal([]byte(result), resultObj); err != nil { + return result, err + } + + // Apply hideSecretData + if resultObj.GetKind() == kube.SecretKind && resultObj.GroupVersionKind().Group == "" { + _, normalized, err := gitopsDiff.HideSecretData(nil, resultObj, s.settingsMgr.GetSensitiveAnnotations()) + if err != nil { + return result, fmt.Errorf("error normalizing secret data: %w", err) + } + + normalizedBytes, err := json.Marshal(normalized) + if err != nil { + return result, fmt.Errorf("error marshaling normalized secret: %w", err) + } + return string(normalizedBytes), nil + } + + return result, nil } func NewOperationState(operation v1alpha1.Operation) *v1alpha1.OperationState { diff --git a/controller/sync_test.go b/controller/sync_test.go index 4a450eb80a70c..6679ab5bebd3f 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1,12 +1,18 @@ package controller import ( + "context" + "encoding/base64" + "encoding/json" + "os" "strconv" "testing" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync" synccommon "github.com/argoproj/argo-cd/gitops-engine/pkg/sync/common" "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube" + "github.com/ghodss/yaml" + openapi_v2 "github.com/google/gnostic-models/openapiv2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -23,8 +29,35 @@ import ( "github.com/argoproj/argo-cd/v3/util/argo/diff" "github.com/argoproj/argo-cd/v3/util/argo/normalizers" "github.com/argoproj/argo-cd/v3/util/settings" + + gitopsDiff "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" + kubefake "k8s.io/client-go/kubernetes/fake" + cmdutil "k8s.io/kubectl/pkg/cmd/util" ) +type fakeDiscovery struct { + schema *openapi_v2.Document +} + +func (f *fakeDiscovery) OpenAPISchema() (*openapi_v2.Document, error) { + return f.schema, nil +} + +func loadCRDSchema(t *testing.T, path string) *openapi_v2.Document { + t.Helper() + + data, err := os.ReadFile(path) + require.NoError(t, err) + + jsonData, err := yaml.YAMLToJSON(data) + require.NoError(t, err) + + doc, err := openapi_v2.ParseDocument(jsonData) + require.NoError(t, err) + + return doc +} + func TestPersistRevisionHistory(t *testing.T) { app := newFakeApp() app.Status.OperationState = nil @@ -1768,3 +1801,136 @@ func TestValidateSyncPermissions(t *testing.T) { assert.NoError(t, err) }) } + +func TestSecretNormalizingApplier(t *testing.T) { + kubeclientset := kubefake.NewSimpleClientset(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "argocd", + Name: "argocd-cm", + Labels: map[string]string{ + "app.kubernetes.io/part-of": "argocd", + }, + }, + }, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argocd-secret", + Namespace: "argocd", + }, + }) + + ctx := context.Background() + settingsMgr := settings.NewSettingsManager(ctx, kubeclientset, "argocd") + + desired := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "test-secret", + "namespace": "default", + }, + "type": "Opaque", + "data": map[string]interface{}{ + "password": base64.StdEncoding.EncodeToString([]byte("vault:test/data/test#TEST")), + }, + }, + } + + live := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "name": "test-secret", + "namespace": "default", + }, + "type": "Opaque", + "data": map[string]interface{}{ + "password": base64.StdEncoding.EncodeToString([]byte("actual-password-from-vault")), + }, + }, + } + + mockApplier := &simpleKubeApplier{ + applyResult: func(obj *unstructured.Unstructured) string { + if obj.GetKind() == "Secret" { + result := live.DeepCopy() + bytes, _ := json.Marshal(result) + return string(bytes) + } + bytes, _ := json.Marshal(obj) + return string(bytes) + }, + } + + normalizer := &secretNormalizingApplier{ + inner: mockApplier, + settingsMgr: settingsMgr, + } + + result, err := normalizer.ApplyResource( + context.Background(), + desired, + cmdutil.DryRunServer, + false, false, true, "argocd", + ) + + require.NoError(t, err) + assert.Contains(t, result, "Secret") + + // verify that the result is normalized + resultObj := &unstructured.Unstructured{} + err = json.Unmarshal([]byte(result), resultObj) + require.NoError(t, err) + + assert.Equal(t, "Secret", resultObj.GetKind()) + assert.Equal(t, "test-secret", resultObj.GetName()) + + // verify that HideSecretData was applied + data, found, err := unstructured.NestedMap(resultObj.Object, "data") + require.NoError(t, err) + assert.True(t, found, "data field should exist") + + // password should exist, but may be normalized + _, passwordExists := data["password"] + assert.True(t, passwordExists, "password should exist after normalization") + + // normalize both using HideSecretData + _, normalizedGit, err := gitopsDiff.HideSecretData(nil, desired, settingsMgr.GetSensitiveAnnotations()) + require.NoError(t, err) + + _, normalizedResult, err := gitopsDiff.HideSecretData(nil, resultObj, settingsMgr.GetSensitiveAnnotations()) + require.NoError(t, err) + + // diff between normalized secrets + diffResult, _ := gitopsDiff.Diff(normalizedGit, normalizedResult) + + // verify that there is NO diff after normalization (mutation webhook changes don't cause OutOfSync) + assert.False(t, diffResult.Modified, "normalized secrets should not show as modified") + if diffResult.Modified { + t.Logf("NormalizedLive: %s", string(diffResult.NormalizedLive)) + t.Logf("PredictedLive: %s", string(diffResult.PredictedLive)) + } + + // both have the same structure + gitData, _, _ := unstructured.NestedMap(normalizedGit.Object, "data") + resultData, _, _ := unstructured.NestedMap(normalizedResult.Object, "data") + + // should have password after normalization + _, gitHasPassword := gitData["password"] + _, resultHasPassword := resultData["password"] + assert.True(t, gitHasPassword, "Git secret should have password field after normalization") + assert.True(t, resultHasPassword, "Result secret should have password field after normalization") +} + +type simpleKubeApplier struct { + applyResult func(*unstructured.Unstructured) string +} + +func (s *simpleKubeApplier) ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { + return s.applyResult(obj), nil +} + +func (s *simpleKubeApplier) ConvertToVersion(obj *unstructured.Unstructured, group, version string) (*unstructured.Unstructured, error) { + return obj, nil +} From 6f30f1924172a3d58c644c00b3dce00bd445b8d1 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Wed, 12 Nov 2025 12:48:45 +0900 Subject: [PATCH 02/18] refactor: resolved lint issues Signed-off-by: downfa11 --- controller/sync_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 6679ab5bebd3f..536e5d85e73ff 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1818,7 +1818,7 @@ func TestSecretNormalizingApplier(t *testing.T) { }, }) - ctx := context.Background() + ctx := t.Context() settingsMgr := settings.NewSettingsManager(ctx, kubeclientset, "argocd") desired := &unstructured.Unstructured{ @@ -1869,7 +1869,7 @@ func TestSecretNormalizingApplier(t *testing.T) { } result, err := normalizer.ApplyResource( - context.Background(), + t.Context(), desired, cmdutil.DryRunServer, false, false, true, "argocd", @@ -1927,10 +1927,10 @@ type simpleKubeApplier struct { applyResult func(*unstructured.Unstructured) string } -func (s *simpleKubeApplier) ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { +func (s *simpleKubeApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { return s.applyResult(obj), nil } -func (s *simpleKubeApplier) ConvertToVersion(obj *unstructured.Unstructured, group, version string) (*unstructured.Unstructured, error) { +func (s *simpleKubeApplier) ConvertToVersion(obj *unstructured.Unstructured, _, version string) (*unstructured.Unstructured, error) { return obj, nil } From 79220c93bec5e10029132a2873773394220a9b37 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Wed, 12 Nov 2025 13:04:14 +0900 Subject: [PATCH 03/18] refactor: lint Signed-off-by: downfa11 --- controller/sync_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 536e5d85e73ff..3c61184059162 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1822,30 +1822,30 @@ func TestSecretNormalizingApplier(t *testing.T) { settingsMgr := settings.NewSettingsManager(ctx, kubeclientset, "argocd") desired := &unstructured.Unstructured{ - Object: map[string]interface{}{ + Object: map[string]any{ "apiVersion": "v1", "kind": "Secret", - "metadata": map[string]interface{}{ + "metadata": map[string]any{ "name": "test-secret", "namespace": "default", }, "type": "Opaque", - "data": map[string]interface{}{ + "data": map[string]any{ "password": base64.StdEncoding.EncodeToString([]byte("vault:test/data/test#TEST")), }, }, } live := &unstructured.Unstructured{ - Object: map[string]interface{}{ + Object: map[string]any{ "apiVersion": "v1", "kind": "Secret", - "metadata": map[string]interface{}{ + "metadata": map[string]any{ "name": "test-secret", "namespace": "default", }, "type": "Opaque", - "data": map[string]interface{}{ + "data": map[string]any{ "password": base64.StdEncoding.EncodeToString([]byte("actual-password-from-vault")), }, }, @@ -1927,10 +1927,10 @@ type simpleKubeApplier struct { applyResult func(*unstructured.Unstructured) string } -func (s *simpleKubeApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { +func (s *simpleKubeApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, _ cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { return s.applyResult(obj), nil } -func (s *simpleKubeApplier) ConvertToVersion(obj *unstructured.Unstructured, _, version string) (*unstructured.Unstructured, error) { +func (s *simpleKubeApplier) ConvertToVersion(obj *unstructured.Unstructured, _, _ string) (*unstructured.Unstructured, error) { return obj, nil } From 140f67420b33cd54e56a28931125d411b10e9161 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Wed, 12 Nov 2025 13:11:17 +0900 Subject: [PATCH 04/18] refactor: resolved lint issues in sync_test Signed-off-by: downfa11 --- controller/sync_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 3c61184059162..e6a820e42a81e 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1927,7 +1927,7 @@ type simpleKubeApplier struct { applyResult func(*unstructured.Unstructured) string } -func (s *simpleKubeApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, _ cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { +func (s *simpleKubeApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, _ cmdutil.DryRunStrategy, _, _, _ bool, _ string) (string, error) { return s.applyResult(obj), nil } From 7cab42da3df6ce1f476a830f942ebc55ea308404 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Tue, 6 Jan 2026 19:05:52 +0900 Subject: [PATCH 05/18] fix: Improve secret normalization handling Signed-off-by: downfa11 --- controller/sync.go | 16 ++++- controller/sync_test.go | 132 ++++++++++++++++++++++++++-------------- 2 files changed, 99 insertions(+), 49 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 575102eff623d..d2298d9a199ac 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -102,14 +102,24 @@ func (s *secretNormalizingApplier) ApplyResource(ctx context.Context, obj *unstr return result, err } + isCoreSecret := obj.GetKind() == kube.SecretKind && (obj.GroupVersionKind().Group == "" || obj.GroupVersionKind().Group == "core") + if dryRunStrategy != cmdutil.DryRunServer || !isCoreSecret { + return result, nil + } + + trimmedResult := strings.TrimSpace(result) + if !strings.HasPrefix(trimmedResult, "{") { + return result, nil + } + resultObj := &unstructured.Unstructured{} if err := json.Unmarshal([]byte(result), resultObj); err != nil { - return result, err + return result, nil } // Apply hideSecretData - if resultObj.GetKind() == kube.SecretKind && resultObj.GroupVersionKind().Group == "" { - _, normalized, err := gitopsDiff.HideSecretData(nil, resultObj, s.settingsMgr.GetSensitiveAnnotations()) + if resultObj.GetKind() == kube.SecretKind && (resultObj.GroupVersionKind().Group == "" || resultObj.GroupVersionKind().Group == "core") { + _, normalized, err := gitopsDiff.HideSecretData(obj, resultObj, s.settingsMgr.GetSensitiveAnnotations()) if err != nil { return result, fmt.Errorf("error normalizing secret data: %w", err) } diff --git a/controller/sync_test.go b/controller/sync_test.go index e6a820e42a81e..2813e42734609 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1868,59 +1868,103 @@ func TestSecretNormalizingApplier(t *testing.T) { settingsMgr: settingsMgr, } - result, err := normalizer.ApplyResource( - t.Context(), - desired, - cmdutil.DryRunServer, - false, false, true, "argocd", - ) + t.Run("core v1 secret is normalized during dry run", func(t *testing.T) { + result, err := normalizer.ApplyResource( + ctx, + desired, + cmdutil.DryRunServer, + false, false, true, "argocd", + ) + require.NoError(t, err) + assert.Contains(t, result, "Secret") - require.NoError(t, err) - assert.Contains(t, result, "Secret") + // verify that the result is normalized + resultObj := &unstructured.Unstructured{} + err = json.Unmarshal([]byte(result), resultObj) + require.NoError(t, err) - // verify that the result is normalized - resultObj := &unstructured.Unstructured{} - err = json.Unmarshal([]byte(result), resultObj) - require.NoError(t, err) + assert.Equal(t, "Secret", resultObj.GetKind()) + assert.Equal(t, "test-secret", resultObj.GetName()) - assert.Equal(t, "Secret", resultObj.GetKind()) - assert.Equal(t, "test-secret", resultObj.GetName()) + // verify that HideSecretData was applied + data, found, err := unstructured.NestedMap(resultObj.Object, "data") + require.NoError(t, err) + assert.True(t, found, "data field should exist") - // verify that HideSecretData was applied - data, found, err := unstructured.NestedMap(resultObj.Object, "data") - require.NoError(t, err) - assert.True(t, found, "data field should exist") + // password should exist, but may be normalized + _, passwordExists := data["password"] + assert.True(t, passwordExists, "password should exist after normalization") + assert.NotEqual(t, live.Object["data"], data, "secret data should be normalized/masked") - // password should exist, but may be normalized - _, passwordExists := data["password"] - assert.True(t, passwordExists, "password should exist after normalization") + // normalize both using HideSecretData + _, normalizedGit, err := gitopsDiff.HideSecretData(nil, desired, settingsMgr.GetSensitiveAnnotations()) + require.NoError(t, err) - // normalize both using HideSecretData - _, normalizedGit, err := gitopsDiff.HideSecretData(nil, desired, settingsMgr.GetSensitiveAnnotations()) - require.NoError(t, err) + _, normalizedResult, err := gitopsDiff.HideSecretData(nil, resultObj, settingsMgr.GetSensitiveAnnotations()) + require.NoError(t, err) - _, normalizedResult, err := gitopsDiff.HideSecretData(nil, resultObj, settingsMgr.GetSensitiveAnnotations()) - require.NoError(t, err) + // diff between normalized secrets + diffResult, _ := gitopsDiff.Diff(normalizedGit, normalizedResult) - // diff between normalized secrets - diffResult, _ := gitopsDiff.Diff(normalizedGit, normalizedResult) + // verify that there is NO diff after normalization (mutation webhook changes don't cause OutOfSync) + assert.False(t, diffResult.Modified, "normalized secrets should not show as modified") + if diffResult.Modified { + t.Logf("NormalizedLive: %s", string(diffResult.NormalizedLive)) + t.Logf("PredictedLive: %s", string(diffResult.PredictedLive)) + } - // verify that there is NO diff after normalization (mutation webhook changes don't cause OutOfSync) - assert.False(t, diffResult.Modified, "normalized secrets should not show as modified") - if diffResult.Modified { - t.Logf("NormalizedLive: %s", string(diffResult.NormalizedLive)) - t.Logf("PredictedLive: %s", string(diffResult.PredictedLive)) - } + // both have the same structure + gitData, _, _ := unstructured.NestedMap(normalizedGit.Object, "data") + resultData, _, _ := unstructured.NestedMap(normalizedResult.Object, "data") + + // should have password after normalization + _, gitHasPassword := gitData["password"] + _, resultHasPassword := resultData["password"] + assert.True(t, gitHasPassword, "git secret should have password field after normalization") + assert.True(t, resultHasPassword, "result secret should have password field after normalization") + }) - // both have the same structure - gitData, _, _ := unstructured.NestedMap(normalizedGit.Object, "data") - resultData, _, _ := unstructured.NestedMap(normalizedResult.Object, "data") + t.Run("non-json result is returned unchanged", func(t *testing.T) { + nonJSON := "kubectl applied (dry-run)" + mockApplier.applyResult = func(obj *unstructured.Unstructured) string { + return nonJSON + } + + result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunServer, false, false, true, "argocd") + assert.NoError(t, err) + assert.Equal(t, nonJSON, result, "wrapper should return non-json result without error") + }) - // should have password after normalization - _, gitHasPassword := gitData["password"] - _, resultHasPassword := resultData["password"] - assert.True(t, gitHasPassword, "Git secret should have password field after normalization") - assert.True(t, resultHasPassword, "Result secret should have password field after normalization") + t.Run("non-core group secret is not normalized", func(t *testing.T) { + customSecret := desired.DeepCopy() + customSecret.SetAPIVersion("custom.io/v1") + + mockApplier.applyResult = func(obj *unstructured.Unstructured) string { + bytes, _ := json.Marshal(obj) + return string(bytes) + } + + result, err := normalizer.ApplyResource(ctx, customSecret, cmdutil.DryRunServer, false, false, true, "argocd") + assert.NoError(t, err) + + resultObj := &unstructured.Unstructured{} + json.Unmarshal([]byte(result), resultObj) + assert.Equal(t, customSecret.Object["data"], resultObj.Object["data"], "non-core secret should not be modified") + }) + + t.Run("normalization is skipped for non-dry run server", func(t *testing.T) { + mockApplier.applyResult = func(obj *unstructured.Unstructured) string { + bytes, _ := json.Marshal(live) + return string(bytes) + } + + result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunNone, false, false, true, "argocd") + assert.NoError(t, err) + + resultObj := &unstructured.Unstructured{} + json.Unmarshal([]byte(result), resultObj) + assert.Equal(t, live.Object["data"], resultObj.Object["data"]) + }) } type simpleKubeApplier struct { @@ -1930,7 +1974,3 @@ type simpleKubeApplier struct { func (s *simpleKubeApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, _ cmdutil.DryRunStrategy, _, _, _ bool, _ string) (string, error) { return s.applyResult(obj), nil } - -func (s *simpleKubeApplier) ConvertToVersion(obj *unstructured.Unstructured, _, _ string) (*unstructured.Unstructured, error) { - return obj, nil -} From 60aba21de4f95d5627559d73309099fc236c9d66 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Tue, 6 Jan 2026 20:35:38 +0900 Subject: [PATCH 06/18] fix: resolved lint issues Signed-off-by: downfa11 --- controller/sync_test.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 2813e42734609..90fdacc06e298 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1925,14 +1925,14 @@ func TestSecretNormalizingApplier(t *testing.T) { }) t.Run("non-json result is returned unchanged", func(t *testing.T) { - nonJSON := "kubectl applied (dry-run)" - mockApplier.applyResult = func(obj *unstructured.Unstructured) string { + nonJSON := "kubectl applied dry-run" + mockApplier.applyResult = func(_ *unstructured.Unstructured) string { return nonJSON } result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunServer, false, false, true, "argocd") - assert.NoError(t, err) - assert.Equal(t, nonJSON, result, "wrapper should return non-json result without error") + require.NoError(t, err) + assert.Equal(t, nonJSON, result, "wrapper should return non-json result without error") //nolint:testifylint }) t.Run("non-core group secret is not normalized", func(t *testing.T) { @@ -1945,24 +1945,28 @@ func TestSecretNormalizingApplier(t *testing.T) { } result, err := normalizer.ApplyResource(ctx, customSecret, cmdutil.DryRunServer, false, false, true, "argocd") - assert.NoError(t, err) + require.NoError(t, err) resultObj := &unstructured.Unstructured{} - json.Unmarshal([]byte(result), resultObj) + err = json.Unmarshal([]byte(result), resultObj) + require.NoError(t, err) + assert.Equal(t, customSecret.Object["data"], resultObj.Object["data"], "non-core secret should not be modified") }) t.Run("normalization is skipped for non-dry run server", func(t *testing.T) { - mockApplier.applyResult = func(obj *unstructured.Unstructured) string { + mockApplier.applyResult = func(_ *unstructured.Unstructured) string { bytes, _ := json.Marshal(live) return string(bytes) } result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunNone, false, false, true, "argocd") - assert.NoError(t, err) + require.NoError(t, err) resultObj := &unstructured.Unstructured{} - json.Unmarshal([]byte(result), resultObj) + err = json.Unmarshal([]byte(result), resultObj) + require.NoError(t, err) + assert.Equal(t, live.Object["data"], resultObj.Object["data"]) }) } From b2a513d8be189170474e4189c9c632aa9b955d78 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Thu, 16 Apr 2026 23:13:45 +0900 Subject: [PATCH 07/18] chore: resolved minor issues Signed-off-by: downfa11 --- controller/sync.go | 1 + go.mod | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index d2298d9a199ac..004fff7059ce2 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "strconv" + "strings" "time" "k8s.io/apimachinery/pkg/util/strategicpatch" diff --git a/go.mod b/go.mod index 5df3a17880df4..9eee2f2af89bb 100644 --- a/go.mod +++ b/go.mod @@ -48,7 +48,7 @@ require ( github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 github.com/golang/protobuf v1.5.4 github.com/google/btree v1.1.3 - github.com/google/gnostic-models v0.7.0 // indirect + github.com/google/gnostic-models v0.7.0 github.com/google/go-cmp v0.7.0 github.com/google/go-github/v69 v69.2.0 github.com/google/go-jsonnet v0.22.0 @@ -182,7 +182,7 @@ require ( github.com/fatih/camelcase v1.0.0 // indirect github.com/fatih/color v1.18.0 // indirect github.com/fxamacker/cbor/v2 v2.9.0 // indirect - github.com/ghodss/yaml v1.0.0 // indirect + github.com/ghodss/yaml v1.0.0 github.com/go-errors/errors v1.5.1 // indirect github.com/go-fed/httpsig v1.1.0 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect From 6468240d01c828a0ad0b3190955a5ae64cc3f72e Mon Sep 17 00:00:00 2001 From: downfa11 Date: Thu, 16 Apr 2026 23:39:45 +0900 Subject: [PATCH 08/18] chore: resolved issues Signed-off-by: downfa11 --- controller/sync_test.go | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 90fdacc06e298..5c995bc1d44b1 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -4,15 +4,12 @@ import ( "context" "encoding/base64" "encoding/json" - "os" "strconv" "testing" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync" synccommon "github.com/argoproj/argo-cd/gitops-engine/pkg/sync/common" "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube" - "github.com/ghodss/yaml" - openapi_v2 "github.com/google/gnostic-models/openapiv2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -35,28 +32,6 @@ import ( cmdutil "k8s.io/kubectl/pkg/cmd/util" ) -type fakeDiscovery struct { - schema *openapi_v2.Document -} - -func (f *fakeDiscovery) OpenAPISchema() (*openapi_v2.Document, error) { - return f.schema, nil -} - -func loadCRDSchema(t *testing.T, path string) *openapi_v2.Document { - t.Helper() - - data, err := os.ReadFile(path) - require.NoError(t, err) - - jsonData, err := yaml.YAMLToJSON(data) - require.NoError(t, err) - - doc, err := openapi_v2.ParseDocument(jsonData) - require.NoError(t, err) - - return doc -} func TestPersistRevisionHistory(t *testing.T) { app := newFakeApp() From eeaa39810244d7f37007f012154956feedf8b893 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Thu, 16 Apr 2026 23:48:02 +0900 Subject: [PATCH 09/18] chore: resolved go.mod issues Signed-off-by: downfa11 --- go.mod | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 9eee2f2af89bb..5df3a17880df4 100644 --- a/go.mod +++ b/go.mod @@ -48,7 +48,7 @@ require ( github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 github.com/golang/protobuf v1.5.4 github.com/google/btree v1.1.3 - github.com/google/gnostic-models v0.7.0 + github.com/google/gnostic-models v0.7.0 // indirect github.com/google/go-cmp v0.7.0 github.com/google/go-github/v69 v69.2.0 github.com/google/go-jsonnet v0.22.0 @@ -182,7 +182,7 @@ require ( github.com/fatih/camelcase v1.0.0 // indirect github.com/fatih/color v1.18.0 // indirect github.com/fxamacker/cbor/v2 v2.9.0 // indirect - github.com/ghodss/yaml v1.0.0 + github.com/ghodss/yaml v1.0.0 // indirect github.com/go-errors/errors v1.5.1 // indirect github.com/go-fed/httpsig v1.1.0 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect From d818657448095afbbd90d143c792356c4b416a8c Mon Sep 17 00:00:00 2001 From: downfa11 Date: Thu, 16 Apr 2026 23:57:09 +0900 Subject: [PATCH 10/18] chore: go format Signed-off-by: downfa11 --- controller/sync_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 5c995bc1d44b1..629fcc0a7635d 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -32,7 +32,6 @@ import ( cmdutil "k8s.io/kubectl/pkg/cmd/util" ) - func TestPersistRevisionHistory(t *testing.T) { app := newFakeApp() app.Status.OperationState = nil From cb35d882efcaa8f6ebdeed5ecb48ae020343b505 Mon Sep 17 00:00:00 2001 From: downfa11 Date: Sat, 18 Apr 2026 01:11:15 +0900 Subject: [PATCH 11/18] fix: resolved review issues Signed-off-by: downfa11 --- controller/sync.go | 14 +++--- controller/sync_test.go | 109 ++++++++-------------------------------- 2 files changed, 27 insertions(+), 96 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 004fff7059ce2..f0e412e8560de 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -84,8 +84,8 @@ func (m *appStateManager) getServerSideDiffDryRunApplier(cluster *v1alpha1.Clust } wrappedOps := &secretNormalizingApplier{ - inner: ops, - settingsMgr: m.settingsMgr, + inner: ops, + sensitiveAnnotations: m.settingsMgr.GetSensitiveAnnotations(), } return wrappedOps, cleanup, nil @@ -93,8 +93,8 @@ func (m *appStateManager) getServerSideDiffDryRunApplier(cluster *v1alpha1.Clust // secretNormalizingApplier wraps a KubeApplier to normalize Secret data type secretNormalizingApplier struct { - inner gitopsDiff.KubeApplier - settingsMgr *settings.SettingsManager + inner gitopsDiff.KubeApplier + sensitiveAnnotations map[string]bool } func (s *secretNormalizingApplier) ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { @@ -103,7 +103,7 @@ func (s *secretNormalizingApplier) ApplyResource(ctx context.Context, obj *unstr return result, err } - isCoreSecret := obj.GetKind() == kube.SecretKind && (obj.GroupVersionKind().Group == "" || obj.GroupVersionKind().Group == "core") + isCoreSecret := obj.GetKind() == kube.SecretKind && obj.GroupVersionKind().Group == "" if dryRunStrategy != cmdutil.DryRunServer || !isCoreSecret { return result, nil } @@ -119,8 +119,8 @@ func (s *secretNormalizingApplier) ApplyResource(ctx context.Context, obj *unstr } // Apply hideSecretData - if resultObj.GetKind() == kube.SecretKind && (resultObj.GroupVersionKind().Group == "" || resultObj.GroupVersionKind().Group == "core") { - _, normalized, err := gitopsDiff.HideSecretData(obj, resultObj, s.settingsMgr.GetSensitiveAnnotations()) + if resultObj.GetKind() == kube.SecretKind && resultObj.GroupVersionKind().Group == "" { + _, normalized, err := gitopsDiff.HideSecretData(obj, resultObj, s.sensitiveAnnotations) if err != nil { return result, fmt.Errorf("error normalizing secret data: %w", err) } diff --git a/controller/sync_test.go b/controller/sync_test.go index 629fcc0a7635d..d02cf9d0ad1f7 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -27,8 +27,6 @@ import ( "github.com/argoproj/argo-cd/v3/util/argo/normalizers" "github.com/argoproj/argo-cd/v3/util/settings" - gitopsDiff "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" - kubefake "k8s.io/client-go/kubernetes/fake" cmdutil "k8s.io/kubectl/pkg/cmd/util" ) @@ -1777,69 +1775,40 @@ func TestValidateSyncPermissions(t *testing.T) { } func TestSecretNormalizingApplier(t *testing.T) { - kubeclientset := kubefake.NewSimpleClientset(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "argocd", - Name: "argocd-cm", - Labels: map[string]string{ - "app.kubernetes.io/part-of": "argocd", - }, - }, - }, &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "argocd-secret", - Namespace: "argocd", - }, - }) - ctx := t.Context() - settingsMgr := settings.NewSettingsManager(ctx, kubeclientset, "argocd") - desired := &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]any{ - "name": "test-secret", - "namespace": "default", - }, - "type": "Opaque", - "data": map[string]any{ - "password": base64.StdEncoding.EncodeToString([]byte("vault:test/data/test#TEST")), - }, - }, + sensitiveAnnots := map[string]bool{ + "my-custom-sensitive-field": true, } - live := &unstructured.Unstructured{ + desired := &unstructured.Unstructured{ Object: map[string]any{ "apiVersion": "v1", "kind": "Secret", "metadata": map[string]any{ "name": "test-secret", "namespace": "default", + "annotations": map[string]any{ + "my-custom-sensitive-field": "very-secret-value", + }, }, "type": "Opaque", "data": map[string]any{ - "password": base64.StdEncoding.EncodeToString([]byte("actual-password-from-vault")), + "password": base64.StdEncoding.EncodeToString([]byte("actual-password")), }, }, } mockApplier := &simpleKubeApplier{ applyResult: func(obj *unstructured.Unstructured) string { - if obj.GetKind() == "Secret" { - result := live.DeepCopy() - bytes, _ := json.Marshal(result) - return string(bytes) - } bytes, _ := json.Marshal(obj) return string(bytes) }, } normalizer := &secretNormalizingApplier{ - inner: mockApplier, - settingsMgr: settingsMgr, + inner: mockApplier, + sensitiveAnnotations: sensitiveAnnots, } t.Run("core v1 secret is normalized during dry run", func(t *testing.T) { @@ -1850,52 +1819,16 @@ func TestSecretNormalizingApplier(t *testing.T) { false, false, true, "argocd", ) require.NoError(t, err) - assert.Contains(t, result, "Secret") - // verify that the result is normalized resultObj := &unstructured.Unstructured{} err = json.Unmarshal([]byte(result), resultObj) require.NoError(t, err) - assert.Equal(t, "Secret", resultObj.GetKind()) - assert.Equal(t, "test-secret", resultObj.GetName()) - - // verify that HideSecretData was applied - data, found, err := unstructured.NestedMap(resultObj.Object, "data") - require.NoError(t, err) - assert.True(t, found, "data field should exist") - - // password should exist, but may be normalized - _, passwordExists := data["password"] - assert.True(t, passwordExists, "password should exist after normalization") - assert.NotEqual(t, live.Object["data"], data, "secret data should be normalized/masked") + data, _, _ := unstructured.NestedMap(resultObj.Object, "data") + assert.Equal(t, "++++++++", data["password"], "Secret data should be masked with asterisks") - // normalize both using HideSecretData - _, normalizedGit, err := gitopsDiff.HideSecretData(nil, desired, settingsMgr.GetSensitiveAnnotations()) - require.NoError(t, err) - - _, normalizedResult, err := gitopsDiff.HideSecretData(nil, resultObj, settingsMgr.GetSensitiveAnnotations()) - require.NoError(t, err) - - // diff between normalized secrets - diffResult, _ := gitopsDiff.Diff(normalizedGit, normalizedResult) - - // verify that there is NO diff after normalization (mutation webhook changes don't cause OutOfSync) - assert.False(t, diffResult.Modified, "normalized secrets should not show as modified") - if diffResult.Modified { - t.Logf("NormalizedLive: %s", string(diffResult.NormalizedLive)) - t.Logf("PredictedLive: %s", string(diffResult.PredictedLive)) - } - - // both have the same structure - gitData, _, _ := unstructured.NestedMap(normalizedGit.Object, "data") - resultData, _, _ := unstructured.NestedMap(normalizedResult.Object, "data") - - // should have password after normalization - _, gitHasPassword := gitData["password"] - _, resultHasPassword := resultData["password"] - assert.True(t, gitHasPassword, "git secret should have password field after normalization") - assert.True(t, resultHasPassword, "result secret should have password field after normalization") + annotations := resultObj.GetAnnotations() + assert.Equal(t, "++++++++", annotations["my-custom-sensitive-field"], "Sensitive annotations should be masked") }) t.Run("non-json result is returned unchanged", func(t *testing.T) { @@ -1906,12 +1839,12 @@ func TestSecretNormalizingApplier(t *testing.T) { result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunServer, false, false, true, "argocd") require.NoError(t, err) - assert.Equal(t, nonJSON, result, "wrapper should return non-json result without error") //nolint:testifylint + assert.Equal(t, nonJSON, result) }) t.Run("non-core group secret is not normalized", func(t *testing.T) { customSecret := desired.DeepCopy() - customSecret.SetAPIVersion("custom.io/v1") + customSecret.SetAPIVersion("custom.io/v1") // Group이 ""이 아님 mockApplier.applyResult = func(obj *unstructured.Unstructured) string { bytes, _ := json.Marshal(obj) @@ -1925,15 +1858,12 @@ func TestSecretNormalizingApplier(t *testing.T) { err = json.Unmarshal([]byte(result), resultObj) require.NoError(t, err) - assert.Equal(t, customSecret.Object["data"], resultObj.Object["data"], "non-core secret should not be modified") + // 데이터가 변하지 않았음을 확인 + assert.Equal(t, customSecret.Object["data"], resultObj.Object["data"]) + assert.Equal(t, "very-secret-value", resultObj.GetAnnotations()["my-custom-sensitive-field"]) }) t.Run("normalization is skipped for non-dry run server", func(t *testing.T) { - mockApplier.applyResult = func(_ *unstructured.Unstructured) string { - bytes, _ := json.Marshal(live) - return string(bytes) - } - result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunNone, false, false, true, "argocd") require.NoError(t, err) @@ -1941,7 +1871,8 @@ func TestSecretNormalizingApplier(t *testing.T) { err = json.Unmarshal([]byte(result), resultObj) require.NoError(t, err) - assert.Equal(t, live.Object["data"], resultObj.Object["data"]) + // DryRun이 아니면 마스킹 되지 않아야 함 + assert.Equal(t, desired.Object["data"], resultObj.Object["data"]) }) } From 73ddf77a408e250ece36504f55b14f87c27e56dd Mon Sep 17 00:00:00 2001 From: downfa11 Date: Sat, 18 Apr 2026 01:22:08 +0900 Subject: [PATCH 12/18] chore: resolved minor issues Signed-off-by: downfa11 --- controller/sync_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index d02cf9d0ad1f7..208f61739c741 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1839,12 +1839,12 @@ func TestSecretNormalizingApplier(t *testing.T) { result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunServer, false, false, true, "argocd") require.NoError(t, err) - assert.Equal(t, nonJSON, result) + assert.Equal(t, nonJSON, result) //nolint:testifylint }) t.Run("non-core group secret is not normalized", func(t *testing.T) { customSecret := desired.DeepCopy() - customSecret.SetAPIVersion("custom.io/v1") // Group이 ""이 아님 + customSecret.SetAPIVersion("custom.io/v1") mockApplier.applyResult = func(obj *unstructured.Unstructured) string { bytes, _ := json.Marshal(obj) @@ -1858,7 +1858,6 @@ func TestSecretNormalizingApplier(t *testing.T) { err = json.Unmarshal([]byte(result), resultObj) require.NoError(t, err) - // 데이터가 변하지 않았음을 확인 assert.Equal(t, customSecret.Object["data"], resultObj.Object["data"]) assert.Equal(t, "very-secret-value", resultObj.GetAnnotations()["my-custom-sensitive-field"]) }) @@ -1871,7 +1870,6 @@ func TestSecretNormalizingApplier(t *testing.T) { err = json.Unmarshal([]byte(result), resultObj) require.NoError(t, err) - // DryRun이 아니면 마스킹 되지 않아야 함 assert.Equal(t, desired.Object["data"], resultObj.Object["data"]) }) } From b656d9676f5ad01ff9300944551701b98edfbf0d Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Wed, 29 Apr 2026 12:03:05 -0700 Subject: [PATCH 13/18] move secret normalizer to caller instead of applier Signed-off-by: Peter Jiang --- controller/state.go | 6 +++++- controller/sync.go | 10 ++-------- controller/sync_test.go | 10 ---------- 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/controller/state.go b/controller/state.go index d354ef68b8b9d..94c232c85ca9a 100644 --- a/controller/state.go +++ b/controller/state.go @@ -841,7 +841,11 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionUnknownError, Message: err.Error(), LastTransitionTime: &now}) } else { defer cleanup() - diffConfigBuilder.WithServerSideDryRunner(diff.NewK8sServerSideDryRunner(applier)) + normalizedApplier := &secretNormalizingApplier{ + inner: applier, + sensitiveAnnotations: m.settingsMgr.GetSensitiveAnnotations(), + } + diffConfigBuilder.WithServerSideDryRunner(diff.NewK8sServerSideDryRunner(normalizedApplier)) } } diff --git a/controller/sync.go b/controller/sync.go index f0e412e8560de..6d71862dae5d5 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -83,12 +83,7 @@ func (m *appStateManager) getServerSideDiffDryRunApplier(cluster *v1alpha1.Clust return nil, nil, fmt.Errorf("error creating kubectl ResourceOperations: %w", err) } - wrappedOps := &secretNormalizingApplier{ - inner: ops, - sensitiveAnnotations: m.settingsMgr.GetSensitiveAnnotations(), - } - - return wrappedOps, cleanup, nil + return ops, cleanup, nil } // secretNormalizingApplier wraps a KubeApplier to normalize Secret data @@ -103,8 +98,7 @@ func (s *secretNormalizingApplier) ApplyResource(ctx context.Context, obj *unstr return result, err } - isCoreSecret := obj.GetKind() == kube.SecretKind && obj.GroupVersionKind().Group == "" - if dryRunStrategy != cmdutil.DryRunServer || !isCoreSecret { + if obj.GetKind() != kube.SecretKind || obj.GroupVersionKind().Group != "" { return result, nil } diff --git a/controller/sync_test.go b/controller/sync_test.go index 208f61739c741..a885ad3a15fd4 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1862,16 +1862,6 @@ func TestSecretNormalizingApplier(t *testing.T) { assert.Equal(t, "very-secret-value", resultObj.GetAnnotations()["my-custom-sensitive-field"]) }) - t.Run("normalization is skipped for non-dry run server", func(t *testing.T) { - result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunNone, false, false, true, "argocd") - require.NoError(t, err) - - resultObj := &unstructured.Unstructured{} - err = json.Unmarshal([]byte(result), resultObj) - require.NoError(t, err) - - assert.Equal(t, desired.Object["data"], resultObj.Object["data"]) - }) } type simpleKubeApplier struct { From a207d1bd5312192797217dcc90e760c613a50525 Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Wed, 29 Apr 2026 12:15:16 -0700 Subject: [PATCH 14/18] lint Signed-off-by: Peter Jiang --- controller/sync_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index a885ad3a15fd4..5fc2d4448449c 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1861,7 +1861,6 @@ func TestSecretNormalizingApplier(t *testing.T) { assert.Equal(t, customSecret.Object["data"], resultObj.Object["data"]) assert.Equal(t, "very-secret-value", resultObj.GetAnnotations()["my-custom-sensitive-field"]) }) - } type simpleKubeApplier struct { From 6464c62f557a66dd1d4acb6f4f5273afd0e1edbc Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Wed, 29 Apr 2026 14:37:26 -0700 Subject: [PATCH 15/18] move HideSecretData to diff logic Signed-off-by: Peter Jiang --- controller/state.go | 6 +- controller/sync.go | 46 ----------- controller/sync_test.go | 101 ----------------------- gitops-engine/pkg/diff/diff.go | 33 +++++++- gitops-engine/pkg/diff/diff_test.go | 119 ++++++++++++++++++++++++++++ 5 files changed, 150 insertions(+), 155 deletions(-) diff --git a/controller/state.go b/controller/state.go index 94c232c85ca9a..d354ef68b8b9d 100644 --- a/controller/state.go +++ b/controller/state.go @@ -841,11 +841,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionUnknownError, Message: err.Error(), LastTransitionTime: &now}) } else { defer cleanup() - normalizedApplier := &secretNormalizingApplier{ - inner: applier, - sensitiveAnnotations: m.settingsMgr.GetSensitiveAnnotations(), - } - diffConfigBuilder.WithServerSideDryRunner(diff.NewK8sServerSideDryRunner(normalizedApplier)) + diffConfigBuilder.WithServerSideDryRunner(diff.NewK8sServerSideDryRunner(applier)) } } diff --git a/controller/sync.go b/controller/sync.go index 6d71862dae5d5..c6f1c2edd27c7 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -2,12 +2,10 @@ package controller import ( "context" - "encoding/json" stderrors "errors" "fmt" "os" "strconv" - "strings" "time" "k8s.io/apimachinery/pkg/util/strategicpatch" @@ -15,7 +13,6 @@ import ( cdcommon "github.com/argoproj/argo-cd/v3/common" gitopsDiff "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" - cmdutil "k8s.io/kubectl/pkg/cmd/util" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync/common" @@ -86,49 +83,6 @@ func (m *appStateManager) getServerSideDiffDryRunApplier(cluster *v1alpha1.Clust return ops, cleanup, nil } -// secretNormalizingApplier wraps a KubeApplier to normalize Secret data -type secretNormalizingApplier struct { - inner gitopsDiff.KubeApplier - sensitiveAnnotations map[string]bool -} - -func (s *secretNormalizingApplier) ApplyResource(ctx context.Context, obj *unstructured.Unstructured, dryRunStrategy cmdutil.DryRunStrategy, force, validate, serverSideApply bool, manager string) (string, error) { - result, err := s.inner.ApplyResource(ctx, obj, dryRunStrategy, force, validate, serverSideApply, manager) - if err != nil { - return result, err - } - - if obj.GetKind() != kube.SecretKind || obj.GroupVersionKind().Group != "" { - return result, nil - } - - trimmedResult := strings.TrimSpace(result) - if !strings.HasPrefix(trimmedResult, "{") { - return result, nil - } - - resultObj := &unstructured.Unstructured{} - if err := json.Unmarshal([]byte(result), resultObj); err != nil { - return result, nil - } - - // Apply hideSecretData - if resultObj.GetKind() == kube.SecretKind && resultObj.GroupVersionKind().Group == "" { - _, normalized, err := gitopsDiff.HideSecretData(obj, resultObj, s.sensitiveAnnotations) - if err != nil { - return result, fmt.Errorf("error normalizing secret data: %w", err) - } - - normalizedBytes, err := json.Marshal(normalized) - if err != nil { - return result, fmt.Errorf("error marshaling normalized secret: %w", err) - } - return string(normalizedBytes), nil - } - - return result, nil -} - func NewOperationState(operation v1alpha1.Operation) *v1alpha1.OperationState { return &v1alpha1.OperationState{ Phase: common.OperationRunning, diff --git a/controller/sync_test.go b/controller/sync_test.go index 5fc2d4448449c..a36f3cad533a5 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1,9 +1,6 @@ package controller import ( - "context" - "encoding/base64" - "encoding/json" "strconv" "testing" @@ -26,8 +23,6 @@ import ( "github.com/argoproj/argo-cd/v3/util/argo/diff" "github.com/argoproj/argo-cd/v3/util/argo/normalizers" "github.com/argoproj/argo-cd/v3/util/settings" - - cmdutil "k8s.io/kubectl/pkg/cmd/util" ) func TestPersistRevisionHistory(t *testing.T) { @@ -1774,99 +1769,3 @@ func TestValidateSyncPermissions(t *testing.T) { }) } -func TestSecretNormalizingApplier(t *testing.T) { - ctx := t.Context() - - sensitiveAnnots := map[string]bool{ - "my-custom-sensitive-field": true, - } - - desired := &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]any{ - "name": "test-secret", - "namespace": "default", - "annotations": map[string]any{ - "my-custom-sensitive-field": "very-secret-value", - }, - }, - "type": "Opaque", - "data": map[string]any{ - "password": base64.StdEncoding.EncodeToString([]byte("actual-password")), - }, - }, - } - - mockApplier := &simpleKubeApplier{ - applyResult: func(obj *unstructured.Unstructured) string { - bytes, _ := json.Marshal(obj) - return string(bytes) - }, - } - - normalizer := &secretNormalizingApplier{ - inner: mockApplier, - sensitiveAnnotations: sensitiveAnnots, - } - - t.Run("core v1 secret is normalized during dry run", func(t *testing.T) { - result, err := normalizer.ApplyResource( - ctx, - desired, - cmdutil.DryRunServer, - false, false, true, "argocd", - ) - require.NoError(t, err) - - resultObj := &unstructured.Unstructured{} - err = json.Unmarshal([]byte(result), resultObj) - require.NoError(t, err) - - data, _, _ := unstructured.NestedMap(resultObj.Object, "data") - assert.Equal(t, "++++++++", data["password"], "Secret data should be masked with asterisks") - - annotations := resultObj.GetAnnotations() - assert.Equal(t, "++++++++", annotations["my-custom-sensitive-field"], "Sensitive annotations should be masked") - }) - - t.Run("non-json result is returned unchanged", func(t *testing.T) { - nonJSON := "kubectl applied dry-run" - mockApplier.applyResult = func(_ *unstructured.Unstructured) string { - return nonJSON - } - - result, err := normalizer.ApplyResource(ctx, desired, cmdutil.DryRunServer, false, false, true, "argocd") - require.NoError(t, err) - assert.Equal(t, nonJSON, result) //nolint:testifylint - }) - - t.Run("non-core group secret is not normalized", func(t *testing.T) { - customSecret := desired.DeepCopy() - customSecret.SetAPIVersion("custom.io/v1") - - mockApplier.applyResult = func(obj *unstructured.Unstructured) string { - bytes, _ := json.Marshal(obj) - return string(bytes) - } - - result, err := normalizer.ApplyResource(ctx, customSecret, cmdutil.DryRunServer, false, false, true, "argocd") - require.NoError(t, err) - - resultObj := &unstructured.Unstructured{} - err = json.Unmarshal([]byte(result), resultObj) - require.NoError(t, err) - - assert.Equal(t, customSecret.Object["data"], resultObj.Object["data"]) - assert.Equal(t, "very-secret-value", resultObj.GetAnnotations()["my-custom-sensitive-field"]) - }) -} - -type simpleKubeApplier struct { - applyResult func(*unstructured.Unstructured) string -} - -func (s *simpleKubeApplier) ApplyResource(_ context.Context, obj *unstructured.Unstructured, _ cmdutil.DryRunStrategy, _, _, _ bool, _ string) (string, error) { - return s.applyResult(obj), nil -} diff --git a/gitops-engine/pkg/diff/diff.go b/gitops-engine/pkg/diff/diff.go index 244f78010e2b5..7cc357feb5068 100644 --- a/gitops-engine/pkg/diff/diff.go +++ b/gitops-engine/pkg/diff/diff.go @@ -189,13 +189,31 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D Normalize(predictedLive, opts...) unstructured.RemoveNestedField(predictedLive.Object, "metadata", "managedFields") - 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) + var predictedLiveBytes []byte + if !isCoreSecret(config) { + 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") + + if isCoreSecret(config) { + // Mask Secret data symmetrically before comparison. + // Equal values get equal placeholders, different values get different placeholders. + maskedPredictedLive, maskedLive, 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(maskedPredictedLive) + if err != nil { + return nil, fmt.Errorf("error marshaling predicted live for resource %s/%s: %w", config.GetKind(), config.GetName(), err) + } + live = maskedLive + } + 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 +373,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) { diff --git a/gitops-engine/pkg/diff/diff_test.go b/gitops-engine/pkg/diff/diff_test.go index 514d60e0d569b..a02dd321ff732 100644 --- a/gitops-engine/pkg/diff/diff_test.go +++ b/gitops-engine/pkg/diff/diff_test.go @@ -1206,6 +1206,125 @@ func TestServerSideDiff(t *testing.T) { assert.Contains(t, liveData, "key3", "key3 should still be in live state") }) + 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 From 2cb3d9d3ab316b771b2b9fe6279e66c523ad5260 Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Wed, 29 Apr 2026 14:43:36 -0700 Subject: [PATCH 16/18] remove spacing Signed-off-by: Peter Jiang --- controller/sync.go | 3 --- controller/sync_test.go | 1 - 2 files changed, 4 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index c6f1c2edd27c7..f67dfc5a28c6b 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -11,9 +11,7 @@ import ( "k8s.io/apimachinery/pkg/util/strategicpatch" cdcommon "github.com/argoproj/argo-cd/v3/common" - gitopsDiff "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" - "github.com/argoproj/argo-cd/gitops-engine/pkg/sync" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync/common" "github.com/argoproj/argo-cd/gitops-engine/pkg/utils/kube" @@ -79,7 +77,6 @@ func (m *appStateManager) getServerSideDiffDryRunApplier(cluster *v1alpha1.Clust if err != nil { return nil, nil, fmt.Errorf("error creating kubectl ResourceOperations: %w", err) } - return ops, cleanup, nil } diff --git a/controller/sync_test.go b/controller/sync_test.go index a36f3cad533a5..4a450eb80a70c 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1768,4 +1768,3 @@ func TestValidateSyncPermissions(t *testing.T) { assert.NoError(t, err) }) } - From f0be0ed10ef92c4397f3fedbc788a0f139849aee Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Wed, 29 Apr 2026 14:44:27 -0700 Subject: [PATCH 17/18] remove spacing Signed-off-by: Peter Jiang --- controller/sync.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controller/sync.go b/controller/sync.go index f67dfc5a28c6b..9afbbe6ba83af 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -11,6 +11,7 @@ import ( "k8s.io/apimachinery/pkg/util/strategicpatch" cdcommon "github.com/argoproj/argo-cd/v3/common" + gitopsDiff "github.com/argoproj/argo-cd/gitops-engine/pkg/diff" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync" "github.com/argoproj/argo-cd/gitops-engine/pkg/sync/common" From 691eb897bfa5b0d761c1773730d6fc6e23f7c034 Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Thu, 30 Apr 2026 12:02:04 -0700 Subject: [PATCH 18/18] reorder normalization and hideSecret Signed-off-by: Peter Jiang --- gitops-engine/pkg/diff/diff.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/gitops-engine/pkg/diff/diff.go b/gitops-engine/pkg/diff/diff.go index c9256287e2677..fef98a01b39e2 100644 --- a/gitops-engine/pkg/diff/diff.go +++ b/gitops-engine/pkg/diff/diff.go @@ -190,14 +190,6 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D unstructured.RemoveNestedField(predictedLive.Object, "metadata", "managedFields") unstructured.RemoveNestedField(predictedLive.Object, "metadata", "resourceVersion") - var predictedLiveBytes []byte - if !isCoreSecret(config) { - 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") unstructured.RemoveNestedField(live.Object, "metadata", "resourceVersion") @@ -205,15 +197,15 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D if isCoreSecret(config) { // Mask Secret data symmetrically before comparison. // Equal values get equal placeholders, different values get different placeholders. - maskedPredictedLive, maskedLive, err := HideSecretData(predictedLive, live, nil) + 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(maskedPredictedLive) - if err != nil { - return nil, fmt.Errorf("error marshaling predicted live for resource %s/%s: %w", config.GetKind(), config.GetName(), err) - } - live = maskedLive + } + + 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) } liveBytes, err := json.Marshal(live)