From e7ceab4c35b57de37235c6af4442314babf9f4e1 Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Thu, 6 Aug 2026 10:34:06 -0500 Subject: [PATCH 1/2] fix/manifest-tag --- .../weightsandbiases_conversion_overrides.go | 13 +++-- ...ghtsandbiases_conversion_overrides_test.go | 53 ++++++++++++++++--- api/v1/weightsandbiases_conversion_test.go | 12 +++++ .../webhook/v2/weightsandbiases_webhook.go | 12 +++++ .../v2/weightsandbiases_webhook_test.go | 39 ++++++++++++++ 5 files changed, 117 insertions(+), 12 deletions(-) diff --git a/api/v1/weightsandbiases_conversion_overrides.go b/api/v1/weightsandbiases_conversion_overrides.go index 204d3714..46f2f8f3 100644 --- a/api/v1/weightsandbiases_conversion_overrides.go +++ b/api/v1/weightsandbiases_conversion_overrides.go @@ -141,8 +141,12 @@ 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") @@ -150,9 +154,8 @@ func mapPerAppLegacyOverrides(values map[string]interface{}, version, globalSize } 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)) diff --git a/api/v1/weightsandbiases_conversion_overrides_test.go b/api/v1/weightsandbiases_conversion_overrides_test.go index c85734af..9c2de740 100644 --- a/api/v1/weightsandbiases_conversion_overrides_test.go +++ b/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") diff --git a/api/v1/weightsandbiases_conversion_test.go b/api/v1/weightsandbiases_conversion_test.go index 014b614c..df08cb89 100644 --- a/api/v1/weightsandbiases_conversion_test.go +++ b/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", diff --git a/internal/webhook/v2/weightsandbiases_webhook.go b/internal/webhook/v2/weightsandbiases_webhook.go index fd229ca4..e856408a 100644 --- a/internal/webhook/v2/weightsandbiases_webhook.go +++ b/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 } diff --git a/internal/webhook/v2/weightsandbiases_webhook_test.go b/internal/webhook/v2/weightsandbiases_webhook_test.go index 8bb0d2ae..f288f28e 100644 --- a/internal/webhook/v2/weightsandbiases_webhook_test.go +++ b/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 = "" From 0f531d2fe7b9535a248247c63d28e954b37ae1de Mon Sep 17 00:00:00 2001 From: Collin Olander Date: Thu, 6 Aug 2026 11:20:45 -0500 Subject: [PATCH 2/2] fix claude comments --- api/v1/weightsandbiases_conversion_overrides.go | 7 +------ internal/webhook/v2/weightsandbiases_webhook.go | 5 +---- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/api/v1/weightsandbiases_conversion_overrides.go b/api/v1/weightsandbiases_conversion_overrides.go index 46f2f8f3..9dc8c00f 100644 --- a/api/v1/weightsandbiases_conversion_overrides.go +++ b/api/v1/weightsandbiases_conversion_overrides.go @@ -141,12 +141,7 @@ func mapLegacyOverrides(values map[string]interface{}, dst *appsv2.WeightsAndBia return nil } -// 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. +// fail on manifest fetch failure 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") diff --git a/internal/webhook/v2/weightsandbiases_webhook.go b/internal/webhook/v2/weightsandbiases_webhook.go index e856408a..404a8929 100644 --- a/internal/webhook/v2/weightsandbiases_webhook.go +++ b/internal/webhook/v2/weightsandbiases_webhook.go @@ -448,10 +448,7 @@ 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. + // Reject latest manifest tag if strings.TrimSpace(wandb.Spec.Wandb.Version) == "latest" { errors = append(errors, field.Invalid( field.NewPath("spec").Child("wandb").Child("version"),