diff --git a/internal/controller/common/condition.go b/internal/controller/common/condition.go index 091f8eae..f1804eff 100644 --- a/internal/controller/common/condition.go +++ b/internal/controller/common/condition.go @@ -19,6 +19,8 @@ const ( UnknownReason = "Unknown" DetachedSpecMismatch = "DetachedSpecMismatch" InvalidNameReason = "InvalidName" + // Non-UTF-8 generated secret; kubelet rejects it as a secretKeyRef env var. + InvalidSecretEncodingReason = "InvalidSecretEncoding" ) const ( diff --git a/internal/controller/reconciler/generate_secrets_test.go b/internal/controller/reconciler/generate_secrets_test.go new file mode 100644 index 00000000..068e53cd --- /dev/null +++ b/internal/controller/reconciler/generate_secrets_test.go @@ -0,0 +1,155 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reconciler + +import ( + "context" + "testing" + "unicode/utf8" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + apiv2 "github.com/wandb/operator/api/v2" + "github.com/wandb/operator/internal/controller/common" + serverManifest "github.com/wandb/operator/pkg/wandb/manifest" +) + +func newGenerateSecretsFixture( + t *testing.T, + seed ...ctrlClient.Object, +) (ctrlClient.Client, *apiv2.WeightsAndBiases) { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, apiv2.AddToScheme(scheme)) + + wandb := &apiv2.WeightsAndBiases{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps.wandb.com/v2", Kind: "WeightsAndBiases"}, + ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "default"}, + } + objects := append([]ctrlClient.Object{wandb}, seed...) + client := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(&apiv2.WeightsAndBiases{}). + WithObjects(objects...). + Build() + return client, wandb +} + +// effectiveSecretValue returns the value under key. The fake client does not +// fold StringData into Data, so prefer StringData then fall back to Data. +func effectiveSecretValue(sec *corev1.Secret, key string) string { + if v, ok := sec.StringData[key]; ok { + return v + } + return string(sec.Data[key]) +} + +func weaveWorkerAuthManifest() serverManifest.Manifest { + return serverManifest.Manifest{ + GeneratedSecrets: []serverManifest.GeneratedSecret{ + {Name: "weave-worker-auth", Length: 32, CharacterType: "password", UseExactName: true}, + }, + } +} + +// TestGenerateSecrets_FailsOnNonUTF8AdoptedSecret: a non-UTF-8 token must fail +// the reconcile loudly (error + Ready=false condition + warning event) rather +// than being silently rewritten. +func TestGenerateSecrets_FailsOnNonUTF8AdoptedSecret(t *testing.T) { + invalid := []byte{0xff, 0xfe, 0xfd, 0x00, 0x80} + require.False(t, utf8.Valid(invalid), "test precondition: bytes must be invalid UTF-8") + + seeded := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "weave-worker-auth", Namespace: "default"}, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{"key": invalid}, + } + client, wandb := newGenerateSecretsFixture(t, seeded) + recorder := record.NewFakeRecorder(10) + + _, err := generateSecrets(context.Background(), client, recorder, wandb, weaveWorkerAuthManifest()) + require.Error(t, err) + require.Contains(t, err.Error(), "non-UTF-8") + + var sec corev1.Secret + require.NoError(t, client.Get(context.Background(), + types.NamespacedName{Name: "weave-worker-auth", Namespace: "default"}, &sec)) + require.Equal(t, invalid, sec.Data["key"], "invalid secret must not be overwritten") + require.NotContains(t, sec.StringData, "key", "no regeneration should have occurred") + + require.False(t, wandb.Status.Ready) + cond := apimeta.FindStatusCondition(wandb.Status.Conditions, readyConditionType) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionFalse, cond.Status) + require.Equal(t, common.InvalidSecretEncodingReason, cond.Reason) + + select { + case ev := <-recorder.Events: + require.Contains(t, ev, common.InvalidSecretEncodingReason) + default: + t.Fatal("expected a warning event to be recorded") + } +} + +// TestGenerateSecrets_LeavesValidExistingValueUntouched: a valid adopted token +// is preserved (no needless rotation). +func TestGenerateSecrets_LeavesValidExistingValueUntouched(t *testing.T) { + valid := []byte("already-valid-token-123") + seeded := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "weave-worker-auth", Namespace: "default"}, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{"key": valid}, + } + client, wandb := newGenerateSecretsFixture(t, seeded) + + _, err := generateSecrets(context.Background(), client, record.NewFakeRecorder(10), wandb, weaveWorkerAuthManifest()) + require.NoError(t, err) + + var sec corev1.Secret + require.NoError(t, client.Get(context.Background(), + types.NamespacedName{Name: "weave-worker-auth", Namespace: "default"}, &sec)) + + require.Equal(t, valid, sec.Data["key"], "valid existing value must not be overwritten") + require.NotContains(t, sec.StringData, "key", "no regeneration should have occurred") +} + +// TestGenerateSecrets_CreatesMissingSecretWithUTF8Token: fresh secrets hold a +// UTF-8-safe token. +func TestGenerateSecrets_CreatesMissingSecretWithUTF8Token(t *testing.T) { + client, wandb := newGenerateSecretsFixture(t) + + _, err := generateSecrets(context.Background(), client, record.NewFakeRecorder(10), wandb, weaveWorkerAuthManifest()) + require.NoError(t, err) + + var sec corev1.Secret + require.NoError(t, client.Get(context.Background(), + types.NamespacedName{Name: "weave-worker-auth", Namespace: "default"}, &sec)) + + value := effectiveSecretValue(&sec, "key") + require.NotEmpty(t, value) + require.True(t, utf8.ValidString(value)) + require.Len(t, value, 32) +} diff --git a/internal/controller/reconciler/reconcile_v2.go b/internal/controller/reconciler/reconcile_v2.go index 57891d89..7b485f27 100644 --- a/internal/controller/reconciler/reconcile_v2.go +++ b/internal/controller/reconciler/reconcile_v2.go @@ -24,6 +24,7 @@ import ( "net/url" "strings" "time" + "unicode/utf8" "github.com/samber/lo" apiv2 "github.com/wandb/operator/api/v2" @@ -374,7 +375,7 @@ func Reconcile( return ctrl.Result{RequeueAfter: defaultRequeueDuration}, nil } - res, err = ReconcileWandbManifest(ctx, client, wandb, manifest, telemetryConfig) + res, err = ReconcileWandbManifest(ctx, client, recorder, wandb, manifest, telemetryConfig) // send up the manifest error for now if err != nil { return res, err @@ -402,6 +403,7 @@ func consolidateResults(results []ctrl.Result) ctrl.Result { func ReconcileWandbManifest( ctx context.Context, client ctrlClient.Client, + recorder record.EventRecorder, wandb *apiv2.WeightsAndBiases, manifest serverManifest.Manifest, telemetryConfig TelemetryRuntimeConfig, @@ -442,7 +444,7 @@ func ReconcileWandbManifest( validateLegacyOverrides(ctx, wandb, manifest) - result, err = generateSecrets(ctx, client, wandb, manifest) + result, err = generateSecrets(ctx, client, recorder, wandb, manifest) if err != nil { return result, err } @@ -1410,7 +1412,7 @@ func runMigrations(ctx context.Context, client ctrlClient.Client, wandb *apiv2.W return ctrl.Result{RequeueAfter: 5 * time.Second}, nil } -func generateSecrets(ctx context.Context, client ctrlClient.Client, wandb *apiv2.WeightsAndBiases, manifest serverManifest.Manifest) (ctrl.Result, error) { +func generateSecrets(ctx context.Context, client ctrlClient.Client, recorder record.EventRecorder, wandb *apiv2.WeightsAndBiases, manifest serverManifest.Manifest) (ctrl.Result, error) { statusBefore := wandb.DeepCopy().Status // Ensure any manifest-declared generated secrets exist and capture their selectors in status if wandb.Status.GeneratedSecrets == nil { @@ -1460,12 +1462,23 @@ func generateSecrets(ctx context.Context, client ctrlClient.Client, wandb *apiv2 return ctrl.Result{}, err } } else { - // Secret exists. Ensure it has the expected key; do not overwrite existing value. - if sec.Data == nil || (sec.Data != nil && sec.Data[keyName] == nil && sec.StringData == nil) { - if sec.StringData == nil { - sec.StringData = map[string]string{} + // Secret exists; don't overwrite a valid existing value. + existing, hasKey := sec.Data[keyName] + // Non-UTF-8 secretKeyRef env vars break container creation. + if hasKey && !utf8.Valid(existing) { + msg := fmt.Sprintf( + "generated secret %q key %q contains non-UTF-8 bytes; values consumed as container environment variables must be valid UTF-8 — replace it with a UTF-8-safe value", + secretName, keyName, + ) + recorder.Event(wandb, corev1.EventTypeWarning, common.InvalidSecretEncodingReason, msg) + if err := updateReadyStatus(ctx, client, wandb, statusBefore, false, common.InvalidSecretEncodingReason, msg); err != nil { + return ctrl.Result{}, err } - // Generate a value only if missing + return ctrl.Result{}, errors.New(msg) + } + if !hasKey && sec.StringData == nil { + // Secret exists but has no usable key; populate one. + sec.StringData = map[string]string{} valueLen := gs.Length if valueLen <= 0 { valueLen = 32 diff --git a/internal/controller/weightsandbiases_controller_networking_test.go b/internal/controller/weightsandbiases_controller_networking_test.go index e8da2ee3..dc909764 100644 --- a/internal/controller/weightsandbiases_controller_networking_test.go +++ b/internal/controller/weightsandbiases_controller_networking_test.go @@ -14,6 +14,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" ) @@ -340,7 +341,7 @@ func reconcileNetworkingManifest(ctx context.Context, wandb *apiv2.WeightsAndBia wandbManifest, err := manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version) Expect(err).NotTo(HaveOccurred()) - _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + _, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).NotTo(HaveOccurred()) } diff --git a/internal/controller/weightsandbiases_controller_test.go b/internal/controller/weightsandbiases_controller_test.go index 93c2a457..8c1237df 100644 --- a/internal/controller/weightsandbiases_controller_test.go +++ b/internal/controller/weightsandbiases_controller_test.go @@ -239,7 +239,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { By("Checking if Applications were NOT created yet (migrations not complete)") wandbManifest, err := manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version) Expect(err).Should(Succeed()) - _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + _, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) By("Checking if the MySQL init job was created") @@ -324,7 +324,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { By("Checking if Applications were NOT created yet (migrations not complete)") wandbManifest, err := manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version) Expect(err).Should(Succeed()) - ctrlResult, err := v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err := v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeNumerically(">", 0)) @@ -342,7 +342,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { Expect(k8sClient.Status().Update(ctx, wandb)).Should(Succeed()) // For now test by calling ReconcileWandbManifest directly, but this will get refactored into the reconciler later - ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeZero()) @@ -412,7 +412,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { By("Reconciling the manifest to completion for the initial generation") wandbManifest, err := manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version) Expect(err).Should(Succeed()) - ctrlResult, err := v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err := v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeZero()) @@ -431,7 +431,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { By("Reconciling while the new version's migration is still pending") wandbManifest, err = manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version) Expect(err).Should(Succeed()) - ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeNumerically(">", 0)) @@ -446,7 +446,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { wandb.Status.Wandb.Migration.Reason = "Complete" Expect(k8sClient.Status().Update(ctx, wandb)).Should(Succeed()) - ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeZero()) @@ -520,7 +520,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { By("Reconciling the manifest to create the Applications") wandbManifest, err := manifest.GetServerManifest(ctx, wandb.Spec.Wandb.ManifestRepository, wandb.Spec.Wandb.Version) Expect(err).Should(Succeed()) - _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + _, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) appList := &apiv2.ApplicationList{} @@ -566,7 +566,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { By("Reconciling again: the gate must pass on live Deployments even though the status map says not-ready") Expect(k8sClient.Get(ctx, wandbLookupKey, wandb)).Should(Succeed()) - _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + _, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) err = k8sClient.Get(ctx, types.NamespacedName{Name: legacy.Name, Namespace: WandbNamespace}, &appsv1.Deployment{}) @@ -581,7 +581,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { Expect(k8sClient.Status().Update(ctx, refreshed)).Should(Succeed()) Expect(k8sClient.Get(ctx, wandbLookupKey, wandb)).Should(Succeed()) - _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + _, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(k8sClient.Get(ctx, wandbLookupKey, wandb)).Should(Succeed()) Expect(wandb.Status.Wandb.Applications[appName].Ready).To(BeTrue(), @@ -640,7 +640,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { wandb.Status.ClickHouseStatus[apiv2.DefaultInstanceName] = clickHouseStatus Expect(k8sClient.Status().Update(ctx, wandb)).Should(Succeed()) - ctrlResult, err := v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err := v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeNumerically(">", 0), "Expected requeue when migration is running") @@ -650,7 +650,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { wandb.Status.Wandb.Migration.Reason = "Failed" Expect(k8sClient.Status().Update(ctx, wandb)).Should(Succeed()) - ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeNumerically(">", 0), "Expected requeue when migration failed") @@ -662,7 +662,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { wandb.Status.Wandb.MySQLInit = map[string]apiv2.MigrationJobStatus{apiv2.DefaultInstanceName: {Succeeded: true}} Expect(k8sClient.Status().Update(ctx, wandb)).Should(Succeed()) - ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + ctrlResult, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) Expect(ctrlResult.RequeueAfter).Should(BeZero(), "Expected no requeue when migration is complete") }) @@ -730,7 +730,7 @@ var _ = Describe("WeightsAndBiases Controller V2", func() { // This call to ReconcileWandbManifest should trigger runMigrations, // which sees version mismatch and starts migrations. - _, err = v2.ReconcileWandbManifest(ctx, k8sClient, wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) + _, err = v2.ReconcileWandbManifest(ctx, k8sClient, record.NewFakeRecorder(100), wandb, wandbManifest, v2.DefaultTelemetryRuntimeConfig()) Expect(err).Should(Succeed()) By("Verifying migration status was reset for the new version")