diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 8f4d51e4f..f7ca96566 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" @@ -1146,6 +1147,204 @@ 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. +func formatValue(v any) string { + if v == nil { + return "" + } + + 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 formatMultipleTemplates(templates) +} + +// 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) + } + } + 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)) + } + sort.Strings(labels) + return fmt.Sprintf("{%s}", strings.Join(labels, ", ")) +} + +// Get storage size from template +func getTemplateStorage(template map[string]any) string { + spec, ok := template["spec"].(map[string]any) + if !ok { + return "" + } + resources, ok := spec["resources"].(map[string]any) + if !ok { + return "" + } + requests, ok := resources["requests"].(map[string]any) + 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 any) string { + return fmt.Sprintf(" - %s:\n from: %s\n to: %s", + 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 { @@ -1197,6 +1396,13 @@ 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 { + if strings.Contains(err.Error(), "updates to statefulset spec for fields other than") { + if t.liveObj != nil && t.targetObj != nil { + if message, hasChanges := sc.validateStatefulSetUpdate(t.liveObj, t.targetObj); hasChanges { + return common.ResultCodeSyncFailed, message + } + } + } return common.ResultCodeSyncFailed, err.Error() } if kubeutil.IsCRD(t.targetObj) && !dryRun { @@ -1205,6 +1411,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 } diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 0e8d01ebb..c54755fc1 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{ {Kind: "Deployment", Group: "apps", Version: "v1", Namespaced: true, Verbs: standardVerbs}, }, @@ -2254,7 +2264,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") @@ -2262,3 +2271,291 @@ func TestNeedsClientSideApplyMigration(t *testing.T) { }) } } + +func templateWithStorage(name, storage string) map[string]any { + return map[string]any{ + "metadata": map[string]any{ + "name": name, + }, + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ + "storage": storage, + }, + }, + }, + } +} + +func TestStatefulSetImmutableFieldErrors(t *testing.T) { + tests := []struct { + name string + currentSpec map[string]any + desiredSpec map[string]any + expectedMessage string + }{ + { + name: "single field change - serviceName", + currentSpec: map[string]any{ + "serviceName": "old-svc", + }, + desiredSpec: map[string]any{ + "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]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ + "name": "data", + }, + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ + "storage": "1Gi", + }, + }, + }, + }, + }, + }, + desiredSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ + "name": "data", + }, + "spec": map[string]any{ + "resources": map[string]any{ + "requests": map[string]any{ + "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]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ + "app": "old-app", + }, + }, + }, + desiredSpec: map[string]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ + "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]any{ + "serviceName": "test-svc", + }, + desiredSpec: map[string]any{ + "serviceName": "test-svc", + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ + "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]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ + "name": "data1", + }, + }, + }, + }, + desiredSpec: map[string]any{ + "volumeClaimTemplates": []any{ + map[string]any{ + "metadata": map[string]any{ + "name": "data1", + }, + }, + map[string]any{ + "metadata": map[string]any{ + "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]any{ + "serviceName": postgresqlSvc, + "selector": map[string]any{ + "matchLabels": map[string]any{ + "app": "postgresql", + }, + }, + "volumeClaimTemplates": []any{ + templateWithStorage(staticFiles, "1Gi"), + templateWithStorage(dexconfig, "1Gi"), + templateWithStorage(argocdDexServerTLS, "1Gi"), + }, + }, + desiredSpec: map[string]any{ + "serviceName": postgresqlSvc, + "selector": map[string]any{ + "matchLabels": map[string]any{ + "app": "postgresql", + }, + }, + "volumeClaimTemplates": []any{ + templateWithStorage(staticFiles, "2Gi"), + templateWithStorage(dexconfig, "3Gi"), + templateWithStorage(argocdDexServerTLS, "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]any{ + "serviceName": postgresqlSvc, + "selector": map[string]any{ + "matchLabels": map[string]any{ + "app": "postgresql", + }, + }, + "volumeClaimTemplates": []any{ + templateWithStorage(staticFiles, "1Gi"), + templateWithStorage(dexconfig, "1Gi"), + templateWithStorage(argocdDexServerTLS, "1Gi"), + }, + }, + desiredSpec: map[string]any{ + "serviceName": "postgresql-svc-new", + "selector": map[string]any{ + "matchLabels": map[string]any{ + "app": "postgresql-new", + }, + }, + "volumeClaimTemplates": []any{ + templateWithStorage(staticFiles, "2Gi"), + templateWithStorage(dexconfig, "1Gi"), + templateWithStorage(argocdDexServerTLS, "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]any{ + "apiVersion": testAPIVersion, + "kind": "StatefulSet", + "metadata": map[string]any{ + "name": testStatefulSet, + "namespace": testNamespace, + }, + "spec": tt.currentSpec, + }, + } + + desired := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": testAPIVersion, + "kind": "StatefulSet", + "metadata": map[string]any{ + "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) + }) + } +}