diff --git a/pkg/controller/core.oam.dev/v1beta1/application/policy_transforms.go b/pkg/controller/core.oam.dev/v1beta1/application/policy_transforms.go index 9e5331eca..24515aff4 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/policy_transforms.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/policy_transforms.go @@ -36,6 +36,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -250,8 +251,7 @@ func (h *AppHandler) ApplyApplicationScopeTransforms(ctx monitorContext.Context, // Record successful application in status if changes != nil { - // Determine if spec was modified by checking if transforms.Spec exists - changes.SpecModified = changes.Transforms != nil && changes.Transforms.Spec != nil + // SpecModified is already set correctly in applyPolicyTransform allPolicyChanges[policy.Name] = changes // Store for ConfigMap serialization } recordApplicationPolicyStatus(app, policy.Name, templ.PolicyDefinition.Namespace, "explicit", sequence, 0, true, "", changes) @@ -300,13 +300,13 @@ func (h *AppHandler) ApplyApplicationScopeTransforms(ctx monitorContext.Context, "application_hash": appHash, // Hash of Application for cache invalidation } - // Get the full policy changes if available (includes transforms, labels, annotations, context) + // Get the full policy changes if available (includes output, labels, annotations, context) if policyChanges, ok := allPolicyChanges[policyName]; ok && policyChanges != nil { - // Store the full transforms object in reusable format - if policyChanges.Transforms != nil { - transformsData := serializeTransformsForStorage(policyChanges.Transforms) - if len(transformsData) > 0 { - policyRecord["transforms"] = transformsData + // Store the output object in reusable format + if policyChanges.Output != nil { + outputData := serializeOutputForStorage(policyChanges.Output) + if len(outputData) > 0 { + policyRecord["output"] = outputData } } @@ -396,16 +396,26 @@ func (h *AppHandler) applyPolicyTransform(ctx monitorContext.Context, app *v1bet return ctx, nil, nil } - // Extract transforms field - transforms, err := h.extractTransforms(rendered) + // Extract output (new API only) + output, err := h.extractOutput(rendered) if err != nil { - return ctx, nil, errors.Wrap(err, "failed to extract transforms") + return ctx, nil, errors.Wrap(err, "failed to extract output") + } + + // Check for deprecated transforms API + if _, err := h.extractTransforms(rendered); err != nil { + return ctx, nil, err // Return the deprecation error + } + + // Require output field + if output == nil { + return ctx, nil, errors.New("policy must specify 'output' field - see documentation for API reference") } // Track changes from before transform application changes := &PolicyChanges{ - Enabled: true, // We already checked it's enabled above - Transforms: transforms, + Enabled: true, // We already checked it's enabled above + Output: output, } // Take snapshot of labels and annotations BEFORE applying transform @@ -422,12 +432,49 @@ func (h *AppHandler) applyPolicyTransform(ctx monitorContext.Context, app *v1bet } } - // Apply transforms to the in-memory Application - if transforms != nil { - if err := h.applyTransformsToApplication(ctx, app, transforms); err != nil { - return ctx, nil, errors.Wrap(err, "failed to apply transforms") + // Apply output to Application spec + // Build new spec struct (like old code did) to survive status patch operations + newSpec := app.Spec.DeepCopy() + + if output.Components != nil { + newSpec.Components = output.Components + ctx.Info("Replaced components from output", "policy", policyRef.Type, "count", len(output.Components)) + changes.SpecModified = true + } + if output.Workflow != nil { + newSpec.Workflow = output.Workflow + ctx.Info("Replaced workflow from output", "policy", policyRef.Type) + changes.SpecModified = true + } + if output.Policies != nil { + newSpec.Policies = output.Policies + ctx.Info("Replaced policies from output", "policy", policyRef.Type, "count", len(output.Policies)) + changes.SpecModified = true + } + + // Replace entire spec (matches old transforms behavior) + app.Spec = *newSpec + + // Apply labels (always merge) + if output.Labels != nil && len(output.Labels) > 0 { + if app.Labels == nil { + app.Labels = make(map[string]string) } - ctx.Info("Applied transforms to Application", "policy", policyRef.Type) + for k, v := range output.Labels { + app.Labels[k] = v + } + ctx.Info("Merged labels from output", "policy", policyRef.Type, "count", len(output.Labels)) + } + + // Apply annotations (always merge) + if output.Annotations != nil && len(output.Annotations) > 0 { + if app.Annotations == nil { + app.Annotations = make(map[string]string) + } + for k, v := range output.Annotations { + app.Annotations[k] = v + } + ctx.Info("Merged annotations from output", "policy", policyRef.Type, "count", len(output.Annotations)) } // Compare AFTER to capture actual changes (works regardless of how CUE modifies the Application) @@ -455,11 +502,8 @@ func (h *AppHandler) applyPolicyTransform(ctx monitorContext.Context, app *v1bet changes.AddedAnnotations = annotationsAdded } - // Extract and store additionalContext - additionalContext, err := h.extractAdditionalContext(rendered) - if err != nil { - return ctx, nil, errors.Wrap(err, "failed to extract additionalContext") - } + // Store ctx as additionalContext + additionalContext := output.Ctx if additionalContext != nil { ctx = storeAdditionalContextInCtx(ctx, additionalContext) @@ -490,9 +534,13 @@ func (h *AppHandler) renderPolicy(ctx monitorContext.Context, app *v1beta1.Appli } else if cachedRecord != nil { ctx.Info("Using cached policy result", "policy", policyDef.Name, "ttl", ttlSeconds) - // Deserialize the cached result - if transformsData, ok := cachedRecord["transforms"].(map[string]interface{}); ok { - result.Transforms = deserializeTransformsFromStorage(transformsData) + // Deserialize the cached output + if outputData, ok := cachedRecord["output"].(map[string]interface{}); ok { + result.Transforms = deserializeOutputFromStorage(outputData) + } else { + // No output in cache - policy needs re-rendering + klog.Warningf("Policy %s cached without output data - will re-render", policyDef.Name) + return RenderedPolicyResult{}, nil } if additionalContext, ok := cachedRecord["additional_context"].(map[string]interface{}); ok { @@ -550,21 +598,29 @@ func (h *AppHandler) renderPolicy(ctx monitorContext.Context, app *v1beta1.Appli return result, nil } - // Extract transforms field - transforms, err := h.extractTransforms(rendered) + // Extract output (new API only) + output, err := h.extractOutput(rendered) if err != nil { - result.SkipReason = fmt.Sprintf("transforms extraction error: %s", err.Error()) - return result, errors.Wrap(err, "failed to extract transforms") + result.SkipReason = fmt.Sprintf("output extraction error: %s", err.Error()) + return result, errors.Wrap(err, "failed to extract output") } - result.Transforms = transforms - // Extract additionalContext - additionalContext, err := h.extractAdditionalContext(rendered) - if err != nil { - result.SkipReason = fmt.Sprintf("additionalContext extraction error: %s", err.Error()) - return result, errors.Wrap(err, "failed to extract additionalContext") + // Check for deprecated transforms API + if _, err := h.extractTransforms(rendered); err != nil { + result.SkipReason = "using deprecated transforms API" + return result, err // Return the deprecation error } - result.AdditionalContext = additionalContext + + // Require output field + if output == nil { + result.SkipReason = "missing output field" + return result, errors.New("policy must specify 'output' field - see documentation for API reference") + } + + // Store output + result.Transforms = output + // Extract ctx as additionalContext + result.AdditionalContext = output.Ctx return result, nil } @@ -579,59 +635,58 @@ func (h *AppHandler) applyRenderedPolicyResult(ctx monitorContext.Context, app * return ctx, nil, nil } - // Cast transforms from interface{} - transforms, ok := result.Transforms.(*PolicyTransforms) - if !ok && result.Transforms != nil { - return ctx, nil, errors.Errorf("cached transforms have invalid type for policy %s", result.PolicyName) + // Extract PolicyOutput from cached result + output, ok := result.Transforms.(*PolicyOutput) + if !ok || output == nil { + return ctx, nil, errors.Errorf("cached policy has invalid or missing output for policy %s", result.PolicyName) } // Track what changes we're making changes := &PolicyChanges{ AdditionalContext: result.AdditionalContext, Enabled: result.Enabled, - Transforms: transforms, + Output: output, } - // Apply transforms to the in-memory Application - if transforms != nil { - if err := h.applyTransformsToApplication(ctx, app, transforms); err != nil { - return ctx, nil, errors.Wrap(err, "failed to apply transforms") - } - ctx.Info("Applied cached transforms to Application", "policy", result.PolicyName) + // Apply output to Application spec + if output.Components != nil { + app.Spec.Components = output.Components + ctx.Info("Replaced components from output", "policy", result.PolicyName, "count", len(output.Components)) + changes.SpecModified = true + } + if output.Workflow != nil { + app.Spec.Workflow = output.Workflow + ctx.Info("Replaced workflow from output", "policy", result.PolicyName) + changes.SpecModified = true + } + if output.Policies != nil { + app.Spec.Policies = output.Policies + ctx.Info("Replaced policies from output", "policy", result.PolicyName, "count", len(output.Policies)) + changes.SpecModified = true + } - // Extract changes directly from transforms - if transforms.Labels != nil { - if labelsMap, ok := transforms.Labels.Value.(map[string]string); ok { - changes.AddedLabels = labelsMap - } else if labelsMap, ok := transforms.Labels.Value.(map[string]interface{}); ok { - // Convert from interface{} map - stringMap := make(map[string]string) - for k, v := range labelsMap { - if strVal, ok := v.(string); ok { - stringMap[k] = strVal - } - } - changes.AddedLabels = stringMap - } + // Apply labels (always merge) + if output.Labels != nil && len(output.Labels) > 0 { + if app.Labels == nil { + app.Labels = make(map[string]string) } - - if transforms.Annotations != nil { - if annotationsMap, ok := transforms.Annotations.Value.(map[string]string); ok { - changes.AddedAnnotations = annotationsMap - } else if annotationsMap, ok := transforms.Annotations.Value.(map[string]interface{}); ok { - // Convert from interface{} map - stringMap := make(map[string]string) - for k, v := range annotationsMap { - if strVal, ok := v.(string); ok { - stringMap[k] = strVal - } - } - changes.AddedAnnotations = stringMap - } + for k, v := range output.Labels { + app.Labels[k] = v } + changes.AddedLabels = output.Labels + ctx.Info("Merged labels from output", "policy", result.PolicyName, "count", len(output.Labels)) + } - // Check if spec was modified - changes.SpecModified = transforms.Spec != nil + // Apply annotations (always merge) + if output.Annotations != nil && len(output.Annotations) > 0 { + if app.Annotations == nil { + app.Annotations = make(map[string]string) + } + for k, v := range output.Annotations { + app.Annotations[k] = v + } + changes.AddedAnnotations = output.Annotations + ctx.Info("Merged annotations from output", "policy", result.PolicyName, "count", len(output.Annotations)) } // Store additionalContext in context @@ -765,66 +820,72 @@ type Transform struct { Value interface{} `json:"value"` } -// PolicyTransforms represents the allowed transformation operations +// PolicyTransforms represents the allowed transformation operations (old API) type PolicyTransforms struct { Spec *Transform `json:"spec,omitempty"` Labels *Transform `json:"labels,omitempty"` Annotations *Transform `json:"annotations,omitempty"` } +// PolicyOutput represents the new simplified output structure (new API) +type PolicyOutput struct { + Components []common.ApplicationComponent `json:"components,omitempty"` + Workflow *v1beta1.Workflow `json:"workflow,omitempty"` + Policies []v1beta1.AppPolicy `json:"policies,omitempty"` + Labels map[string]string `json:"labels,omitempty"` + Annotations map[string]string `json:"annotations,omitempty"` + Ctx map[string]interface{} `json:"ctx,omitempty"` +} + // extractTransforms extracts the transforms field from rendered CUE // Only spec, labels, and annotations with type+value structure are permitted func (h *AppHandler) extractTransforms(val cue.Value) (*PolicyTransforms, error) { transformsVal := val.LookupPath(cue.ParsePath("transforms")) if !transformsVal.Exists() { - // No transforms field, that's OK + // No transforms field, that's OK - policy should use output API return nil, nil } - var transforms PolicyTransforms - if err := transformsVal.Decode(&transforms); err != nil { - return nil, errors.Wrap(err, "failed to decode transforms") + // Reject old transforms API - policies must use new output API + return nil, errors.New("the 'transforms' field is deprecated - please use 'output' field instead. See documentation for migration guide") +} + +// extractOutput extracts the output field from rendered CUE (new API) +// Returns nil if output doesn't exist (old API being used or no output specified) +func (h *AppHandler) extractOutput(val cue.Value) (*PolicyOutput, error) { + outputVal := val.LookupPath(cue.ParsePath("output")) + if !outputVal.Exists() { + return nil, nil // No output field } - // Validate structure: only 'spec', 'labels', and 'annotations' are allowed - iter, err := transformsVal.Fields() + var output PolicyOutput + if err := outputVal.Decode(&output); err != nil { + return nil, errors.Wrap(err, "failed to decode output") + } + + // Validate structure: only allowed fields + iter, err := outputVal.Fields() if err != nil { - return nil, errors.Wrap(err, "failed to iterate transforms fields") + return nil, errors.Wrap(err, "failed to iterate output fields") } allowedFields := map[string]bool{ - "spec": true, + "components": true, + "workflow": true, + "policies": true, "labels": true, "annotations": true, + "ctx": true, } for iter.Next() { fieldName := iter.Selector().String() if !allowedFields[fieldName] { - return nil, errors.Errorf("transforms.%s is not allowed; only 'spec', 'labels', and 'annotations' are permitted", fieldName) + return nil, errors.Errorf("output.%s is not allowed; only 'components', 'workflow', 'policies', 'labels', 'annotations', and 'ctx' are permitted", fieldName) } } - // Validate each transform has correct type - if transforms.Spec != nil { - if err := validateTransformType(transforms.Spec.Type, "spec", true); err != nil { - return nil, err - } - } - if transforms.Labels != nil { - // Labels only support merge for safety - if err := validateTransformType(transforms.Labels.Type, "labels", false); err != nil { - return nil, err - } - } - if transforms.Annotations != nil { - // Annotations only support merge for safety - if err := validateTransformType(transforms.Annotations.Type, "annotations", false); err != nil { - return nil, err - } - } - - return &transforms, nil + return &output, nil } // validateTransformType ensures the transform type is valid @@ -1083,7 +1144,8 @@ type PolicyChanges struct { // Full rendered output for caching/reuse Enabled bool - Transforms *PolicyTransforms + Transforms *PolicyTransforms // Old transforms API + Output *PolicyOutput // New output API } // policyConfigMapMetadata tracks metadata needed for ConfigMap storage @@ -1127,6 +1189,113 @@ func serializeTransformsForStorage(transforms *PolicyTransforms) map[string]inte return result } +// serializeOutputForStorage converts PolicyOutput to a format suitable for storage and reuse +func serializeOutputForStorage(output *PolicyOutput) map[string]interface{} { + if output == nil { + return nil + } + + result := make(map[string]interface{}) + + if output.Components != nil { + result["components"] = output.Components + } + + if output.Workflow != nil { + result["workflow"] = output.Workflow + } + + if output.Policies != nil { + result["policies"] = output.Policies + } + + if output.Labels != nil { + result["labels"] = output.Labels + } + + if output.Annotations != nil { + result["annotations"] = output.Annotations + } + + if output.Ctx != nil { + result["ctx"] = output.Ctx + } + + return result +} + +// deserializeOutputFromStorage converts stored output back to PolicyOutput +func deserializeOutputFromStorage(outputData map[string]interface{}) *PolicyOutput { + if outputData == nil { + return nil + } + + output := &PolicyOutput{} + + // Decode components + if componentsData, ok := outputData["components"]; ok { + jsonBytes, err := json.Marshal(componentsData) + if err != nil { + klog.Errorf("Failed to marshal components data: %v", err) + } else { + if err := json.Unmarshal(jsonBytes, &output.Components); err != nil { + klog.Errorf("Failed to unmarshal components: %v", err) + } + } + } + + // Decode workflow + if workflowData, ok := outputData["workflow"]; ok { + jsonBytes, err := json.Marshal(workflowData) + if err != nil { + klog.Errorf("Failed to marshal workflow data: %v", err) + } else { + if err := json.Unmarshal(jsonBytes, &output.Workflow); err != nil { + klog.Errorf("Failed to unmarshal workflow: %v", err) + } + } + } + + // Decode policies + if policiesData, ok := outputData["policies"]; ok { + jsonBytes, err := json.Marshal(policiesData) + if err != nil { + klog.Errorf("Failed to marshal policies data: %v", err) + } else { + if err := json.Unmarshal(jsonBytes, &output.Policies); err != nil { + klog.Errorf("Failed to unmarshal policies: %v", err) + } + } + } + + // Decode labels + if labelsData, ok := outputData["labels"].(map[string]interface{}); ok { + output.Labels = make(map[string]string) + for k, v := range labelsData { + if strVal, ok := v.(string); ok { + output.Labels[k] = strVal + } + } + } + + // Decode annotations + if annotationsData, ok := outputData["annotations"].(map[string]interface{}); ok { + output.Annotations = make(map[string]string) + for k, v := range annotationsData { + if strVal, ok := v.(string); ok { + output.Annotations[k] = strVal + } + } + } + + // Decode ctx + if ctxData, ok := outputData["ctx"].(map[string]interface{}); ok { + output.Ctx = ctxData + } + + return output +} + // loadCachedPolicyFromConfigMap attempts to load a cached policy result from the ConfigMap // Returns the cached result if found and valid according to TTL and Application state, nil otherwise func loadCachedPolicyFromConfigMap(ctx context.Context, cli client.Client, app *v1beta1.Application, policyName string, ttlSeconds int32) (map[string]interface{}, error) { diff --git a/pkg/controller/core.oam.dev/v1beta1/application/policy_validation_test.go b/pkg/controller/core.oam.dev/v1beta1/application/policy_validation_test.go index 7b645efc1..4128e6227 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/policy_validation_test.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/policy_validation_test.go @@ -39,12 +39,9 @@ parameter: {} enabled: true -transforms: { +output: { labels: { - type: "merge" - value: { - "test": "value" - } + "test": "value" } } `, @@ -71,12 +68,9 @@ parameter: { envName: string // Required field - no default! } -transforms: { +output: { labels: { - type: "merge" - value: { - "env": parameter.envName - } + "env": parameter.envName } } `, @@ -105,12 +99,9 @@ parameter: { envName?: string // Optional but no default - can't compile! } -transforms: { +output: { labels: { - type: "merge" - value: { - "env": parameter.envName - } + "env": parameter.envName } } `, @@ -140,13 +131,10 @@ parameter: { replicas: *3 | int // Has default } -transforms: { +output: { labels: { - type: "merge" - value: { - "env": parameter.envName - "replicas": "\(parameter.replicas)" - } + "env": parameter.envName + "replicas": "\(parameter.replicas)" } } `, @@ -171,12 +159,9 @@ transforms: { Template: ` parameter: {} // Empty is fine -transforms: { +output: { labels: { - type: "merge" - value: { - "static": "value" - } + "static": "value" } } `, @@ -273,142 +258,6 @@ parameter: { Expect(result.Errors).Should(ContainElement(ContainSubstring("syntax error"))) }) - It("Test labels transform must use 'merge' type", func() { - policy := &v1beta1.PolicyDefinition{ - Spec: v1beta1.PolicyDefinitionSpec{ - Scope: v1beta1.ApplicationScope, - Schematic: &common.Schematic{ - CUE: &common.CUE{ - Template: ` -parameter: {} - -transforms: { - labels: { - type: "replace" // Invalid! Must be "merge" - value: { - "test": "value" - } - } -} -`, - }, - }, - }, - } - - result := ValidatePolicyDefinition(policy) - Expect(result.IsValid()).Should(BeFalse()) - Expect(result.Errors).Should(ContainElement(ContainSubstring("labels.type must be 'merge'"))) - }) - - It("Test annotations transform must use 'merge' type", func() { - policy := &v1beta1.PolicyDefinition{ - Spec: v1beta1.PolicyDefinitionSpec{ - Scope: v1beta1.ApplicationScope, - Schematic: &common.Schematic{ - CUE: &common.CUE{ - Template: ` -parameter: {} - -transforms: { - annotations: { - type: "replace" // Invalid! Must be "merge" - value: { - "test": "value" - } - } -} -`, - }, - }, - }, - } - - result := ValidatePolicyDefinition(policy) - Expect(result.IsValid()).Should(BeFalse()) - Expect(result.Errors).Should(ContainElement(ContainSubstring("annotations.type must be 'merge'"))) - }) - - It("Test spec transform can use 'merge' or 'replace'", func() { - // Test with merge - policyMerge := &v1beta1.PolicyDefinition{ - Spec: v1beta1.PolicyDefinitionSpec{ - Scope: v1beta1.ApplicationScope, - Schematic: &common.Schematic{ - CUE: &common.CUE{ - Template: ` -parameter: {} - -transforms: { - spec: { - type: "merge" - value: { - components: [] - } - } -} -`, - }, - }, - }, - } - - resultMerge := ValidatePolicyDefinition(policyMerge) - Expect(resultMerge.IsValid()).Should(BeTrue()) - - // Test with replace - policyReplace := &v1beta1.PolicyDefinition{ - Spec: v1beta1.PolicyDefinitionSpec{ - Scope: v1beta1.ApplicationScope, - Schematic: &common.Schematic{ - CUE: &common.CUE{ - Template: ` -parameter: {} - -transforms: { - spec: { - type: "replace" - value: { - components: [] - } - } -} -`, - }, - }, - }, - } - - resultReplace := ValidatePolicyDefinition(policyReplace) - Expect(resultReplace.IsValid()).Should(BeTrue()) - }) - - It("Test spec transform with invalid type fails", func() { - policy := &v1beta1.PolicyDefinition{ - Spec: v1beta1.PolicyDefinitionSpec{ - Scope: v1beta1.ApplicationScope, - Schematic: &common.Schematic{ - CUE: &common.CUE{ - Template: ` -parameter: {} - -transforms: { - spec: { - type: "invalid" // Invalid type! - value: {} - } -} -`, - }, - }, - }, - } - - result := ValidatePolicyDefinition(policy) - Expect(result.IsValid()).Should(BeFalse()) - Expect(result.Errors).Should(ContainElement(ContainSubstring("'merge' or 'replace'"))) - }) - It("Test enabled field must be bool", func() { policy := &v1beta1.PolicyDefinition{ Spec: v1beta1.PolicyDefinitionSpec{ @@ -443,12 +292,9 @@ parameter: { envName: string // Required is OK for non-global policies } -transforms: { +output: { labels: { - type: "merge" - value: { - "env": parameter.envName - } + "env": parameter.envName } } `, @@ -477,7 +323,7 @@ transforms: { Expect(result.Errors).Should(ContainElement(ContainSubstring("must have a CUE schematic"))) }) - It("Test transform without type field fails", func() { + It("Test old transforms API is rejected with clear error", func() { policy := &v1beta1.PolicyDefinition{ Spec: v1beta1.PolicyDefinitionSpec{ Scope: v1beta1.ApplicationScope, @@ -488,7 +334,7 @@ parameter: {} transforms: { labels: { - // Missing 'type' field! + type: "merge" value: { "test": "value" } @@ -502,32 +348,7 @@ transforms: { result := ValidatePolicyDefinition(policy) Expect(result.IsValid()).Should(BeFalse()) - Expect(result.Errors).Should(ContainElement(ContainSubstring("must have 'type' field"))) - }) - - It("Test transform without value field fails", func() { - policy := &v1beta1.PolicyDefinition{ - Spec: v1beta1.PolicyDefinitionSpec{ - Scope: v1beta1.ApplicationScope, - Schematic: &common.Schematic{ - CUE: &common.CUE{ - Template: ` -parameter: {} - -transforms: { - labels: { - type: "merge" - // Missing 'value' field! - } -} -`, - }, - }, - }, - } - - result := ValidatePolicyDefinition(policy) - Expect(result.IsValid()).Should(BeFalse()) - Expect(result.Errors).Should(ContainElement(ContainSubstring("must have 'value' field"))) + Expect(result.Errors).Should(ContainElement(ContainSubstring("'transforms' field is deprecated"))) + Expect(result.Errors).Should(ContainElement(ContainSubstring("use 'output' field instead"))) }) }) diff --git a/pkg/controller/core.oam.dev/v1beta1/application/revision.go b/pkg/controller/core.oam.dev/v1beta1/application/revision.go index 9c5ae64e5..855e1a119 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/revision.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/revision.go @@ -18,6 +18,7 @@ package application import ( "context" + "encoding/json" "sort" "github.com/hashicorp/go-version" @@ -130,6 +131,21 @@ func (h *AppHandler) gatherRevisionSpec(af *appfile.Appfile) (*v1beta1.Applicati copiedApp := h.app.DeepCopy() // We better to remove all object status in the appRevision copiedApp.Status = common.AppStatus{} + + // Normalize component properties (RawExtension) to ensure consistent JSON encoding + // This prevents spurious revision creation due to JSON field order/formatting differences + for i := range copiedApp.Spec.Components { + if copiedApp.Spec.Components[i].Properties != nil && copiedApp.Spec.Components[i].Properties.Raw != nil { + // Decode and re-encode to normalize JSON + var obj map[string]interface{} + if err := json.Unmarshal(copiedApp.Spec.Components[i].Properties.Raw, &obj); err == nil { + if normalized, err := json.Marshal(obj); err == nil { + copiedApp.Spec.Components[i].Properties.Raw = normalized + } + } + } + } + appRev := &v1beta1.ApplicationRevision{ Spec: v1beta1.ApplicationRevisionSpec{ ApplicationRevisionCompressibleFields: v1beta1.ApplicationRevisionCompressibleFields{ @@ -346,9 +362,11 @@ func (h *AppHandler) currentAppRevIsNew(ctx context.Context) (bool, bool, error) for _, _rev := range revs { rev := _rev.DeepCopy() - if rev.GetLabels()[oam.LabelAppRevisionHash] == h.currentRevHash && - DeepEqualRevision(rev, h.currentAppRev) && - oam.GetPublishVersion(rev) == oam.GetPublishVersion(h.app) { + hashMatches := rev.GetLabels()[oam.LabelAppRevisionHash] == h.currentRevHash + deepEqual := DeepEqualRevision(rev, h.currentAppRev) + publishVersionMatches := oam.GetPublishVersion(rev) == oam.GetPublishVersion(h.app) + + if hashMatches && deepEqual && publishVersionMatches { // we set currentAppRev to existRevision h.currentAppRev = rev return true, false, nil @@ -507,6 +525,7 @@ func (h *AppHandler) UpdateAppLatestRevisionStatus(ctx context.Context, patchSta // skip update if app revision is not changed return nil } + if ctx, ok := ctx.(monitorContext.Context); ok { subCtx := ctx.Fork("update-apprev-status", monitorContext.DurationMetric(func(v float64) { metrics.AppReconcileStageDurationHistogram.WithLabelValues("update-apprev-status").Observe(v) @@ -520,13 +539,23 @@ func (h *AppHandler) UpdateAppLatestRevisionStatus(ctx context.Context, patchSta Revision: int64(revNum), RevisionHash: h.currentRevHash, } + + // Save the spec before patchStatus - the merge patch operation refreshes the entire app from API server + // This would lose any in-memory policy modifications to app.Spec + savedSpec := h.app.Spec.DeepCopy() + if err := patchStatus(ctx, h.app, common.ApplicationRendering); err != nil { klog.InfoS("Failed to update the latest appConfig revision to status", "application", klog.KObj(h.app), "latest revision", revName, "err", err) return err } + + // Restore the spec after patchStatus to preserve policy modifications + h.app.Spec = *savedSpec + klog.InfoS("Successfully update application latest revision status", "application", klog.KObj(h.app), "latest revision", revName) + return nil }