diff --git a/api/v1/weightsandbiases_conversion_overrides.go b/api/v1/weightsandbiases_conversion_overrides.go index 204d3714..9dc8c00f 100644 --- a/api/v1/weightsandbiases_conversion_overrides.go +++ b/api/v1/weightsandbiases_conversion_overrides.go @@ -141,8 +141,7 @@ 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. +// 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") @@ -150,9 +149,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..404a8929 100644 --- a/internal/webhook/v2/weightsandbiases_webhook.go +++ b/internal/webhook/v2/weightsandbiases_webhook.go @@ -448,6 +448,15 @@ func validateWandbSpec(wandb *appsv2.WeightsAndBiases) field.ErrorList { )) } + // Reject latest manifest tag + 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 = ""