Skip to content

Commit dec840e

Browse files
lllamnyplllamnyp
committed
fix(ci): address review of the API gate
Fix schema-diff correctness: guard nil head on removed child schemas, flag adding a type to a previously-unconstrained field, stop reporting a head becoming unconstrained as narrowing, detect additionalProperties:false, and only recurse items/additionalProperties when both sides carry a schema. Gate resource and group removal as new categories. Filter CRD versions to served ones so the deprecation flow surfaces as a removed version, and flag added x-kubernetes-validations (CEL) rules. Discover first-party cozystack.io CRDs by walking crds/ and definitions/ dirs so a relocated CRD (e.g. cozystack-scheduler) is covered, and error on an empty snapshot. Surface unmapped apiserver storage prefixes and allow hyphens in plurals; drop Markdown from text-mode reports. Workflow: diff against the true merge base, count only each reviewer's latest review on the current head commit, and scope the token to the approval step with persist-credentials:false. Assisted-By: Claude <[email protected]> Signed-off-by: Timofei Larkin <[email protected]>
1 parent fa87124 commit dec840e

10 files changed

Lines changed: 445 additions & 72 deletions

File tree

‎.github/workflows/api-review-gate.yaml‎

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -34,27 +34,37 @@ jobs:
3434
# Space-separated GitHub logins allowed to satisfy the gate. Edit here to
3535
# change who can approve sizeable API changes.
3636
API_OWNERS: "lllamnyp kvaps"
37-
BASE_SHA: ${{ github.event.pull_request.base.sha }}
37+
BASE_REF: ${{ github.event.pull_request.base.ref }}
3838
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
3939
PR_NUMBER: ${{ github.event.pull_request.number }}
40-
GH_TOKEN: ${{ github.token }}
4140
steps:
4241
- name: Checkout head
4342
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
4443
with:
4544
ref: ${{ github.event.pull_request.head.sha }}
4645
fetch-depth: 0
46+
# Do not leave a usable token in the repo's git config: the PR-authored
47+
# build and api-gate steps below must not have access to it. The token
48+
# is scoped to the approval step alone, which is the only step that
49+
# calls the GitHub API.
50+
persist-credentials: false
4751

4852
- name: Set up Go
4953
uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5
5054
with:
5155
go-version-file: go.mod
5256
cache: true
5357

54-
- name: Materialize base checkout
58+
- name: Materialize base checkout at the merge base
5559
run: |
5660
set -euo pipefail
57-
git worktree add --detach "${RUNNER_TEMP}/base" "${BASE_SHA}"
61+
# Diff against the PR's actual merge base, not the base branch tip:
62+
# otherwise unrelated commits landing on the base branch while the PR
63+
# is open would invent or hide API findings.
64+
git fetch --no-tags origin "${BASE_REF}"
65+
merge_base="$(git merge-base "${HEAD_SHA}" "origin/${BASE_REF}")"
66+
echo "Merge base: ${merge_base}"
67+
git worktree add --detach "${RUNNER_TEMP}/base" "${merge_base}"
5868
5969
- name: Build api-gate
6070
run: go build -o "${RUNNER_TEMP}/api-gate" ./cmd/api-gate
@@ -80,6 +90,8 @@ jobs:
8090
8191
- name: Check for an API owner approval
8292
if: steps.detect.outputs.sizeable == 'true'
93+
env:
94+
GH_TOKEN: ${{ github.token }}
8395
run: |
8496
set -euo pipefail
8597
{
@@ -88,13 +100,25 @@ jobs:
88100
cat "${RUNNER_TEMP}/report.md"
89101
} >> "$GITHUB_STEP_SUMMARY"
90102
91-
# Latest review state per reviewer; an APPROVED review by any API
92-
# owner satisfies the gate. GitHub forbids self-approval, so a
93-
# sizeable PR authored by an owner still needs the other owner.
94-
approvals="$(gh api \
95-
"repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" \
96-
--paginate \
97-
--jq '[.[] | select(.state=="APPROVED") | .user.login] | unique | .[]')"
103+
# An APPROVED review by any API owner satisfies the gate, but only
104+
# each reviewer's *latest* review *on the current head commit* counts:
105+
# this drops approvals that a later commit invalidated, and approvals
106+
# an owner subsequently superseded with CHANGES_REQUESTED. GitHub
107+
# forbids self-approval, so a sizeable PR authored by an owner still
108+
# needs the other owner.
109+
approvals="$(
110+
gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate |
111+
jq -rs --arg head "${HEAD_SHA}" '
112+
add
113+
| map(select(.commit_id == $head))
114+
| sort_by(.submitted_at // "")
115+
| group_by(.user.login)
116+
| map(last)
117+
| .[]
118+
| select(.state == "APPROVED")
119+
| .user.login
120+
'
121+
)"
98122
99123
for owner in ${API_OWNERS}; do
100124
if grep -qxi "${owner}" <<<"${approvals}"; then

‎internal/apigate/classify.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,49 @@ func Classify(base, head Snapshot) []Finding {
7272
})
7373
}
7474

75+
// Removed API groups: present in base, absent in head. Reported once per
76+
// group against the first (sorted) base resource that carried it.
77+
headGroups := head.groups()
78+
removedGroups := map[string]struct{}{}
79+
for _, res := range sortedResources(base) {
80+
if _, ok := headGroups[res.Group]; ok {
81+
continue
82+
}
83+
if _, done := removedGroups[res.Group]; done {
84+
continue
85+
}
86+
removedGroups[res.Group] = struct{}{}
87+
findings = append(findings, Finding{
88+
Category: RemovedGroup,
89+
Group: res.Group,
90+
Kind: res.Kind,
91+
Plural: res.Plural,
92+
Source: res.Source,
93+
Origin: res.Origin,
94+
Detail: fmt.Sprintf("API group %q is removed by this change", res.Group),
95+
})
96+
}
97+
98+
// Removed resources: present in base, absent in head. A resource whose
99+
// whole group was removed is already covered above, so skip it here.
100+
for _, res := range sortedResources(base) {
101+
if _, ok := head[res.key()]; ok {
102+
continue
103+
}
104+
if _, groupGone := removedGroups[res.Group]; groupGone {
105+
continue
106+
}
107+
findings = append(findings, Finding{
108+
Category: RemovedResource,
109+
Group: res.Group,
110+
Kind: res.Kind,
111+
Plural: res.Plural,
112+
Source: res.Source,
113+
Origin: res.Origin,
114+
Detail: fmt.Sprintf("resource %s (%s) is removed from group %q", res.Plural, displayKind(res), res.Group),
115+
})
116+
}
117+
75118
// Breaking changes: resources present in both, whose schema regresses.
76119
for _, res := range sortedResources(head) {
77120
prev, ok := base[res.key()]

‎internal/apigate/classify_test.go‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,29 @@ func TestClassify(t *testing.T) {
8888
}
8989
})
9090

91-
t.Run("removed resource is not sizeable by these rules", func(t *testing.T) {
92-
// Removing a resource is a different (also serious) event, but it is
93-
// not one of the three gated categories; ensure we do not misreport it
94-
// as any of them.
95-
got := Classify(snap(pgBase), snap())
96-
if len(got) != 0 {
97-
t.Fatalf("expected no findings for a pure removal, got %v", got)
91+
t.Run("removed resource is gated", func(t *testing.T) {
92+
// Deleting a resource is the most disruptive single-resource change and
93+
// must require owner review. The resource's group still has another
94+
// member here, so this is a resource removal, not a group removal.
95+
other := res("apps.cozystack.io", "Redis", "redises", SourceCozyRD, Schema{"type": "object"})
96+
got := Classify(snap(pgBase, other), snap(other))
97+
if n := countCategory(got, RemovedResource); n != 1 {
98+
t.Fatalf("expected 1 removed-resource finding, got %d (%v)", n, got)
99+
}
100+
if countCategory(got, RemovedGroup) != 0 {
101+
t.Fatalf("group still has a member; must not be flagged as removed group: %v", got)
102+
}
103+
})
104+
105+
t.Run("removed group flags group once and does not double-count its resources", func(t *testing.T) {
106+
g1 := res("gone.cozystack.io", "Widget", "widgets", SourceCRD, Schema{"type": "object"})
107+
g2 := res("gone.cozystack.io", "Gadget", "gadgets", SourceCRD, Schema{"type": "object"})
108+
got := Classify(snap(pgBase, g1, g2), snap(pgBase))
109+
if n := countCategory(got, RemovedGroup); n != 1 {
110+
t.Fatalf("expected exactly 1 removed-group finding, got %d (%v)", n, got)
111+
}
112+
if n := countCategory(got, RemovedResource); n != 0 {
113+
t.Fatalf("resources of a fully-removed group must not also be flagged as removed resources, got %d (%v)", n, got)
98114
}
99115
})
100116

‎internal/apigate/model.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,13 @@ const (
3737
// (removed field, narrowed type/enum, newly-required field, tightened
3838
// constraint, removed served version).
3939
Breaking Category = "breaking-change"
40+
// RemovedGroup is emitted when an entire API group present in base is gone
41+
// from head. Deleting a group withdraws its whole API surface.
42+
RemovedGroup Category = "removed-api-group"
43+
// RemovedResource is emitted when a (group, resource) present in base is
44+
// gone from head — the most disruptive single-resource change, since every
45+
// stored object and client of that resource breaks.
46+
RemovedResource Category = "removed-resource"
4047
)
4148

4249
// Source identifies which checked-in artifact a resource was parsed from.

‎internal/apigate/parse.go‎

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ func ParseCRDs(origin string, data []byte) ([]Resource, error) {
9090
} `json:"names"`
9191
Versions []struct {
9292
Name string `json:"name"`
93+
Served bool `json:"served"`
9394
Schema struct {
9495
OpenAPIV3Schema Schema `json:"openAPIV3Schema"`
9596
} `json:"schema"`
@@ -114,7 +115,11 @@ func ParseCRDs(origin string, data []byte) ([]Resource, error) {
114115
Versions: map[string]Schema{},
115116
}
116117
for _, v := range doc.Spec.Versions {
117-
if v.Name == "" {
118+
// Only served versions are live API surface. Skipping unserved
119+
// versions keeps the classifier from diffing schemas no client can
120+
// call, and makes the standard CRD deprecation step (flip
121+
// served:true -> false) surface as a removed served version.
122+
if v.Name == "" || !v.Served {
118123
continue
119124
}
120125
res.Versions[v.Name] = v.Schema.OpenAPIV3Schema
@@ -137,22 +142,38 @@ var storageVarGroups = map[string]string{
137142
// apiserverStorageRe matches `<var>["<plural>"]` storage registrations, e.g.
138143
//
139144
// coreV1alpha1Storage["tenantsecrets"] = ...
140-
var apiserverStorageRe = regexp.MustCompile(`(\w+Storage)\["([a-z0-9]+)"\]`)
145+
//
146+
// The plural class allows hyphens so a future hyphenated resource name is not
147+
// silently dropped. The apps storage map is indexed by a variable
148+
// (appsV1alpha1Storage[resConfig.Application.Plural]), not a string literal, so
149+
// it never matches here — the apps API is covered by the cozyrds instead.
150+
var apiserverStorageRe = regexp.MustCompile(`(\w+Storage)\["([a-z0-9-]+)"\]`)
151+
152+
// unmappedStoragePrefix labels resources whose storage variable prefix is not
153+
// in storageVarGroups. Rather than silently dropping them — which would let a
154+
// contributor add a brand-new aggregated group under a new variable and bypass
155+
// the gate — they are surfaced under this synthetic group so the classifier
156+
// reports them as a new group/resource, prompting the reviewer to map the
157+
// prefix in storageVarGroups.
158+
const unmappedStoragePrefix = "unmapped-apiserver-storage:"
141159

142160
// ParseAPIServerStorages extracts the static Go-backed aggregated resources
143-
// registered in pkg/apiserver/apiserver.go for the groups in storageVarGroups.
144-
// These resources carry no checked-in schema, so they populate only the
145-
// new-group / new-resource detection. Kind is left empty (not recoverable from
146-
// the registration line alone); reporting falls back to the plural.
161+
// registered in pkg/apiserver/apiserver.go. Known variable prefixes map to
162+
// their real group (storageVarGroups); an unknown prefix is surfaced under a
163+
// synthetic group (unmappedStoragePrefix) so drift cannot silently defeat the
164+
// gate. These resources carry no checked-in schema, so they populate only the
165+
// new-group / new-resource / removal detection. Kind is left empty (not
166+
// recoverable from the registration line alone); reporting falls back to the
167+
// plural.
147168
func ParseAPIServerStorages(origin string, data []byte) []Resource {
148169
var out []Resource
149170
seen := map[resourceKey]struct{}{}
150171
for _, m := range apiserverStorageRe.FindAllStringSubmatch(string(data), -1) {
151-
group, ok := storageVarGroups[m[1]]
172+
prefix, plural := m[1], m[2]
173+
group, ok := storageVarGroups[prefix]
152174
if !ok {
153-
continue
175+
group = unmappedStoragePrefix + prefix
154176
}
155-
plural := m[2]
156177
k := resourceKey{Group: group, Plural: plural}
157178
if _, dup := seen[k]; dup {
158179
continue

‎internal/apigate/parse_test.go‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package apigate
1919
import (
2020
"os"
2121
"path/filepath"
22+
"strings"
2223
"testing"
2324
)
2425

@@ -47,6 +48,8 @@ spec:
4748
scope: Namespaced
4849
versions:
4950
- name: v1alpha1
51+
served: true
52+
storage: true
5053
schema:
5154
openAPIV3Schema:
5255
type: object
@@ -88,6 +91,40 @@ func TestParseCRDs(t *testing.T) {
8891
}
8992
}
9093

94+
// TestParseCRDsSkipsUnservedVersions covers B7(a): a version flipped to
95+
// served:false is not live API surface and must be excluded, so the standard
96+
// CRD deprecation step surfaces later as a removed served version.
97+
func TestParseCRDsSkipsUnservedVersions(t *testing.T) {
98+
const crd = `apiVersion: apiextensions.k8s.io/v1
99+
kind: CustomResourceDefinition
100+
metadata:
101+
name: widgets.example.cozystack.io
102+
spec:
103+
group: example.cozystack.io
104+
names: {kind: Widget, plural: widgets}
105+
versions:
106+
- name: v1alpha1
107+
served: false
108+
schema: {openAPIV3Schema: {type: object}}
109+
- name: v1beta1
110+
served: true
111+
schema: {openAPIV3Schema: {type: object}}
112+
`
113+
rs, err := ParseCRDs("widget.yaml", []byte(crd))
114+
if err != nil {
115+
t.Fatal(err)
116+
}
117+
if len(rs) != 1 {
118+
t.Fatalf("expected 1 resource, got %d", len(rs))
119+
}
120+
if _, ok := rs[0].Versions["v1alpha1"]; ok {
121+
t.Fatalf("unserved v1alpha1 must be excluded, got versions %v", rs[0].Versions)
122+
}
123+
if _, ok := rs[0].Versions["v1beta1"]; !ok {
124+
t.Fatalf("served v1beta1 must be present, got versions %v", rs[0].Versions)
125+
}
126+
}
127+
91128
func TestParseAPIServerStorages(t *testing.T) {
92129
src := `
93130
coreV1alpha1Storage["tenantsecrets"] = cozyregistry.RESTInPeace(x)
@@ -110,6 +147,53 @@ func TestParseAPIServerStorages(t *testing.T) {
110147
}
111148
}
112149

150+
// TestParseAPIServerStoragesSurfacesUnknownPrefix covers the drift guard: a new
151+
// aggregated group registered under an unlisted *Storage variable must not be
152+
// silently dropped, or it would bypass the gate entirely.
153+
func TestParseAPIServerStoragesSurfacesUnknownPrefix(t *testing.T) {
154+
src := `newgroupV1alpha1Storage["foos"] = cozyregistry.RESTInPeace(x)`
155+
rs := ParseAPIServerStorages("apiserver.go", []byte(src))
156+
if len(rs) != 1 {
157+
t.Fatalf("expected the unknown-prefix resource to be surfaced, got %v", rs)
158+
}
159+
if !strings.HasPrefix(rs[0].Group, unmappedStoragePrefix) {
160+
t.Fatalf("unknown prefix must surface under the unmapped group, got %q", rs[0].Group)
161+
}
162+
}
163+
164+
// TestLoadSnapshotDiscoversCRDsUnderChartsCrds covers B6: a first-party CRD that
165+
// lives in a chart's crds/ directory (rather than a definitions/ dir) is still
166+
// discovered.
167+
func TestLoadSnapshotDiscoversCRDsUnderChartsCrds(t *testing.T) {
168+
root := t.TempDir()
169+
write := func(rel, content string) {
170+
p := filepath.Join(root, filepath.FromSlash(rel))
171+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
172+
t.Fatal(err)
173+
}
174+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
175+
t.Fatal(err)
176+
}
177+
}
178+
write("packages/system/foo/charts/foo/crds/example.cozystack.io_widgets.yaml", sampleCRD)
179+
180+
snap, err := LoadSnapshot(root)
181+
if err != nil {
182+
t.Fatal(err)
183+
}
184+
if _, ok := snap[resourceKey{Group: "example.cozystack.io", Plural: "widgets"}]; !ok {
185+
t.Fatalf("CRD under charts/*/crds/ was not discovered: %v", snap)
186+
}
187+
}
188+
189+
// TestLoadSnapshotEmptyErrors covers the silent-bypass guard: an empty result
190+
// (e.g. wrong directory) must be an error, not a clean pass.
191+
func TestLoadSnapshotEmptyErrors(t *testing.T) {
192+
if _, err := LoadSnapshot(t.TempDir()); err == nil {
193+
t.Fatal("expected an error for a directory with no API resources")
194+
}
195+
}
196+
113197
// TestLoadSnapshotEndToEnd builds two minimal checkouts on disk and verifies
114198
// the loader + classifier wire together across all three sources.
115199
func TestLoadSnapshotEndToEnd(t *testing.T) {

0 commit comments

Comments
 (0)