Skip to content

Commit ce3eb30

Browse files
authored
fix(api): refuse only what a name declaration actually breaks (#4429)
## What this PR does Follow-up to #4367, taking the non-blocking findings from its approving review. Each one is a case where a name declaration (`x-cozystack-name`) was handled more harshly, or more quietly, than the declaration itself warrants. - **An unparseable schema is no longer blamed on a declaration.** A schema that is not a JSON object has no declaration in it, and the spec schema loader already reports it. The API server used to refuse every create of the kind with "declares an invalid x-cozystack-name". It now treats the schema as declaring nothing: creates keep working, and `Kubernetes` and `Kafka` keep their legacy caps. - **Annotation keywords no longer take a kind offline.** Any unknown key inside a declaration used to refuse creates, so a generator adding a `title` or an example would have done the same. JSON Schema annotation keywords (`title`, `default`, `examples`, `deprecated`, `readOnly`, `writeOnly`, `$comment`) and `x-` extensions are now ignored. A constraint keyword the server does not implement, such as `format` or a misspelled `maxLenght`, is still refused, because accepting it would drop it silently. - **The docs no longer overclaim.** They said a declaration no name can satisfy is refused. That is true only for lengths: a pattern is checked to compile, not to match anything, so `$^` rejects every name and names the pattern as the reason. The docs now say that. - **The API review gate sees the declarations the server refuses.** The gate only read a declaration that was an object, and only diffed the keywords it knew. A declaration turned into a string or null, or a misspelled keyword, looked like no change. A changed declaration now goes through the same parser the server uses, and the gate reports it if it cannot be enforced. The parser moves to `pkg/apis/apps/validation` so there is one copy of the rules. - **The declaration no longer leaks into the published OpenAPI for `spec`.** The whole `openAPISchema` is published as the schema of `spec`, so `x-cozystack-name` appeared there as a vendor extension of `spec`. It is now stripped from the published copy. `metadata.name` still carries no limit in the published OpenAPI: it is the shared `ObjectMeta` reference, and a per-kind copy is a bigger change than this one. Not changed: a pattern that can never match is still accepted. Deciding whether a regular expression matches any string within the declared lengths is a much larger piece of code than the mistake it would catch. ### How it is tested - `TestUnparseableSchemaDeclaresNothing`: three non-object schemas; creates still pass, and the Kubernetes legacy cap still holds. - `TestNameDeclarationIgnoresAnnotations`: a declaration carrying `title`, `examples`, `$comment` and an `x-` extension still enforces its `maxLength`. - `TestInvalidNameSchemaFailsClosed` gains `format` as an unsupported constraint keyword. - `TestClassifyNameDeclaration` gains a declaration turned into a string, a misspelled keyword and a null declaration, all reported, and an added annotation, which is not. - `TestPatchSpecLeavesNameDeclarationOffSpec`: the published `spec` drops `x-cozystack-name` and keeps an unrelated vendor extension. ### Downstream repositories No repository is affected beyond the follow-ups already noted on #4367 (the `cozystack/website` pages on annotations and ApplicationDefinitions). Those are still unfiled, so no box is ticked. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(api): An ApplicationDefinition whose openAPISchema cannot be parsed no longer refuses every create of its kind as an invalid name declaration, and annotation keywords (`title`, `examples`, `x-` extensions and the like) inside `x-cozystack-name` are now ignored instead of taking the kind offline. The API review gate now reports a changed name declaration that the API server would refuse, and `x-cozystack-name` no longer appears on the published OpenAPI schema of `spec`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Application name declarations now support minimum and maximum lengths and regular-expression patterns. Matching patterns are checked for compilation; a pattern that matches no names rejects all names. * **Bug Fixes** * Invalid or unsupported name constraints now produce configuration errors and block every create of the affected kind. * Descriptive annotations and `x-` extensions are ignored when validating name declarations. * The custom name declaration is omitted from the published API specification. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 4b6933c + a461dc4 commit ce3eb30

9 files changed

Lines changed: 214 additions & 107 deletions

File tree

‎docs/agents/overview.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ Build environment variables:
9999

100100
Package `values.yaml` files carry annotations (`@param`, `@typedef`, `@field`, `@enum`, `@section`) that `cozyvalues-gen` turns into `values.schema.json` and `README.md`. Run `make generate` in the package after editing annotations. A pre-commit hook runs `make generate` across `packages/apps/*` and `packages/extra/*` on every commit to keep these in sync — see [`contributing.md`](./contributing.md) for the regenerate-before-commit rule.
101101

102-
A chart also declares the limits on its own resource name there, with `## @name {string} - <why>` followed by the constraint tags (`@maxLength`, `@minLength`, `@pattern`) it needs. `cozyvalues-gen` publishes the declaration at the root of `values.schema.json` as `x-cozystack-name`, from where `hack/update-crd.sh` carries it into the ApplicationDefinition, and the aggregated API server enforces it on `metadata.name` for whatever kind the definition describes. The description is the message a rejected name is answered with, so write it as the explanation of the limit. A `@pattern` is unanchored, as JSON Schema specifies, so one meant for the whole name is written `^...$`. A declaration the server cannot enforce as written — an unknown keyword, a wrong type, a limit no name satisfies, a pattern that is not RE2 — refuses every create of that kind with a configuration error rather than being dropped. A cap only ever tightens: `prefix + name` must still fit the 53-character Helm release name limit, and a declaration looser than that budget does not widen it. The limits belong in the chart because that is where the facts behind them are — which sub-resources it renders and what release names they take — and a chart that needs nothing beyond the platform-wide limits declares nothing.
102+
A chart also declares the limits on its own resource name there, with `## @name {string} - <why>` followed by the constraint tags (`@maxLength`, `@minLength`, `@pattern`) it needs. `cozyvalues-gen` publishes the declaration at the root of `values.schema.json` as `x-cozystack-name`, from where `hack/update-crd.sh` carries it into the ApplicationDefinition, and the aggregated API server enforces it on `metadata.name` for whatever kind the definition describes. The description is the message a rejected name is answered with, so write it as the explanation of the limit. A `@pattern` is unanchored, as JSON Schema specifies, so one meant for the whole name is written `^...$`. A declaration the server cannot enforce as written — a constraint keyword it does not implement, a wrong type, a null, lengths no name satisfies, a pattern that is not RE2 — refuses every create of that kind with a configuration error rather than being dropped. A pattern is checked only to compile, not to match anything, so one that matches no name (`$^`) rejects every name with the pattern as the reason. Annotation keywords such as `title` or `examples`, and `x-` extensions, are ignored. A cap only ever tightens: `prefix + name` must still fit the 53-character Helm release name limit, and a declaration looser than that budget does not widen it. The limits belong in the chart because that is where the facts behind them are — which sub-resources it renders and what release names they take — and a chart that needs nothing beyond the platform-wide limits declares nothing.
103103

104104
## Testing
105105

‎internal/apigate/classify_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,10 @@ func TestClassifyNameDeclaration(t *testing.T) {
139139
return res("apps.cozystack.io", "Kubernetes", "kuberneteses", SourceCozyRD, s)
140140
}
141141
capAt := func(n float64) map[string]any { return map[string]any{"type": "string", "maxLength": n} }
142+
kubeRaw := func(declaration any) Resource {
143+
s := Schema{"type": "object", "properties": map[string]any{}, "x-cozystack-name": declaration}
144+
return res("apps.cozystack.io", "Kubernetes", "kuberneteses", SourceCozyRD, s)
145+
}
142146

143147
for _, tc := range []struct {
144148
name string
@@ -151,6 +155,10 @@ func TestClassifyNameDeclaration(t *testing.T) {
151155
{"cap raised", kube(capAt(32)), kube(capAt(40)), false},
152156
{"cap dropped", kube(capAt(32)), kube(nil), false},
153157
{"unchanged", kube(capAt(32)), kube(capAt(32)), false},
158+
{"declaration turned into a string", kube(capAt(32)), kubeRaw("32"), true},
159+
{"misspelled keyword", kube(nil), kube(map[string]any{"maxLenght": 32.0}), true},
160+
{"null declaration", kube(capAt(32)), kubeRaw(nil), true},
161+
{"annotation added", kube(capAt(32)), kube(map[string]any{"type": "string", "maxLength": 32.0, "title": "Name"}), false},
154162
} {
155163
t.Run(tc.name, func(t *testing.T) {
156164
got := countCategory(Classify(snap(tc.base), snap(tc.head)), Breaking)

‎internal/apigate/schemadiff.go‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,14 @@ limitations under the License.
1717
package apigate
1818

1919
import (
20+
"encoding/json"
2021
"fmt"
22+
"reflect"
2123
"sort"
2224
"strings"
2325

2426
appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"
27+
"github.com/cozystack/cozystack/pkg/apis/apps/validation"
2528
)
2629

2730
// diffSchema walks a base and head OpenAPIv3Schema node in lockstep and
@@ -43,14 +46,23 @@ func diffSchema(path string, base, head Schema) []string {
4346

4447
// diffNameSchema reports a tightened constraint on metadata.name. An
4548
// application declares those beside "properties" at the schema root, where the
46-
// walk over spec never looks. The type is left out: a name is a string whether
47-
// or not the declaration says so.
49+
// walk over spec never looks. A declaration the API server cannot enforce
50+
// refuses every create of the kind, the harshest outcome a schema change can
51+
// have, so it is reported whenever it changes. The type is left out of the
52+
// diff: a name is a string whether or not the declaration says so.
4853
func diffNameSchema(base, head Schema) []string {
49-
headName := nameDeclaration(head)
50-
if headName == nil {
54+
headRaw, present := head[appsv1alpha1.NameSchemaExtension]
55+
if !present || reflect.DeepEqual(base[appsv1alpha1.NameSchemaExtension], headRaw) {
5156
return nil
5257
}
53-
return diffSchema("metadata.name", nameDeclaration(base), headName)
58+
encoded, err := json.Marshal(headRaw)
59+
if err == nil {
60+
_, err = validation.ParseNameDeclaration(encoded)
61+
}
62+
if err != nil {
63+
return []string{fmt.Sprintf("metadata.name: %s cannot be enforced, so every create of this kind is refused: %v", appsv1alpha1.NameSchemaExtension, err)}
64+
}
65+
return diffSchema("metadata.name", nameDeclaration(base), nameDeclaration(head))
5466
}
5567

5668
func nameDeclaration(s Schema) Schema {
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
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 validation
18+
19+
import (
20+
"bytes"
21+
"encoding/json"
22+
"fmt"
23+
"regexp"
24+
"strings"
25+
26+
appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"
27+
)
28+
29+
// NameSchema is an application's own declaration of what its name may look
30+
// like. A chart's naming budget follows from the sub-resources the chart
31+
// renders and the names they take — facts that live in the chart and nowhere
32+
// else — so the budget travels with the chart's schema instead of being a
33+
// branch on Kind in the API server.
34+
type NameSchema struct {
35+
Type string `json:"type,omitempty"`
36+
Description string `json:"description,omitempty"`
37+
MinLength *int64 `json:"minLength,omitempty"`
38+
MaxLength *int64 `json:"maxLength,omitempty"`
39+
Pattern string `json:"pattern,omitempty"`
40+
41+
compiledPattern *regexp.Regexp
42+
}
43+
44+
// MatchesPattern reports whether name satisfies the declared pattern, if any.
45+
// The pattern is unanchored, as JSON Schema specifies: a declaration that means
46+
// the whole name writes ^...$.
47+
func (s *NameSchema) MatchesPattern(name string) bool {
48+
return s.compiledPattern == nil || s.compiledPattern.MatchString(name)
49+
}
50+
51+
// nameSchemaKeywords are the keywords a declaration may carry that the API
52+
// server interprets.
53+
var nameSchemaKeywords = map[string]bool{
54+
"type": true, "description": true, "minLength": true, "maxLength": true, "pattern": true,
55+
}
56+
57+
// jsonSchemaAnnotations constrain nothing, so a declaration carrying one is
58+
// enforced as if it did not. Rejecting them would turn a generator adding a
59+
// title or an example into an outage for every kind it regenerates.
60+
var jsonSchemaAnnotations = map[string]bool{
61+
"title": true, "default": true, "examples": true, "deprecated": true,
62+
"readOnly": true, "writeOnly": true, "$comment": true,
63+
}
64+
65+
// ParseNameSchema extracts the x-cozystack-name declaration from an
66+
// ApplicationDefinition's openAPISchema. Returns (nil, nil) when the schema
67+
// declares nothing about the name, which includes a schema that is not a JSON
68+
// object at all: there is no declaration in it to enforce, and the spec
69+
// schema's own loader is where that failure is reported.
70+
func ParseNameSchema(raw string) (*NameSchema, error) {
71+
var doc map[string]json.RawMessage
72+
if err := json.Unmarshal([]byte(raw), &doc); err != nil {
73+
return nil, nil
74+
}
75+
declared, ok := doc[appsv1alpha1.NameSchemaExtension]
76+
if !ok {
77+
return nil, nil
78+
}
79+
return ParseNameDeclaration(declared)
80+
}
81+
82+
// ParseNameDeclaration parses the value of an x-cozystack-name key. The
83+
// openAPISchema is untrusted input — definitions from other repositories are
84+
// written by hand — so a constraint keyword the server does not enforce, a
85+
// wrong type, a null, or lengths no name can satisfy are an error rather than
86+
// a constraint silently dropped. A pattern is only checked to compile as RE2;
87+
// whether it matches anything is the chart's to get right.
88+
func ParseNameDeclaration(declared []byte) (*NameSchema, error) {
89+
// encoding/json decodes null as "leave unset", which would turn a null
90+
// declaration into an empty one and a null keyword into a dropped
91+
// constraint.
92+
var keywords map[string]json.RawMessage
93+
if err := json.Unmarshal(declared, &keywords); err != nil {
94+
return nil, fmt.Errorf("decode: %w", err)
95+
}
96+
if keywords == nil {
97+
return nil, fmt.Errorf("declaration is null")
98+
}
99+
for k, v := range keywords {
100+
switch {
101+
case jsonSchemaAnnotations[k], strings.HasPrefix(k, "x-"):
102+
continue
103+
case !nameSchemaKeywords[k]:
104+
return nil, fmt.Errorf("unsupported keyword %q", k)
105+
case string(bytes.TrimSpace(v)) == "null":
106+
return nil, fmt.Errorf("%s is null", k)
107+
}
108+
}
109+
110+
var ns NameSchema
111+
if err := json.Unmarshal(declared, &ns); err != nil {
112+
return nil, fmt.Errorf("decode: %w", err)
113+
}
114+
switch {
115+
case ns.Type != "" && ns.Type != "string":
116+
return nil, fmt.Errorf("type %q: a name is a string", ns.Type)
117+
case ns.MaxLength != nil && *ns.MaxLength < 1:
118+
return nil, fmt.Errorf("maxLength %d admits no name", *ns.MaxLength)
119+
case ns.MinLength != nil && *ns.MinLength < 0:
120+
return nil, fmt.Errorf("minLength %d is negative", *ns.MinLength)
121+
case ns.MinLength != nil && ns.MaxLength != nil && *ns.MinLength > *ns.MaxLength:
122+
return nil, fmt.Errorf("minLength %d exceeds maxLength %d", *ns.MinLength, *ns.MaxLength)
123+
}
124+
if ns.Pattern != "" {
125+
re, err := regexp.Compile(ns.Pattern)
126+
if err != nil {
127+
return nil, fmt.Errorf("pattern %q does not compile as RE2: %w", ns.Pattern, err)
128+
}
129+
ns.compiledPattern = re
130+
}
131+
return &ns, nil
132+
}

‎pkg/cmd/server/openapi.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"strings"
77

8+
appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"
89
"github.com/cozystack/cozystack/pkg/apiserver"
910
"github.com/cozystack/cozystack/pkg/config"
1011
sampleopenapi "github.com/cozystack/cozystack/pkg/generated/openapi"
@@ -155,6 +156,8 @@ func patchSpec(target *spec.Schema, raw string) error {
155156
if err := json.Unmarshal([]byte(raw), &custom); err != nil {
156157
return err
157158
}
159+
// The name declaration describes metadata.name, not anything inside spec.
160+
delete(custom.Extensions, appsv1alpha1.NameSchemaExtension)
158161

159162
// Neutralize any crash-prone boolean additionalProperties at any depth in
160163
// the user-supplied schema before publishing it.

‎pkg/cmd/server/openapi_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"strings"
2222
"testing"
2323

24+
appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"
2425
celopenapi "k8s.io/apiserver/pkg/cel/openapi"
2526
"k8s.io/kube-openapi/pkg/validation/spec"
2627
)
@@ -116,6 +117,23 @@ func TestPatchSpecKeepsObjectFormAdditionalProperties(t *testing.T) {
116117
}
117118
}
118119

120+
// TestPatchSpecLeavesNameDeclarationOffSpec keeps the published spec from
121+
// carrying a declaration about metadata.name.
122+
func TestPatchSpecLeavesNameDeclarationOffSpec(t *testing.T) {
123+
raw := `{"type":"object","x-cozystack-name":{"type":"string","maxLength":32},"x-other":true,"properties":{}}`
124+
target := newObjectContainer()
125+
if err := patchSpec(target, raw); err != nil {
126+
t.Fatalf("patchSpec: %v", err)
127+
}
128+
specSchema := target.Properties["spec"]
129+
if _, ok := specSchema.Extensions[appsv1alpha1.NameSchemaExtension]; ok {
130+
t.Errorf("published spec carries %s", appsv1alpha1.NameSchemaExtension)
131+
}
132+
if _, ok := specSchema.Extensions["x-other"]; !ok {
133+
t.Errorf("unrelated vendor extension dropped from spec")
134+
}
135+
}
136+
119137
// TestPatchedSpecDoesNotPanicVAPTypeChecker is the behavioral regression test
120138
// for #2863. SchemaDeclType is the exact entry point kube-controller-manager's
121139
// ValidatingAdmissionPolicy status controller uses to type-check a VAP against

0 commit comments

Comments
 (0)