fix(api): declare OpenAPIModelName for core and sdn types - #3808
Conversation
The aggregated apiserver publishes the core.cozystack.io and sdn.cozystack.io models under their Go import path, so a $ref to any of them does not resolve and client-side validation fails on every resource of those groups, not only the one named in the error: error validating data: SchemaError(…core/v1alpha1.Option.spec): unknown model in reference: "github.com~1cozystack~1…v1alpha1.OptionSpec" Since Kubernetes 0.35 the apiserver's DefinitionNamer.GetDefinitionName returns the model name it is handed verbatim; it no longer converts the Go import-path form into the "friendly" reversed-path form. Whatever name the generated openapi map uses therefore becomes both the published definition key and, JSON pointer escaped, the $ref pointing at it. A name containing "/" ships as a definition keyed on the raw path while every reference to it spells each slash "~1", and clients resolve a $ref by trimming "#/definitions/" without unescaping (kube-openapi pkg/util/proto/document.go). The two spellings never meet, the reference dangles, and validation of the whole document fails. Mirror b916d74, which fixed the same disagreement for apps: add the +k8s:openapi-model-package marker to each doc.go and declare OpenAPIModelName for all 11 core and 9 sdn types, so GetCanonicalTypeName, Scheme.ToOpenAPIDefinitionName and the generated map key all agree on a slash-free dotted name. That takes the number of slash-bearing definition names in the published document from 20 to 0. The methods are hand-written rather than emitted by openapi-gen's --output-model-name-file because that flag also rewrites zz_generated.model_name.go inside the read-only apimachinery and apiextensions module-cache packages the shared gen_openapi helper always passes as inputs, which fails on any consumer including CI that vendors deps from the cache. With the marker in place openapi-gen emits Type{}.OpenAPIModelName() for every type it references, so a future type without a method fails the build instead of silently reintroducing a Go-path name. The generated openapi is the output of the root `make generate`; nothing else in the generated tree moved. kubectl apply --validate has been broken against any cozystack-api built after 2026-07-21 on every resource, and this shipped in v1.6.0 and v1.6.1. In e2e it surfaces as the platform install hanging on cozy-backup-controller/backupstrategy-controller, which takes tenant-root and everything behind cozystack-basics with it. Refs: #3806 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Nothing asserted that a published definition name contains no slash, which is why the core and sdn groups shipped as Go import paths in v1.6.0 and v1.6.1 without anyone noticing. The same omission on a future group would be just as invisible. TestDefinitionNamesAreDottedModelNames fails on any definition name containing "/", names the offender and points at the fix (the +k8s:openapi-model-package marker plus an OpenAPIModelName method). TestDefinitionRefsResolve is the failure mode itself: it builds each $ref the way kube-openapi's builder does, JSON pointer escaped, then resolves it the way a client does, trimming "#/definitions/" with no unescaping, and reports any that dangles. It also checks the declared Dependencies against the published names, which catches a group whose types disagree with each other rather than uniformly. Both assert the invariant instead of listing today's 20 names, so a new group that omits the marker is caught rather than a changed count, and both guard against going vacuous if the definition map is ever emptied or stops covering cozystack's own types. Both fail on the pre-fix tree: the first names all 20 offending definitions, and the second reproduces the reported error verbatim, down to "github.com~1cozystack~1cozystack~1pkg~1apis~1core~1v1alpha1.OptionSpec". Refs: #3806 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe core and SDN v1alpha1 API packages now declare Kubernetes OpenAPI model packages and provide canonical dotted names for their models. ChangesOpenAPI model names
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change publishes dotted OpenAPI model names for core and sdn resources so client references resolve correctly; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 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 |
|
LGTM. I measured the fix instead of reading it, and the counts in the description hold. Driving the real The guard tests bite. Restoring only The mechanism checks out at the consumer rather than at the definition. On the window,
Two non-blocking notes. The tests do not catch a name that is slash-free but wrong. A method returning another type's model name would give the generated map two entries under one key, later one winning, and every ref would still resolve, so both tests stay green while a schema ships under the wrong name. The The description reports "all 23 cozystack kinds" carrying the group-version-kind extension. My run reproduces 23 as the total number of definitions carrying it, 10 of them cozystack-owned and 13 from apimachinery and apiextensions. Worth pinning down which number was meant before it lands in history. Everything above is local measurement; the two end-to-end checks were still running when I looked, so that side is unverified here. Operationally, this changes only the document the aggregated apiserver serves, not the wire format of any resource, so nothing that reads or writes objects is touched. A client holding a cached copy of the old document keeps failing until it refetches, so the fix can look absent until that cache turns over. |
|
Successfully created backport PR for |
…bootstrap (#3815) ## What this PR does Closes #3814. Unblocks a fresh install of `main`. Since #3562 the `backupstrategy-controller` chart renders one default Strategy CR ungated, and that makes the whole release fail on a cluster where its CRDs are not yet applied: ``` Helm install failed for release cozy-backup-controller/backupstrategy-controller resource mapping not found for name: "cozy-default-mongodb" no matches for kind "MongoDB" in version "strategy.backups.cozystack.io/v1alpha1" ``` The chart ships its own eight `strategy.backups.cozystack.io` CRDs as ordinary templates, since `templates/crds.yaml` globs `definitions/*.yaml`, and Helm pre-applies only a `crds/` directory. Helm then resolves every document in the release manifest through the cluster's RESTMapper in `Build()` before it applies any of them, so an ungated Strategy CR has no mapping on the first install, `Build()` fails, and the release applies **zero** objects, including the CRDs that would have made the mapping resolve. Nothing recovers from that. Every retry renders the same manifest, so it is permanent rather than a race, and Helm's kind sorter cannot help because it orders the apply and the failure is before the apply. The install log shows `Warning InstallFailed 43s (x36 over 20m)` and then `Install failed (no retry)`. ### Why the seven siblings never hit this Each of them gates on a resolved `$bucketName`, which is empty until the COSI BucketClaim status is reconciled, so they are simply absent from the first render. By the time `DefaultObjectsGate` forces a second revision the CRDs from revision 1 exist and the mapping resolves. That gate is documented as "the CR needs the bucket name", and it silently doubles as the CRD-ordering guard. MongoDB genuinely does not need the bucket name, the template argues that correctly, so the gate was left off and the accidental protection went with it. This gates it too, for the ordering reason rather than the value, and records that reason in the template so the next bucket-independent strategy keeps it. Convergence is unchanged. `DefaultObjectsGate` already routes `apps.cozystack.io/MongoDB` to `cozy-default-mongodb` and forces a revision once any routed object is missing, which is exactly how the seven siblings materialise today. ### Why this was invisible `tests/rendering_test.yaml` asserted `count: 1` for the MongoDB template under a case named "bootstrap window (no resolvable bucket name)", so the defect was codified as the expected behaviour and shipped green. helm-unittest has no RESTMapper and cannot see the consequence. That assertion is flipped here, with the ordering reason written next to it. It was also masked in CI until yesterday by #3806. `StreamVisitor.Visit` returns immediately on a validation error but only continues on a mapping error, so the OpenAPI defect aborted the manifest stream at the first document and the mapping check was never reached. #3808 fixed that and this surfaced underneath it. ### Screenshots Not applicable. ### Downstream repositories - [x] No downstream repository is affected by this change ### Testing Verified by rendering both windows, since the whole bug is which documents appear in the first render and a cluster adds nothing to that. Before, bootstrap window: one Strategy document, `MongoDB`, alongside the eight not-yet-applied CRDs. After: none. With `backupStorage.bucketNameOverride=b`, which stands in for a resolved BucketClaim: all eight, MongoDB included. ``` helm template bsc packages/system/backupstrategy-controller | grep -c '^kind: MongoDB$' ``` `helm unittest packages/system/backupstrategy-controller` passes, 4 suites, 18 tests. ### Follow-up, not in this PR The durable fix is to stop shipping these CRDs as templates and split `definitions/` into a `backupstrategy-controller-crds` package with a `dependsOn` edge, which is the convention six other packages already follow. That cannot be a hotfix: none of the eight definitions carries `helm.sh/resource-policy: keep`, so the upgrade that removes them from this chart's manifest would have Helm delete the eight CRDs and cascade-delete every live Strategy CR on existing clusters. It needs the `keep` annotation shipped a release ahead. ### Release note ```release-note fix(backupstrategy-controller): gate the default MongoDB backup strategy on the bootstrap window, so a fresh platform install no longer fails with "no matches for kind MongoDB" and applies nothing ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented the default MongoDB backup strategy from rendering before the required S3 storage configuration is available. * Improved first-install reliability by avoiding premature strategy resource creation. * **Tests** * Updated bootstrap validation to confirm the MongoDB strategy is omitted until a bucket name is resolved. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Fixes #3806.
Declares
OpenAPIModelNamefor thecoreandsdnAPI groups the way #3273 already did forapps, so every published OpenAPI definition name is the dotted Kubernetes model name rather than a Go import path.Worth noting the two are the same event. #3273 landed the k8s 0.35 work, hit the loud half of this in server-side apply where no apps kind resolved at all, fixed
apps, and leftcoreandsdnwith the quieter half. Its merge commit960d85370is exactly where the window opens: 190 failed-E2E logs from 2026-06-25 to 07-19 carry no occurrence, and the first one is 11 hours after it.Before this, 20 definitions shipped under names like
github.com/cozystack/cozystack/pkg/apis/core/v1alpha1.Option, while every$refpointing at them spelled each slash~1. Clients resolve a$refby trimming#/definitions/and looking the remainder up verbatim with no unescaping, so the two spellings never meet and the reference dangles.kubectl apply --validateagainst a cluster running acozystack-apibuilt after 2026-07-21 therefore fails on any resource, and in e2e the platform install fails oncozy-backup-controller/backupstrategy-controllerand never completes.The mechanism is a Kubernetes 0.35 change:
DefinitionNamer.GetDefinitionNamenow returns the model name verbatim, where it previously converted the Go import-path form into the friendly reversed-path form. So whatever name the generated map uses becomes both the published key and the escaped$ref, and slash-freedom stops being a nice-to-have and becomes the correctness condition.Why hand-written methods
openapi-gen can emit these with
--output-model-name-file, but that also rewriteszz_generated.model_name.goinside the read-only apimachinery and apiextensions module-cache packages that the sharedgen_openapihelper always passes as inputs, which fails for any consumer that vendors deps from the cache, CI included. Theappsgroup settled this the same way and the reasoning is repeated in each newmodel_name.goso nobody simplifies it back into the flag.Once a package carries
+k8s:openapi-model-package, openapi-gen emitsType{}.OpenAPIModelName()for every type it writes a schema or a$reffor, so a missing method is a build failure rather than a silent skip. That makes a future omission inside these groups loud by construction, and the new tests make an omission on a future group loud at the group level.Guard tests
pkg/generated/openapi/definitions_test.goadds two, both asserting the invariant rather than today's 20 names, and both refusing to pass on an empty or cozystack-free definition map so they cannot go vacuously green.TestDefinitionNamesAreDottedModelNamesfails on any definition name containing a slash, naming the offender and the fix.TestDefinitionRefsResolvebuilds each$refthe way kube-openapi's builder does and resolves it the way a client does, which checks the failure directly rather than through the slash-freedom proxy, and also catches a group whose types disagree with each other.Nothing asserted this before, which is why the defect reached v1.6.0 and v1.6.1 unnoticed.
Screenshots
Not applicable.
Downstream repositories
Walked the trigger map against the diff. Published model names change, so I swept for consumers pinning the old form: every hit on
github.com/cozystack/cozystack/pkg/apisin the tree is a Go import, which is unaffected because the package path does not change, andapi/api-rules/cozystack_api_violation_exceptions.listkeys off the Go package path in its own format and was unchanged by regeneration. No string literal anywhere pins the old model-name form, andapiPrefixinpkg/cmd/server/openapi.gois an apps-only const that this does not touch.Testing
go build ./... && go vet ./...go test ./...make generate && git diff --exit-code, clean and idempotentpre-commit run --all-filesTestDefinitionNamesAreDottedModelNamesnamed all 20 offenders andTestDefinitionRefsResolvereproduced the reported error string with 15 dangling refs, fewer than 20 because some of the 20 are only referenced-from and never referencing.openapi.NewDefinitionNamer(apiserver.Scheme)reports 104 published definitions with 0 slashes, 106 emitted refs with 0 dangling, and all 23 cozystack kinds resolving to a published definition carryingx-kubernetes-group-version-kind, which also confirms theappsserver-side-apply fix still holds.make unit-testsaborts athack/ghcr-mirror_test.bats, which fails on a pristineorigin/mainin my environment while passing in CI, so I ran the chain standalone: the other 60 bats files pass, as dohelm-unit-tests,go-unit-tests,rd-presets-check,test-check-readinessandmigrations-target-check.Release note
Summary by CodeRabbit