Skip to content

✨ Change CRD generation logic to honor k8s:immutable #1216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −0
Draft

✨ Change CRD generation logic to honor k8s:immutable #1216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/crd/markers/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ var FieldOnlyMarkers = []*definitionWithHelp{

must(markers.MakeAnyTypeDefinition("kubebuilder:title", markers.DescribesField, Title{})).
WithHelp(Title{}.Help()),

must(markers.MakeDefinition("k8s:immutable", markers.DescribesField, Immutable{})).
WithHelp(Immutable{}.Help()),
}

// ValidationIshMarkers are field-and-type markers that don't fall under the
Expand Down Expand Up @@ -325,6 +328,19 @@ type XIntOrString struct{}
// to be used only as a last resort.
type Schemaless struct{}

// +controllertools:marker:generateHelp:category="CRD validation"
// Immutable marks a field as immutable.
// The value of an immutable field may not be changed after creation.
Copy link
Contributor

@jpbetz jpbetz Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's expand the semantics here to clarify what exact transitions are allowed.

Let's use the term "set" instead of "creation" here since immutable fields can set after initial resource creation.

type Immutable struct{}

func (Immutable) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
schema.XValidations = append(schema.XValidations, apiext.ValidationRule{
Message: "Value is immutable",
Rule: "self == oldSelf",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this error out in the create case as oldSelf will be undefined?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't, there is always a "oldSelf" since the rule is attached to the field and only takes effect when the field exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the rule is attached to the field and only takes effect when the field exists.

Not quite true, but it's still safe.

Rules that include oldSelf are known as transition rules, and the CEL implementation detects that a rule is a transition rule.

It will only run a transition rule when it has a value for both self and oldSelf. So only on updates, not when the field first appears.

But, you could have a rule without oldSelf, in which case, the rule would run even when the field first appears.

There's also optionalOldSelf which allows transition rules to run when there is no value for oldSelf, but you have to handle those differently.

Which brings me to... The semantic here is that I could have an object with a field we say is immutable. What does it actually mean to be immutable?

There are two interpretations IMO:

  • Once a value has been set for X, it cannot be changed (omitted -> set is allowed on updated)
  • Value for X can only be set on create (omitted counts as a value)

This implements the first of those, how does it work in the declarative validation project?

If it is the latter, we would need an optionalOldSelf rule and implement a different validation rule

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @JoelSpeed  for providing insights on CEL rules.

+k8s:immutable in Declarative validation project can allow setting empty value to non-empty value during update, but it doesn't allow changing value from one non-empty to another non-empty.   So, it is the following interpretation.
Once a value has been set for X, it cannot be changed (omitted -> set is allowed on updated)

It doesn't allow setting non-empty to empty in update. Which the current cel expression will allow. I will see how this can be achieved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way for us to have tests that actually test the contents of the CEL rule? This seems pretty easy to get wrong and once we've released something like this, fixing it up will be extremely difficult/impossible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could set up an envtest style integration suite and create a series of Create/Update calls to test the validations. That would be my preferred route for testing these at least

Which the current cel expression will allow. I will see how this can be achieved.

The only way I've found to do this is to make sure that the nearest required parent has a rule that says if the field exists, it cannot be removed. But there may be something smarter we can do

})
return nil
}

func hasNumericType(schema *apiext.JSONSchemaProps) bool {
return schema.Type == string(Integer) || schema.Type == string(Number)
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/crd/markers/zz_generated.markerhelp.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.