Skip to content

[KEP-5073] Update KEP with section of validation ratcheting #5292

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

Merged
k8s-ci-robot merged 3 commits into from
Jun 9, 2025
+55 −3
Merged

[KEP-5073] Update KEP with section of validation ratcheting #5292

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01f1372
[KEP-5073] Update KEP with section of validation ratcheting
yongruilin May 8, 2025
04e333a
KEP-5073: Add policy for order-sensitive list validation in declarati…
yongruilin Jun 9, 2025
f9f82d5
apply feedback
yongruilin Jun 9, 2025
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
58 changes: 55 additions & 3 deletions keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@
- [Scale-Type Subresources](#scale-type-subresources)
- [Streaming Subresources](#streaming-subresources)
- [Ratcheting](#ratcheting)
- [Core Principles](#core-principles)
- [Default Ratcheting Behavior](#default-ratcheting-behavior)
- [Definition of Semantic Equivalence](#definition-of-semantic-equivalence)
- [Policy on Order-Sensitive List Validation](#policy-on-order-sensitive-list-validation)
- [Ratcheting and Cross-Field Validation](#ratcheting-and-cross-field-validation)
- [Test Plan](#test-plan)
- [Prerequisite testing updates](#prerequisite-testing-updates)
- [Unit tests](#unit-tests)
Expand Down Expand Up @@ -1421,10 +1426,57 @@ The streamed data does not require declarative validation, as it is not structur

### Ratcheting

TODO: Document and explain how:
As Kubernetes APIs evolve, validation rules change. To minimize disruption for users with existing objects created under older rules, declarative validation will incorporate **Validation Ratcheting**. This mechanism aims to selectively bypass new or changed validation rules during object updates (`UPDATE`, `PATCH`, `APPLY`) for fields that have not been modified from their previously persisted state.

- Add general purpose ratcheting to automatically skip validation of unchanged fields
- Catalog and handle complex cases where strict equality checks are not sufficient (lots of non-trivial cases exist today)
#### Core Principles

The design adheres to the following core principles:

1. **Stored data is considered valid:** Any object successfully persisted was once considered valid. Subsequent apiservers must not retroactively invalidate stored objects. (Implication: fixing validation bugs post-release is challenging).
2. **Unchanged fields do not cause update rejections:** An `UPDATE` operation must not fail validation due to fields that were not modified in that operation. (Rationale: HTTP 4xx errors imply a client request problem).
3. **Semantic deep-equal is always sufficient to elide re-validation:** Kubernetes API objects adhere to canonical semantic equivalence rules (`equality.Semantic.DeepEqual`). If a deserialized object satisfies that equivalence with its prior state, re-validation can be bypassed.
* **Subtle:** List elements might individually bypass re-validation, but the list itself might still be re-validated (e.g., if reordered).

Ratcheting is the **default behavior** during `UPDATE` operations.

#### Default Ratcheting Behavior

The default mechanism handles common cases by skipping re-validation if a field hasn't changed based on semantic equivalence.

##### Definition of Semantic Equivalence

"Semantic equivalence" builds on `k8s.io/apimachinery/pkg/api/equality.Semantic.DeepEqual` (similar to `reflect.DeepEqual` but `nil` and empty slices/maps are equivalent). The table below outlines the behavior:

| Value type | Semantic Equivalence | Ratcheting | CRD Comparison (KEP-4008) |
| :---------------------------------- | :------------------------------------------------------------- | :----------------------------------------------------------------------------- | :------------------------------------------------------------------------------------------------------------------ |
| Scalars (int, string, etc.) | direct equality of the value | revalidate the value if changed | same |
| Pointers | equivalence of the pointee | revalidate the value if changed | same |
| Struct | all fields are equivalent | revalidate the struct if any field changed | same |
| Struct fields<br/>`structType=granular` | - | revalidate changed fields | same |
| Struct fields<br/>`structType=atomic` | - | revalidate changed fields | [Issue #131566](https://github.com/kubernetes/kubernetes/issues/131566) (Alignment needed) |
| Slices | all elements are equivalent and the is order unchanged | revalidate the slice if any element changed or order changed | `listType=map`: no validate when re-order<br/>`listType=set`: re-validate when re-order<br/>`listType=atomic`: re-validate when re-order |
| Slice values<br/>`listType=atomic` | - | validate items which are not found (by value) in the old slice | Validate all elements (CRDs ratcheting may be expanded to match in-tree ratcheting) |
| Slice values<br/>`listType=map` | - | (re)validate items which are not found (by key) in the old slice or are changed | same |
Copy link
Contributor

@deads2k deads2k May 21, 2025

Choose a reason for hiding this comment

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

if order of a slice that is a map changes, but all the keys and values are the same, do we skip validation? I would expect us to skip.

Copy link
Contributor Author

@yongruilin yongruilin May 21, 2025

Choose a reason for hiding this comment

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

it would skip for the validation on the individual element. But should we still validate the listmap as a whole? If it is declared with "immutable", we consider the order as part of the API and validate the order?

Copy link
Contributor

@deads2k deads2k May 28, 2025

Choose a reason for hiding this comment

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

it would skip for the validation on the individual element. But should we still validate the listmap as a whole? If it is declared with "immutable", we consider the order as part of the API and validate the order?

Do you have an example? I think for our validation tag purposes I would expect immutable on a list of type map to mean the key/value pairs cannot change, but I don't think I'd expect revalidation on ordering. That means that certain kinds of validation cannot be expressed, but it's not so clear to me that we'd want to support validation on lists of type map that were order dependent.

Copy link
Contributor Author

@yongruilin yongruilin May 28, 2025

Choose a reason for hiding this comment

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

Container.Env
The order here matters as "Variables making use of others defined in the same context must come later in the list." (ref doc) This is maybe a mistake for marking it as listtype=map, but it is already in used widely.

Copy link
Contributor

@jpbetz jpbetz May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

pod.spec.container[*].env is also the case I wanted to make sure I fully considered as I was when reading this thread.

env is problematic because it represents variable assignments (order matters,repeated "keys" are allowed). It breaks listType=map in quite a few ways.

Despite all of those problems, I'm not aware of any validation rules that would need to be reevaluated if the order changes for env. And I think that's what we're looking for here. If we can't find any actual cases. I'm inclined to not reevaluated validation when only order changes like @deads2k suggests.

Copy link
Member

@thockin thockin Jun 5, 2025

Choose a reason for hiding this comment

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

Self-followup:

I don't know of any places where the validation logic depends on the order of items in a list-map or list-set, and I am hard-pressed to synthesize a realistic one (I can dream up phony ones, though). env doesn't count here because it's REALLY NOT a map.

Likewise, I can't really dream up a case where a reorder-only update is a thing someone would want to do.

That makes this discussion somewhat academic, but it changes core principles so I think we need to pin it down.

For me it mostly comes down to consistency. If we write to storage and trigger admission and send WATCH events, then I think we should validate. You can argue that it's a map, so any validation that would fail on a reorder is broken, and I not even going to disagree.

The second concern is complexity. To skip validation in the event of a reorder-only update, we need to write more code and/or do slow things like maps and write more tests than if we just let the revalidation happen. I doubt very very much that reorder-only updates are a thing people do, so it's really moot.

Why write more code to skip a corner-case that doesn't really exist?

Copy link
Member

@thockin thockin Jun 5, 2025

Choose a reason for hiding this comment

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

OK here's a semi-real case.

Service.Spec.ClusterIPs (it's not declared as a set but it probably should be - it has to be unique elements). There's a rule that says that .clusterIPs[0] must == .clusterIP. If you reorder that list, validation SHOULD fail.

Copy link
Contributor

@deads2k deads2k Jun 6, 2025

Choose a reason for hiding this comment

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

OK here's a semi-real case.
Service.Spec.ClusterIPs (it's not declared as a set but it probably should be - it has to be unique elements). There's a rule that says that .clusterIPs[0] must == .clusterIP. If you reorder that list, validation SHOULD fail.

I think a distinguishing feature of a set or a map is that the order doesn't matter. Re-validating changes in those cases allows people to write not-map and not-set validation. For ClusterIPs, I counter-claim that someone reading the type, seeing set, treating it as a set, and then getting validation errors incompatible with a set is a bug in the type description and a kind of bug we can make impossible to write by not revalidating.

Copy link
Contributor

@jpbetz jpbetz Jun 6, 2025

Choose a reason for hiding this comment

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

We've got a variety of semantics in the use cases. Regardless of what we pick for the default ratcheting behavior of listType=map, we're going to have to deal with the use cases that don't align.

I don't love that we've accumulated order dependent use cases on listType=map. But I can see how we got here:

  • We alphanumerically sort JSON maps when we serialize, but we preserve the order of listType=map.
  • When types were annotated as "maps" by SMD and SSA, no special considerations were made for lists where order matters.

If we continue to force uniform treatment of all listType=map entries have the same semantics, I don't see the situation getting a whole lot better.

Would it be reasonable to do something like this?

  • Clarify (presumably with more tags) all use cases of listType=maps where order matters.
  • Once we've divided the use cases up, provide sane default ratcheting for each category.

?

Copy link
Member

@thockin thockin Jun 6, 2025

Choose a reason for hiding this comment

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

I assert that these are not "maps" (or sets), they are "kubernetes list-maps", so the semantics are really up to us to define.

We could define different attributes like "// +listType=set" vs "// +unique". Maps are more fiddly since the way we define keys is called listMapKey. I suppose we could do "orderedSet" and "orderedMap", but I think we would use the ordered variants for all existing-field conversions, just out of fear of breaking clients' assumptions.

If we can find better ways to express things like env which doesn't break clients but also doesn't carry untrue implications, great! But that's not what this is about. As it stands, env is a bad candidate for being converted to declarative :)

Let's assume we filter out all the things that are declared as maps/sets but really aren't. For the rest: would you be open to changes which provide "real" map semantics? IOW: don't write to disk (but do return 200), sort the elements by key (e.g. in JSON/YAML), don't trigger WATCH, don't call admission, etc? I suspect that will 100% break someone.

IMO consistency and simplicity are important. Pragmatically, less code has less bugs. I don't see the value in supporting "map" and "orderedMap" as different things.

| Slice values<br/>`listType=set` | - | validate items which are not found (by value) in the old slice | Validate all elements (CRDs ratcheting may be expanded to match in-tree ratcheting) |
| Maps | all elements are equivalent | revalidate the map if any element changed | same |
| Map values<br/>`mapType=granular` | - | (re)validate items which are not found (by key) in the old map or are changed | same |
| Map values<br/>`mapType=atomic` | - | (re)validate items which are not found (by key) in the old map or are changed | [Issue #131566](https://github.com/kubernetes/kubernetes/issues/131566) (Alignment needed) |

**Note on Atomic Types:** The behavior for `structType=atomic` and `mapType=atomic` intentionally deviates from strict atomic re-validation. Only the specific sub-fields or key-value pairs *that were actually modified* are re-validated. This prioritizes user experience but requires alignment with CRD behavior (tracked in Issue #131566).

#### Policy on Order-Sensitive List Validation

The handling of `listtype=map` requires specific clarification. While its name implies order is irrelevant, the validation mechanism for native types re-validates a list if its elements are reordered.

To ensure predictable behavior, the following rule applies: **all new declarative validation rules for list-type=map must be order-insensitive**.

This requirement is enforced through vigilant code review to reject any proposed rule that violates this principle.

#### Ratcheting and Cross-Field Validation

A challenge arises if a cross-field validation rule (e.g. `X < Y`) is defined on a common ancestor struct, and an unrelated field (e.g. `Z`) within that same ancestor is modified. This change to `Z` makes the common ancestor “changed” overall, triggering re-validation of the `X < Y` rule. If this rule was recently evolved (e.g., made stricter), it might now fail even if `X` and `Y` themselves are not modified by the user’s update. This could violate the principle “Unchanged fields do not cause update rejections.”

For the initial implementation, this behavior will be documented. Furthermore, cross-field validation rules themselves must incorporate internal ratcheting logic. For instance, generated code for dedicated cross-field tags (like `+k8s:unionMember`) will be designed to only act upon changes to the specific fields they govern and were designed to validate.
Comment on lines +1477 to +1479
Copy link
Contributor

@deads2k deads2k May 21, 2025

Choose a reason for hiding this comment

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

Please build us an example as soon as we have enough merged so we can see how painful (or not) it seems to be.


### Test Plan

Expand Down