🐛 fix(samples): make the advanced and team-operator overlays apply again - #327
Merged
Conversation
TestSampleKindsDecode decoded samples into map[string]any and asserted only that kind was non-empty, so it could not catch a schema-invalid sample. It also enumerated nine hardcoded filenames, leaving every other sample ungated. Replace it with a walk over config/samples/ that, for each kollect.dev document, decodes strictly into the registered Go type and validates the document against the openAPIV3Schema committed in config/crd/bases/. Both layers are needed: the Go types accept shapes the CRDs reject (InventorySinkRefList still unmarshals the legacy bare-string sinkRef form), and CRD schema validation prunes rather than rejects unknown fields. Nothing may opt out silently, because a gate that skips is a gate that lies: - a kollect.dev document with no matching CRD schema fails; - a document whose group is neither kollect.dev nor on the closed nonKollectSampleGroups allowlist fails, so a group typo such as `kolect.dev/v1alpha1` cannot pass itself off as somebody else's object; - a non-empty document without apiVersion or kind fails, so dropping the apiVersion line cannot smuggle a bogus spec past the walk; - the kollect.dev document count must reach minValidatedSampleDocs (55 against 57 today), so deleting a sample directory turns the suite red. Also assert the team-operator snapshot sink against ValidateSnapshotSinkSpec: its pathTemplate has to satisfy webhook rules that the CRD schema cannot express. The suite is red at this commit: it names the invalid samples the previous assertion hid.
Both `kubectl apply -k config/samples/advanced/` and
`kubectl apply -k config/samples/team-operator/` are documented entry points and
the API server rejected both. Five files, six documents, seven rejections.
advanced/: three KollectInventory samples (four documents) wrote
`databaseSinkRefs: [<bare string>]`, but the CRD requires objects with a required
`name`, so the apiserver answered
`spec.databaseSinkRefs[0]: Invalid value: "string": ... must be of type object`.
Give every entry the `name:` key the snapshot refs already used.
team-operator/: the KollectScope used a `sinkRefs` field that does not exist —
KollectScope splits refs per sink family (ADR-0414) — and the KollectSnapshotSink
omitted the required `spec.type` while putting `endpoint` and `pathTemplate` under
`spec.git`, where neither field lives. Use `snapshotSinkRefs`, declare `type: git`,
and lift the two common fields to spec level next to the other snapshot samples.
The snapshot sink pathTemplate had to change value, not just position: internal
validation requires {namespace} and {name}, so the literal `inventory/snapshot.json`
would still have been rejected by the webhook after the fields moved. Use
`inventory/{namespace}/{name}{extension}` so the extension follows
serialization.format instead of being hardcoded.
config/samples/kustomization.yaml carried neither apiVersion nor kind. Kustomize
tolerates that, but it also means the file cannot be told apart from a Kubernetes
manifest that lost its apiVersion, which the samples gate must reject. Declare
both, matching every other kustomization.yaml in the tree.
Both overlays now render with kustomize and pass the CRD schema gate.
Two corrections to comments in test/samples/crd_schema_test.go. No behaviour
change: minValidatedSampleDocs, the group allowlist, and every assertion are
untouched, and the diff contains no non-comment line.
The minValidatedSampleDocs comment claimed the floor turns the suite red when a
single sample directory is deleted. It does not. Eleven directories hold
kollect.dev documents and six of them hold exactly two (demo/git-postgres, e2e,
and pipeline/{deployment-images,helm-releases,ingress-hosts,namespaces}), so
removing any of those six leaves 55 and still clears the floor. Say what the
floor actually guarantees — a tripwire for a truncated or broken walk, 57 to
roughly zero — and say why it is not raised to the exact count: that would red
the build on every legitimate sample removal.
Also state what the gate does not cover, so it cannot overclaim again: it reads
source files rather than rendered overlays; it skips allowlisted foreign-group
documents entirely, because the allowlist keys on group alone; and it checks the
CRD schema rather than this project's validating webhooks, which
config/samples/team-operator/snapshot-sink.yaml showed are not the same thing.
Correction to the previous commit's body: declaring apiVersion and kind on
config/samples/kustomization.yaml matches every kustomization.yaml inside
config/samples/ (5 of 5), not the whole tree — 6 of the 12 under config/ declare
neither. The change itself stands: editing the file beats a filename exemption,
which would have reintroduced a silent skip in the one place the gate must not
skip.
Collaborator
Author
|
Added a 4th commit |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was broken
kubectl apply -k config/samples/advanced/was failing. The samples wrotewhere the CRD requires objects (
- name: postgres-inventory-demo). The API server rejected the documents on apply.It survived the existing gate because
TestSampleKindsDecodedecoded each document intomap[string]anyand asserted only thatkind != "". Any syntactically valid YAML passed.Blast radius
Larger than the two files originally filed: 5 files / 6 documents / 7 rejections.
kollect_v1alpha1_kollectinventory_sharded.yamlcarried the same bug in both of its documents, not one.kubectl apply -k config/samples/team-operator/— documented as an entry point atdocs/examples/team-operator.md:10— was a second broken documented entry point, not collateral. Itssnapshot-sink.yamlwas additionally rejected by this project's own validating webhook.The replacement gate
The new gate in
test/samples/does two things to every one of the 57kollect.devdocuments underconfig/samples/:config/crd/basesschemas.Both layers are load-bearing and neither subsumes the other:
InventorySinkRefList.UnmarshalJSONdeliberately accepts a bare string for backwards compatibility, so strict decode alone would not have caught theadvanced/bugs.spec.sinkRefsfield.A document-count floor (
minValidatedSampleDocs) guards against a truncated or broken directory walk reporting a vacuous green.Verification
go test ./test/samples/ ./test/schema/ ./test/ci/— greentask lint(golangci-lint 0 issues, go-arch-lint OK) — greentask verify— greenkustomize build config/samples/advanced/andconfig/samples/team-operator/— both build, and the rendereddatabaseSinkRefsare objects