From 317527d1036fef90777b3196aa41403ba1115f7c Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Fri, 13 Dec 2024 15:05:20 -0500 Subject: [PATCH 01/11] This is a combination of 8 commits. feat: enhance StatefulSet immutable field error message to show specific field changes Signed-off-by: Atif Ali formatting && added tests Signed-off-by: Atif Ali --- pkg/sync/sync_context.go | 145 ++++++++++++++++ pkg/sync/sync_context_test.go | 302 +++++++++++++++++++++++++++++++++- 2 files changed, 445 insertions(+), 2 deletions(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 675e1d04e..0667191ed 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "reflect" "sort" "strings" "sync" @@ -1155,6 +1156,85 @@ func (sc *syncContext) performClientSideApplyMigration(targetObj *unstructured.U return nil } +func formatValue(v interface{}) string { + if v == nil { + return "" + } + + // Special case for volumeClaimTemplates + if templates, ok := v.([]interface{}); ok { + // For a single volumeClaimTemplate field change + if len(templates) == 1 { + if template, ok := templates[0].(map[string]interface{}); ok { + if storage := getTemplateStorage(template); storage != "" { + return fmt.Sprintf("%q", storage) + } + } + } + // For multiple templates or other array types format + var names []string + for _, t := range templates { + if template, ok := t.(map[string]interface{}); ok { + if metadata, ok := template["metadata"].(map[string]interface{}); ok { + if name, ok := metadata["name"].(string); ok { + if storage := getTemplateStorage(template); storage != "" { + names = append(names, fmt.Sprintf("%s(%s)", name, storage)) + continue + } + names = append(names, name) + } + } + } + } + return fmt.Sprintf("[%s]", strings.Join(names, ", ")) + } + + // Special case for selector matchLabels + if m, ok := v.(map[string]interface{}); ok { + if matchLabels, exists := m["matchLabels"].(map[string]interface{}); exists { + var labels []string + for k, v := range matchLabels { + labels = append(labels, fmt.Sprintf("%s:%s", k, v)) + } + sort.Strings(labels) + return fmt.Sprintf("{%s}", strings.Join(labels, ", ")) + } + } + // Add quotes for string values + if str, ok := v.(string); ok { + return fmt.Sprintf("%q", str) + } + // For other types, use standard formatting + return fmt.Sprintf("%v", v) +} + +// Get storage size from template +func getTemplateStorage(template map[string]interface{}) string { + spec, ok := template["spec"].(map[string]interface{}) + if !ok { + return "" + } + resources, ok := spec["resources"].(map[string]interface{}) + if !ok { + return "" + } + requests, ok := resources["requests"].(map[string]interface{}) + if !ok { + return "" + } + storage, ok := requests["storage"].(string) + if !ok { + return "" + } + return storage +} + +// Format field changes for error messages +func formatFieldChange(field string, currentVal, desiredVal interface{}) string { + return fmt.Sprintf(" - %s:\n from: %s\n to: %s", + field, formatValue(currentVal), formatValue(desiredVal)) +} + func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) { dryRunStrategy := cmdutil.DryRunNone if dryRun { @@ -1206,6 +1286,71 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R message, err = sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, force, validate, serverSideApply, sc.serverSideApplyManager) } if err != nil { + // Check if this is a StatefulSet immutable field error + if strings.Contains(err.Error(), "updates to statefulset spec for fields other than") { + current := t.liveObj + desired := t.targetObj + + if current != nil && desired != nil { + currentSpec, _, _ := unstructured.NestedMap(current.Object, "spec") + desiredSpec, _, _ := unstructured.NestedMap(desired.Object, "spec") + + mutableFields := map[string]bool{ + "replicas": true, + "ordinals": true, + "template": true, + "updateStrategy": true, + "persistentVolumeClaimRetentionPolicy": true, + "minReadySeconds": true, + } + + var changes []string + for k, desiredVal := range desiredSpec { + if !mutableFields[k] { + currentVal, exists := currentSpec[k] + if !exists { + changes = append(changes, formatFieldChange(k, nil, desiredVal)) + } else if !reflect.DeepEqual(currentVal, desiredVal) { + if k == "volumeClaimTemplates" { + // Handle volumeClaimTemplates specially + currentTemplates := currentVal.([]interface{}) + desiredTemplates := desiredVal.([]interface{}) + + // If template count differs or we're adding/removing templates, + // use the standard array format + if len(currentTemplates) != len(desiredTemplates) { + changes = append(changes, formatFieldChange(k, currentVal, desiredVal)) + } else { + // Compare each template + for i, desired := range desiredTemplates { + current := currentTemplates[i] + desiredTemplate := desired.(map[string]interface{}) + currentTemplate := current.(map[string]interface{}) + + name := desiredTemplate["metadata"].(map[string]interface{})["name"].(string) + desiredStorage := getTemplateStorage(desiredTemplate) + currentStorage := getTemplateStorage(currentTemplate) + + if currentStorage != desiredStorage { + changes = append(changes, fmt.Sprintf(" - volumeClaimTemplates.%s:\n from: %q\n to: %q", + name, currentStorage, desiredStorage)) + } + } + } + } else { + changes = append(changes, formatFieldChange(k, currentVal, desiredVal)) + } + } + } + } + if len(changes) > 0 { + sort.Strings(changes) + message := fmt.Sprintf("attempting to change immutable fields:\n%s\n\nForbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden", + strings.Join(changes, "\n")) + return common.ResultCodeSyncFailed, message + } + } + } return common.ResultCodeSyncFailed, err.Error() } if kubeutil.IsCRD(t.targetObj) && !dryRun { diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 59f4c52eb..8cd0d2c0c 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -38,6 +38,16 @@ import ( testingutils "github.com/argoproj/gitops-engine/pkg/utils/testing" ) +const ( + testAPIVersion = "apps/v1" + testStatefulSet = "test-statefulset" + testNamespace = "default" + staticFiles = "static-files" + argocdDexServerTLS = "argocd-dex-server-tls" + postgresqlSvc = "postgresql-svc" + dexconfig = "dexconfig" +) + var standardVerbs = metav1.Verbs{"create", "delete", "deletecollection", "get", "list", "patch", "update", "watch"} func newTestSyncCtx(getResourceFunc *func(ctx context.Context, config *rest.Config, gvk schema.GroupVersionKind, name string, namespace string) (*unstructured.Unstructured, error), opts ...SyncOpt) *syncContext { @@ -52,7 +62,7 @@ func newTestSyncCtx(getResourceFunc *func(ctx context.Context, config *rest.Conf }, }, &metav1.APIResourceList{ - GroupVersion: "apps/v1", + GroupVersion: testAPIVersion, APIResources: []metav1.APIResource{ {Name: "deployments", Kind: "Deployment", Group: "apps", Version: "v1", Namespaced: true, Verbs: standardVerbs}, }, @@ -2416,7 +2426,6 @@ func TestNeedsClientSideApplyMigration(t *testing.T) { expected: true, }, } - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := syncCtx.needsClientSideApplyMigration(tt.liveObj, "kubectl-client-side-apply") @@ -2425,6 +2434,7 @@ func TestNeedsClientSideApplyMigration(t *testing.T) { } } +<<<<<<< HEAD func diffResultListClusterResource() *diff.DiffResultList { ns1 := testingutils.NewNamespace() ns1.SetName("ns-1") @@ -2448,4 +2458,292 @@ func diffResultListClusterResource() *diff.DiffResultList { diffResultList.Diffs = append(diffResultList.Diffs, diff.DiffResult{NormalizedLive: nsBytes, PredictedLive: nsBytes, Modified: false}) return &diffResultList +======= +func templateWithStorage(name, storage string) map[string]interface{} { + return map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": name, + }, + "spec": map[string]interface{}{ + "resources": map[string]interface{}{ + "requests": map[string]interface{}{ + "storage": storage, + }, + }, + }, + } +} + +func TestStatefulSetImmutableFieldErrors(t *testing.T) { + tests := []struct { + name string + currentSpec map[string]interface{} + desiredSpec map[string]interface{} + expectedMessage string + }{ + { + name: "single field change - serviceName", + currentSpec: map[string]interface{}{ + "serviceName": "old-svc", + }, + desiredSpec: map[string]interface{}{ + "serviceName": "new-svc", + }, + expectedMessage: `attempting to change immutable fields: + - serviceName: + from: "old-svc" + to: "new-svc" + +Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`, + }, + { + name: "volumeClaimTemplates change with storage size", + currentSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "data", + }, + "spec": map[string]interface{}{ + "resources": map[string]interface{}{ + "requests": map[string]interface{}{ + "storage": "1Gi", + }, + }, + }, + }, + }, + }, + desiredSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "data", + }, + "spec": map[string]interface{}{ + "resources": map[string]interface{}{ + "requests": map[string]interface{}{ + "storage": "2Gi", + }, + }, + }, + }, + }, + }, + expectedMessage: `attempting to change immutable fields: + - volumeClaimTemplates.data: + from: "1Gi" + to: "2Gi" + +Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`, + }, + { + name: "selector change", + currentSpec: map[string]interface{}{ + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "old-app", + }, + }, + }, + desiredSpec: map[string]interface{}{ + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "new-app", + }, + }, + }, + expectedMessage: `attempting to change immutable fields: + - selector: + from: {app:old-app} + to: {app:new-app} + +Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`, + }, + { + name: "volumeClaimTemplates change from nil", + currentSpec: map[string]interface{}{ + "serviceName": "test-svc", + }, + desiredSpec: map[string]interface{}{ + "serviceName": "test-svc", + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "data", + }, + }, + }, + }, + expectedMessage: `attempting to change immutable fields: + - volumeClaimTemplates: + from: + to: [data] + +Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`, + }, + { + name: "complex volumeClaimTemplates change", + currentSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "data1", + }, + }, + }, + }, + desiredSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "data1", + }, + }, + map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "data2", + }, + }, + }, + }, + expectedMessage: `attempting to change immutable fields: + - volumeClaimTemplates: + from: [data1] + to: [data1, data2] + +Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`, + }, + { + name: "multiple volumeClaimTemplate change", + currentSpec: map[string]interface{}{ + "serviceName": "postgresql-svc", + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "postgresql", + }, + }, + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "1Gi"), + templateWithStorage("dexconfig", "1Gi"), + templateWithStorage("argocd-dex-server-tls", "1Gi"), + }, + }, + desiredSpec: map[string]interface{}{ + "serviceName": "postgresql-svc", + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "postgresql", + }, + }, + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "2Gi"), + templateWithStorage("dexconfig", "3Gi"), + templateWithStorage("argocd-dex-server-tls", "4Gi"), + }, + }, + expectedMessage: `attempting to change immutable fields: + - volumeClaimTemplates.argocd-dex-server-tls: + from: "1Gi" + to: "4Gi" + - volumeClaimTemplates.dexconfig: + from: "1Gi" + to: "3Gi" + - volumeClaimTemplates.static-files: + from: "1Gi" + to: "2Gi" + +Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`, + }, + { + name: "multiple field changes", + currentSpec: map[string]interface{}{ + "serviceName": "postgresql-svc", + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "postgresql", + }, + }, + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "1Gi"), + templateWithStorage("dexconfig", "1Gi"), + templateWithStorage("argocd-dex-server-tls", "1Gi"), + }, + }, + desiredSpec: map[string]interface{}{ + "serviceName": "postgresql-svc-new", + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "app": "postgresql-new", + }, + }, + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "2Gi"), + templateWithStorage("dexconfig", "1Gi"), + templateWithStorage("argocd-dex-server-tls", "1Gi"), + }, + }, + expectedMessage: `attempting to change immutable fields: + - selector: + from: {app:postgresql} + to: {app:postgresql-new} + - serviceName: + from: "postgresql-svc" + to: "postgresql-svc-new" + - volumeClaimTemplates.static-files: + from: "1Gi" + to: "2Gi" + +Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + current := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": testAPIVersion, + "kind": "StatefulSet", + "metadata": map[string]interface{}{ + "name": testStatefulSet, + "namespace": testNamespace, + }, + "spec": tt.currentSpec, + }, + } + + desired := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": testAPIVersion, + "kind": "StatefulSet", + "metadata": map[string]interface{}{ + "name": testStatefulSet, + "namespace": testNamespace, + }, + "spec": tt.desiredSpec, + }, + } + + task := &syncTask{ + liveObj: current, + targetObj: desired, + } + + sc := newTestSyncCtx(nil) + + // Mock the resource operations to return immutable field error + mockResourceOps := &kubetest.MockResourceOps{ + Commands: map[string]kubetest.KubectlOutput{ + testStatefulSet: { + Err: errors.New("Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden"), + }, + }, + } + sc.resourceOps = mockResourceOps + + _, message := sc.applyObject(task, false, false) + assert.Equal(t, tt.expectedMessage, message) + }) + } +>>>>>>> 58d4d7c (This is a combination of 8 commits.) } From 9ed0ce7e9e16b89230c5e94088aa168ac5fe52d6 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 18 Dec 2024 14:17:51 -0500 Subject: [PATCH 02/11] reduce sonarcloud issues Signed-off-by: Atif Ali --- pkg/sync/sync_context_test.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 8cd0d2c0c..986741cc9 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -2617,29 +2617,29 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina { name: "multiple volumeClaimTemplate change", currentSpec: map[string]interface{}{ - "serviceName": "postgresql-svc", + "serviceName": postgresqlSvc, "selector": map[string]interface{}{ "matchLabels": map[string]interface{}{ "app": "postgresql", }, }, "volumeClaimTemplates": []interface{}{ - templateWithStorage("static-files", "1Gi"), - templateWithStorage("dexconfig", "1Gi"), - templateWithStorage("argocd-dex-server-tls", "1Gi"), + templateWithStorage(staticFiles, "1Gi"), + templateWithStorage(dexconfig, "1Gi"), + templateWithStorage(argocdDexServerTLS, "1Gi"), }, }, desiredSpec: map[string]interface{}{ - "serviceName": "postgresql-svc", + "serviceName": postgresqlSvc, "selector": map[string]interface{}{ "matchLabels": map[string]interface{}{ "app": "postgresql", }, }, "volumeClaimTemplates": []interface{}{ - templateWithStorage("static-files", "2Gi"), - templateWithStorage("dexconfig", "3Gi"), - templateWithStorage("argocd-dex-server-tls", "4Gi"), + templateWithStorage(staticFiles, "2Gi"), + templateWithStorage(dexconfig, "3Gi"), + templateWithStorage(argocdDexServerTLS, "4Gi"), }, }, expectedMessage: `attempting to change immutable fields: @@ -2658,16 +2658,16 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina { name: "multiple field changes", currentSpec: map[string]interface{}{ - "serviceName": "postgresql-svc", + "serviceName": postgresqlSvc, "selector": map[string]interface{}{ "matchLabels": map[string]interface{}{ "app": "postgresql", }, }, "volumeClaimTemplates": []interface{}{ - templateWithStorage("static-files", "1Gi"), - templateWithStorage("dexconfig", "1Gi"), - templateWithStorage("argocd-dex-server-tls", "1Gi"), + templateWithStorage(staticFiles, "1Gi"), + templateWithStorage(dexconfig, "1Gi"), + templateWithStorage(argocdDexServerTLS, "1Gi"), }, }, desiredSpec: map[string]interface{}{ @@ -2678,9 +2678,9 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, }, "volumeClaimTemplates": []interface{}{ - templateWithStorage("static-files", "2Gi"), - templateWithStorage("dexconfig", "1Gi"), - templateWithStorage("argocd-dex-server-tls", "1Gi"), + templateWithStorage(staticFiles, "2Gi"), + templateWithStorage(dexconfig, "1Gi"), + templateWithStorage(argocdDexServerTLS, "1Gi"), }, }, expectedMessage: `attempting to change immutable fields: From f32f3ad37c43eace8b956812f8835fbf197cc78c Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Mon, 10 Feb 2025 19:06:28 -0500 Subject: [PATCH 03/11] resolve linter issues Signed-off-by: Atif Ali --- pkg/sync/sync_context.go | 34 ++++----- pkg/sync/sync_context_test.go | 130 ++++++++++++++++++---------------- 2 files changed, 85 insertions(+), 79 deletions(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 0667191ed..a6ec975b3 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1156,16 +1156,16 @@ func (sc *syncContext) performClientSideApplyMigration(targetObj *unstructured.U return nil } -func formatValue(v interface{}) string { +func formatValue(v any) string { if v == nil { return "" } // Special case for volumeClaimTemplates - if templates, ok := v.([]interface{}); ok { + if templates, ok := v.([]any); ok { // For a single volumeClaimTemplate field change if len(templates) == 1 { - if template, ok := templates[0].(map[string]interface{}); ok { + if template, ok := templates[0].(map[string]any); ok { if storage := getTemplateStorage(template); storage != "" { return fmt.Sprintf("%q", storage) } @@ -1174,8 +1174,8 @@ func formatValue(v interface{}) string { // For multiple templates or other array types format var names []string for _, t := range templates { - if template, ok := t.(map[string]interface{}); ok { - if metadata, ok := template["metadata"].(map[string]interface{}); ok { + if template, ok := t.(map[string]any); ok { + if metadata, ok := template["metadata"].(map[string]any); ok { if name, ok := metadata["name"].(string); ok { if storage := getTemplateStorage(template); storage != "" { names = append(names, fmt.Sprintf("%s(%s)", name, storage)) @@ -1190,8 +1190,8 @@ func formatValue(v interface{}) string { } // Special case for selector matchLabels - if m, ok := v.(map[string]interface{}); ok { - if matchLabels, exists := m["matchLabels"].(map[string]interface{}); exists { + if m, ok := v.(map[string]any); ok { + if matchLabels, exists := m["matchLabels"].(map[string]any); exists { var labels []string for k, v := range matchLabels { labels = append(labels, fmt.Sprintf("%s:%s", k, v)) @@ -1209,16 +1209,16 @@ func formatValue(v interface{}) string { } // Get storage size from template -func getTemplateStorage(template map[string]interface{}) string { - spec, ok := template["spec"].(map[string]interface{}) +func getTemplateStorage(template map[string]any) string { + spec, ok := template["spec"].(map[string]any) if !ok { return "" } - resources, ok := spec["resources"].(map[string]interface{}) + resources, ok := spec["resources"].(map[string]any) if !ok { return "" } - requests, ok := resources["requests"].(map[string]interface{}) + requests, ok := resources["requests"].(map[string]any) if !ok { return "" } @@ -1230,7 +1230,7 @@ func getTemplateStorage(template map[string]interface{}) string { } // Format field changes for error messages -func formatFieldChange(field string, currentVal, desiredVal interface{}) string { +func formatFieldChange(field string, currentVal, desiredVal any) string { return fmt.Sprintf(" - %s:\n from: %s\n to: %s", field, formatValue(currentVal), formatValue(desiredVal)) } @@ -1313,8 +1313,8 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R } else if !reflect.DeepEqual(currentVal, desiredVal) { if k == "volumeClaimTemplates" { // Handle volumeClaimTemplates specially - currentTemplates := currentVal.([]interface{}) - desiredTemplates := desiredVal.([]interface{}) + currentTemplates := currentVal.([]any) + desiredTemplates := desiredVal.([]any) // If template count differs or we're adding/removing templates, // use the standard array format @@ -1324,10 +1324,10 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R // Compare each template for i, desired := range desiredTemplates { current := currentTemplates[i] - desiredTemplate := desired.(map[string]interface{}) - currentTemplate := current.(map[string]interface{}) + desiredTemplate := desired.(map[string]any) + currentTemplate := current.(map[string]any) - name := desiredTemplate["metadata"].(map[string]interface{})["name"].(string) + name := desiredTemplate["metadata"].(map[string]any)["name"].(string) desiredStorage := getTemplateStorage(desiredTemplate) currentStorage := getTemplateStorage(currentTemplate) diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 986741cc9..8a21aa423 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -2434,6 +2434,7 @@ func TestNeedsClientSideApplyMigration(t *testing.T) { } } +<<<<<<< HEAD <<<<<<< HEAD func diffResultListClusterResource() *diff.DiffResultList { ns1 := testingutils.NewNamespace() @@ -2462,11 +2463,16 @@ func diffResultListClusterResource() *diff.DiffResultList { func templateWithStorage(name, storage string) map[string]interface{} { return map[string]interface{}{ "metadata": map[string]interface{}{ +======= +func templateWithStorage(name, storage string) map[string]any { + return map[string]any{ + "metadata": map[string]any{ +>>>>>>> 2a210a3 (resolve linter issues) "name": name, }, - "spec": map[string]interface{}{ - "resources": map[string]interface{}{ - "requests": map[string]interface{}{ + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ "storage": storage, }, }, @@ -2477,16 +2483,16 @@ func templateWithStorage(name, storage string) map[string]interface{} { func TestStatefulSetImmutableFieldErrors(t *testing.T) { tests := []struct { name string - currentSpec map[string]interface{} - desiredSpec map[string]interface{} + currentSpec map[string]any + desiredSpec map[string]any expectedMessage string }{ { name: "single field change - serviceName", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": "old-svc", }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": "new-svc", }, expectedMessage: `attempting to change immutable fields: @@ -2498,15 +2504,15 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "volumeClaimTemplates change with storage size", - currentSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + currentSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data", }, - "spec": map[string]interface{}{ - "resources": map[string]interface{}{ - "requests": map[string]interface{}{ + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ "storage": "1Gi", }, }, @@ -2514,15 +2520,15 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, }, }, - desiredSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + desiredSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data", }, - "spec": map[string]interface{}{ - "resources": map[string]interface{}{ - "requests": map[string]interface{}{ + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ "storage": "2Gi", }, }, @@ -2539,16 +2545,16 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "selector change", - currentSpec: map[string]interface{}{ - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + currentSpec: map[string]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "old-app", }, }, }, - desiredSpec: map[string]interface{}{ - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + desiredSpec: map[string]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "new-app", }, }, @@ -2562,14 +2568,14 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "volumeClaimTemplates change from nil", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": "test-svc", }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": "test-svc", - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data", }, }, @@ -2584,24 +2590,24 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "complex volumeClaimTemplates change", - currentSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + currentSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data1", }, }, }, }, - desiredSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + desiredSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data1", }, }, - map[string]interface{}{ - "metadata": map[string]interface{}{ + map[string]any{ + "metadata": map[string]any{ "name": "data2", }, }, @@ -2616,27 +2622,27 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "multiple volumeClaimTemplate change", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": postgresqlSvc, - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage(staticFiles, "1Gi"), templateWithStorage(dexconfig, "1Gi"), templateWithStorage(argocdDexServerTLS, "1Gi"), }, }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": postgresqlSvc, - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage(staticFiles, "2Gi"), templateWithStorage(dexconfig, "3Gi"), templateWithStorage(argocdDexServerTLS, "4Gi"), @@ -2657,27 +2663,27 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "multiple field changes", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": postgresqlSvc, - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage(staticFiles, "1Gi"), templateWithStorage(dexconfig, "1Gi"), templateWithStorage(argocdDexServerTLS, "1Gi"), }, }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": "postgresql-svc-new", - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql-new", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage(staticFiles, "2Gi"), templateWithStorage(dexconfig, "1Gi"), templateWithStorage(argocdDexServerTLS, "1Gi"), @@ -2701,10 +2707,10 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { current := &unstructured.Unstructured{ - Object: map[string]interface{}{ + Object: map[string]any{ "apiVersion": testAPIVersion, "kind": "StatefulSet", - "metadata": map[string]interface{}{ + "metadata": map[string]any{ "name": testStatefulSet, "namespace": testNamespace, }, @@ -2713,10 +2719,10 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina } desired := &unstructured.Unstructured{ - Object: map[string]interface{}{ + Object: map[string]any{ "apiVersion": testAPIVersion, "kind": "StatefulSet", - "metadata": map[string]interface{}{ + "metadata": map[string]any{ "name": testStatefulSet, "namespace": testNamespace, }, From cfb403e6dc4cca2d322cbe270ecaa3a1e76d8904 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 13 Feb 2025 16:06:51 -0500 Subject: [PATCH 04/11] refactor applyObject method to reduce complexity Signed-off-by: Atif Ali --- pkg/sync/sync_context.go | 137 ++++++++++++++++++++++----------------- 1 file changed, 77 insertions(+), 60 deletions(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index a6ec975b3..cb0495e72 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1235,6 +1235,80 @@ func formatFieldChange(field string, currentVal, desiredVal any) string { field, formatValue(currentVal), formatValue(desiredVal)) } +// validateStatefulSetUpdate checks for changes to immutable fields in a StatefulSet +// Returns the formatted error message and true if immutable fields were changed +func (sc *syncContext) validateStatefulSetUpdate(current, desired *unstructured.Unstructured) (string, bool) { + currentSpec, _, _ := unstructured.NestedMap(current.Object, "spec") + desiredSpec, _, _ := unstructured.NestedMap(desired.Object, "spec") + + changes := getImmutableFieldChanges(currentSpec, desiredSpec) + if len(changes) == 0 { + return "", false + } + + sort.Strings(changes) + message := fmt.Sprintf("attempting to change immutable fields:\n%s\n\nForbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden", + strings.Join(changes, "\n")) + return message, true +} + +// getImmutableFieldChanges compares specs and returns a list of changes to immutable fields +func getImmutableFieldChanges(currentSpec, desiredSpec map[string]any) []string { + mutableFields := map[string]bool{ + "replicas": true, "ordinals": true, "template": true, + "updateStrategy": true, "persistentVolumeClaimRetentionPolicy": true, + "minReadySeconds": true, + } + + var changes []string + for k, desiredVal := range desiredSpec { + if mutableFields[k] { + continue + } + + currentVal, exists := currentSpec[k] + if !exists { + changes = append(changes, formatFieldChange(k, nil, desiredVal)) + continue + } + + if !reflect.DeepEqual(currentVal, desiredVal) { + if k == "volumeClaimTemplates" { + changes = append(changes, formatVolumeClaimChanges(currentVal, desiredVal)...) + } else { + changes = append(changes, formatFieldChange(k, currentVal, desiredVal)) + } + } + } + return changes +} + +// formatVolumeClaimChanges handles the special case of formatting changes to volumeClaimTemplates +func formatVolumeClaimChanges(currentVal, desiredVal any) []string { + currentTemplates := currentVal.([]any) + desiredTemplates := desiredVal.([]any) + + if len(currentTemplates) != len(desiredTemplates) { + return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} + } + + var changes []string + for i := range desiredTemplates { + desiredTemplate := desiredTemplates[i].(map[string]any) + currentTemplate := currentTemplates[i].(map[string]any) + + name := desiredTemplate["metadata"].(map[string]any)["name"].(string) + desiredStorage := getTemplateStorage(desiredTemplate) + currentStorage := getTemplateStorage(currentTemplate) + + if currentStorage != desiredStorage { + changes = append(changes, fmt.Sprintf(" - volumeClaimTemplates.%s:\n from: %q\n to: %q", + name, currentStorage, desiredStorage)) + } + } + return changes +} + func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) { dryRunStrategy := cmdutil.DryRunNone if dryRun { @@ -1286,67 +1360,9 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R message, err = sc.resourceOps.ApplyResource(context.TODO(), t.targetObj, dryRunStrategy, force, validate, serverSideApply, sc.serverSideApplyManager) } if err != nil { - // Check if this is a StatefulSet immutable field error if strings.Contains(err.Error(), "updates to statefulset spec for fields other than") { - current := t.liveObj - desired := t.targetObj - - if current != nil && desired != nil { - currentSpec, _, _ := unstructured.NestedMap(current.Object, "spec") - desiredSpec, _, _ := unstructured.NestedMap(desired.Object, "spec") - - mutableFields := map[string]bool{ - "replicas": true, - "ordinals": true, - "template": true, - "updateStrategy": true, - "persistentVolumeClaimRetentionPolicy": true, - "minReadySeconds": true, - } - - var changes []string - for k, desiredVal := range desiredSpec { - if !mutableFields[k] { - currentVal, exists := currentSpec[k] - if !exists { - changes = append(changes, formatFieldChange(k, nil, desiredVal)) - } else if !reflect.DeepEqual(currentVal, desiredVal) { - if k == "volumeClaimTemplates" { - // Handle volumeClaimTemplates specially - currentTemplates := currentVal.([]any) - desiredTemplates := desiredVal.([]any) - - // If template count differs or we're adding/removing templates, - // use the standard array format - if len(currentTemplates) != len(desiredTemplates) { - changes = append(changes, formatFieldChange(k, currentVal, desiredVal)) - } else { - // Compare each template - for i, desired := range desiredTemplates { - current := currentTemplates[i] - desiredTemplate := desired.(map[string]any) - currentTemplate := current.(map[string]any) - - name := desiredTemplate["metadata"].(map[string]any)["name"].(string) - desiredStorage := getTemplateStorage(desiredTemplate) - currentStorage := getTemplateStorage(currentTemplate) - - if currentStorage != desiredStorage { - changes = append(changes, fmt.Sprintf(" - volumeClaimTemplates.%s:\n from: %q\n to: %q", - name, currentStorage, desiredStorage)) - } - } - } - } else { - changes = append(changes, formatFieldChange(k, currentVal, desiredVal)) - } - } - } - } - if len(changes) > 0 { - sort.Strings(changes) - message := fmt.Sprintf("attempting to change immutable fields:\n%s\n\nForbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden", - strings.Join(changes, "\n")) + if t.liveObj != nil && t.targetObj != nil { + if message, hasChanges := sc.validateStatefulSetUpdate(t.liveObj, t.targetObj); hasChanges { return common.ResultCodeSyncFailed, message } } @@ -1359,6 +1375,7 @@ func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.R sc.log.Error(err, fmt.Sprintf("failed to ensure that CRD %s is ready", crdName)) } } + return common.ResultCodeSynced, message } From c06739397ce696063a4549b78d100afdb8f175b1 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 13 Feb 2025 16:26:15 -0500 Subject: [PATCH 05/11] refactor formatValue method to reduce complexity Signed-off-by: Atif Ali --- pkg/sync/sync_context.go | 126 +++++++++++++++++++++++++++------------ 1 file changed, 87 insertions(+), 39 deletions(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index cb0495e72..5979277fa 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1110,6 +1110,7 @@ func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstruct return sc.serverSideApply || resourceutil.HasAnnotationOption(targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply) } +<<<<<<< HEAD // needsClientSideApplyMigration checks if a resource has fields managed by the specified manager // that need to be migrated to the server-side apply manager func (sc *syncContext) needsClientSideApplyMigration(liveObj *unstructured.Unstructured, fieldManager string) bool { @@ -1156,56 +1157,103 @@ func (sc *syncContext) performClientSideApplyMigration(targetObj *unstructured.U return nil } +======= +// formatValue converts any value to its string representation with special handling for +// templates, maps, and strings. Returns "" for nil values. +>>>>>>> 3df63b1 (refactor formatValue method to reduce complexity) func formatValue(v any) string { if v == nil { return "" } - // Special case for volumeClaimTemplates - if templates, ok := v.([]any); ok { - // For a single volumeClaimTemplate field change - if len(templates) == 1 { - if template, ok := templates[0].(map[string]any); ok { - if storage := getTemplateStorage(template); storage != "" { - return fmt.Sprintf("%q", storage) - } - } - } - // For multiple templates or other array types format - var names []string - for _, t := range templates { - if template, ok := t.(map[string]any); ok { - if metadata, ok := template["metadata"].(map[string]any); ok { - if name, ok := metadata["name"].(string); ok { - if storage := getTemplateStorage(template); storage != "" { - names = append(names, fmt.Sprintf("%s(%s)", name, storage)) - continue - } - names = append(names, name) - } - } - } + switch val := v.(type) { + case []any: + return formatTemplates(val) + case map[string]any: + return formatMap(val) + case string: + return fmt.Sprintf("%q", val) + default: + return fmt.Sprintf("%v", val) + } +} + +// formatTemplates handles the formatting of volumeClaimTemplates arrays. +// For single templates with storage, returns the storage value. +// For multiple templates, returns a formatted list of template names with storage. +func formatTemplates(templates []any) string { + if len(templates) == 1 { + if storage := getSingleTemplateStorage(templates[0]); storage != "" { + return fmt.Sprintf("%q", storage) } - return fmt.Sprintf("[%s]", strings.Join(names, ", ")) } + return formatMultipleTemplates(templates) +} - // Special case for selector matchLabels - if m, ok := v.(map[string]any); ok { - if matchLabels, exists := m["matchLabels"].(map[string]any); exists { - var labels []string - for k, v := range matchLabels { - labels = append(labels, fmt.Sprintf("%s:%s", k, v)) - } - sort.Strings(labels) - return fmt.Sprintf("{%s}", strings.Join(labels, ", ")) +// getSingleTemplateStorage extracts storage value from a single template. +// Returns empty string if template is invalid or has no storage. +func getSingleTemplateStorage(t any) string { + template, ok := t.(map[string]any) + if !ok { + return "" + } + return getTemplateStorage(template) +} + +// formatMultipleTemplates formats an array of templates into a string list +// of template names, optionally including storage information. +func formatMultipleTemplates(templates []any) string { + var names []string + for _, t := range templates { + if name := getTemplateName(t); name != "" { + names = append(names, name) } } - // Add quotes for string values - if str, ok := v.(string); ok { - return fmt.Sprintf("%q", str) + return fmt.Sprintf("[%s]", strings.Join(names, ", ")) +} + +// getTemplateName extracts and formats the name from a template. +// If template has storage, appends it to the name in parentheses. +func getTemplateName(t any) string { + template, ok := t.(map[string]any) + if !ok { + return "" + } + + metadata, ok := template["metadata"].(map[string]any) + if !ok { + return "" + } + + name, ok := metadata["name"].(string) + if !ok { + return "" + } + + if storage := getTemplateStorage(template); storage != "" { + return fmt.Sprintf("%s(%s)", name, storage) + } + return name +} + +// formatMap handles special case formatting for maps, particularly for matchLabels. +// Returns standard string representation for non-matchLabel maps. +func formatMap(m map[string]any) string { + if matchLabels, exists := m["matchLabels"].(map[string]any); exists { + return formatMatchLabels(matchLabels) + } + return fmt.Sprintf("%v", m) +} + +// formatMatchLabels converts a matchLabels map into a sorted string list +// of key:value pairs enclosed in curly braces. +func formatMatchLabels(matchLabels map[string]any) string { + var labels []string + for k, v := range matchLabels { + labels = append(labels, fmt.Sprintf("%s:%s", k, v)) } - // For other types, use standard formatting - return fmt.Sprintf("%v", v) + sort.Strings(labels) + return fmt.Sprintf("{%s}", strings.Join(labels, ", ")) } // Get storage size from template From 842e2054b8e493f25115877c85198de3f679f0f7 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 26 Jun 2025 11:35:03 -0400 Subject: [PATCH 06/11] special formatting for volumeClaimTemplates changes Signed-off-by: Atif Ali --- pkg/sync/sync_context.go | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 5979277fa..b38887a9a 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1333,28 +1333,9 @@ func getImmutableFieldChanges(currentSpec, desiredSpec map[string]any) []string // formatVolumeClaimChanges handles the special case of formatting changes to volumeClaimTemplates func formatVolumeClaimChanges(currentVal, desiredVal any) []string { - currentTemplates := currentVal.([]any) - desiredTemplates := desiredVal.([]any) - - if len(currentTemplates) != len(desiredTemplates) { - return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} - } - - var changes []string - for i := range desiredTemplates { - desiredTemplate := desiredTemplates[i].(map[string]any) - currentTemplate := currentTemplates[i].(map[string]any) - - name := desiredTemplate["metadata"].(map[string]any)["name"].(string) - desiredStorage := getTemplateStorage(desiredTemplate) - currentStorage := getTemplateStorage(currentTemplate) - - if currentStorage != desiredStorage { - changes = append(changes, fmt.Sprintf(" - volumeClaimTemplates.%s:\n from: %q\n to: %q", - name, currentStorage, desiredStorage)) - } - } - return changes + // Special formatting for volumeClaimTemplates changes + // Shows storage size changes for each template + return []string{} } func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) { From cadfa1c82844404d9e3b75354e1afae7342861b7 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 26 Jun 2025 11:44:23 -0400 Subject: [PATCH 07/11] fix test failures Signed-off-by: Atif Ali --- pkg/sync/sync_context.go | 47 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index b38887a9a..144180f3d 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1333,9 +1333,50 @@ func getImmutableFieldChanges(currentSpec, desiredSpec map[string]any) []string // formatVolumeClaimChanges handles the special case of formatting changes to volumeClaimTemplates func formatVolumeClaimChanges(currentVal, desiredVal any) []string { - // Special formatting for volumeClaimTemplates changes - // Shows storage size changes for each template - return []string{} + currentTemplates, ok := currentVal.([]any) + if !ok { + return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} + } + + desiredTemplates, ok := desiredVal.([]any) + if !ok { + return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} + } + + if len(currentTemplates) != len(desiredTemplates) { + return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} + } + + var changes []string + for i := range desiredTemplates { + desiredTemplate, ok := desiredTemplates[i].(map[string]any) + if !ok { + continue + } + currentTemplate, ok := currentTemplates[i].(map[string]any) + if !ok { + continue + } + + // Extract just the template name without storage size + metadata, ok := desiredTemplate["metadata"].(map[string]any) + if !ok { + continue + } + name, ok := metadata["name"].(string) + if !ok || name == "" { + continue + } + + desiredStorage := getTemplateStorage(desiredTemplate) + currentStorage := getTemplateStorage(currentTemplate) + + if currentStorage != desiredStorage { + changes = append(changes, fmt.Sprintf(" - volumeClaimTemplates.%s:\n from: %q\n to: %q", + name, currentStorage, desiredStorage)) + } + } + return changes } func (sc *syncContext) applyObject(t *syncTask, dryRun, validate bool) (common.ResultCode, string) { From 9260828a16ce69d9300d530ecaa14cf73c8edb82 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 26 Jun 2025 13:21:40 -0400 Subject: [PATCH 08/11] fix test failures2 Signed-off-by: Atif Ali --- pkg/sync/sync_context.go | 35 +++++------------------------------ 1 file changed, 5 insertions(+), 30 deletions(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 144180f3d..1e11fb214 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1110,7 +1110,6 @@ func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstruct return sc.serverSideApply || resourceutil.HasAnnotationOption(targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply) } -<<<<<<< HEAD // needsClientSideApplyMigration checks if a resource has fields managed by the specified manager // that need to be migrated to the server-side apply manager func (sc *syncContext) needsClientSideApplyMigration(liveObj *unstructured.Unstructured, fieldManager string) bool { @@ -1157,10 +1156,8 @@ func (sc *syncContext) performClientSideApplyMigration(targetObj *unstructured.U return nil } -======= // formatValue converts any value to its string representation with special handling for // templates, maps, and strings. Returns "" for nil values. ->>>>>>> 3df63b1 (refactor formatValue method to reduce complexity) func formatValue(v any) string { if v == nil { return "" @@ -1333,15 +1330,8 @@ func getImmutableFieldChanges(currentSpec, desiredSpec map[string]any) []string // formatVolumeClaimChanges handles the special case of formatting changes to volumeClaimTemplates func formatVolumeClaimChanges(currentVal, desiredVal any) []string { - currentTemplates, ok := currentVal.([]any) - if !ok { - return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} - } - - desiredTemplates, ok := desiredVal.([]any) - if !ok { - return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} - } + currentTemplates := currentVal.([]any) + desiredTemplates := desiredVal.([]any) if len(currentTemplates) != len(desiredTemplates) { return []string{formatFieldChange("volumeClaimTemplates", currentVal, desiredVal)} @@ -1349,25 +1339,10 @@ func formatVolumeClaimChanges(currentVal, desiredVal any) []string { var changes []string for i := range desiredTemplates { - desiredTemplate, ok := desiredTemplates[i].(map[string]any) - if !ok { - continue - } - currentTemplate, ok := currentTemplates[i].(map[string]any) - if !ok { - continue - } - - // Extract just the template name without storage size - metadata, ok := desiredTemplate["metadata"].(map[string]any) - if !ok { - continue - } - name, ok := metadata["name"].(string) - if !ok || name == "" { - continue - } + desiredTemplate := desiredTemplates[i].(map[string]any) + currentTemplate := currentTemplates[i].(map[string]any) + name := desiredTemplate["metadata"].(map[string]any)["name"].(string) desiredStorage := getTemplateStorage(desiredTemplate) currentStorage := getTemplateStorage(currentTemplate) From 96e95e623c3c8a414087ad6bfb57ff7837bb3b81 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 10 Sep 2025 21:25:17 -0400 Subject: [PATCH 09/11] Removed all merge conflict markers Signed-off-by: Atif Ali --- pkg/sync/sync_context_test.go | 159 ++++++++++++++++------------------ 1 file changed, 76 insertions(+), 83 deletions(-) diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 8a21aa423..846afb409 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -2434,8 +2434,6 @@ func TestNeedsClientSideApplyMigration(t *testing.T) { } } -<<<<<<< HEAD -<<<<<<< HEAD func diffResultListClusterResource() *diff.DiffResultList { ns1 := testingutils.NewNamespace() ns1.SetName("ns-1") @@ -2459,15 +2457,11 @@ func diffResultListClusterResource() *diff.DiffResultList { diffResultList.Diffs = append(diffResultList.Diffs, diff.DiffResult{NormalizedLive: nsBytes, PredictedLive: nsBytes, Modified: false}) return &diffResultList -======= -func templateWithStorage(name, storage string) map[string]interface{} { - return map[string]interface{}{ - "metadata": map[string]interface{}{ -======= +} + func templateWithStorage(name, storage string) map[string]any { return map[string]any{ "metadata": map[string]any{ ->>>>>>> 2a210a3 (resolve linter issues) "name": name, }, "spec": map[string]any{ @@ -2483,16 +2477,16 @@ func templateWithStorage(name, storage string) map[string]any { func TestStatefulSetImmutableFieldErrors(t *testing.T) { tests := []struct { name string - currentSpec map[string]any - desiredSpec map[string]any + currentSpec map[string]interface{} + desiredSpec map[string]interface{} expectedMessage string }{ { name: "single field change - serviceName", - currentSpec: map[string]any{ + currentSpec: map[string]interface{}{ "serviceName": "old-svc", }, - desiredSpec: map[string]any{ + desiredSpec: map[string]interface{}{ "serviceName": "new-svc", }, expectedMessage: `attempting to change immutable fields: @@ -2504,15 +2498,15 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "volumeClaimTemplates change with storage size", - currentSpec: map[string]any{ - "volumeClaimTemplates": []any{ - map[string]any{ - "metadata": map[string]any{ + currentSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ "name": "data", }, - "spec": map[string]any{ - "resources": map[string]any{ - "requests": map[string]any{ + "spec": map[string]interface{}{ + "resources": map[string]interface{}{ + "requests": map[string]interface{}{ "storage": "1Gi", }, }, @@ -2520,15 +2514,15 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, }, }, - desiredSpec: map[string]any{ - "volumeClaimTemplates": []any{ - map[string]any{ - "metadata": map[string]any{ + desiredSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ "name": "data", }, - "spec": map[string]any{ - "resources": map[string]any{ - "requests": map[string]any{ + "spec": map[string]interface{}{ + "resources": map[string]interface{}{ + "requests": map[string]interface{}{ "storage": "2Gi", }, }, @@ -2545,16 +2539,16 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "selector change", - currentSpec: map[string]any{ - "selector": map[string]any{ - "matchLabels": map[string]any{ + currentSpec: map[string]interface{}{ + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ "app": "old-app", }, }, }, - desiredSpec: map[string]any{ - "selector": map[string]any{ - "matchLabels": map[string]any{ + desiredSpec: map[string]interface{}{ + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ "app": "new-app", }, }, @@ -2568,14 +2562,14 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "volumeClaimTemplates change from nil", - currentSpec: map[string]any{ + currentSpec: map[string]interface{}{ "serviceName": "test-svc", }, - desiredSpec: map[string]any{ + desiredSpec: map[string]interface{}{ "serviceName": "test-svc", - "volumeClaimTemplates": []any{ - map[string]any{ - "metadata": map[string]any{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ "name": "data", }, }, @@ -2590,24 +2584,24 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "complex volumeClaimTemplates change", - currentSpec: map[string]any{ - "volumeClaimTemplates": []any{ - map[string]any{ - "metadata": map[string]any{ + currentSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ "name": "data1", }, }, }, }, - desiredSpec: map[string]any{ - "volumeClaimTemplates": []any{ - map[string]any{ - "metadata": map[string]any{ + desiredSpec: map[string]interface{}{ + "volumeClaimTemplates": []interface{}{ + map[string]interface{}{ + "metadata": map[string]interface{}{ "name": "data1", }, }, - map[string]any{ - "metadata": map[string]any{ + map[string]interface{}{ + "metadata": map[string]interface{}{ "name": "data2", }, }, @@ -2622,30 +2616,30 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "multiple volumeClaimTemplate change", - currentSpec: map[string]any{ - "serviceName": postgresqlSvc, - "selector": map[string]any{ - "matchLabels": map[string]any{ + currentSpec: map[string]interface{}{ + "serviceName": "postgresql-svc", + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ "app": "postgresql", }, }, - "volumeClaimTemplates": []any{ - templateWithStorage(staticFiles, "1Gi"), - templateWithStorage(dexconfig, "1Gi"), - templateWithStorage(argocdDexServerTLS, "1Gi"), + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "1Gi"), + templateWithStorage("dexconfig", "1Gi"), + templateWithStorage("argocd-dex-server-tls", "1Gi"), }, }, - desiredSpec: map[string]any{ - "serviceName": postgresqlSvc, - "selector": map[string]any{ - "matchLabels": map[string]any{ + desiredSpec: map[string]interface{}{ + "serviceName": "postgresql-svc", + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ "app": "postgresql", }, }, - "volumeClaimTemplates": []any{ - templateWithStorage(staticFiles, "2Gi"), - templateWithStorage(dexconfig, "3Gi"), - templateWithStorage(argocdDexServerTLS, "4Gi"), + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "2Gi"), + templateWithStorage("dexconfig", "3Gi"), + templateWithStorage("argocd-dex-server-tls", "4Gi"), }, }, expectedMessage: `attempting to change immutable fields: @@ -2663,30 +2657,30 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "multiple field changes", - currentSpec: map[string]any{ - "serviceName": postgresqlSvc, - "selector": map[string]any{ - "matchLabels": map[string]any{ + currentSpec: map[string]interface{}{ + "serviceName": "postgresql-svc", + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ "app": "postgresql", }, }, - "volumeClaimTemplates": []any{ - templateWithStorage(staticFiles, "1Gi"), - templateWithStorage(dexconfig, "1Gi"), - templateWithStorage(argocdDexServerTLS, "1Gi"), + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "1Gi"), + templateWithStorage("dexconfig", "1Gi"), + templateWithStorage("argocd-dex-server-tls", "1Gi"), }, }, - desiredSpec: map[string]any{ + desiredSpec: map[string]interface{}{ "serviceName": "postgresql-svc-new", - "selector": map[string]any{ - "matchLabels": map[string]any{ + "selector": map[string]interface{}{ + "matchLabels": map[string]interface{}{ "app": "postgresql-new", }, }, - "volumeClaimTemplates": []any{ - templateWithStorage(staticFiles, "2Gi"), - templateWithStorage(dexconfig, "1Gi"), - templateWithStorage(argocdDexServerTLS, "1Gi"), + "volumeClaimTemplates": []interface{}{ + templateWithStorage("static-files", "2Gi"), + templateWithStorage("dexconfig", "1Gi"), + templateWithStorage("argocd-dex-server-tls", "1Gi"), }, }, expectedMessage: `attempting to change immutable fields: @@ -2707,10 +2701,10 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { current := &unstructured.Unstructured{ - Object: map[string]any{ + Object: map[string]interface{}{ "apiVersion": testAPIVersion, "kind": "StatefulSet", - "metadata": map[string]any{ + "metadata": map[string]interface{}{ "name": testStatefulSet, "namespace": testNamespace, }, @@ -2719,10 +2713,10 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina } desired := &unstructured.Unstructured{ - Object: map[string]any{ + Object: map[string]interface{}{ "apiVersion": testAPIVersion, "kind": "StatefulSet", - "metadata": map[string]any{ + "metadata": map[string]interface{}{ "name": testStatefulSet, "namespace": testNamespace, }, @@ -2751,5 +2745,4 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina assert.Equal(t, tt.expectedMessage, message) }) } ->>>>>>> 58d4d7c (This is a combination of 8 commits.) } From f9ad8d8ca7c6053e83bb5bea0650f8e9147a114d Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Wed, 10 Sep 2025 21:39:55 -0400 Subject: [PATCH 10/11] fix: convert interface{} to any in test file and fix syntax error Signed-off-by: Atif Ali --- pkg/sync/sync_context_test.go | 48 +++++++++++++++++------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 846afb409..345dddc4f 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -2477,16 +2477,16 @@ func templateWithStorage(name, storage string) map[string]any { func TestStatefulSetImmutableFieldErrors(t *testing.T) { tests := []struct { name string - currentSpec map[string]interface{} - desiredSpec map[string]interface{} + currentSpec map[string]any + desiredSpec map[string]any expectedMessage string }{ { name: "single field change - serviceName", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": "old-svc", }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": "new-svc", }, expectedMessage: `attempting to change immutable fields: @@ -2498,15 +2498,15 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "volumeClaimTemplates change with storage size", - currentSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + currentSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data", }, - "spec": map[string]interface{}{ - "resources": map[string]interface{}{ - "requests": map[string]interface{}{ + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ "storage": "1Gi", }, }, @@ -2514,15 +2514,15 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, }, }, - desiredSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + desiredSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data", }, - "spec": map[string]interface{}{ - "resources": map[string]interface{}{ - "requests": map[string]interface{}{ + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ "storage": "2Gi", }, }, @@ -2539,16 +2539,16 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "selector change", - currentSpec: map[string]interface{}{ - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + currentSpec: map[string]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "old-app", }, }, }, - desiredSpec: map[string]interface{}{ - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + desiredSpec: map[string]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "new-app", }, }, From 88a977f921adb864aab3e8ad190c6965d01316a9 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Thu, 11 Sep 2025 08:08:45 -0400 Subject: [PATCH 11/11] converted interface{} to any for Go 1.18+ compatibility Signed-off-by: Atif Ali --- pkg/sync/sync_context_test.go | 70 +++++++++++++++++------------------ 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 345dddc4f..8e59ed0ec 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -2562,14 +2562,14 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "volumeClaimTemplates change from nil", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": "test-svc", }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": "test-svc", - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data", }, }, @@ -2584,24 +2584,24 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "complex volumeClaimTemplates change", - currentSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + currentSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data1", }, }, }, }, - desiredSpec: map[string]interface{}{ - "volumeClaimTemplates": []interface{}{ - map[string]interface{}{ - "metadata": map[string]interface{}{ + desiredSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ "name": "data1", }, }, - map[string]interface{}{ - "metadata": map[string]interface{}{ + map[string]any{ + "metadata": map[string]any{ "name": "data2", }, }, @@ -2616,27 +2616,27 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "multiple volumeClaimTemplate change", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": "postgresql-svc", - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage("static-files", "1Gi"), templateWithStorage("dexconfig", "1Gi"), templateWithStorage("argocd-dex-server-tls", "1Gi"), }, }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": "postgresql-svc", - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage("static-files", "2Gi"), templateWithStorage("dexconfig", "3Gi"), templateWithStorage("argocd-dex-server-tls", "4Gi"), @@ -2657,27 +2657,27 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina }, { name: "multiple field changes", - currentSpec: map[string]interface{}{ + currentSpec: map[string]any{ "serviceName": "postgresql-svc", - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage("static-files", "1Gi"), templateWithStorage("dexconfig", "1Gi"), templateWithStorage("argocd-dex-server-tls", "1Gi"), }, }, - desiredSpec: map[string]interface{}{ + desiredSpec: map[string]any{ "serviceName": "postgresql-svc-new", - "selector": map[string]interface{}{ - "matchLabels": map[string]interface{}{ + "selector": map[string]any{ + "matchLabels": map[string]any{ "app": "postgresql-new", }, }, - "volumeClaimTemplates": []interface{}{ + "volumeClaimTemplates": []any{ templateWithStorage("static-files", "2Gi"), templateWithStorage("dexconfig", "1Gi"), templateWithStorage("argocd-dex-server-tls", "1Gi"), @@ -2701,10 +2701,10 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { current := &unstructured.Unstructured{ - Object: map[string]interface{}{ + Object: map[string]any{ "apiVersion": testAPIVersion, "kind": "StatefulSet", - "metadata": map[string]interface{}{ + "metadata": map[string]any{ "name": testStatefulSet, "namespace": testNamespace, }, @@ -2713,10 +2713,10 @@ Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordina } desired := &unstructured.Unstructured{ - Object: map[string]interface{}{ + Object: map[string]any{ "apiVersion": testAPIVersion, "kind": "StatefulSet", - "metadata": map[string]interface{}{ + "metadata": map[string]any{ "name": testStatefulSet, "namespace": testNamespace, },