diff --git a/packages/client/__tests__/unit/manifests.test.ts b/packages/client/__tests__/unit/manifests.test.ts index bff0da1..c634bfc 100644 --- a/packages/client/__tests__/unit/manifests.test.ts +++ b/packages/client/__tests__/unit/manifests.test.ts @@ -1,4 +1,4 @@ -import { getOperatorResources } from "@kubernetesjs/manifests"; +import { getOperatorResources, getOperatorVersions } from "@kubernetesjs/manifests"; describe("manifests: metadata coverage", () => { const operatorNamespaceMap = { @@ -12,14 +12,23 @@ describe("manifests: metadata coverage", () => { traefik: "traefik", }; - it("exports namespaces for supported operators", () => { + // Every version, not one. This used to call getOperatorResources(operator) + // with no version, which returned whichever version was vendored last — so + // the assertion silently stopped covering the versions it no longer chose, + // and covering knative-serving at all became a matter of pull order. + it("exports namespaces for supported operators, at every carried version", () => { for (const operator of Object.keys(operatorNamespaceMap)) { - const manifests = getOperatorResources(operator); - const ns = manifests.find((m) => m.kind === "Namespace"); - expect(ns).toBeTruthy(); - expect((ns?.metadata as any).name).toBe( - operatorNamespaceMap[operator as keyof typeof operatorNamespaceMap] - ); + const versions = getOperatorVersions(operator); + expect(versions.length).toBeGreaterThan(0); + + for (const version of versions) { + const manifests = getOperatorResources(operator, version); + const ns = manifests.find((m) => m.kind === "Namespace"); + expect(ns).toBeTruthy(); + expect((ns?.metadata as any).name).toBe( + operatorNamespaceMap[operator as keyof typeof operatorNamespaceMap] + ); + } } }); }); diff --git a/packages/manifests/__tests__/unit/version-resolution.test.ts b/packages/manifests/__tests__/unit/version-resolution.test.ts new file mode 100644 index 0000000..1c39548 --- /dev/null +++ b/packages/manifests/__tests__/unit/version-resolution.test.ts @@ -0,0 +1,67 @@ +import { + getOperatorResources, + getOperatorManifestPaths, + getOperatorVersions, +} from '../../src'; + +/** + * The versionless default used to be "whichever version was vendored last". + * + * That is how adding Knative v1.22.1 alongside v1.15.0 moved every versionless + * caller forward a minor-and-a-half with nothing in the diff naming the jump. + * These assert the ambiguity is refused rather than guessed. + */ +describe('operator version resolution', () => { + const multi = 'knative-serving'; + const single = 'cert-manager'; + + it('knative-serving is the multi-version case these tests depend on', () => { + // Guards the premise: if this drops to one version, the assertions below + // stop testing anything and would keep passing. + expect(getOperatorVersions(multi).length).toBeGreaterThan(1); + expect(getOperatorVersions(single)).toHaveLength(1); + }); + + it('refuses a versionless call for a multi-version operator', () => { + expect(() => getOperatorResources(multi)).toThrow(/carries 2 versions/); + expect(() => getOperatorManifestPaths(multi)).toThrow(/pass one explicitly/); + }); + + it('names the available versions so the caller can pick', () => { + for (const version of getOperatorVersions(multi)) { + expect(() => getOperatorResources(multi)).toThrow(new RegExp(version.replace('.', '\\.'))); + } + }); + + it('still allows a versionless call for a single-version operator', () => { + expect(getOperatorResources(single).length).toBeGreaterThan(0); + expect(getOperatorManifestPaths(single).length).toBeGreaterThan(0); + }); + + it('returns the version that was actually asked for', () => { + // The bug this whole path exists to prevent: asking for one version and + // being handed another, while every log line still names the one you asked + // for. Checked against the resources' own version label rather than the + // filename, so a mislabelled vendored file cannot pass. + for (const version of getOperatorVersions(multi)) { + const labels = getOperatorResources(multi, version) + .map((r) => (r.metadata as any)?.labels?.['app.kubernetes.io/version']) + .filter(Boolean); + + expect(labels.length).toBeGreaterThan(0); + for (const label of labels) { + expect(`v${label}`).toBe(version); + } + } + }); + + it('rejects an unknown version instead of falling back', () => { + expect(() => getOperatorResources(multi, 'v0.0.0')).toThrow(/has no version 'v0\.0\.0'/); + expect(() => getOperatorManifestPaths(multi, 'v0.0.0')).toThrow(/has no version 'v0\.0\.0'/); + }); + + it('rejects an unknown operator by name', () => { + expect(() => getOperatorVersions('nope')).toThrow(/Unknown operator 'nope'/); + expect(() => getOperatorResources('nope')).toThrow(/Unknown operator 'nope'/); + }); +}); diff --git a/packages/manifests/src/index.ts b/packages/manifests/src/index.ts index 3592b34..4964624 100644 --- a/packages/manifests/src/index.ts +++ b/packages/manifests/src/index.ts @@ -61,13 +61,68 @@ export function getOperatorInfo(operatorId: string): OperatorCatalogEntry { } export function getOperatorVersions(operatorId: string): string[] { - return OPERATOR_MAP[operatorId].versions as string[]; + const entry = OPERATOR_MAP[operatorId]; + if (!entry) { + throw new Error( + `Unknown operator '${operatorId}' (have: ${(OPERATOR_IDS as string[]).join(', ')})` + ); + } + return entry.versions as string[]; +} + +/** + * Which version a caller meant, or an error explaining why that is unanswerable. + * + * An operator carrying one version has an unambiguous default. An operator + * carrying several does not, and picking one for the caller is how this package + * shipped its worst bug: `getOperatorResources` returned whichever version was + * vendored last, so adding Knative v1.22.1 alongside v1.15.0 silently moved + * every versionless caller a minor-and-a-half forward. Nothing in that diff + * named the jump, and the e2e that pinned v1.15.0 logged v1.15.0 while applying + * v1.22.1 — which took days to see precisely because the default looked stable. + * + * So a default that moves under callers is not offered. Adding a second version + * to an operator is now a compile-clean, run-time-loud change: every versionless + * caller of that operator fails on the next run with the list of versions to + * choose from, rather than quietly installing a different one. + */ +function resolveOperatorVersion(operatorId: string, version: string | undefined): string { + const versions = getOperatorVersions(operatorId); + if (versions.length === 0) { + throw new Error(`Operator '${operatorId}' carries no versions`); + } + + if (version !== undefined) { + if (!versions.includes(version)) { + throw new Error( + `Operator '${operatorId}' has no version '${version}' (has: ${versions.join(', ')})` + ); + } + return version; + } + + if (versions.length > 1) { + throw new Error( + `Operator '${operatorId}' carries ${versions.length} versions ` + + `(${versions.join(', ')}) — pass one explicitly.\n` + + `There is no safe default here: it would be whichever version was vendored ` + + `last, which moves the moment a version is added and gives no signal at the ` + + `call site that it moved.` + ); + } + return versions[0]; } export function getOperatorResources(operatorId: string, version?: string): KubernetesResource[] { - // Without a version, the generated objects: typed, already in memory, and - // what every existing caller expects. - if (!version) return OPERATOR_MAP[operatorId].resources as KubernetesResource[]; + // The generated objects are typed and already in memory, so they stay the fast + // path — but only where they are unambiguous. For a single-version operator + // the generated set and that version are the same thing by construction; for a + // multi-version one they are whatever codegen emitted last, which is exactly + // the ambiguity resolveOperatorVersion refuses. + const resolved = resolveOperatorVersion(operatorId, version); + if (getOperatorVersions(operatorId).length === 1) { + return OPERATOR_MAP[operatorId].resources as KubernetesResource[]; + } // With one, parse that version's vendored YAML rather than returning the // generated default and hoping they match. They did not: the generated set is @@ -79,7 +134,7 @@ export function getOperatorResources(operatorId: string, version?: string): Kube // multiply the generated output by the number of versions, and the YAML is // already shipped for getOperatorManifestPaths. const docs: KubernetesResource[] = []; - for (const file of getOperatorManifestPaths(operatorId, version)) { + for (const file of getOperatorManifestPaths(operatorId, resolved)) { const parsed = yaml.loadAll(fs.readFileSync(file, 'utf-8')); for (const doc of parsed) { if (doc && typeof doc === 'object') docs.push(doc as KubernetesResource); @@ -104,17 +159,7 @@ export function getOperatorResources(operatorId: string, version?: string): Kube * cannot get that wrong by accident. */ export function getOperatorManifestPaths(operatorId: string, version?: string): string[] { - const versions = getOperatorVersions(operatorId); - if (!versions || versions.length === 0) { - throw new Error(`Unknown operator '${operatorId}'`); - } - - const resolved = version ?? versions[versions.length - 1]; - if (!versions.includes(resolved)) { - throw new Error( - `Operator '${operatorId}' has no version '${resolved}' (has: ${versions.join(', ')})` - ); - } + const resolved = resolveOperatorVersion(operatorId, version); const root = path.join(operatorsRoot(), operatorId); const asDir = path.join(root, resolved);