From 8b16b9dfaa773862cde7168876f3c0d8e0e3d92f Mon Sep 17 00:00:00 2001 From: casey-coreweave Date: Fri, 7 Aug 2026 11:13:14 -0700 Subject: [PATCH] fix(operator): Resolve boolean CR fields as env values --- .../controller/reconciler/oidc_env_test.go | 104 +++++++++++++++++- internal/controller/reconciler/pods.go | 2 +- .../controller/reconciler/reconcile_v2.go | 23 ++-- 3 files changed, 120 insertions(+), 9 deletions(-) diff --git a/internal/controller/reconciler/oidc_env_test.go b/internal/controller/reconciler/oidc_env_test.go index e73f12d5..bdc773d6 100644 --- a/internal/controller/reconciler/oidc_env_test.go +++ b/internal/controller/reconciler/oidc_env_test.go @@ -2,6 +2,8 @@ package reconciler import ( "context" + "path/filepath" + "strconv" "testing" apiv2 "github.com/wandb/operator/api/v2" @@ -57,6 +59,40 @@ func TestResolveCRFieldSecretSelector(t *testing.T) { }) } +func TestResolveCRFieldEnvValue(t *testing.T) { + wandb := oidcTestCR() + + t.Run("returns a string unchanged", func(t *testing.T) { + value, ok := resolveCRFieldEnvValue(wandb, "spec.wandb.oidc.sessionLength") + if !ok || value != "48h" { + t.Fatalf("expected session length to resolve to %q, got %q (found: %t)", "48h", value, ok) + } + }) + + for _, value := range []bool{true, false} { + t.Run(strconv.FormatBool(value), func(t *testing.T) { + wandb.Spec.Wandb.BucketProxy = value + + resolved, ok := resolveCRFieldEnvValue(wandb, "spec.wandb.bucketProxy") + if !ok || resolved != strconv.FormatBool(value) { + t.Fatalf("expected bucketProxy to resolve to %q, got %q (found: %t)", strconv.FormatBool(value), resolved, ok) + } + }) + } + + t.Run("rejects a structured value", func(t *testing.T) { + if _, ok := resolveCRFieldEnvValue(wandb, "spec.wandb.oidc.clientId"); ok { + t.Fatal("expected a secret selector not to resolve as a literal env value") + } + }) + + t.Run("rejects a missing path", func(t *testing.T) { + if _, ok := resolveCRFieldEnvValue(wandb, "spec.wandb.nope"); ok { + t.Fatal("expected a missing path not to resolve as a literal env value") + } + }) +} + func TestResolveEnvvarsCustomResourceOIDC(t *testing.T) { client := fake.NewClientBuilder().Build() wandb := oidcTestCR() @@ -99,6 +135,72 @@ func TestResolveEnvvarsCustomResourceOIDC(t *testing.T) { } } +func TestResolveEnvvarsCustomResourceBoolean(t *testing.T) { + client := fake.NewClientBuilder().Build() + envs := []serverManifest.EnvVar{ + { + Name: "BUCKET_PROXY", + Sources: []serverManifest.EnvSource{ + {Type: "custom-resource", Name: "bucket-proxy", Field: "spec.wandb.bucketProxy"}, + }, + }, + } + + for _, value := range []bool{true, false} { + t.Run(strconv.FormatBool(value), func(t *testing.T) { + wandb := &apiv2.WeightsAndBiases{ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "default"}} + wandb.Spec.Wandb.BucketProxy = value + + resolved, err := resolveEnvvars(context.Background(), client, wandb, serverManifest.Manifest{}, nil, envs) + if err != nil { + t.Fatalf("resolveEnvvars returned error: %v", err) + } + + bucketProxy := mustFindEnvVar(t, resolved, "BUCKET_PROXY") + if bucketProxy.Value != strconv.FormatBool(value) { + t.Fatalf("expected BUCKET_PROXY=%q, got %q", strconv.FormatBool(value), bucketProxy.Value) + } + }) + } +} + +func TestResolveEnvvarsManifestBucketProxyBooleans(t *testing.T) { + repoRoot, err := filepath.Abs(filepath.Join("..", "..", "..")) + if err != nil { + t.Fatalf("resolve repository root: %v", err) + } + manifest, err := serverManifest.GetServerManifest( + context.Background(), + "file://"+filepath.Join(repoRoot, "hack", "testing-manifests", "server-manifest"), + "0.83.0-clickhouse-keeper.2", + ) + if err != nil { + t.Fatalf("load test server manifest: %v", err) + } + apiApp, ok := manifest.Applications["api"] + if !ok { + t.Fatal("test server manifest does not define the api application") + } + + client := fake.NewClientBuilder().Build() + wandb := &apiv2.WeightsAndBiases{ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "default"}} + wandb.Spec.Wandb.BucketProxy = true + resolved, err := resolveEnvvars(context.Background(), client, wandb, manifest, apiApp.CommonEnvs, apiApp.Env) + if err != nil { + t.Fatalf("resolve api environment: %v", err) + } + + for _, name := range []string{ + "BUCKET_PROXY", + "GORILLA_GLUE_FILE_STORE_IS_PROXIED", + "GORILLA_FILE_STORE_IS_PROXIED", + } { + if value := mustFindEnvVar(t, resolved, name).Value; value != "true" { + t.Errorf("expected %s=true, got %q", name, value) + } + } +} + func TestResolveEnvvarsSessionLengthDefault(t *testing.T) { client := fake.NewClientBuilder().Build() // CR with no OIDC/session config set. @@ -123,4 +225,4 @@ func TestResolveEnvvarsSessionLengthDefault(t *testing.T) { if sessionLen.Value != "720h" { t.Fatalf("expected session length to fall back to default, got %q", sessionLen.Value) } -} \ No newline at end of file +} diff --git a/internal/controller/reconciler/pods.go b/internal/controller/reconciler/pods.go index ec03d584..643de0eb 100644 --- a/internal/controller/reconciler/pods.go +++ b/internal/controller/reconciler/pods.go @@ -438,7 +438,7 @@ func resolveEnvvars(ctx context.Context, client ctrlClient.Client, wandb *v2.Wei singleSecretSelector = sel secretOnlyCount++ addSecretComponent(sel, idx) - } else if val, ok := resolveCRFieldString(wandb, src.Field); ok { + } else if val, ok := resolveCRFieldEnvValue(wandb, src.Field); ok { // Treat as a literal component (not secret-backed) logger.Debug("field found in CR", "cr", wandb.Name, "field", src.Field, "value", val) components = append(components, val) diff --git a/internal/controller/reconciler/reconcile_v2.go b/internal/controller/reconciler/reconcile_v2.go index 409a35d2..2da77ebf 100644 --- a/internal/controller/reconciler/reconcile_v2.go +++ b/internal/controller/reconciler/reconcile_v2.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "net/url" + "strconv" "strings" "time" @@ -1498,7 +1499,7 @@ func generateSecrets(ctx context.Context, client ctrlClient.Client, wandb *apiv2 // resolveCRField traverses a dotted field path (e.g., "spec.wandb.license") in the // provided custom resource object and returns the raw terminal value if present. -// Typed accessors (resolveCRFieldString, resolveCRFieldSecretSelector, ...) build on +// Typed accessors (resolveCRFieldEnvValue, resolveCRFieldSecretSelector, ...) build on // top of this to validate and cast the result to the type they expect. func resolveCRField(obj any, path string) (any, bool) { if obj == nil || path == "" { @@ -1528,16 +1529,24 @@ func resolveCRField(obj any, path string) (any, bool) { return cur, true } -// resolveCRFieldString resolves a dotted field path from the provided custom resource -// object, returning the string value if present. Non-string terminal values are -// treated as not found. -func resolveCRFieldString(obj any, path string) (string, bool) { +// resolveCRFieldEnvValue resolves a dotted field path from the provided custom +// resource object into a literal Kubernetes environment variable value. String +// values pass through unchanged, while booleans use their lowercase Go/Kubernetes +// representation. Other terminal values are treated as not found. +func resolveCRFieldEnvValue(obj any, path string) (string, bool) { cur, ok := resolveCRField(obj, path) if !ok { return "", false } - s, ok := cur.(string) - return s, ok + + switch value := cur.(type) { + case string: + return value, true + case bool: + return strconv.FormatBool(value), true + default: + return "", false + } } func resolveCRFieldSecretSelector(obj any, path string) (corev1.SecretKeySelector, bool) {