Skip to content

Commit 821bb9e

Browse files
lllamnyplllamnyp
committed
fix(ci): catch templated CRDs and APIService-only groups in the API gate
Shadow-observation of the merged gate found a real miss: a CRD shipped inside a chart's templates/ dir (guarded by a Helm {{- if }}) was invisible because discovery was gated on directory name (crds/, definitions/, manifests/), and a group served only by an external aggregated apiserver (kind: APIService, no CRD, no vendored Go types) had no representation at all. Switch CRD/APIService discovery from directory-name to content sniffing, and tolerate Helm directive lines sharing a document with otherwise-valid YAML (the closing `{{- end }}` of a wrapping conditional has no leading `---`, so it lands in the same document as the manifest). Add a fourth source, SourceAPIService, reusing the existing empty-schema mechanism so a first-party group's first APIService appearance requires review without pretending to diff a schema that lives outside this repository. Assisted-By: Claude <[email protected]> Signed-off-by: Timofei Larkin <[email protected]>
1 parent 71ca2b1 commit 821bb9e

4 files changed

Lines changed: 358 additions & 35 deletions

File tree

‎internal/apigate/model.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,12 @@ const (
6262
// schema, so they participate only in new-group / new-resource
6363
// detection.
6464
SourceAPIServer Source = "apiserver"
65+
// SourceAPIService is a group/version registered via a kind: APIService
66+
// manifest, delegating to an aggregated apiserver that may be built
67+
// entirely outside this repository (no vendored Go types, no CRD). Like
68+
// SourceAPIServer, these carry no checked-in schema, so they participate
69+
// only in new-group / new-resource / removal detection.
70+
SourceAPIService Source = "apiservice"
6571
)
6672

6773
// Resource is the normalized identity + schema of one API resource, keyed

‎internal/apigate/parse.go‎

Lines changed: 82 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,14 @@ func ParseCozyRD(origin string, data []byte) (Resource, bool, error) {
7373

7474
// ParseCRDs parses a (possibly multi-document) CustomResourceDefinition YAML
7575
// file into one Resource per CRD, keyed by spec.group + spec.names.plural with
76-
// each served version's openAPIV3Schema.
76+
// each served version's openAPIV3Schema. CRD discovery is content-based (any
77+
// file under the walked roots, regardless of directory name — see
78+
// discoverYAMLFiles), so a single file routinely mixes genuine CRD documents
79+
// with unrendered Helm markup (a `{{- if }}` guard, an `{{ include }}` call)
80+
// in neighboring documents of the same `---`-delimited stream. A document
81+
// that fails to parse as YAML is therefore skipped on its own rather than
82+
// aborting the whole file, so a real CRD sharing a file with templated
83+
// documents is still found.
7784
func ParseCRDs(origin string, data []byte) ([]Resource, error) {
7885
var out []Resource
7986
for i, docBytes := range splitYAMLDocs(data) {
@@ -97,8 +104,8 @@ func ParseCRDs(origin string, data []byte) ([]Resource, error) {
97104
} `json:"versions"`
98105
} `json:"spec"`
99106
}
100-
if err := yaml.Unmarshal(docBytes, &doc); err != nil {
101-
return nil, fmt.Errorf("%s (document %d): %w", origin, i, err)
107+
if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
108+
continue
102109
}
103110
if doc.Kind != "CustomResourceDefinition" {
104111
continue
@@ -190,6 +197,78 @@ func ParseAPIServerStorages(origin string, data []byte) []Resource {
190197
return out
191198
}
192199

200+
// apiServicePlural is the sentinel Plural used for SourceAPIService
201+
// resources. An APIService does not describe an individual resource the way
202+
// a CRD or cozyrd does — it registers a whole (group, version) pair, served
203+
// by whatever aggregated apiserver binary the group's registration points
204+
// at, which may be built entirely outside this repository (no vendored Go
205+
// types, no checked-in CRD). The literal is namespaced with "$" so it can
206+
// never collide with a real plural name.
207+
const apiServicePlural = "$apiservice"
208+
209+
// ParseAPIServices parses a (possibly multi-document) YAML file for
210+
// kind: APIService manifests, surfacing one schema-less Resource per
211+
// first-party (group, version) registration. Like ParseAPIServerStorages,
212+
// these resources carry no checked-in schema, so they participate only in
213+
// new-group / new-resource / removal detection via the shared
214+
// empty-Versions mechanism — diffResource has nothing to iterate, so no
215+
// Breaking finding is ever produced from one of these alone. Documents that
216+
// fail to parse (Helm markup sharing a file with a real APIService, as with
217+
// ParseCRDs) are skipped rather than failing the file.
218+
func ParseAPIServices(origin string, data []byte) []Resource {
219+
var out []Resource
220+
for _, docBytes := range splitYAMLDocs(data) {
221+
if len(strings.TrimSpace(string(docBytes))) == 0 {
222+
continue
223+
}
224+
var doc struct {
225+
Kind string `json:"kind"`
226+
Spec struct {
227+
Group string `json:"group"`
228+
Version string `json:"version"`
229+
} `json:"spec"`
230+
}
231+
if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
232+
continue
233+
}
234+
if doc.Kind != "APIService" || doc.Spec.Group == "" || doc.Spec.Version == "" {
235+
continue
236+
}
237+
out = append(out, Resource{
238+
Group: doc.Spec.Group,
239+
Kind: "(aggregated apiserver)",
240+
Plural: apiServicePlural,
241+
Source: SourceAPIService,
242+
Origin: origin,
243+
Versions: map[string]Schema{doc.Spec.Version: {}},
244+
})
245+
}
246+
return out
247+
}
248+
249+
// helmDirectiveLine matches a line that is entirely Go-template/Helm markup
250+
// (`{{- if ... }}`, `{{- end }}`, `{{- /* comment */ -}}`), including its
251+
// trailing newline.
252+
var helmDirectiveLine = regexp.MustCompile(`(?m)^[ \t]*\{\{-?[^\n]*-?\}\}[ \t]*\r?\n?`)
253+
254+
// unmarshalYAMLDoc decodes one `---`-delimited document, tolerating Helm
255+
// directive lines that share a document with otherwise-valid YAML. The
256+
// convention this repo's charts use for a raw CRD/APIService body is to wrap
257+
// the whole manifest in a single `{{- if }}...{{- end }}` block; the `{{- if
258+
// }}` half is naturally its own document (nothing precedes it since it opens
259+
// the block before a `---`), but the closing `{{- end }}` has no `---` before
260+
// it and so lands in the same document as the manifest itself — plain
261+
// yaml.Unmarshal rejects that whole document as invalid. Stripping
262+
// whole-line directives before decoding recovers the manifest; a document
263+
// that still fails after stripping is genuinely not YAML and is left to the
264+
// caller to skip.
265+
func unmarshalYAMLDoc(doc []byte, out any) error {
266+
if err := yaml.Unmarshal(doc, out); err == nil {
267+
return nil
268+
}
269+
return yaml.Unmarshal(helmDirectiveLine.ReplaceAll(doc, nil), out)
270+
}
271+
193272
// splitYAMLDocs splits a YAML stream into individual document byte slices on
194273
// `---` separators. It intentionally mirrors the simple splitting Helm and
195274
// kubectl use; CRD manifests in this repo do not embed `---` inside block

‎internal/apigate/parse_test.go‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,68 @@ spec:
125125
}
126126
}
127127

128+
// TestParseCRDsToleratesTemplatedNeighborDocument reproduces the cozyplane
129+
// bug: a real CRD living in a chart's templates/ dir, guarded by a
130+
// `{{- if }}` block that is not valid YAML on its own. The malformed
131+
// neighbor document must not take the real CRD down with it.
132+
func TestParseCRDsToleratesTemplatedNeighborDocument(t *testing.T) {
133+
const doc = `{{- /* CRDs are installed only in CRD mode */ -}}
134+
{{- if not .Values.apiserver.enabled }}
135+
---
136+
apiVersion: apiextensions.k8s.io/v1
137+
kind: CustomResourceDefinition
138+
metadata:
139+
name: vpcs.sdn.cozystack.io
140+
spec:
141+
group: sdn.cozystack.io
142+
names: {kind: VPC, plural: vpcs}
143+
versions:
144+
- name: v1alpha1
145+
served: true
146+
schema: {openAPIV3Schema: {type: object}}
147+
{{- end }}
148+
`
149+
rs, err := ParseCRDs("templates/crds.yaml", []byte(doc))
150+
if err != nil {
151+
t.Fatalf("ParseCRDs failed: %v", err)
152+
}
153+
if len(rs) != 1 || rs[0].Plural != "vpcs" {
154+
t.Fatalf("expected the CRD sharing the file with templated markup to survive, got %v", rs)
155+
}
156+
}
157+
158+
// TestParseAPIServices covers the new APIService source: a first-party group
159+
// registration is surfaced as a schema-less resource, and a non-APIService or
160+
// incomplete document is ignored.
161+
func TestParseAPIServices(t *testing.T) {
162+
const doc = `apiVersion: apiregistration.k8s.io/v1
163+
kind: APIService
164+
metadata:
165+
name: v1alpha1.sdn.cozystack.io
166+
spec:
167+
group: sdn.cozystack.io
168+
version: v1alpha1
169+
groupPriorityMinimum: 1000
170+
versionPriority: 10
171+
---
172+
apiVersion: v1
173+
kind: Service
174+
metadata:
175+
name: cozyplane-apiserver
176+
`
177+
rs := ParseAPIServices("apiserver.yaml", []byte(doc))
178+
if len(rs) != 1 {
179+
t.Fatalf("expected exactly 1 APIService resource, got %v", rs)
180+
}
181+
r := rs[0]
182+
if r.Group != "sdn.cozystack.io" || r.Source != SourceAPIService || r.Plural != apiServicePlural {
183+
t.Fatalf("unexpected APIService resource: %+v", r)
184+
}
185+
if _, ok := r.Versions["v1alpha1"]; !ok {
186+
t.Fatalf("expected v1alpha1 in versions, got %v", r.Versions)
187+
}
188+
}
189+
128190
func TestParseAPIServerStorages(t *testing.T) {
129191
src := `
130192
coreV1alpha1Storage["tenantsecrets"] = cozyregistry.RESTInPeace(x)
@@ -186,6 +248,160 @@ func TestLoadSnapshotDiscoversCRDsUnderChartsCrds(t *testing.T) {
186248
}
187249
}
188250

251+
// TestLoadSnapshotDiscoversCRDUnderTemplatesDir reproduces the real gap found
252+
// in the cozyplane PR: a raw CRD document guarded by `{{- if }}`, sitting in a
253+
// chart's templates/ dir (not crds/), must still be discovered — directory
254+
// name is no longer the discovery signal, file content is.
255+
func TestLoadSnapshotDiscoversCRDUnderTemplatesDir(t *testing.T) {
256+
root := t.TempDir()
257+
write := func(rel, content string) {
258+
p := filepath.Join(root, filepath.FromSlash(rel))
259+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
260+
t.Fatal(err)
261+
}
262+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
263+
t.Fatal(err)
264+
}
265+
}
266+
write("packages/system/cozyplane/templates/crds.yaml", `{{- if not .Values.apiserver.enabled }}
267+
---
268+
apiVersion: apiextensions.k8s.io/v1
269+
kind: CustomResourceDefinition
270+
metadata:
271+
name: vpcs.sdn.cozystack.io
272+
spec:
273+
group: sdn.cozystack.io
274+
names: {kind: VPC, plural: vpcs}
275+
versions:
276+
- name: v1alpha1
277+
served: true
278+
schema: {openAPIV3Schema: {type: object}}
279+
{{- end }}
280+
`)
281+
282+
snap, err := LoadSnapshot(root)
283+
if err != nil {
284+
t.Fatal(err)
285+
}
286+
if _, ok := snap[resourceKey{Group: "sdn.cozystack.io", Plural: "vpcs"}]; !ok {
287+
t.Fatalf("CRD under templates/ was not discovered: %v", snap)
288+
}
289+
}
290+
291+
// TestLoadSnapshotDiscoversAPIServiceAndTripsNewGroup covers the other half of
292+
// the cozyplane gap: a group served only by an aggregated apiserver (no CRD,
293+
// no cozyrd, no apiserver.go entry) — its schema lives entirely outside this
294+
// repo, but the group's first appearance must still require review. Versions
295+
// registered via separate sibling APIService objects (one per version, the
296+
// real-world shape) must merge onto the same resource.
297+
func TestLoadSnapshotDiscoversAPIServiceAndTripsNewGroup(t *testing.T) {
298+
root := t.TempDir()
299+
write := func(rel, content string) {
300+
p := filepath.Join(root, filepath.FromSlash(rel))
301+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
302+
t.Fatal(err)
303+
}
304+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
305+
t.Fatal(err)
306+
}
307+
}
308+
write("packages/system/cozyplane/templates/apiserver-v1alpha1.yaml", `apiVersion: apiregistration.k8s.io/v1
309+
kind: APIService
310+
metadata: {name: v1alpha1.sdn.cozystack.io}
311+
spec: {group: sdn.cozystack.io, version: v1alpha1}
312+
`)
313+
write("packages/system/cozyplane/templates/apiserver-v1beta1.yaml", `apiVersion: apiregistration.k8s.io/v1
314+
kind: APIService
315+
metadata: {name: v1beta1.sdn.cozystack.io}
316+
spec: {group: sdn.cozystack.io, version: v1beta1}
317+
`)
318+
319+
// A minimal but non-empty base, unrelated to sdn.cozystack.io.
320+
baseRoot := t.TempDir()
321+
writeBase := func(rel, content string) {
322+
p := filepath.Join(baseRoot, filepath.FromSlash(rel))
323+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
324+
t.Fatal(err)
325+
}
326+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
327+
t.Fatal(err)
328+
}
329+
}
330+
writeBase("packages/system/postgres-rd/cozyrds/postgres.yaml", sampleCozyRD)
331+
332+
base, err := LoadSnapshot(baseRoot)
333+
if err != nil {
334+
t.Fatal(err)
335+
}
336+
head, err := LoadSnapshot(root)
337+
if err != nil {
338+
t.Fatal(err)
339+
}
340+
341+
key := resourceKey{Group: "sdn.cozystack.io", Plural: apiServicePlural}
342+
res, ok := head[key]
343+
if !ok {
344+
t.Fatalf("APIService group was not discovered: %v", head)
345+
}
346+
if _, ok := res.Versions["v1alpha1"]; !ok {
347+
t.Fatalf("expected v1alpha1 merged in, got %v", res.Versions)
348+
}
349+
if _, ok := res.Versions["v1beta1"]; !ok {
350+
t.Fatalf("expected v1beta1 from the sibling APIService object merged in, got %v", res.Versions)
351+
}
352+
353+
if countCategory(Classify(base, head), NewGroup) != 1 {
354+
t.Fatalf("expected the APIService-only group to trip NewGroup, got %v", Classify(base, head))
355+
}
356+
}
357+
358+
// TestLoadSnapshotIgnoresThirdPartyManifestsUnderTemplates guards against the
359+
// content-based walk becoming too permissive: a vendored CRD and APIService
360+
// for a non-cozystack.io group, sitting in the exact same templates/ shape,
361+
// must still be filtered out.
362+
func TestLoadSnapshotIgnoresThirdPartyManifestsUnderTemplates(t *testing.T) {
363+
root := t.TempDir()
364+
write := func(rel, content string) {
365+
p := filepath.Join(root, filepath.FromSlash(rel))
366+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
367+
t.Fatal(err)
368+
}
369+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
370+
t.Fatal(err)
371+
}
372+
}
373+
write("packages/system/cert-manager/templates/crds.yaml", `apiVersion: apiextensions.k8s.io/v1
374+
kind: CustomResourceDefinition
375+
metadata: {name: certificates.cert-manager.io}
376+
spec:
377+
group: cert-manager.io
378+
names: {kind: Certificate, plural: certificates}
379+
versions:
380+
- name: v1
381+
served: true
382+
schema: {openAPIV3Schema: {type: object}}
383+
`)
384+
write("packages/system/metrics/templates/apiservice.yaml", `apiVersion: apiregistration.k8s.io/v1
385+
kind: APIService
386+
metadata: {name: v1beta1.metrics.k8s.io}
387+
spec: {group: metrics.k8s.io, version: v1beta1}
388+
`)
389+
// Give the checkout a first-party resource so LoadSnapshot's
390+
// empty-snapshot guard doesn't fire for an unrelated reason.
391+
write("packages/system/postgres-rd/cozyrds/postgres.yaml", sampleCozyRD)
392+
393+
snap, err := LoadSnapshot(root)
394+
if err != nil {
395+
t.Fatal(err)
396+
}
397+
if _, ok := snap[resourceKey{Group: "cert-manager.io", Plural: "certificates"}]; ok {
398+
t.Fatalf("vendored third-party CRD must not be discovered: %v", snap)
399+
}
400+
if _, ok := snap[resourceKey{Group: "metrics.k8s.io", Plural: apiServicePlural}]; ok {
401+
t.Fatalf("vendored third-party APIService must not be discovered: %v", snap)
402+
}
403+
}
404+
189405
// TestLoadSnapshotEmptyErrors covers the silent-bypass guard: an empty result
190406
// (e.g. wrong directory) must be an error, not a clean pass.
191407
func TestLoadSnapshotEmptyErrors(t *testing.T) {

0 commit comments

Comments
 (0)