fix/manifest-tag

api/v1/weightsandbiases_conversion_overrides.go

@@ -141,18 +141,21 @@ func mapLegacyOverrides(values map[string]interface{}, dst *appsv2.WeightsAndBia
 	return nil
 }
 
-// mapPerAppLegacyOverrides is best-effort: a manifest fetch failure must never
-// make v1 objects unservable, so it logs and skips instead of erroring.
+// mapPerAppLegacyOverrides fails when the server manifest can't be resolved.
+// The manifest is the only authority on which values sections are applications,
+// so continuing would silently discard every per-application env and resource
+// override while reporting a successful conversion — the failure mode that gets
+// discovered in production. Rejecting the write is recoverable; a silently
+// gutted CR is not.
 func mapPerAppLegacyOverrides(values map[string]interface{}, version, globalSize string, overrides map[string]appsv2.LegacyOverrides) error {
 	if version == "" {
 		logger.Info("no version derived from v1 values; skipping per-application legacy overrides")
 		return nil
 	}
 	apps, err := legacyManifestApps(version)
 	if err != nil {
-		logger.Error(err, "failed to resolve server manifest; skipping per-application legacy overrides",
-			"version", version)
-		return nil
+		return fmt.Errorf("resolve server manifest for version %q (required to map per-application "+
+			"env/resources overrides): %w", version, err)
 	}
 
 	appNames := make([]string, 0, len(apps))

api/v1/weightsandbiases_conversion_overrides_test.go

@@ -401,13 +401,52 @@ func TestConvertTo_LegacyOverridesManifestUnavailable(t *testing.T) {
 			"env": map[string]interface{}{"API_VAR": "1"},
 		},
 	}))
-	// A manifest fetch failure must never fail conversion: global env still
-	// converts, per-app extraction is skipped.
+	// Converting anyway would drop api's env while reporting success, so the
+	// write is rejected instead.
+	err := src.ConvertTo(dst)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "resolve server manifest")
+	require.Contains(t, err.Error(), testLegacyVersion)
+	require.Contains(t, err.Error(), "registry unreachable")
+}
+
+// TestConvertTo_LegacyOverridesManifestUnavailableNoAppSections: with nothing
+// per-application to lose, an unresolvable manifest is still fatal — the
+// manifest decides what counts as an application, so we can't know there was
+// nothing to map.
+func TestConvertTo_LegacyOverridesManifestUnavailableNoAppSections(t *testing.T) {
+	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
+		return serverManifest.Manifest{}, errors.New("registry unreachable")
+	})
+	t.Cleanup(disableConversionManifestFetch)
+
+	dst := &appsv2.WeightsAndBiases{}
+	src := newV1(withVersion(map[string]interface{}{
+		"global": map[string]interface{}{"host": "http://wandb.example.com"},
+	}))
+	require.Error(t, src.ConvertTo(dst))
+}
+
+// TestConvertTo_NoVersionSkipsManifestFetch: without a version there is nothing
+// to resolve, so conversion proceeds and global env still converts.
+func TestConvertTo_NoVersionSkipsManifestFetch(t *testing.T) {
+	var calls atomic.Int32
+	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
+		calls.Add(1)
+		return serverManifest.Manifest{}, errors.New("registry unreachable")
+	})
+	t.Cleanup(disableConversionManifestFetch)
+
+	dst := &appsv2.WeightsAndBiases{}
+	src := newV1(map[string]interface{}{
+		"global": map[string]interface{}{
+			"env": map[string]interface{}{"HTTP_PROXY": "http://proxy"},
+		},
+	})
 	require.NoError(t, src.ConvertTo(dst))
 
-	overrides := dst.Spec.Wandb.LegacyOverrides
-	require.Contains(t, overrides, appsv2.LegacyOverridesGlobalKey)
-	require.NotContains(t, overrides, "api")
+	require.Contains(t, dst.Spec.Wandb.LegacyOverrides, appsv2.LegacyOverridesGlobalKey)
+	require.Equal(t, int32(0), calls.Load(), "no version means no manifest fetch")
 }
 
 func TestConvertTo_LegacyOverridesManifestFailureCooldown(t *testing.T) {
@@ -426,8 +465,8 @@ func TestConvertTo_LegacyOverridesManifestFailureCooldown(t *testing.T) {
 				"env": map[string]interface{}{"API_VAR": "1"},
 			},
 		}))
-		require.NoError(t, src.ConvertTo(dst))
-		require.NotContains(t, dst.Spec.Wandb.LegacyOverrides, "api")
+		// Every attempt fails, but from the cached failure rather than a refetch.
+		require.Error(t, src.ConvertTo(dst))
 	}
 
 	require.Equal(t, int32(1), calls.Load(), "repeat conversions within the cooldown must not retry the fetch")

api/v1/weightsandbiases_conversion_test.go

@@ -160,6 +160,8 @@ func TestConvertTo_CustomCACerts(t *testing.T) {
 }
 
 func TestConvertTo_VersionFromAppImageTag(t *testing.T) {
+	// A resolvable manifest: these assert version mapping, not per-app overrides.
+	withConversionManifestApps(t)
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(map[string]interface{}{
 		"app": map[string]interface{}{
@@ -171,6 +173,8 @@ func TestConvertTo_VersionFromAppImageTag(t *testing.T) {
 }
 
 func TestConvertTo_VersionFallsBackToApiImageTag(t *testing.T) {
+	// A resolvable manifest: these assert version mapping, not per-app overrides.
+	withConversionManifestApps(t)
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(map[string]interface{}{
 		"api": map[string]interface{}{
@@ -182,6 +186,8 @@ func TestConvertTo_VersionFallsBackToApiImageTag(t *testing.T) {
 }
 
 func TestConvertTo_VersionAppWinsOverApi(t *testing.T) {
+	// A resolvable manifest: these assert version mapping, not per-app overrides.
+	withConversionManifestApps(t)
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(map[string]interface{}{
 		"app": map[string]interface{}{
@@ -196,6 +202,8 @@ func TestConvertTo_VersionAppWinsOverApi(t *testing.T) {
 }
 
 func TestConvertTo_VersionEmptyAppFallsBackToApi(t *testing.T) {
+	// A resolvable manifest: these assert version mapping, not per-app overrides.
+	withConversionManifestApps(t)
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(map[string]interface{}{
 		"app": map[string]interface{}{
@@ -219,6 +227,8 @@ func TestConvertTo_VersionAbsent(t *testing.T) {
 }
 
 func TestConvertTo_VersionWithoutGlobal(t *testing.T) {
+	// A resolvable manifest: these assert version mapping, not per-app overrides.
+	withConversionManifestApps(t)
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(map[string]interface{}{
 		"app": map[string]interface{}{
@@ -1687,6 +1697,8 @@ func TestConvertFrom_NoAnnotations(t *testing.T) {
 }
 
 func TestConvertTo_ActiveSpecSecretOverridesCRValues(t *testing.T) {
+	// A resolvable manifest: these assert version mapping, not per-app overrides.
+	withConversionManifestApps(t)
 	withConversionReader(t, activeSpecSecret(t, "default", "wandb", map[string]interface{}{
 		"global": map[string]interface{}{
 			"host":    "http://wandb.from-active-spec",

internal/webhook/v2/weightsandbiases_webhook.go

@@ -448,6 +448,18 @@ func validateWandbSpec(wandb *appsv2.WeightsAndBiases) field.ErrorList {
 		))
 	}
 
+	// v1 installs commonly ran app.image.tag=latest, which conversion copies
+	// verbatim. No server-manifest artifact is published under a mutable tag, so
+	// the manifest lookup fails and the whole reconcile aborts. Reject it at
+	// admission, where the message is actionable, instead of at reconcile time.
+	if strings.TrimSpace(wandb.Spec.Wandb.Version) == "latest" {
+		errors = append(errors, field.Invalid(
+			field.NewPath("spec").Child("wandb").Child("version"),
+			wandb.Spec.Wandb.Version,
+			"must be pinned to a published server version; no server-manifest is published for the \"latest\" tag",
+		))
+	}
+
 	return errors
 }
 

internal/webhook/v2/weightsandbiases_webhook_test.go

@@ -187,6 +187,45 @@ var _ = Describe("WeightsAndBiases Webhook", func() {
 			Expect(err.Error()).To(ContainSubstring("spec.wandb.hostname"))
 		})
 
+		It("rejects create when version is the mutable latest tag", func() {
+			obj.Spec.Wandb.Version = "latest"
+
+			_, err := validator.ValidateCreate(ctx, obj)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec.wandb.version"))
+			Expect(err.Error()).To(ContainSubstring("must be pinned"))
+		})
+
+		It("rejects a whitespace-padded latest", func() {
+			obj.Spec.Wandb.Version = " latest "
+
+			_, err := validator.ValidateCreate(ctx, obj)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec.wandb.version"))
+		})
+
+		It("rejects update to the latest tag", func() {
+			obj.Spec.Wandb.Version = "latest"
+
+			_, err := validator.ValidateUpdate(ctx, oldObj, obj)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec.wandb.version"))
+		})
+
+		It("accepts a pinned version", func() {
+			obj.Spec.Wandb.Version = "0.83.1"
+
+			_, err := validator.ValidateCreate(ctx, obj)
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		It("accepts an empty version (nothing pinned in v1 values)", func() {
+			obj.Spec.Wandb.Version = ""
+
+			_, err := validator.ValidateCreate(ctx, obj)
+			Expect(err).ToNot(HaveOccurred())
+		})
+
 		It("rejects update when hostname is missing", func() {
 			obj.Spec.Wandb.Hostname = ""