fix(ci): catch templated CRDs and APIService-only groups in the API gate#3184
fix(ci): catch templated CRDs and APIService-only groups in the API gate#3184Timofei Larkin (lllamnyp) wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Timofei Larkin (lllamnyp)
requested a review
from Andrei Kvapil (kvaps)
as a code owner
github-actions
Bot
added
area/ci
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of the API-owner-review gate by shifting from directory-based discovery to content-based discovery. By scanning all YAML files and implementing robust handling for Helm templating, the system now correctly identifies CRDs and APIService registrations that were previously missed due to their directory structure or templated wrappers. This ensures that all first-party API surface changes are properly captured for review. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughAdds ChangesAPIService Discovery and Tolerant Parsing
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant LoadSnapshot
participant discoverYAMLFiles
participant ParseCRDs
participant ParseAPIServices
participant unmarshalYAMLDoc
LoadSnapshot->>discoverYAMLFiles: walk crdRoots for .yaml/.yml files
discoverYAMLFiles-->>LoadSnapshot: candidate file paths
loop each candidate file
LoadSnapshot->>ParseCRDs: parse content
ParseCRDs->>unmarshalYAMLDoc: decode document
unmarshalYAMLDoc-->>ParseCRDs: skip on error or return parsed CRD
LoadSnapshot->>ParseAPIServices: parse content
ParseAPIServices->>unmarshalYAMLDoc: decode document
unmarshalYAMLDoc-->>ParseAPIServices: skip on error or return parsed Resource
end
LoadSnapshot->>LoadSnapshot: merge APIService Versions by (group, plural)
LoadSnapshot->>LoadSnapshot: add merged entries if not already present
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the API gate's discovery mechanism to walk all YAML files under the walked roots instead of relying on specific directory names, allowing it to discover CRDs and APIServices embedded in Helm templates. It also introduces parsing for APIService manifests to support aggregated apiservers. The review feedback focuses on several key improvements: optimizing the file walk by performing a fast substring check for first-party groups before parsing, addressing redundant checks and potential issues with map value updates, correcting misleading comments regarding error handling in ParseCRDs, and reducing string allocations in the parsing loop by using bytes.TrimSpace.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err != nil { | ||
| // A file living in a crds/ or definitions/ dir that does not parse | ||
| // as YAML is almost always an upstream/templated artifact, not a | ||
| // first-party CRD; skip it rather than failing the whole gate. | ||
| // A file that does not parse as YAML at all (not even one | ||
| // tolerated document) is almost always an upstream/templated | ||
| // artifact, not a first-party CRD; skip it rather than failing | ||
| // the whole gate. | ||
| continue | ||
| } |
There was a problem hiding this comment.
The comment here states that we skip files that do not parse as YAML at all. However, ParseCRDs has been updated to internally catch and skip YAML parsing errors on individual documents (returning continue instead of an error).
As a result, ParseCRDs will only return a non-nil error if a document successfully parses as a CustomResourceDefinition but is malformed (e.g., missing spec.group or spec.names.plural). By silently ignoring err here, we are hiding genuine malformations in first-party CRDs rather than skipping unrelated third-party files.
We should update the comment to reflect this, and consider logging or failing when a genuine malformed CRD error is returned.
if err != nil {
// ParseCRDs only returns an error if a document successfully parses
// as a CRD but is malformed (e.g. missing group or plural).
// Skip it to avoid failing the gate on malformed upstream/templated files.
continue
}| for _, path := range yamlFiles { | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| resources, err := ParseCRDs(rel(dir, path), data) | ||
| origin := rel(dir, path) |
There was a problem hiding this comment.
Since the API gate now walks all .yaml/.yml files under packages/ and internal/ (which includes all vendored upstream Helm charts under charts/ directories), it will read and attempt to parse thousands of third-party files. This can significantly slow down the CI gate due to disk I/O and YAML parsing overhead.
Since all first-party API groups must belong to the cozystack.io family, we can optimize this by performing a fast substring check on the raw file content before attempting any YAML parsing. If the file does not contain the string "cozystack.io", we can safely skip it.
for _, path := range yamlFiles {
data, err := os.ReadFile(path)
if err != nil {
return nil, err
}
if !bytes.Contains(data, []byte("cozystack.io")) {
continue
}
origin := rel(dir, path)| key := resourceKey{Group: res.Group, Plural: res.Plural} | ||
| existing, ok := apiServices[key] | ||
| if !ok { | ||
| apiServices[key] = res | ||
| continue | ||
| } | ||
| maps.Copy(existing.Versions, res.Versions) | ||
| } |
There was a problem hiding this comment.
In the current implementation, apiServices is a map of Resource values (map[resourceKey]Resource). When we retrieve existing, ok := apiServices[key], existing is a copy of the Resource struct.
While maps.Copy(existing.Versions, res.Versions) works because Versions is a map (reference type), modifying any non-reference fields on existing (or if Versions were ever re-assigned) would not propagate back to the map. To make the code more robust, idiomatic, and clear, we should write the updated existing struct back to the map.
key := resourceKey{Group: res.Group, Plural: res.Plural}
existing, ok := apiServices[key]
if !ok {
apiServices[key] = res
continue
}
maps.Copy(existing.Versions, res.Versions)
apiServices[key] = existing
}| for key, res := range apiServices { | ||
| // Do not overwrite a richer cozyrd/CRD/apiserver-storage resource | ||
| // that shares the same key; apiServicePlural is reserved and cannot | ||
| // collide with a real plural, so in practice this only guards | ||
| // against processing the same merged group twice. | ||
| if _, exists := snap[key]; !exists { | ||
| add(res) | ||
| } | ||
| } |
There was a problem hiding this comment.
The check if _, exists := snap[key]; !exists is redundant.
Since apiServicePlural is a reserved sentinel ($apiservice) that cannot collide with any real resource plural name, no other source (cozyrd, CRD, or apiserver storage) can ever populate snap with this key. Furthermore, since apiServices is a map with unique keys, we only iterate over each key once. Thus, exists will always be false. We can simplify this by directly adding the resource.
for _, res := range apiServices {
add(res)
}| if len(strings.TrimSpace(string(docBytes))) == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Converting docBytes to a string and calling strings.TrimSpace allocates a new string for every document in every YAML file. Since we are now walking all YAML files in the repository, this can lead to significant garbage collection overhead.
Using bytes.TrimSpace(docBytes) avoids this allocation entirely.
Note: You will also need to add "bytes" to the imports at the top of internal/apigate/parse.go.
| if len(strings.TrimSpace(string(docBytes))) == 0 { | |
| continue | |
| } | |
| if len(bytes.TrimSpace(docBytes)) == 0 { | |
| continue | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/apigate/snapshot.go (1)
89-140: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueContent walk now parses every YAML under
packages/internaltwice per file.Discovery previously narrowed to a directory allowlist; it now walks all
.yaml/.ymlfiles undercrdRootsand runs bothParseCRDsandParseAPIServices(each re-splitting the document stream) on every file. In a large chart-heavy monorepo this can be a meaningful CI cost. If it becomes a bottleneck, consider a single split/decode pass per file that dispatches onkind, and/or a cheapkind:prefilter before full parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/apigate/snapshot.go` around lines 89 - 140, The snapshot content walk is doing redundant work by parsing each YAML file twice via ParseCRDs and ParseAPIServices after discoverYAMLFiles, which can become expensive in large trees. Refactor the file processing in snapshot.go so a single split/decode pass per file can feed both CRD and APIService handling, and consider adding a cheap kind-based prefilter before full parsing to avoid unnecessary work. Preserve the existing merging logic around apiServices, add, and resourceKey while changing only the per-file parse flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/apigate/snapshot.go`:
- Around line 106-118: In LoadSnapshot, the current ParseCRDs error handling in
the loop over resources treats every failure as a harmless skip, which hides
malformed first-party CRDs and stops later processing. Update the logic around
ParseCRDs, isFirstPartyGroup, and add so that non-YAML/unparseable files are
skipped, but structural CRD parse errors are surfaced instead of ignored. Also
ensure ParseAPIServices is invoked independently for the same input so
APIService documents are still processed even when CRD parsing fails.
---
Nitpick comments:
In `@internal/apigate/snapshot.go`:
- Around line 89-140: The snapshot content walk is doing redundant work by
parsing each YAML file twice via ParseCRDs and ParseAPIServices after
discoverYAMLFiles, which can become expensive in large trees. Refactor the file
processing in snapshot.go so a single split/decode pass per file can feed both
CRD and APIService handling, and consider adding a cheap kind-based prefilter
before full parsing to avoid unnecessary work. Preserve the existing merging
logic around apiServices, add, and resourceKey while changing only the per-file
parse flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34e05b74-0540-488f-ab30-816bb18d13d5
📒 Files selected for processing (4)
internal/apigate/model.gointernal/apigate/parse.gointernal/apigate/parse_test.gointernal/apigate/snapshot.go
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 <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/apigate/snapshot.go`:
- Around line 215-224: The YAML snapshot filtering in the directory walk should
reject symlinks and other non-regular files before checking size. In the walker
logic where filepath.Ext, d.Info(), and maxYAMLFileSize are used, inspect the
file mode from DirEntry.Info() first and skip anything that is not a regular
file; only then apply the size limit for .yaml/.yml entries. Keep the fix
localized to the snapshot traversal path so os.ReadFile is only reached for
regular files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31ad83d6-0354-4dab-95c5-8eb733acd505
📒 Files selected for processing (3)
internal/apigate/parse.gointernal/apigate/parse_test.gointernal/apigate/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/apigate/parse_test.go
| if ext := filepath.Ext(path); ext != ".yaml" && ext != ".yml" { | ||
| return nil | ||
| } | ||
| if _, ok := crdDirNames[filepath.Base(filepath.Dir(path))]; !ok { | ||
| info, err := d.Info() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if info.Size() > maxYAMLFileSize { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby logic
grep -nR "maxYAMLFileSize\|LoadSnapshot\|filepath.Ext(path)" -n internal/apigate/snapshot.go
sed -n '1,320p' internal/apigate/snapshot.go
# Find any other snapshot/file-loading guards or symlink handling in the repo
rg -n "IsRegular\(|Lstat\(|ReadFile\(|WalkDir\(|maxYAMLFileSize|symlink" internal docs -g '!**/vendor/**'Repository: cozystack/cozystack
Length of output: 10667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read Go stdlib docs for the relevant APIs.
go doc filepath.WalkDir
printf '\n---\n'
go doc io/fs.DirEntry
printf '\n---\n'
go doc os.ReadFileRepository: cozystack/cozystack
Length of output: 2494
Skip symlinks before the size check. DirEntry.Info() reports a symlink’s own tiny size, but os.ReadFile follows the link, so a .yaml symlink can bypass maxYAMLFileSize and trigger an unbounded read. Reject non-regular files first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/apigate/snapshot.go` around lines 215 - 224, The YAML snapshot
filtering in the directory walk should reject symlinks and other non-regular
files before checking size. In the walker logic where filepath.Ext, d.Info(),
and maxYAMLFileSize are used, inspect the file mode from DirEntry.Info() first
and skip anything that is not a regular file; only then apply the size limit for
.yaml/.yml entries. Keep the fix localized to the snapshot traversal path so
os.ReadFile is only reached for regular files.
1 participant
What this PR does
Closes two coverage gaps found while shadow-observing the API-owner-review gate (#3167) against real PRs in the first hours after merge:
crds/,definitions/,manifests/), so a CRD shipped inside a chart'stemplates/dir — a valid, existing convention when the CRD is wrapped in a{{- if }}guard — was invisible to the gate. This was not hypothetical:#3149(cozyplane) adds four brand-new CRDs (VPC,VPCBinding,VPCPeering,Port) exactly this way, and none of them tripped a review requirement.#3149also registerssdn.cozystack.iovia akind: APIServicepointing at a prebuilt image built outside this repository — no CRD, no vendored Go types, nothing to diff.Fix
.yaml/.ymlfile underpackages/andinternal/is a candidate, and whether it's kept depends on whether it parses into the rightkindwith a first-party group — not which directory it sits in.{{- if }},{{- end }}, a comment block) sharing a----delimited document with otherwise-valid YAML, rather than dropping the whole document. This matters because the closing{{- end }}of a wrapping conditional has no---before it, so it lands in the same document as the manifest it closes.SourceAPIService, forkind: APIServicemanifests registering a first-party group. Like the existing static Go-backed groups, these carry no checked-in schema, so they participate only in new-group / new-resource / removal detection — never a fabricated breaking-change diff against a schema this repo can't see.Verified against the real
feat/cozyplanebranch: the gate now reports all four new CRDs, plus the removal of the oldsecuritygroups/APIServiceregistration for the same group — a full API-surface migration that was previously invisible end to end.Release note
Summary by CodeRabbit