Merge pull request #565 from rushmash91/main

Switch ACK to CEL-Based Immutability and Remove Runtime Checks

Author: michaelhtm

pkg/config/field.go

@@ -408,8 +408,9 @@ type FieldConfig struct {
 	// IsSecret instructs the code generator that this field should be a
 	// SecretKeyReference.
 	IsSecret bool `json:"is_secret"`
-	// IsImmutable instructs the code generator to add advisory conditions
-	// if user modifies the spec field after resource was created.
+	// IsImmutable indicates that the field is enforced as immutable at the
+	// admission layer. The code generator will add kubebuilder:validation:XValidation
+	// lines to the CRD, preventing changes to this field after it’s set.
 	IsImmutable bool `json:"is_immutable"`
 	// From instructs the code generator that the value of the field should
 	// be retrieved from the specified operation and member path

pkg/model/crd.go

@@ -336,33 +336,6 @@ func (r *CRD) IsSecretField(path string) bool {
 	return false
 }
 
-// GetImmutableFieldPaths returns list of immutable field paths present in CRD
-func (r *CRD) GetImmutableFieldPaths() []string {
-	fConfigs := r.cfg.GetFieldConfigs(r.Names.Original)
-	var immutableFields []string
-
-	for field, fieldConfig := range fConfigs {
-		if fieldConfig.IsImmutable {
-			immutableFields = append(immutableFields, field)
-		}
-	}
-
-	// We need a deterministic order to traverse the immutable fields
-	sort.Strings(immutableFields)
-	return immutableFields
-}
-
-// HasImmutableFieldChanges helper function that return true if there are any immutable field changes
-func (r *CRD) HasImmutableFieldChanges() bool {
-	fConfigs := r.cfg.GetFieldConfigs(r.Names.Original)
-	for _, fieldConfig := range fConfigs {
-		if fieldConfig.IsImmutable {
-			return true
-		}
-	}
-	return false
-}
-
 // OmitUnchangedFieldsOnUpdate returns whether the controller needs to omit
 // unchanged fields from an update request or not.
 func (r *CRD) OmitUnchangedFieldsOnUpdate() bool {

pkg/model/field.go

@@ -188,6 +188,18 @@ func (f *Field) IsRequired() bool {
 	)
 }
 
+// IsImmutable checks the FieldConfig for the Field and returns true if the
+// field is marked as immutable.
+//
+// If the FieldConfig is nil or IsImmutable is false (or not set), this function
+// returns false.
+func (f *Field) IsImmutable() bool {
+	if f.FieldConfig != nil && f.FieldConfig.IsImmutable {
+		return true
+	}
+	return false
+}
+
 // GetSetterConfig returns the SetFieldConfig object associated with this field
 // and a supplied operation type, or nil if none exists.
 func (f *Field) GetSetterConfig(opType OpType) *ackgenconfig.SetFieldConfig {

templates/apis/crd.go.tpl

@@ -19,9 +19,15 @@ type {{ .CRD.Kind }}Spec struct {
 {{ if $field.GetDocumentation -}}
     {{ $field.GetDocumentation }}
 {{ end -}}
-{{- if and ($field.IsRequired) (not $field.HasReference) -}}
+
+{{- if $field.IsImmutable }}
+    // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable once set"
+{{- end }}
+
+{{- if and ($field.IsRequired) (not $field.HasReference) }}
     // +kubebuilder:validation:Required
-{{ end -}}
+{{- end }}
+
     {{ $field.Names.Camel }} {{ $field.GoType }} {{ $field.GetGoTag }}
 {{- end }}
 }
@@ -33,7 +39,7 @@ type {{ .CRD.Kind }}Status struct {
 	// constructed ARN for the resource
 	// +kubebuilder:validation:Optional
 	ACKResourceMetadata *ackv1alpha1.ResourceMetadata `json:"ackResourceMetadata"`
-	// All CRS managed by ACK have a common `Status.Conditions` member that
+	// All CRs managed by ACK have a common `Status.Conditions` member that
 	// contains a collection of `ackv1alpha1.Condition` objects that describe
 	// the various terminal states of the CR and its backend AWS service API
 	// resource
@@ -80,4 +86,4 @@ type {{ .CRD.Kind }}List struct {
 
 func init() {
 	SchemeBuilder.Register(&{{ .CRD.Kind }}{}, &{{ .CRD.Kind }}List{})
-}
+}

templates/crossplane/apis/crd.go.tpl

@@ -93,5 +93,4 @@ var (
 
 func init() {
 	SchemeBuilder.Register(&{{ .CRD.Kind }}{}, &{{ .CRD.Kind }}List{})
-}
-
+}

templates/pkg/resource/sdk.go.tpl

@@ -332,23 +332,6 @@ func (rm *resourceManager) terminalAWSError(err error) bool {
 {{- end }}
 }
 
-{{- if .CRD.HasImmutableFieldChanges }}
-// getImmutableFieldChanges returns list of immutable fields from the
-func (rm *resourceManager) getImmutableFieldChanges(
-	delta *ackcompare.Delta,
-) []string {
-	var fields []string;
-{{- $prefixConfig := .CRD.Config.PrefixConfig }}
-	{{- range $immutableField := .CRD.GetImmutableFieldPaths }}
-{{- $specPrefix := $prefixConfig.SpecField }}
-		if delta.DifferentAt("{{ TrimPrefix $specPrefix "." }}.{{$immutableField}}") {
-			fields = append(fields,"{{$immutableField}}")
-		}
-	{{- end }}
-
-	return fields
-}
-{{- end }}
 {{- if $hookCode := Hook .CRD "sdk_file_end" }}
 {{ $hookCode }}
 {{- end }}

templates/pkg/resource/sdk_update.go.tpl

@@ -10,12 +10,6 @@ func (rm *resourceManager) sdkUpdate(
 	defer func() {
 		exit(err)
 	}()
-{{- if .CRD.HasImmutableFieldChanges }}
-    if immutableFieldChanges := rm.getImmutableFieldChanges(delta); len(immutableFieldChanges) > 0 {
-        msg := fmt.Sprintf("Immutable Spec fields have been modified: %s", strings.Join(immutableFieldChanges, ","))
-        return nil, ackerr.NewTerminalError(fmt.Errorf(msg))
-    }
-{{- end }}
 {{- if $hookCode := Hook .CRD "sdk_update_pre_build_request" }}
 {{ $hookCode }}
 {{- end }}