fix(manifests): refuse an ambiguous operator version instead of guessing

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]
+        );
+      }
     }
   });
 });

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'/);
+  });
+});

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);