feat(validation-gen): Add +k8s:immutable and +k8s:frozen tags with KEP update semantics

[aaron-prindle]

PR Changes

• renames the previous +k8s:immutable tag to +k8s:frozen to align with the semantics in the immutability KEP update
• adds a "new" tag +k8s:immutable which has the semantics:
  • Can be unset at creation
  • Allows ONE transition: set (unset->set)
  • Forbids: modify and clear (set->unset)

More semantics:

• pointers

  • nil is considered "unset"
  • Any non-nil pointer is considered "set" (incl. ptrs to zero values)

• non-pointers

  • zero values (struct w/ zero values, int==0, bool==false, string=="") considered unset
  • if +default then:
    • for non-zero value default, value is considered set
    • for zero value defaults, value is considered unset

CSR Example Usage For v1.34

// +k8s:ifNotSubresource("/status")=+k8s:frozen  
// +k8s:ifSubresource("/status")=+k8s:immutable
Certificate []byte `json:"certificate,omitempty"`

[thockin]

/hold I want to look at this again. I don't love the name and "immutable" is already the tag we use today.

[thockin]

From the cited use-case:

// +k8s:ifNotSubresource("/status")=+k8s:frozen  
// +k8s:ifSubresource("/status")=+k8s:immutable
Certificate []byte `json:"certificate,omitempty"`

Is this CertificateSigningRequestStatus.Certificate? The whole status block should be immutable when subresource is not /status -- do we need to encode that here or is that something we should be denoting elsewhere? If we get to validation for "/" and "/status/..." is being modified, doesn't that mean that the REST stack screwed up?

[jpbetz]

Also "string value to nil" ?

[jpbetz]

From the cited use-case:

// +k8s:ifNotSubresource("/status")=+k8s:frozen  
// +k8s:ifSubresource("/status")=+k8s:immutable
Certificate []byte `json:"certificate,omitempty"`

Is this CertificateSigningRequestStatus.Certificate? The whole status block should be immutable when subresource is not /status -- do we need to encode that here or is that something we should be denoting elsewhere? If we get to validation for "/" and "/status/..." is being modified, doesn't that mean that the REST stack screwed up?

I am expecting us to retain the field wiping we already have to prevent changes to status.certificate field unless performed via /status so this can simply be +k8s:immutable.

While I don't love field how field wiping is implemented today, it feels like something we can improve post-declarative validation.

[thockin]

I expanded on my concerns in: #63 (comment)

I am expecting us to retain the field wiping we already have to prevent changes to status.certificate field unless performed via /status so this can simply be +k8s:immutable.

So what do we want to say in validation? We could say nothing and just do the validation (including ratcheting) which is what a lot of handwritten code does. Field wiping will ensure that it's a no-op.

We could say that all of the status field is +k8s:ifNotSubresource("/status")=+k8s:immutable - so any change to it will be flagged as an error (defense in depth). We don't need it on each child field, just the whole status/spec.

We could instead say "spec is part of subresource==/" and "status is part of subresource==/status" and emit the immutability check (describing the data more than the validation).

[jpbetz]

I expanded on my concerns in: #63 (comment)

I am expecting us to retain the field wiping we already have to prevent changes to status.certificate field unless performed via /status so this can simply be +k8s:immutable.

So what do we want to say in validation? We could say nothing and just do the validation (including ratcheting) which is what a lot of handwritten code does. Field wiping will ensure that it's a no-op.

We could say that all of the status field is +k8s:ifNotSubresource("/status")=+k8s:immutable - so any change to it will be flagged as an error (defense in depth). We don't need it on each child field, just the whole status/spec.

We could instead say "spec is part of subresource==/" and "status is part of subresource==/status" and emit the immutability check (describing the data more than the validation).

I'm in favor of the latter: annotating which stanzas/fields are "writable" via which resources/subresources.

Part of the problem here is that field wiping is mechanically different that validation. Today, field wiping tolerates a write to /status that includes changes spec values (with invalid values, even), because the field wiping resets the values of the spec fields before validating.

We can't change that behavior because it would be breaking.

Long term, I'd really like to get to the point where we drive the field wiping declarative from something that (very roughly) looks like:

//+subresource("/", "/status")
metadata: ...

//+subresource("/")
spec: ...

// +subresource("/status")
status: ...

The tags need to somehow convey that these stanzas are "part" of the resource/subresource for field wiping purposes. I'm struggling a bit in how to make this semantic visually distinct from a validation rule.

[thockin]

Long term, I'd really like to get to the point where we drive the field wiping declarative from something that (very roughly) looks like:

100% agree - describe the schema in tags and drive field wiping from that - one step closer to IDL.

//+subresource("/", "/status")
metadata: ...

//+subresource("/")
spec: ...

// +subresource("/status")
status: ...

This is not bad (though the args grammar doesn't allow it since we require named args for n>1, and we don't have a list syntax.

it could just be;

//+subresource="/"
//+subresource="/status"
metadata: ...

//+subresource="/"
spec: ...

// +subresource="/status]"
status: ...

...because we do support multiple values for tags, right? Need to handle carve-outs like pod resources which are in multiple subresources with different validations. This notation feels achievable, without not driving the wiping behavior (yet). It would mean this notation and wiping would be redundant, but that's part of introducing DV overall.

If wiping works, validation will never see an actual change in value anyway?

[jpbetz]

This is not bad (though the args grammar doesn't allow it since we require named args for n>1, and we don't have a list syntax.

it could just be;

//+subresource="/"
//+subresource="/status"
metadata: ...

//+subresource="/"
spec: ...

// +subresource="/status]"
status: ...

Yes, sorry. I threw those tag examples up really quick without any thought to grammar. Repeating the tag for multiple resources/subresources SGTM.

...because we do support multiple values for tags, right? Need to handle carve-outs like pod resources which are in multiple subresources with different validations. This notation feels achievable, without not driving the wiping behavior (yet). It would mean this notation and wiping would be redundant, but that's part of introducing DV overall.

If wiping works, validation will never see an actual change in value anyway?

Exactly. And my intuition is that ratcheting should make it sufficiently safe/cheap to validate unchanged data that we don't urgently need to optimize further.

[aaron-prindle]

From discussions in #63, the +k8s:frozen tag and +k8s:immutable semantics presented in this PR are no longer the desired syntax here. Closing