fix(manifests): refuse an ambiguous operator version instead of guessing - #38
Open
pyramation wants to merge 1 commit into
Open
fix(manifests): refuse an ambiguous operator version instead of guessing#38pyramation wants to merge 1 commit into
pyramation wants to merge 1 commit into
Conversation
getOperatorResources(id) with no version returned the generated objects, which codegen emits from whichever version was vendored last. So adding Knative v1.22.1 alongside v1.15.0 silently moved every versionless caller forward a minor-and-a-half. The objects were "unchanged" in the sense that the same export was still there; their contents were a different Knative, and nothing in that diff named the jump. This is the same failure the version argument was recently fixed to prevent, one function down: the caller gets a version nobody chose and no log line reveals it. That is what made the earlier version of this bug take days to find -- the e2e pinned v1.15.0, logged v1.15.0, and applied v1.22.1, and the default looked stable the whole time. The fix is to make the ambiguity unrepresentable rather than to pick better. A single-version operator has an unambiguous default and keeps working exactly as before -- cert-manager, cloudnative-pg, minio-operator, traefik, tekton-pipelines and kube-prometheus-stack are untouched. A multi-version operator now throws, naming the versions available, on both getOperatorResources and getOperatorManifestPaths (which had the same silent "latest wins" default). Adding a second version to an operator therefore becomes a loud change rather than a quiet one: every versionless caller of that operator fails on the next run with a list to choose from. Also: - getOperatorVersions threw a bare TypeError on an unknown operator, because it indexed OPERATOR_MAP without checking. It now says which operator is unknown and lists the ones that exist. - The client's metadata-coverage test called getOperatorResources with no version, so it was one of the callers silently redirected. It now asserts across every carried version, which is what it was trying to say -- under the old default, which versions it covered depended on pull order. Adds unit tests for both directions: the versionless multi-version call throws, the single-version one still works, an unknown version and an unknown operator are rejected, and each version returns resources whose own app.kubernetes.io/version label matches what was asked for -- checked against the label rather than the filename so a mislabelled vendored file cannot pass. Note: packages/ops-cli __tests__/setup.test.ts has one pre-existing failure (a CLI option count of 7 vs 8). It fails identically on the base commit and is untouched here.
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
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.
getOperatorResources(id)with no version returned the generated objects — which codegen emits from whichever version was vendored last. So adding Knativev1.22.1alongsidev1.15.0silently moved every versionless caller forward a minor-and-a-half. The export was still there; its contents were a different Knative, and nothing in that diff named the jump.This is the same failure the
versionargument was recently fixed to prevent, one function down: the caller gets a version nobody chose, and no log line reveals it. That is precisely what made the earlier form of this bug take days to find — the e2e pinnedv1.15.0, loggedv1.15.0, and appliedv1.22.1, and the default looked stable throughout.The fix
Make the ambiguity unrepresentable rather than pick a better default.
Single-version operators are untouched:
cert-manager,cloudnative-pg,minio-operator,traefik,tekton-pipelines,kube-prometheus-stack. Onlyknative-servingcurrently carries two.getOperatorManifestPathshad the same silent "latest wins" default and is fixed the same way, so both entry points agree.The point is that adding a second version to an operator becomes a loud change. Every versionless caller of that operator fails on the next run with a list to choose from, instead of quietly installing something else.
Also
getOperatorVersionsthrew a bareTypeErroron an unknown operator — it indexedOPERATOR_MAPwithout checking. It now names the unknown operator and lists the ones that exist.getOperatorResourceswith no version, so it was itself one of the silently-redirected callers. It now asserts across every carried version — which is what it was trying to say. Under the old default, which versions it covered depended on pull order.Tests
New
packages/manifests/__tests__/unit/version-resolution.test.ts, 7 cases covering both directions: versionless multi-version throws, single-version still works, unknown version and unknown operator are rejected, and each version returns resources whose ownapp.kubernetes.io/versionlabel matches what was requested.That last one is checked against the resources' label rather than the filename, so a mislabelled vendored file cannot pass. There's also a guard asserting
knative-servingstill carries more than one version — without it, the multi-version assertions would keep passing while testing nothing.Verification
packages/ops-cli/__tests__/setup.test.tshas one pre-existing failure (CLI option count 7 vs 8). Verified it fails identically on the base commit; untouched here.