Skip to content

Commit fa3ff56

Browse files
lllamnyplllamnyp
committed
fix(ci): stop double-parsing every file in the API gate's content walk
branch-review on #3184 measured a real regression: switching CRD/ APIService discovery to content-based scanning made every file in packages/+internal a candidate, and each was split into documents and fully unmarshaled twice — once per parser — inflating a full snapshot load from ~0.5s to ~3.4s. Split each file's documents once and probe each document's kind once before deciding which shape to try, so the common case (a document that is neither a CRD nor an APIService — the overwhelming majority of what the wider walk now sees) pays for at most one parse attempt instead of two. Add a raw substring fast-reject, at both the per-document and whole-file level, ahead of any YAML parsing: neither kind this package cares about can appear without its own name appearing as a literal substring first. Also fixes two smaller gaps flagged in the same review: an untrusted PR head checkout could point discovery at an arbitrarily large file with no size guard (capped at 8 MiB), and a group's merged APIService resource only recorded the first contributing file's path, dropping the others from the report. Assisted-By: Claude <[email protected]> Signed-off-by: Timofei Larkin <[email protected]>
1 parent 821bb9e commit fa3ff56

3 files changed

Lines changed: 292 additions & 90 deletions

File tree

‎internal/apigate/parse.go‎

Lines changed: 135 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package apigate
1818

1919
import (
20+
"bytes"
2021
"encoding/json"
2122
"fmt"
2223
"regexp"
@@ -87,53 +88,73 @@ func ParseCRDs(origin string, data []byte) ([]Resource, error) {
8788
if len(strings.TrimSpace(string(docBytes))) == 0 {
8889
continue
8990
}
90-
var doc struct {
91-
Kind string `json:"kind"`
92-
Spec struct {
93-
Group string `json:"group"`
94-
Names struct {
95-
Kind string `json:"kind"`
96-
Plural string `json:"plural"`
97-
} `json:"names"`
98-
Versions []struct {
99-
Name string `json:"name"`
100-
Served bool `json:"served"`
101-
Schema struct {
102-
OpenAPIV3Schema Schema `json:"openAPIV3Schema"`
103-
} `json:"schema"`
104-
} `json:"versions"`
105-
} `json:"spec"`
91+
res, ok, err := crdFromDoc(origin, i, docBytes)
92+
if err != nil {
93+
return nil, err
10694
}
107-
if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
108-
continue
95+
if ok {
96+
out = append(out, res)
10997
}
110-
if doc.Kind != "CustomResourceDefinition" {
98+
}
99+
return out, nil
100+
}
101+
102+
// crdFromDoc attempts to parse a single YAML document as a
103+
// CustomResourceDefinition. ok is false whenever the document is not a CRD,
104+
// including when it fails to parse at all; err is non-nil only when the
105+
// document unambiguously claims to be a CRD (right kind) but is structurally
106+
// incomplete. docKind is checked first so the fuller CRD-shaped unmarshal —
107+
// and its cost — is paid only for documents that are actually CRDs, since
108+
// LoadSnapshot's content-based walk runs this against every YAML file in the
109+
// tree, the overwhelming majority of which are not CRDs at all.
110+
func crdFromDoc(origin string, i int, docBytes []byte) (Resource, bool, error) {
111+
if docKind(docBytes) != "CustomResourceDefinition" {
112+
return Resource{}, false, nil
113+
}
114+
var doc struct {
115+
Spec struct {
116+
Group string `json:"group"`
117+
Names struct {
118+
Kind string `json:"kind"`
119+
Plural string `json:"plural"`
120+
} `json:"names"`
121+
Versions []struct {
122+
Name string `json:"name"`
123+
Served bool `json:"served"`
124+
Schema struct {
125+
OpenAPIV3Schema Schema `json:"openAPIV3Schema"`
126+
} `json:"schema"`
127+
} `json:"versions"`
128+
} `json:"spec"`
129+
}
130+
if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
131+
// docKind above already parsed this document successfully, so this
132+
// would be surprising; tolerate it like any other unparseable
133+
// document rather than erroring.
134+
return Resource{}, false, nil
135+
}
136+
if doc.Spec.Group == "" || doc.Spec.Names.Plural == "" {
137+
return Resource{}, false, fmt.Errorf("%s (document %d): CRD missing spec.group or spec.names.plural", origin, i)
138+
}
139+
res := Resource{
140+
Group: doc.Spec.Group,
141+
Kind: doc.Spec.Names.Kind,
142+
Plural: doc.Spec.Names.Plural,
143+
Source: SourceCRD,
144+
Origin: origin,
145+
Versions: map[string]Schema{},
146+
}
147+
for _, v := range doc.Spec.Versions {
148+
// Only served versions are live API surface. Skipping unserved
149+
// versions keeps the classifier from diffing schemas no client can
150+
// call, and makes the standard CRD deprecation step (flip
151+
// served:true -> false) surface as a removed served version.
152+
if v.Name == "" || !v.Served {
111153
continue
112154
}
113-
if doc.Spec.Group == "" || doc.Spec.Names.Plural == "" {
114-
return nil, fmt.Errorf("%s (document %d): CRD missing spec.group or spec.names.plural", origin, i)
115-
}
116-
res := Resource{
117-
Group: doc.Spec.Group,
118-
Kind: doc.Spec.Names.Kind,
119-
Plural: doc.Spec.Names.Plural,
120-
Source: SourceCRD,
121-
Origin: origin,
122-
Versions: map[string]Schema{},
123-
}
124-
for _, v := range doc.Spec.Versions {
125-
// Only served versions are live API surface. Skipping unserved
126-
// versions keeps the classifier from diffing schemas no client can
127-
// call, and makes the standard CRD deprecation step (flip
128-
// served:true -> false) surface as a removed served version.
129-
if v.Name == "" || !v.Served {
130-
continue
131-
}
132-
res.Versions[v.Name] = v.Schema.OpenAPIV3Schema
133-
}
134-
out = append(out, res)
155+
res.Versions[v.Name] = v.Schema.OpenAPIV3Schema
135156
}
136-
return out, nil
157+
return res, true, nil
137158
}
138159

139160
// storageVarGroups maps the storage-map variable prefixes used in
@@ -221,31 +242,42 @@ func ParseAPIServices(origin string, data []byte) []Resource {
221242
if len(strings.TrimSpace(string(docBytes))) == 0 {
222243
continue
223244
}
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"`
245+
if res, ok := apiServiceFromDoc(origin, docBytes); ok {
246+
out = append(out, res)
230247
}
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-
})
245248
}
246249
return out
247250
}
248251

252+
// apiServiceFromDoc attempts to parse a single YAML document as an
253+
// APIService. See crdFromDoc for why docKind is checked before the fuller
254+
// unmarshal.
255+
func apiServiceFromDoc(origin string, docBytes []byte) (Resource, bool) {
256+
if docKind(docBytes) != "APIService" {
257+
return Resource{}, false
258+
}
259+
var doc struct {
260+
Spec struct {
261+
Group string `json:"group"`
262+
Version string `json:"version"`
263+
} `json:"spec"`
264+
}
265+
if err := unmarshalYAMLDoc(docBytes, &doc); err != nil {
266+
return Resource{}, false
267+
}
268+
if doc.Spec.Group == "" || doc.Spec.Version == "" {
269+
return Resource{}, false
270+
}
271+
return Resource{
272+
Group: doc.Spec.Group,
273+
Kind: "(aggregated apiserver)",
274+
Plural: apiServicePlural,
275+
Source: SourceAPIService,
276+
Origin: origin,
277+
Versions: map[string]Schema{doc.Spec.Version: {}},
278+
}, true
279+
}
280+
249281
// helmDirectiveLine matches a line that is entirely Go-template/Helm markup
250282
// (`{{- if ... }}`, `{{- end }}`, `{{- /* comment */ -}}`), including its
251283
// trailing newline.
@@ -269,12 +301,52 @@ func unmarshalYAMLDoc(doc []byte, out any) error {
269301
return yaml.Unmarshal(helmDirectiveLine.ReplaceAll(doc, nil), out)
270302
}
271303

304+
// docKind cheaply extracts a YAML document's top-level kind field, without
305+
// paying for a full CRD- or APIService-shaped unmarshal on documents that
306+
// are neither. LoadSnapshot's discovery walk scans every .yaml/.yml file
307+
// under packages/ and internal/ rather than a curated directory allowlist
308+
// (see discoverYAMLFiles), so the overwhelming majority of documents it
309+
// sees are not API manifests at all; probing kind first, once, keeps that
310+
// walk cheap.
311+
// mightBeAPIManifest is a cheap fast-reject shared by docKind (per document)
312+
// and LoadSnapshot (per whole file, before even splitting it into
313+
// documents). Both kinds this package looks for (crdFromDoc,
314+
// apiServiceFromDoc) appear as their own kind name verbatim in a valid
315+
// manifest; content containing neither substring cannot be either kind. This
316+
// is the overwhelming majority of what a content-based walk across the whole
317+
// tree sees — Deployments, Services, plain values trees, and everything else
318+
// that is neither a CRD nor an APIService — so rejecting on a raw substring
319+
// scan avoids the cost of the `---` split, the YAML parse attempt, and (for
320+
// a document that isn't valid strict YAML at all — an ordinary Helm
321+
// template) the further regex-stripped retry in unmarshalYAMLDoc.
322+
func mightBeAPIManifest(data []byte) bool {
323+
return bytes.Contains(data, []byte("CustomResourceDefinition")) || bytes.Contains(data, []byte("APIService"))
324+
}
325+
326+
func docKind(docBytes []byte) string {
327+
if !mightBeAPIManifest(docBytes) {
328+
return ""
329+
}
330+
var probe struct {
331+
Kind string `json:"kind"`
332+
}
333+
if err := unmarshalYAMLDoc(docBytes, &probe); err != nil {
334+
return ""
335+
}
336+
return probe.Kind
337+
}
338+
339+
// yamlDocSeparator matches a `---` document-separator line. Compiled once at
340+
// package init rather than per splitYAMLDocs call, since LoadSnapshot's
341+
// content-based walk now runs this against every YAML file in the tree.
342+
var yamlDocSeparator = regexp.MustCompile(`(?m)^---\s*$`)
343+
272344
// splitYAMLDocs splits a YAML stream into individual document byte slices on
273345
// `---` separators. It intentionally mirrors the simple splitting Helm and
274346
// kubectl use; CRD manifests in this repo do not embed `---` inside block
275347
// scalars, so line-based splitting is safe.
276348
func splitYAMLDocs(data []byte) [][]byte {
277-
parts := regexp.MustCompile(`(?m)^---\s*$`).Split(string(data), -1)
349+
parts := yamlDocSeparator.Split(string(data), -1)
278350
out := make([][]byte, 0, len(parts))
279351
for _, p := range parts {
280352
out = append(out, []byte(p))

‎internal/apigate/parse_test.go‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,99 @@ spec: {group: metrics.k8s.io, version: v1beta1}
402402
}
403403
}
404404

405+
// TestLoadSnapshotSkipsOversizedFiles guards the size cap added alongside
406+
// content-based discovery: since every .yaml/.yml file in the tree is now a
407+
// candidate (no directory allowlist), an untrusted PR head checkout could
408+
// otherwise force an unbounded read/parse of a pathologically large file. A
409+
// first-party CRD past maxYAMLFileSize must be excluded from discovery.
410+
func TestLoadSnapshotSkipsOversizedFiles(t *testing.T) {
411+
root := t.TempDir()
412+
write := func(rel, content string) {
413+
p := filepath.Join(root, filepath.FromSlash(rel))
414+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
415+
t.Fatal(err)
416+
}
417+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
418+
t.Fatal(err)
419+
}
420+
}
421+
padding := strings.Repeat("# padding\n", (maxYAMLFileSize/len("# padding\n"))+1)
422+
write("packages/system/oversized/crds/widgets.yaml", padding+sampleCRD)
423+
// A small, legitimate resource so the empty-snapshot guard doesn't fire
424+
// for an unrelated reason.
425+
write("packages/system/postgres-rd/cozyrds/postgres.yaml", sampleCozyRD)
426+
427+
snap, err := LoadSnapshot(root)
428+
if err != nil {
429+
t.Fatal(err)
430+
}
431+
if _, ok := snap[resourceKey{Group: "example.cozystack.io", Plural: "widgets"}]; ok {
432+
t.Fatalf("CRD in a file past maxYAMLFileSize must be excluded: %v", snap)
433+
}
434+
}
435+
436+
// TestLoadSnapshotMergesAPIServiceOriginAcrossFiles covers the traceability
437+
// fix: when a group's versions are registered via several sibling APIService
438+
// objects in different files (the real-world shape — one object per
439+
// version), the merged resource's Origin must mention every contributing
440+
// file, not just the first one encountered.
441+
func TestLoadSnapshotMergesAPIServiceOriginAcrossFiles(t *testing.T) {
442+
root := t.TempDir()
443+
write := func(rel, content string) {
444+
p := filepath.Join(root, filepath.FromSlash(rel))
445+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
446+
t.Fatal(err)
447+
}
448+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
449+
t.Fatal(err)
450+
}
451+
}
452+
write("packages/system/cozyplane/templates/apiserver-v1alpha1.yaml", `apiVersion: apiregistration.k8s.io/v1
453+
kind: APIService
454+
metadata: {name: v1alpha1.sdn.cozystack.io}
455+
spec: {group: sdn.cozystack.io, version: v1alpha1}
456+
`)
457+
write("packages/system/cozyplane/templates/apiserver-v1beta1.yaml", `apiVersion: apiregistration.k8s.io/v1
458+
kind: APIService
459+
metadata: {name: v1beta1.sdn.cozystack.io}
460+
spec: {group: sdn.cozystack.io, version: v1beta1}
461+
`)
462+
463+
snap, err := LoadSnapshot(root)
464+
if err != nil {
465+
t.Fatal(err)
466+
}
467+
res, ok := snap[resourceKey{Group: "sdn.cozystack.io", Plural: apiServicePlural}]
468+
if !ok {
469+
t.Fatalf("APIService group was not discovered: %v", snap)
470+
}
471+
for _, want := range []string{
472+
"packages/system/cozyplane/templates/apiserver-v1alpha1.yaml",
473+
"packages/system/cozyplane/templates/apiserver-v1beta1.yaml",
474+
} {
475+
if !strings.Contains(res.Origin, want) {
476+
t.Fatalf("expected Origin to mention %q, got %q", want, res.Origin)
477+
}
478+
}
479+
}
480+
481+
// BenchmarkLoadSnapshotRealTree tracks LoadSnapshot's wall-clock cost against
482+
// this repository's own checkout — the actual worst case the CI gate runs
483+
// against on every PR. Content-based discovery (see discoverYAMLFiles) scans
484+
// every .yaml/.yml file under packages/ and internal/, not a curated
485+
// directory allowlist, so this is meant to catch a future change that
486+
// reintroduces redundant per-file work (e.g. splitting or parsing the same
487+
// file's documents more than once). Run with:
488+
//
489+
// go test ./internal/apigate/ -run '^$' -bench LoadSnapshotRealTree
490+
func BenchmarkLoadSnapshotRealTree(b *testing.B) {
491+
for b.Loop() {
492+
if _, err := LoadSnapshot("../.."); err != nil {
493+
b.Fatal(err)
494+
}
495+
}
496+
}
497+
405498
// TestLoadSnapshotEmptyErrors covers the silent-bypass guard: an empty result
406499
// (e.g. wrong directory) must be an error, not a clean pass.
407500
func TestLoadSnapshotEmptyErrors(t *testing.T) {

0 commit comments

Comments
 (0)