Teach the Platform CR walker the list-map uniqueness rule - #202
Merged
Conversation
The walker checked two of the three things the API server checks. Required
properties had to be present, excess properties were caught as pruned — and
`x-kubernetes-list-type` was read as documentation rather than as the
validation rule it is.
A Platform declaring two datastores both named `main` therefore passed every
gate in the org green. Every property was present, none was excess, and the
CRD's `x-kubernetes-list-type: map` with `listMapKeys: [name]` meant the API
server refused the whole object:
spec.datastores: Duplicate value
Not a warning and not a merge of the two entries — a hard rejection, so
nothing is applied and the Application never reaches Healthy.
WHAT CHANGED
`check_list_uniqueness` runs wherever the walker meets an array, before it
descends. `map` identifies an entry by its listMapKeys tuple; `set` identifies
a scalar entry by itself; `atomic` and unannotated arrays impose no uniqueness
and are left alone, because flagging them would be a false positive that
teaches an operator to work around the gate.
Two details the rule turns on:
- Uniqueness is per listMapKeys, not per whole entry. Two datastores named
`main` with different `kind` values collide, because `kind` is not a key.
- An unset key is a value. Under keys [group, name], two entries that both
omit `name` collide on the same identity, so reading only the fields that
happen to be set would miss it.
The operator CRDs carry sixteen of these arrays across nine kinds, so this was
not a narrow case.
The self-test grew nine cases covering both list types, composite keys, the
unset-key identity, and atomic's permissiveness. It is the part that matters:
a walker that silently descends into nothing passes every catalog it is ever
pointed at.
CI Results
All checks passed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The walker checked two of the three things the API server checks: required properties present, excess properties caught as pruned.
x-kubernetes-list-typewas read as documentation rather than as the validation rule it is.A Platform declaring two datastores both named
mainpassed every gate in the org green. Every property was present, none was excess — and the API server refused the whole object:Not a warning, not a merge of the two entries. A hard rejection, so nothing is applied and the Application never reaches Healthy.
What changed
check_list_uniquenessruns wherever the walker meets an array, before it descends:map— an entry is identified by itslistMapKeystupleset— a scalar entry is identified by itselfatomic/ unannotated — no uniqueness imposed, left alone. Flagging these would be a false positive that teaches an operator to work around the gate.Two details the rule turns on:
mainwith differentkindvalues collide, becausekindis not a key.[group, name], two entries that both omitnamecollide on the same identity, so reading only the fields that happen to be set would miss it.The operator CRDs carry sixteen of these arrays across nine kinds, so this was not a narrow case.
Proof
The self-test grew nine cases covering both list types, composite keys, the unset-key identity, and atomic's permissiveness — all sixteen pass. That is the part that matters: a walker that silently descends into nothing passes every catalog it is ever pointed at.
Against the real catalog:
ok: 4 platform CR(s) admissible against operator chart 0.6.5.