diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 2482712d0..ccde40d14 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1 +1,11 @@ * @kvaps @lllamnyp @lexfrei @androndo @IvanHunters @sircthulhu @myasnikovdaniil + +# The API review gate must not be editable without an API-owner review, or a +# PR could neuter the detector (it is compiled from the head checkout during +# bootstrap) and slip a sizeable API change through unreviewed. Scoping these +# paths to the API owners overrides the catch-all above (last match wins), so +# changes here require @kvaps or @lllamnyp specifically. Enable "Require review +# from Code Owners" in branch protection for this to bind. +/cmd/api-gate/ @kvaps @lllamnyp +/internal/apigate/ @kvaps @lllamnyp +/.github/workflows/api-review-gate.yaml @kvaps @lllamnyp diff --git a/.github/workflows/api-review-gate.yaml b/.github/workflows/api-review-gate.yaml index 6116e3c3b..7e7ab903d 100644 --- a/.github/workflows/api-review-gate.yaml +++ b/.github/workflows/api-review-gate.yaml @@ -67,7 +67,23 @@ jobs: git worktree add --detach "${RUNNER_TEMP}/base" "${merge_base}" - name: Build api-gate - run: go build -o "${RUNNER_TEMP}/api-gate" ./cmd/api-gate + run: | + set -euo pipefail + # Prefer building the detector from the trusted base checkout so a PR + # cannot neuter the gate by editing its own detector code — the tool + # analyzes the head as *data*, not as code. Fall back to the head + # checkout only while the tool has not yet landed on the base branch + # (bootstrap); in that window CODEOWNERS on cmd/api-gate, + # internal/apigate, and this workflow keeps tampering behind an + # API-owner review. + if [ -d "${RUNNER_TEMP}/base/cmd/api-gate" ]; then + build_dir="${RUNNER_TEMP}/base" + echo "Building api-gate from the trusted base checkout." + else + build_dir="${GITHUB_WORKSPACE}" + echo "::warning::api-gate not present on the base branch yet; building from head (bootstrap). CODEOWNERS enforces API-owner review on the gate's own files." + fi + ( cd "${build_dir}" && go build -o "${RUNNER_TEMP}/api-gate" ./cmd/api-gate ) - name: Detect sizeable API changes id: detect diff --git a/internal/apigate/schemadiff.go b/internal/apigate/schemadiff.go index 1af411927..0d6ce2075 100644 --- a/internal/apigate/schemadiff.go +++ b/internal/apigate/schemadiff.go @@ -115,6 +115,26 @@ func diffNode(path string, base, head Schema, out *[]string) { *out = append(*out, fmt.Sprintf("%s: validation rule added (%q)", path, rule)) } + // nullable: Kubernetes expresses "may be null" via nullable:true rather than + // a "null" entry in the type set. Dropping it rejects previously-valid null + // values, so a true -> false/absent transition is breaking. + if schemaBool(base, "nullable") && !schemaBool(head, "nullable") { + *out = append(*out, fmt.Sprintf("%s: no longer nullable (null values rejected)", path)) + } + + // Composition keywords (oneOf/allOf/not): introducing one where base had + // none can reject previously-valid objects. Conservative like the rest of + // the file — flag an addition; removing a composition constraint is a + // relaxation and is safe. + for _, kw := range []string{"oneOf", "allOf", "not"} { + if _, inBase := base[kw]; inBase { + continue + } + if _, inHead := head[kw]; inHead { + *out = append(*out, fmt.Sprintf("%s: %s composition constraint added", path, kw)) + } + } + // Properties: recurse into shared properties; flag removals. A brand-new // property is additive unless it is required (handled above). baseProps, headProps := schemaProps(base), schemaProps(head) @@ -288,6 +308,13 @@ func schemaString(s Schema, key string) string { return "" } +// schemaBool reads a boolean keyword (e.g. nullable), defaulting to false when +// absent or non-boolean. +func schemaBool(s Schema, key string) bool { + b, _ := s[key].(bool) + return b +} + // schemaNumber reads a numeric keyword. JSON numbers decode as float64 through // sigs.k8s.io/yaml; integers may also appear as int/int64 depending on the // decoder, so both are handled. diff --git a/internal/apigate/schemadiff_test.go b/internal/apigate/schemadiff_test.go index b974b9861..590d1bbd7 100644 --- a/internal/apigate/schemadiff_test.go +++ b/internal/apigate/schemadiff_test.go @@ -217,6 +217,32 @@ func TestDiffSchema(t *testing.T) { base: `{"type":"object","properties":{"v":{"type":"integer","x-kubernetes-validations":[{"rule":"self > 0"}]}}}`, head: `{"type":"object","properties":{"v":{"type":"integer"}}}`, }, + // nullable removal is breaking; adding nullable is a widening. + { + name: "removing nullable is breaking", + base: `{"type":"object","properties":{"v":{"type":"string","nullable":true}}}`, + head: `{"type":"object","properties":{"v":{"type":"string"}}}`, + breaking: true, + wantSub: "no longer nullable", + }, + { + name: "adding nullable is safe (widening)", + base: `{"type":"object","properties":{"v":{"type":"string"}}}`, + head: `{"type":"object","properties":{"v":{"type":"string","nullable":true}}}`, + }, + // oneOf/allOf/not composition added is breaking; removed is safe. + { + name: "adding oneOf composition is breaking", + base: `{"type":"object","properties":{"v":{"type":"object"}}}`, + head: `{"type":"object","properties":{"v":{"type":"object","oneOf":[{"required":["a"]},{"required":["b"]}]}}}`, + breaking: true, + wantSub: "oneOf composition constraint added", + }, + { + name: "removing allOf composition is safe", + base: `{"type":"object","properties":{"v":{"type":"object","allOf":[{"required":["a"]}]}}}`, + head: `{"type":"object","properties":{"v":{"type":"object"}}}`, + }, } for _, tc := range tests {