From 455e3d97fe343ca05dbb294088c43b5cd24aa1ca Mon Sep 17 00:00:00 2001 From: arush sharma Date: Tue, 21 Jan 2025 21:22:26 -0800 Subject: [PATCH] Switch ACK to CEL-Based Immutability and Remove Runtime Checks --- pkg/config/field.go | 5 +++-- pkg/model/crd.go | 27 ------------------------ pkg/model/field.go | 12 +++++++++++ templates/apis/crd.go.tpl | 14 ++++++++---- templates/crossplane/apis/crd.go.tpl | 3 +-- templates/pkg/resource/sdk.go.tpl | 17 --------------- templates/pkg/resource/sdk_update.go.tpl | 6 ------ 7 files changed, 26 insertions(+), 58 deletions(-) diff --git a/pkg/config/field.go b/pkg/config/field.go index 7e774cb5..c7dda412 100644 --- a/pkg/config/field.go +++ b/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 diff --git a/pkg/model/crd.go b/pkg/model/crd.go index 7aca21b1..9781665f 100644 --- a/pkg/model/crd.go +++ b/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 { diff --git a/pkg/model/field.go b/pkg/model/field.go index a2e41a3a..5535a421 100644 --- a/pkg/model/field.go +++ b/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 { diff --git a/templates/apis/crd.go.tpl b/templates/apis/crd.go.tpl index f09ae5dd..4a1933b1 100644 --- a/templates/apis/crd.go.tpl +++ b/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{}) -} +} \ No newline at end of file diff --git a/templates/crossplane/apis/crd.go.tpl b/templates/crossplane/apis/crd.go.tpl index 224485a8..afa7bebd 100644 --- a/templates/crossplane/apis/crd.go.tpl +++ b/templates/crossplane/apis/crd.go.tpl @@ -93,5 +93,4 @@ var ( func init() { SchemeBuilder.Register(&{{ .CRD.Kind }}{}, &{{ .CRD.Kind }}List{}) -} - +} \ No newline at end of file diff --git a/templates/pkg/resource/sdk.go.tpl b/templates/pkg/resource/sdk.go.tpl index 9dd49db7..96d04caa 100644 --- a/templates/pkg/resource/sdk.go.tpl +++ b/templates/pkg/resource/sdk.go.tpl @@ -328,23 +328,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 }} diff --git a/templates/pkg/resource/sdk_update.go.tpl b/templates/pkg/resource/sdk_update.go.tpl index f613572d..306a8c13 100644 --- a/templates/pkg/resource/sdk_update.go.tpl +++ b/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 }}