diff --git a/pkg/appfile/template.go b/pkg/appfile/template.go index d326fd48e..9722c1652 100644 --- a/pkg/appfile/template.go +++ b/pkg/appfile/template.go @@ -44,6 +44,8 @@ const ( UsageTag = "+usage=" // ShortTag is the short alias annotation ShortTag = "+short" + // ImmutableTag marks a parameter field as immutable + ImmutableTag = "+immutable" ) // Template is a helper struct for processing capability including diff --git a/pkg/oam/labels.go b/pkg/oam/labels.go index 87111e1a7..40eba051d 100644 --- a/pkg/oam/labels.go +++ b/pkg/oam/labels.go @@ -159,6 +159,9 @@ const ( // AnnotationAutoUpdate is annotation that let application auto update when it finds definition changes AnnotationAutoUpdate = "app.oam.dev/autoUpdate" + // AnnotationForceParamMutations bypasses all immutable parameter field validation when set to "true". + AnnotationForceParamMutations = "app.oam.dev/force-param-mutations" + // AnnotationWorkflowName specifies the workflow name for execution. AnnotationWorkflowName = "app.oam.dev/workflowName" diff --git a/pkg/schema/immutable.go b/pkg/schema/immutable.go new file mode 100644 index 000000000..dd4258908 --- /dev/null +++ b/pkg/schema/immutable.go @@ -0,0 +1,114 @@ +/* +Copyright 2026 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package schema + +import ( + "strings" + + "cuelang.org/go/cue/ast" + "cuelang.org/go/cue/parser" + + "github.com/oam-dev/kubevela/pkg/appfile" +) + +// ImmutableFieldsFromTemplate parses a CUE template string and returns the set of +// dotted parameter field paths that are marked with the +immutable comment marker. +func ImmutableFieldsFromTemplate(templateStr string) map[string]bool { + if templateStr == "" { + return nil + } + f, err := parser.ParseFile("-", templateStr, parser.ParseComments) + if err != nil { + return nil + } + + paramStruct := getParameterStruct(f) + if paramStruct == nil { + return nil + } + + immutableFields := make(map[string]bool) + collectImmutableFields(paramStruct, "", immutableFields) + if len(immutableFields) == 0 { + return nil + } + return immutableFields +} + +// collectImmutableFields recursively walks a struct, collecting dotted +// paths of fields marked with +immutable into the result map. +func collectImmutableFields(structLit *ast.StructLit, prefix string, result map[string]bool) { + for _, element := range structLit.Elts { + field, ok := element.(*ast.Field) + if !ok { + continue + } + name := extractFieldName(field) + if name == "" { + continue + } + fieldPath := name + if prefix != "" { + fieldPath = prefix + "." + name + } + if hasImmutableComment(field) { + result[fieldPath] = true + continue + } + if nested, ok := field.Value.(*ast.StructLit); ok { + collectImmutableFields(nested, fieldPath, result) + } + } +} + +// getParameterStruct gets the top-level `parameter` struct +func getParameterStruct(f *ast.File) *ast.StructLit { + for _, decl := range f.Decls { + field, ok := decl.(*ast.Field) + if !ok || extractFieldName(field) != "parameter" { + continue + } + if structLit, ok := field.Value.(*ast.StructLit); ok { + return structLit + } + } + return nil +} + +// hasImmutableComment reports whether any comment group attached to field +// contains a line matching the +immutable marker. +func hasImmutableComment(field *ast.Field) bool { + for _, commentGroup := range field.Comments() { + for _, comment := range commentGroup.List { + marker := strings.TrimSpace(strings.TrimPrefix(comment.Text, "//")) + if marker == appfile.ImmutableTag { + return true + } + } + } + return false +} + +func extractFieldName(field *ast.Field) string { + switch label := field.Label.(type) { + case *ast.Ident: + return label.Name + case *ast.BasicLit: + return strings.Trim(label.Value, `"`) + } + return "" +} diff --git a/pkg/schema/immutable_test.go b/pkg/schema/immutable_test.go new file mode 100644 index 000000000..f002ef219 --- /dev/null +++ b/pkg/schema/immutable_test.go @@ -0,0 +1,138 @@ +/* +Copyright 2026 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package schema + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestImmutableFieldsFromTemplate(t *testing.T) { + cases := []struct { + name string + template string + want map[string]bool + }{ + { + name: "empty template", + template: "", + want: nil, + }, + { + name: "no parameter block", + template: ` +output: {}`, + want: nil, + }, + { + name: "parameter block with no immutable fields", + template: ` +parameter: { + // +usage=The image to use + image: string + replicas: int +}`, + want: nil, + }, + { + name: "parameter with one immutable field", + template: ` +parameter: { + // +immutable + image: string + replicas: int +}`, + want: map[string]bool{"image": true}, + }, + { + name: "multiple immutable fields", + template: ` +parameter: { + // +immutable + image: string + // +immutable + storageClass: string + replicas: int +}`, + want: map[string]bool{"image": true, "storageClass": true}, + }, + { + name: "immutable with usage and short tags on same field", + template: ` +parameter: { + // +usage=The image to use + // +short=i + // +immutable + image: string +}`, + want: map[string]bool{"image": true}, + }, + { + name: "nested immutable field returns dotted path", + template: ` +parameter: { + governance: { + // +immutable + tenant: string + region: string + } + image: string +}`, + want: map[string]bool{"governance.tenant": true}, + }, + { + name: "deeply nested immutable field", + template: ` +parameter: { + config: { + db: { + // +immutable + name: string + } + } +}`, + want: map[string]bool{"config.db.name": true}, + }, + { + name: "mixed top-level and nested immutable fields", + template: ` +parameter: { + // +immutable + image: string + governance: { + // +immutable + tenant: string + region: string + } +}`, + want: map[string]bool{"image": true, "governance.tenant": true}, + }, + { + name: "invalid CUE returns nil", + template: `parameter: { image: }`, + want: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := ImmutableFieldsFromTemplate(tc.template) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/pkg/schema/schema.go b/pkg/schema/schema.go index 2c44333fe..858fdf913 100644 --- a/pkg/schema/schema.go +++ b/pkg/schema/schema.go @@ -48,6 +48,9 @@ context: { // ErrGenerateOpenAPIV2JSONSchemaForCapability is the error while generating OpenAPI v3 schema const ErrGenerateOpenAPIV2JSONSchemaForCapability = "cannot generate OpenAPI v3 JSON schema for capability %s: %v" +// ExtensionImmutable is the OpenAPI extension key that marks a parameter field as immutable +const ExtensionImmutable = "x-immutable" + // ParsePropertiesToSchema parse the properties in cue script to the openapi schema func ParsePropertiesToSchema(ctx context.Context, s string, templateFieldPath ...string) (*openapi3.Schema, error) { t := s + "\n" + BaseTemplate @@ -110,6 +113,14 @@ func FixOpenAPISchema(name string, schema *openapi3.Schema) { } description := schema.Description + // Extract +immutable first, before other tags may strip the lines containing it. + if cleaned, found := extractMarkerFromDescription(description, appfile.ImmutableTag); found { + description = cleaned + if schema.Extensions == nil { + schema.Extensions = make(map[string]any) + } + schema.Extensions[ExtensionImmutable] = true + } if strings.Contains(description, appfile.UsageTag) { description = strings.Split(description, appfile.UsageTag)[1] } @@ -119,3 +130,22 @@ func FixOpenAPISchema(name string, schema *openapi3.Schema) { } schema.Description = description } + +// extractMarkerFromDescription removes all lines containing marker from description +// and reports whether the marker was found. Used to extract flag-style markers +// like +immutable that have no value component. +func extractMarkerFromDescription(description, marker string) (string, bool) { + found := false + var lines []string + for _, line := range strings.Split(description, "\n") { + if strings.TrimSpace(line) == marker { + found = true + } else { + lines = append(lines, line) + } + } + if !found { + return description, false + } + return strings.TrimSpace(strings.Join(lines, "\n")), true +} diff --git a/pkg/schema/schema_test.go b/pkg/schema/schema_test.go index 8089e28f8..98f9248bd 100644 --- a/pkg/schema/schema_test.go +++ b/pkg/schema/schema_test.go @@ -24,7 +24,9 @@ import ( "github.com/getkin/kin-openapi/openapi3" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/oam-dev/kubevela/pkg/appfile" "github.com/oam-dev/kubevela/pkg/cue/process" "github.com/oam-dev/kubevela/pkg/oam/util" ) @@ -196,3 +198,117 @@ func TestConvertOpenAPISchema2SwaggerObject(t *testing.T) { }) } } + +func TestExtractMarkerFromDescription(t *testing.T) { + cases := []struct { + name string + description string + marker string + wantDescription string + wantFound bool + }{ + { + name: "marker on its own line", + description: "some usage\n+immutable\nmore text", + marker: appfile.ImmutableTag, + wantDescription: "some usage\nmore text", + wantFound: true, + }, + { + name: "marker not present", + description: "some usage\nmore text", + marker: appfile.ImmutableTag, + wantDescription: "some usage\nmore text", + wantFound: false, + }, + { + name: "marker with leading whitespace", + description: "some usage\n +immutable\nmore text", + marker: appfile.ImmutableTag, + wantDescription: "some usage\nmore text", + wantFound: true, + }, + { + name: "marker is only content", + description: "+immutable", + marker: appfile.ImmutableTag, + wantDescription: "", + wantFound: true, + }, + { + name: "multiple marker lines", + description: "+immutable\nusage text\n+immutable", + marker: appfile.ImmutableTag, + wantDescription: "usage text", + wantFound: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, found := extractMarkerFromDescription(tc.description, tc.marker) + assert.Equal(t, tc.wantFound, found) + assert.Equal(t, tc.wantDescription, got) + }) + } +} + +func TestFixOpenAPISchemaImmutable(t *testing.T) { + cases := []struct { + name string + description string + wantImmutable bool + wantDesc string + }{ + { + name: "immutable marker sets extension and cleans description", + description: "+usage=The image to use\n+immutable", + wantImmutable: true, + wantDesc: "The image to use", + }, + { + name: "no immutable marker leaves schema unchanged", + description: "+usage=The image to use", + wantImmutable: false, + wantDesc: "The image to use", + }, + { + name: "immutable alongside short tag", + description: "+usage=The image to use\n+short=i\n+immutable", + wantImmutable: true, + wantDesc: "The image to use", + }, + { + name: "immutable before short tag", + description: "+immutable\n+usage=The image to use\n+short=i", + wantImmutable: true, + wantDesc: "The image to use", + }, + { + name: "immutable with no usage tag", + description: "+immutable", + wantImmutable: true, + wantDesc: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + schema := &openapi3.Schema{Description: tc.description} + FixOpenAPISchema("field", schema) + + assert.Equal(t, tc.wantDesc, schema.Description) + if tc.wantImmutable { + require.NotNil(t, schema.Extensions) + val, ok := schema.Extensions[ExtensionImmutable] + require.True(t, ok, "expected x-immutable extension to be set") + assert.Equal(t, true, val) + } else { + if schema.Extensions != nil { + _, ok := schema.Extensions[ExtensionImmutable] + assert.False(t, ok, "expected x-immutable extension to not be set") + } + } + }) + } +} diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/immutable.go b/pkg/webhook/core.oam.dev/v1beta1/application/immutable.go new file mode 100644 index 000000000..66e1283fc --- /dev/null +++ b/pkg/webhook/core.oam.dev/v1beta1/application/immutable.go @@ -0,0 +1,242 @@ +/* +Copyright 2026 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package application + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "strings" + + wfTypesv1alpha1 "github.com/kubevela/pkg/apis/oam/v1alpha1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/klog/v2" + + "github.com/oam-dev/kubevela/apis/core.oam.dev/common" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" + "github.com/oam-dev/kubevela/apis/types" + "github.com/oam-dev/kubevela/pkg/appfile" + "github.com/oam-dev/kubevela/pkg/oam" + oamutil "github.com/oam-dev/kubevela/pkg/oam/util" + "github.com/oam-dev/kubevela/pkg/schema" +) + +// ValidateImmutableFields validates that immutable parameter fields (`// +immutable`) have not changed +// All checks are bypassed if the app carries the app.oam.dev/force-mutate: "true" annotation. +func (h *ValidatingHandler) ValidateImmutableFields(ctx context.Context, newApp, oldApp *v1beta1.Application) field.ErrorList { + if newApp.Annotations[oam.AnnotationForceParamMutations] == "true" { + return nil + } + defCtx := oamutil.SetNamespaceInCtx(ctx, newApp.Namespace) + + var errs field.ErrorList + errs = append(errs, h.validateComponentImmutableFields(defCtx, newApp, oldApp)...) + errs = append(errs, h.validatePolicyImmutableFields(defCtx, newApp, oldApp)...) + errs = append(errs, h.validateWorkflowStepImmutableFields(defCtx, newApp, oldApp)...) + return errs +} + +// validateComponentImmutableFields checks immutability for components and their traits. +func (h *ValidatingHandler) validateComponentImmutableFields(ctx context.Context, newApp, oldApp *v1beta1.Application) field.ErrorList { + oldByName := make(map[string]common.ApplicationComponent, len(oldApp.Spec.Components)) + for _, c := range oldApp.Spec.Components { + oldByName[c.Name] = c + } + + var errs field.ErrorList + for i, newComp := range newApp.Spec.Components { + oldComp, exists := oldByName[newComp.Name] + if !exists || oldComp.Type != newComp.Type { + continue + } + + tmpl, err := appfile.LoadTemplate(ctx, h.Client, newComp.Type, types.TypeComponentDefinition, newApp.Annotations) + if err != nil { + klog.V(4).Infof("immutable check: skipping component %q (type %q): %v", newComp.Name, newComp.Type, err) + continue + } + fp := field.NewPath("spec", "components").Index(i).Child("properties") + errs = append(errs, checkImmutableParams(tmpl.TemplateStr, fp, oldComp.Properties, newComp.Properties)...) + errs = append(errs, h.validateTraitImmutableFields(ctx, newApp, i, newComp.Traits, oldComp.Traits)...) + } + return errs +} + +// validateTraitImmutableFields checks immutability for the traits on a single component. +func (h *ValidatingHandler) validateTraitImmutableFields(ctx context.Context, app *v1beta1.Application, compIdx int, newTraits, oldTraits []common.ApplicationTrait) field.ErrorList { + var errs field.ErrorList + for j, newTrait := range newTraits { + if j >= len(oldTraits) || oldTraits[j].Type != newTrait.Type { + continue + } + oldTrait := oldTraits[j] + tmpl, err := appfile.LoadTemplate(ctx, h.Client, newTrait.Type, types.TypeTrait, app.Annotations) + if err != nil { + klog.V(4).Infof("immutable check: skipping trait %q on component[%d]: %v", newTrait.Type, compIdx, err) + continue + } + fp := field.NewPath("spec", "components").Index(compIdx).Child("traits").Index(j).Child("properties") + errs = append(errs, checkImmutableParams(tmpl.TemplateStr, fp, oldTrait.Properties, newTrait.Properties)...) + } + return errs +} + +// validatePolicyImmutableFields checks immutability for policies. +func (h *ValidatingHandler) validatePolicyImmutableFields(ctx context.Context, newApp, oldApp *v1beta1.Application) field.ErrorList { + oldByName := make(map[string]v1beta1.AppPolicy, len(oldApp.Spec.Policies)) + for _, p := range oldApp.Spec.Policies { + oldByName[p.Name] = p + } + + var errs field.ErrorList + for i, newPolicy := range newApp.Spec.Policies { + oldPolicy, exists := oldByName[newPolicy.Name] + if !exists || oldPolicy.Type != newPolicy.Type { + continue + } + tmpl, err := appfile.LoadTemplate(ctx, h.Client, newPolicy.Type, types.TypePolicy, newApp.Annotations) + if err != nil { + klog.V(4).Infof("immutable check: skipping policy %q (type %q): %v", newPolicy.Name, newPolicy.Type, err) + continue + } + fp := field.NewPath("spec", "policies").Index(i).Child("properties") + errs = append(errs, checkImmutableParams(tmpl.TemplateStr, fp, oldPolicy.Properties, newPolicy.Properties)...) + } + return errs +} + +// validateWorkflowStepImmutableFields checks immutability for workflow steps. +func (h *ValidatingHandler) validateWorkflowStepImmutableFields(ctx context.Context, newApp, oldApp *v1beta1.Application) field.ErrorList { + if newApp.Spec.Workflow == nil || oldApp.Spec.Workflow == nil { + return nil + } + + oldByName := indexWorkflowStepsByName(oldApp.Spec.Workflow.Steps) + + var errs field.ErrorList + for i, newStep := range newApp.Spec.Workflow.Steps { + oldStep, exists := oldByName[newStep.Name] + if !exists || oldStep.Type != newStep.Type { + continue + } + tmpl, err := appfile.LoadTemplate(ctx, h.Client, newStep.Type, types.TypeWorkflowStep, newApp.Annotations) + if err != nil { + klog.V(4).Infof("immutable check: skipping workflow step %q (type %q): %v", newStep.Name, newStep.Type, err) + continue + } + fp := field.NewPath("spec", "workflow", "steps").Index(i).Child("properties") + errs = append(errs, checkImmutableParams(tmpl.TemplateStr, fp, oldStep.Properties, newStep.Properties)...) + } + return errs +} + +// checkImmutableParams extracts immutable field paths from templateStr and verifies +// that those fields have not changed between oldProps and newProps +func checkImmutableParams(templateStr string, fp *field.Path, oldProps, newProps *runtime.RawExtension) field.ErrorList { + immutableFields := schema.ImmutableFieldsFromTemplate(templateStr) + if len(immutableFields) == 0 { + return nil + } + + oldMap := rawExtensionToMap(oldProps) + newMap := rawExtensionToMap(newProps) + + var errs field.ErrorList + for fieldPath := range immutableFields { + oldVal, oldOK := getNestedValue(oldMap, fieldPath) + newVal, newOK := getNestedValue(newMap, fieldPath) + + if !oldOK { + // Field was not set before — first-time population is allowed + continue + } + if !newOK { + errs = append(errs, field.Forbidden(fp.Key(fieldPath), + fmt.Sprintf("immutable field cannot be removed (current: %s)", jsonString(oldVal)))) + continue + } + if !jsonEqual(oldVal, newVal) { + errs = append(errs, field.Forbidden(fp.Key(fieldPath), + fmt.Sprintf("immutable field cannot be changed (current: %s, new: %s)", jsonString(oldVal), jsonString(newVal)))) + } + } + return errs +} + +// getNestedValue retrieves a value from a nested map using a dotted path. +// Returns (value, true) if found, (nil, false) if any segment is missing. +func getNestedValue(m map[string]any, path string) (any, bool) { + parts := strings.SplitN(path, ".", 2) + val, ok := m[parts[0]] + if !ok { + return nil, false + } + if len(parts) == 1 { + return val, true + } + nested, ok := val.(map[string]any) + if !ok { + return nil, false + } + return getNestedValue(nested, parts[1]) +} + +// rawExtensionToMap decodes a *runtime.RawExtension into a map[string]any. +// Returns nil on nil input or parse error. +func rawExtensionToMap(ext *runtime.RawExtension) map[string]any { + if ext == nil || len(ext.Raw) == 0 { + return nil + } + var m map[string]any + if err := json.Unmarshal(ext.Raw, &m); err != nil { + return nil + } + return m +} + +// jsonString returns a compact JSON representation of v, or "" on error. +func jsonString(v any) string { + b, err := json.Marshal(v) + if err != nil { + return "" + } + return string(b) +} + +// jsonEqual reports whether two values are equal by comparing their JSON encodings. +func jsonEqual(a, b any) bool { + aJSON, err := json.Marshal(a) + if err != nil { + return false + } + bJSON, err := json.Marshal(b) + if err != nil { + return false + } + return bytes.Equal(aJSON, bJSON) +} + +// indexWorkflowStepsByName returns a map of workflow steps indexed by name. +func indexWorkflowStepsByName(steps []wfTypesv1alpha1.WorkflowStep) map[string]wfTypesv1alpha1.WorkflowStep { + byName := make(map[string]wfTypesv1alpha1.WorkflowStep, len(steps)) + for _, step := range steps { + byName[step.Name] = step + } + return byName +} diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/immutable_test.go b/pkg/webhook/core.oam.dev/v1beta1/application/immutable_test.go new file mode 100644 index 000000000..8071c0d2c --- /dev/null +++ b/pkg/webhook/core.oam.dev/v1beta1/application/immutable_test.go @@ -0,0 +1,246 @@ +/* +Copyright 2026 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package application + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/oam-dev/kubevela/apis/core.oam.dev/common" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" +) + +// mustRaw marshals v to a *runtime.RawExtension, panicking on error. +func mustRaw(t *testing.T, v map[string]any) *runtime.RawExtension { + t.Helper() + b, err := json.Marshal(v) + require.NoError(t, err) + return &runtime.RawExtension{Raw: b} +} + +func TestCheckImmutableParams(t *testing.T) { + const tmplWithImmutable = ` +parameter: { + // +immutable + image: string + replicas: int +}` + + fp := field.NewPath("spec", "components").Index(0).Child("properties") + + cases := []struct { + name string + template string + oldProps *runtime.RawExtension + newProps *runtime.RawExtension + wantErrCount int + }{ + { + name: "no immutable fields in template", + template: `parameter: { image: string }`, + oldProps: mustRaw(t, map[string]any{"image": "v1"}), + newProps: mustRaw(t, map[string]any{"image": "v2"}), + wantErrCount: 0, + }, + { + name: "immutable field unchanged", + template: tmplWithImmutable, + oldProps: mustRaw(t, map[string]any{"image": "v1", "replicas": 2}), + newProps: mustRaw(t, map[string]any{"image": "v1", "replicas": 3}), + wantErrCount: 0, + }, + { + name: "immutable field changed", + template: tmplWithImmutable, + oldProps: mustRaw(t, map[string]any{"image": "v1"}), + newProps: mustRaw(t, map[string]any{"image": "v2"}), + wantErrCount: 1, + }, + { + name: "immutable field removed", + template: tmplWithImmutable, + oldProps: mustRaw(t, map[string]any{"image": "v1"}), + newProps: mustRaw(t, map[string]any{}), + wantErrCount: 1, + }, + { + name: "immutable field first set (was absent before)", + template: tmplWithImmutable, + oldProps: mustRaw(t, map[string]any{}), + newProps: mustRaw(t, map[string]any{"image": "v1"}), + wantErrCount: 0, + }, + { + name: "nil old props", + template: tmplWithImmutable, + oldProps: nil, + newProps: mustRaw(t, map[string]any{"image": "v1"}), + wantErrCount: 0, + }, + { + name: "nested immutable field unchanged", + template: `parameter: { governance: { // +immutable +tenant: string } }`, + oldProps: mustRaw(t, map[string]any{"governance": map[string]any{"tenant": "acme"}}), + newProps: mustRaw(t, map[string]any{"governance": map[string]any{"tenant": "acme"}}), + wantErrCount: 0, + }, + { + name: "nested immutable field changed", + template: ` +parameter: { + governance: { + // +immutable + tenant: string + region: string + } +}`, + oldProps: mustRaw(t, map[string]any{"governance": map[string]any{"tenant": "acme", "region": "us-east"}}), + newProps: mustRaw(t, map[string]any{"governance": map[string]any{"tenant": "other", "region": "us-east"}}), + wantErrCount: 1, + }, + { + name: "nested mutable sibling field can change", + template: ` +parameter: { + governance: { + // +immutable + tenant: string + region: string + } +}`, + oldProps: mustRaw(t, map[string]any{"governance": map[string]any{"tenant": "acme", "region": "us-east"}}), + newProps: mustRaw(t, map[string]any{"governance": map[string]any{"tenant": "acme", "region": "eu-west"}}), + wantErrCount: 0, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + errs := checkImmutableParams(tc.template, fp, tc.oldProps, tc.newProps) + assert.Len(t, errs, tc.wantErrCount) + }) + } +} + +func TestValidateImmutableFields(t *testing.T) { + const immutableTemplate = ` +parameter: { + // +immutable + image: string + replicas: int +}` + + scheme := runtime.NewScheme() + _ = v1beta1.AddToScheme(scheme) + + // Seed a ComponentDefinition with an immutable field in vela-system + compDef := &v1beta1.ComponentDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "webservice", + Namespace: "vela-system", + }, + Spec: v1beta1.ComponentDefinitionSpec{ + Schematic: &common.Schematic{ + CUE: &common.CUE{Template: immutableTemplate}, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(compDef). + Build() + + handler := &ValidatingHandler{Client: fakeClient} + ctx := context.Background() + + makeApp := func(image string, annotations map[string]string) *v1beta1.Application { + return &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + Annotations: annotations, + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + Properties: mustRaw(t, map[string]any{"image": image, "replicas": 3}), + }, + }, + }, + } + } + + t.Run("immutable field unchanged - no error", func(t *testing.T) { + errs := handler.ValidateImmutableFields(ctx, makeApp("v1", nil), makeApp("v1", nil)) + assert.Empty(t, errs) + }) + + t.Run("immutable field changed - error", func(t *testing.T) { + errs := handler.ValidateImmutableFields(ctx, makeApp("v2", nil), makeApp("v1", nil)) + require.Len(t, errs, 1) + assert.Contains(t, errs[0].Detail, `immutable field cannot be changed (current: "v1", new: "v2")`) + }) + + t.Run("force-param-mutations bypasses all checks", func(t *testing.T) { + errs := handler.ValidateImmutableFields(ctx, + makeApp("v2", map[string]string{"app.oam.dev/force-param-mutations": "true"}), + makeApp("v1", nil), + ) + assert.Empty(t, errs) + }) + + t.Run("unknown definition type - skip gracefully", func(t *testing.T) { + newApp := &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "test-app", Namespace: "default"}, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "unknown-type", + Properties: mustRaw(t, map[string]any{"image": "v2"}), + }, + }, + }, + } + oldApp := &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "test-app", Namespace: "default"}, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "unknown-type", + Properties: mustRaw(t, map[string]any{"image": "v1"}), + }, + }, + }, + } + errs := handler.ValidateImmutableFields(ctx, newApp, oldApp) + assert.Empty(t, errs) + }) +} diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go b/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go index b02f65d73..8260e6411 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go @@ -56,8 +56,8 @@ func simplifyError(err error) error { func mergeErrors(errs field.ErrorList) error { s := "" - for _, err := range errs { - s += fmt.Sprintf("field \"%s\": %s error encountered, %s. ", err.Field, err.Type, err.Detail) + for i, err := range errs { + s += fmt.Sprintf("\n %d) %q: %s.", i+1, err.Field, err.Detail) } return errors.New(s) } diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validation.go b/pkg/webhook/core.oam.dev/v1beta1/application/validation.go index efc5152f1..ab658345f 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/validation.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validation.go @@ -464,9 +464,8 @@ func (h *ValidatingHandler) ValidateCreate(ctx context.Context, app *v1beta1.App } // ValidateUpdate validates the Application on update -func (h *ValidatingHandler) ValidateUpdate(ctx context.Context, newApp, _ *v1beta1.Application, req admission.Request) field.ErrorList { - // check if the newApp is valid +func (h *ValidatingHandler) ValidateUpdate(ctx context.Context, newApp, oldApp *v1beta1.Application, req admission.Request) field.ErrorList { errs := h.ValidateCreate(ctx, newApp, req) - // TODO: add more validating + errs = append(errs, h.ValidateImmutableFields(ctx, newApp, oldApp)...) return errs }