Skip to content

Commit ebde1c9

Browse files
myasnikovdaniilIvanHunters
myasnikovdaniil
authored and
IvanHunters
committed
fix(api): publish open spec as x-kubernetes-preserve-unknown-fields, not additionalProperties:true
cozystack-api published the free-form ".spec" of aggregated apps.cozystack.io resources (Tenant, Application, ...) as the boolean `additionalProperties: true`. The Kubernetes ValidatingAdmissionPolicy status type-checker run by kube-controller-manager nil-dereferences when it recurses into that node, so as soon as a VAP matches one of these resources (cozystack-tenant-host-policy on tenants) KCM CrashLoopBackOffs cluster-wide and stalls reconciliation, failing cold installs once the 3x install retry was removed. Emit `x-kubernetes-preserve-unknown-fields: true` instead. It is semantically equivalent ("arbitrary fields allowed") and the type-checker handles it safely. All app custom schemas already use the safe object-form additionalProperties, so patchSpec was the only source of the boolean form. Adds a regression test that reproduces the exact crash through k8s.io/apiserver/pkg/cel/openapi.SchemaDeclType (the entry point KCM uses): it panics on the pre-fix code and passes after the fix. Fixes #2863 Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]> Signed-off-by: Ivan Okhotnikov <[email protected]>
1 parent 0794c85 commit ebde1c9

2 files changed

Lines changed: 193 additions & 2 deletions

File tree

‎pkg/cmd/server/openapi.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,37 @@ func findSpecContainer(s *spec.Schema) *spec.Schema {
6363
return nil
6464
}
6565

66+
// markOpenObject marks a schema as a free-form object that accepts arbitrary
67+
// properties, using the x-kubernetes-preserve-unknown-fields extension rather
68+
// than the boolean additionalProperties:true form.
69+
//
70+
// This distinction matters: the boolean additionalProperties:true construct
71+
// makes the Kubernetes ValidatingAdmissionPolicy status type-checker (run by
72+
// kube-controller-manager) nil-dereference when it recurses into the node,
73+
// crash-looping KCM cluster-wide as soon as a VAP matches one of these
74+
// aggregated resources (e.g. cozystack-tenant-host-policy on tenants). The
75+
// extension is semantically equivalent ("arbitrary fields allowed") and the
76+
// type-checker handles it safely. See
77+
// https://github.com/cozystack/cozystack/issues/2863.
78+
func markOpenObject(s *spec.Schema) {
79+
if len(s.Type) == 0 {
80+
s.Type = spec.StringOrArray{"object"}
81+
}
82+
s.AdditionalProperties = nil
83+
if s.Extensions == nil {
84+
s.Extensions = map[string]any{}
85+
}
86+
s.Extensions["x-kubernetes-preserve-unknown-fields"] = true
87+
}
88+
6689
// patchSpec injects/overrides ".spec" with user JSON (or schemaless object).
6790
func patchSpec(target *spec.Schema, raw string) error {
6891
if strings.TrimSpace(raw) == "" {
6992
if target.Properties == nil {
7093
target.Properties = map[string]spec.Schema{}
7194
}
7295
prop := target.Properties["spec"]
73-
prop.AdditionalProperties = &spec.SchemaOrBool{Allows: true}
96+
markOpenObject(&prop)
7497
target.Properties["spec"] = prop
7598
return nil
7699
}
@@ -80,7 +103,7 @@ func patchSpec(target *spec.Schema, raw string) error {
80103
return err
81104
}
82105
if custom.AdditionalProperties == nil {
83-
custom.AdditionalProperties = &spec.SchemaOrBool{Allows: true}
106+
markOpenObject(&custom)
84107
}
85108
if target.Properties == nil {
86109
target.Properties = map[string]spec.Schema{}

‎pkg/cmd/server/openapi_test.go‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
/*
2+
Copyright 2026 The Cozystack Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package server
18+
19+
import (
20+
"encoding/json"
21+
"strings"
22+
"testing"
23+
24+
celopenapi "k8s.io/apiserver/pkg/cel/openapi"
25+
"k8s.io/kube-openapi/pkg/validation/spec"
26+
)
27+
28+
// tenantOpenAPISchema mirrors the shape of the real Tenant "Chart Values"
29+
// schema (packages/system/tenant-rd/cozyrds/tenant.yaml): a typed object with
30+
// declared properties and no top-level additionalProperties, plus a nested
31+
// map that uses the safe object-form additionalProperties.
32+
const tenantOpenAPISchema = `{
33+
"title": "Chart Values",
34+
"type": "object",
35+
"properties": {
36+
"host": {"type": "string"},
37+
"etcd": {"type": "boolean"},
38+
"resourceQuotas": {
39+
"type": "object",
40+
"additionalProperties": {
41+
"anyOf": [{"type": "integer"}, {"type": "string"}],
42+
"x-kubernetes-int-or-string": true
43+
}
44+
}
45+
}
46+
}`
47+
48+
func newObjectContainer() *spec.Schema {
49+
return &spec.Schema{SchemaProps: spec.SchemaProps{
50+
Type: spec.StringOrArray{"object"},
51+
Properties: map[string]spec.Schema{},
52+
}}
53+
}
54+
55+
// TestPatchSpecOpenSpecPublishesPreserveUnknownFields asserts that the open
56+
// ".spec" cozystack-api injects is published as
57+
// x-kubernetes-preserve-unknown-fields:true and never as the boolean
58+
// additionalProperties:true form that crashes the VAP type-checker (#2863).
59+
// It covers both code paths: a schemaless resource and a custom schema that
60+
// declares properties but no top-level additionalProperties (like Tenant).
61+
func TestPatchSpecOpenSpecPublishesPreserveUnknownFields(t *testing.T) {
62+
cases := map[string]string{
63+
"schemaless": "",
64+
"custom-no-additional": tenantOpenAPISchema,
65+
}
66+
67+
for name, raw := range cases {
68+
t.Run(name, func(t *testing.T) {
69+
target := newObjectContainer()
70+
if err := patchSpec(target, raw); err != nil {
71+
t.Fatalf("patchSpec: %v", err)
72+
}
73+
specSchema := target.Properties["spec"]
74+
75+
if specSchema.AdditionalProperties != nil {
76+
t.Errorf("spec must not carry additionalProperties; got %#v", specSchema.AdditionalProperties)
77+
}
78+
if v, ok := specSchema.Extensions.GetBool("x-kubernetes-preserve-unknown-fields"); !ok || !v {
79+
t.Errorf("spec must set x-kubernetes-preserve-unknown-fields:true; extensions=%v", specSchema.Extensions)
80+
}
81+
if !specSchema.Type.Contains("object") {
82+
t.Errorf("spec must be type object; got %v", specSchema.Type)
83+
}
84+
85+
// The published JSON is the actual contract KCM type-checks against.
86+
out, err := json.Marshal(specSchema)
87+
if err != nil {
88+
t.Fatalf("marshal: %v", err)
89+
}
90+
if strings.Contains(string(out), `"additionalProperties":true`) {
91+
t.Errorf("published spec still emits boolean additionalProperties:true (#2863): %s", out)
92+
}
93+
if !strings.Contains(string(out), `"x-kubernetes-preserve-unknown-fields":true`) {
94+
t.Errorf("published spec missing x-kubernetes-preserve-unknown-fields:true: %s", out)
95+
}
96+
})
97+
}
98+
}
99+
100+
// TestPatchSpecKeepsObjectFormAdditionalProperties ensures we only rewrite the
101+
// open/free-form spec: a custom schema that already declares an object-form
102+
// additionalProperties (a real map type, which is safe for the type-checker)
103+
// must be left untouched rather than overwritten with the extension.
104+
func TestPatchSpecKeepsObjectFormAdditionalProperties(t *testing.T) {
105+
raw := `{"type":"object","additionalProperties":{"type":"string"}}`
106+
target := newObjectContainer()
107+
if err := patchSpec(target, raw); err != nil {
108+
t.Fatalf("patchSpec: %v", err)
109+
}
110+
specSchema := target.Properties["spec"]
111+
if specSchema.AdditionalProperties == nil || specSchema.AdditionalProperties.Schema == nil {
112+
t.Fatalf("object-form additionalProperties must be preserved; got %#v", specSchema.AdditionalProperties)
113+
}
114+
if v, ok := specSchema.Extensions.GetBool("x-kubernetes-preserve-unknown-fields"); ok && v {
115+
t.Errorf("must not add preserve-unknown-fields when a real map schema is declared")
116+
}
117+
}
118+
119+
// TestPatchedSpecDoesNotPanicVAPTypeChecker is the behavioral regression test
120+
// for #2863. SchemaDeclType is the exact entry point kube-controller-manager's
121+
// ValidatingAdmissionPolicy status controller uses to type-check a VAP against
122+
// the published resource schema. Before the fix, the boolean
123+
// additionalProperties:true on ".spec" made it nil-dereference
124+
// (k8s.io/apiserver/pkg/cel/openapi.isExtension), crash-looping KCM. With the
125+
// preserve-unknown-fields form it returns a valid type.
126+
//
127+
// This test panics (fails) on the pre-fix code and passes after it.
128+
func TestPatchedSpecDoesNotPanicVAPTypeChecker(t *testing.T) {
129+
cases := map[string]string{
130+
"schemaless": "",
131+
"tenant-like": tenantOpenAPISchema,
132+
}
133+
134+
for name, raw := range cases {
135+
t.Run(name, func(t *testing.T) {
136+
// Resource-shaped object, as published per kind by cozystack-api.
137+
obj := &spec.Schema{SchemaProps: spec.SchemaProps{
138+
Type: spec.StringOrArray{"object"},
139+
Properties: map[string]spec.Schema{
140+
"apiVersion": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}},
141+
"kind": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}},
142+
"spec": {},
143+
},
144+
}}
145+
if err := patchSpec(obj, raw); err != nil {
146+
t.Fatalf("patchSpec: %v", err)
147+
}
148+
149+
// Round-trip through JSON so we type-check the serialized form that
150+
// is actually served on /openapi/v3 and read by KCM.
151+
rawJSON, err := json.Marshal(obj)
152+
if err != nil {
153+
t.Fatalf("marshal: %v", err)
154+
}
155+
var published spec.Schema
156+
if err := json.Unmarshal(rawJSON, &published); err != nil {
157+
t.Fatalf("unmarshal: %v", err)
158+
}
159+
160+
// isResourceRoot=true matches how the type-checker treats a matched
161+
// top-level resource.
162+
declType := celopenapi.SchemaDeclType(&published, true)
163+
if declType == nil {
164+
t.Fatalf("SchemaDeclType returned nil for %s", name)
165+
}
166+
})
167+
}
168+
}

0 commit comments

Comments
 (0)