From 257d9de1ade366b7853ce97e55cd4c5461373078 Mon Sep 17 00:00:00 2001 From: Atif Ali Date: Fri, 13 Dec 2024 15:05:20 -0500 Subject: [PATCH] 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 | 296 +++++++++++++++++++++++++++++++++- 2 files changed, 440 insertions(+), 1 deletion(-) diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 35981ebaa..17a142016 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" @@ -965,6 +966,85 @@ func (sc *syncContext) shouldUseServerSideApply(targetObj *unstructured.Unstruct return sc.serverSideApply || resourceutil.HasAnnotationOption(targetObj, common.AnnotationSyncOptions, common.SyncOptionServerSideApply) } +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 { @@ -1005,6 +1085,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, false) } 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 kube.IsCRD(t.targetObj) && !dryRun { diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 7e416d20b..d5715300c 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -38,6 +38,12 @@ import ( testingutils "github.com/argoproj/gitops-engine/pkg/utils/testing" ) +const ( + testAPIVersion = "apps/v1" + testStatefulSet = "test-statefulset" + testNamespace = "default" +) + var standardVerbs = v1.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 +58,7 @@ func newTestSyncCtx(getResourceFunc *func(ctx context.Context, config *rest.Conf }, }, &v1.APIResourceList{ - GroupVersion: "apps/v1", + GroupVersion: testAPIVersion, APIResources: []v1.APIResource{ {Kind: "Deployment", Group: "apps", Version: "v1", Namespaced: true, Verbs: standardVerbs}, }, @@ -2039,3 +2045,291 @@ func TestWaitForCleanUpBeforeNextWave(t *testing.T) { assert.Equal(t, synccommon.ResultCodePruned, result[2].Status) } + +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) + }) + } +}