From 75afc748838bd84a730b35355e7bba165b779016 Mon Sep 17 00:00:00 2001 From: stxkxs <139715017+stxkxs@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:40:05 -0700 Subject: [PATCH] Teach the Platform CR walker the list-map uniqueness rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/check-platform-crs.py | 109 +++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 3 deletions(-) diff --git a/scripts/check-platform-crs.py b/scripts/check-platform-crs.py index d56f3a5..e15c26a 100755 --- a/scripts/check-platform-crs.py +++ b/scripts/check-platform-crs.py @@ -119,8 +119,76 @@ def crd_schemas(version: str, workdir: Path) -> dict[str, dict]: return out +def list_identities(value, schema): + """What the API server compares two entries of this list by, or None. + + `map` identifies an entry by its listMapKeys tuple; `set` identifies a scalar + entry by itself. Any other list-type (`atomic`, or none) imposes no + uniqueness, so there is nothing to compare. + + Absent keys participate in a map identity: under listMapKeys ["name"], + `[{"name": "a"}, {"name": "a", "kind": "cache"}]` is a duplicate, and reading + only the fields that happen to be set would miss it. Entries of the wrong + shape are skipped rather than guessed at — a non-dict in a map list is a type + error the type checker below reports on its own terms. + """ + kind_of_list = schema.get("x-kubernetes-list-type") + if kind_of_list == "map": + keys = schema.get("x-kubernetes-list-map-keys") or [] + if not keys: + return None + pairs = [ + (i, tuple(v.get(k) for k in keys)) + for i, v in enumerate(value) + if isinstance(v, dict) + ] + return "+".join(keys), pairs + if kind_of_list == "set": + pairs = [ + (i, v) for i, v in enumerate(value) + if isinstance(v, (str, int, float, bool)) + ] + return "value", pairs + return None + + +def check_list_uniqueness(value, schema, path, kind, source, problems): + """x-kubernetes-list-type is a validation rule, not documentation. + + A violation is a hard rejection of the whole object at admission — not a + warning, not a merge of the two entries. + + Nothing in the org checked this, which is how a Platform declaring two + datastores both named `main` passed every gate green and was refused by the + API server on a live cluster. `required`-and-pruning is the obvious half of + admissibility; this is the half a schema walker written from first + principles does not think to add, and the CRDs carry 16 of these arrays. + """ + resolved = list_identities(value, schema) + if resolved is None: + return + label, pairs = resolved + + seen: dict = {} + for i, identity in pairs: + if identity in seen: + shown = ( + "/".join("" if p is None else str(p) for p in identity) + if isinstance(identity, tuple) else str(identity) + ) + problems.append( + f"{source}: {kind} {path}[{i}] repeats {label}={shown}, already at " + f"{path}[{seen[identity]}] — the CRD declares this array " + f"`x-kubernetes-list-type: {schema['x-kubernetes-list-type']}`, so the API " + f"server rejects the whole object with `{path.lstrip('.')}: Duplicate value`. " + f"Nothing partial is applied and the Application never reaches Healthy" + ) + continue + seen[identity] = i + + def walk(value, schema, path, kind, source, problems): - """Required present, nothing excess — arrays transparent. + """Required present, nothing excess, list identities unique — arrays transparent. Stops descending wherever the schema declines to describe the shape (x-kubernetes-preserve-unknown-fields, or an object with no properties), @@ -130,6 +198,7 @@ def walk(value, schema, path, kind, source, problems): return if isinstance(value, list): + check_list_uniqueness(value, schema, path, kind, source, problems) items = schema.get("items") if isinstance(items, dict): for i, item in enumerate(value): @@ -234,9 +303,10 @@ def check(listing: bool, offline: bool) -> int: def self_test() -> int: """The walker has to be wrong loudly, not quietly. - A walker that descends into nothing passes every catalog. These pin the four + A walker that descends into nothing passes every catalog. These pin the properties the check depends on: required is enforced, excess is caught, - arrays are transparent, and an unrestricted schema is left alone. + arrays are transparent, list identities are unique, and an unrestricted + schema is left alone. """ schema = { "properties": { @@ -251,6 +321,18 @@ def self_test() -> int: "required": ["persona"], "properties": {"persona": {"default": "generic"}}, }, + "datastores": { + "x-kubernetes-list-type": "map", + "x-kubernetes-list-map-keys": ["name"], + "items": {"properties": {"name": {}, "kind": {}}}, + }, + "routes": { + "x-kubernetes-list-type": "map", + "x-kubernetes-list-map-keys": ["group", "name"], + "items": {"properties": {"group": {}, "name": {}}}, + }, + "finalizers": {"x-kubernetes-list-type": "set", "items": {}}, + "ordered": {"x-kubernetes-list-type": "atomic", "items": {"properties": {"name": {}}}}, }, } cases = [ @@ -263,6 +345,27 @@ def self_test() -> int: ("excess property", {"agents": [{"name": "a", "image": "i", "tools": []}]}, 1), ("preserve-unknown-fields is left alone", {"free": {"anything": {"nested": 1}}}, 0), ("unknown top-level key", {"nope": 1}, 1), + # The live failure: a Platform with two datastores both named `main`. Every + # property is present and none is excess, so every other rule here passes it. + ("duplicate list-map key", {"datastores": [{"name": "main"}, {"name": "main"}]}, 1), + ("distinct list-map keys", {"datastores": [{"name": "main"}, {"name": "logstream"}]}, 0), + # Same name, different kind — a duplicate, because `kind` is not a map key. + # Uniqueness is per listMapKeys, not per whole entry. + ( + "duplicate on the key alone, not the whole entry", + {"datastores": [{"name": "main", "kind": "relational"}, {"name": "main", "kind": "cache"}]}, + 1, + ), + # Composite keys collide only when every key matches. + ("composite key differing in one field", {"routes": [{"group": "g", "name": "a"}, {"group": "g", "name": "b"}]}, 0), + ("composite key matching in both", {"routes": [{"group": "g", "name": "a"}, {"group": "g", "name": "a"}]}, 1), + # An unset key is a value: two entries omitting it collide on . + ("unset key participates in the identity", {"routes": [{"group": "g"}, {"group": "g"}]}, 1), + ("duplicate set member", {"finalizers": ["a", "a"]}, 1), + ("distinct set members", {"finalizers": ["a", "b"]}, 0), + # atomic imposes no uniqueness — flagging it would be a false positive that + # teaches operators to work around the gate. + ("atomic list permits repeats", {"ordered": [{"name": "a"}, {"name": "a"}]}, 0), ] bad = 0 for name, value, want in cases: