diff --git a/api/v1/weightsandbiases_conversion_mapping.go b/api/v1/weightsandbiases_conversion_mapping.go index d1c94b76..fdd635d9 100644 --- a/api/v1/weightsandbiases_conversion_mapping.go +++ b/api/v1/weightsandbiases_conversion_mapping.go @@ -77,7 +77,7 @@ func applyValueMappings(src *WeightsAndBiases, dst *appsv2.WeightsAndBiases) err if err := mapVersion(values, dst); err != nil { return err } - if err := mapServiceAccountAnnotations(values, dst); err != nil { + if err := mapServiceAccount(values, dst); err != nil { return err } if err := mapInternalJWTIssuer(values, dst); err != nil { @@ -166,26 +166,128 @@ func mapVersion(values map[string]interface{}, dst *appsv2.WeightsAndBiases) err return nil } -// mapServiceAccountAnnotations maps v1's per-sub-chart ServiceAccount -// annotations to v2's single spec.wandb.serviceAccount.annotations, -// preferring `app` and falling back to `api`. -func mapServiceAccountAnnotations(values map[string]interface{}, dst *appsv2.WeightsAndBiases) error { - anns, err := readServiceAccountAnnotations(values, "app") +// v1ServiceAccountSubcharts are the v1 subcharts whose serviceAccount block +// describes the W&B application identity, in precedence order. Infra subcharts +// (mysql, redis, …) are deliberately excluded: their serviceAccount blocks +// configure those workloads, and v2 models them separately as +// ManagedServiceAccountSpec. +var v1ServiceAccountSubcharts = []string{"app", "api"} + +// v1ServiceAccount is one subchart's serviceAccount block. +type v1ServiceAccount struct { + subchart string + create *bool + name string + annotations map[string]string +} + +// mapServiceAccount maps v1's per-subchart ServiceAccount blocks to v2's single +// spec.wandb.serviceAccount. +// +// create and name must be carried together: v2's CRD defaults create=true and +// serviceAccountName=wandb, so dropping either one makes the operator stand up +// its own ServiceAccount and orphan the identity the deployment's cloud IAM +// binding is attached to. Carrying create without name is worse still — pods +// would reference a ServiceAccount nothing creates and fail admission. +func mapServiceAccount(values map[string]interface{}, dst *appsv2.WeightsAndBiases) error { + blocks, err := readV1ServiceAccounts(values) if err != nil { return err } - if len(anns) == 0 { - anns, err = readServiceAccountAnnotations(values, "api") - if err != nil { - return err + if len(blocks) == 0 { + return nil + } + if err := assertServiceAccountAgreement(blocks); err != nil { + return err + } + + // First non-empty wins, so earlier subcharts take precedence. + sa := &dst.Spec.Wandb.ServiceAccount + for _, block := range blocks { + if block.create != nil && sa.Create == nil { + sa.Create = ptr.To(*block.create) + } + if block.name != "" && sa.ServiceAccountName == "" { + sa.ServiceAccountName = block.name + } + if len(block.annotations) > 0 && len(sa.Annotations) == 0 { + sa.Annotations = block.annotations } } - if len(anns) > 0 { - dst.Spec.Wandb.ServiceAccount.Annotations = anns + return nil +} + +// assertServiceAccountAgreement fails when subcharts disagree on the identity. +// v2 has one application ServiceAccount, so silently picking a winner would +// point pods at the wrong cloud identity — the exact failure this mapper +// exists to prevent. +func assertServiceAccountAgreement(blocks []v1ServiceAccount) error { + var nameOwner, createOwner *v1ServiceAccount + for i := range blocks { + block := &blocks[i] + if block.name != "" { + if nameOwner != nil && nameOwner.name != block.name { + return fmt.Errorf( + "spec.values: %s.serviceAccount.name=%q conflicts with %s.serviceAccount.name=%q; "+ + "v2 has a single spec.wandb.serviceAccount, so set it explicitly", + nameOwner.subchart, nameOwner.name, block.subchart, block.name) + } + if nameOwner == nil { + nameOwner = block + } + } + if block.create != nil { + if createOwner != nil && *createOwner.create != *block.create { + return fmt.Errorf( + "spec.values: %s.serviceAccount.create=%v conflicts with %s.serviceAccount.create=%v; "+ + "v2 has a single spec.wandb.serviceAccount, so set it explicitly", + createOwner.subchart, *createOwner.create, block.subchart, *block.create) + } + if createOwner == nil { + createOwner = block + } + } } return nil } +// readV1ServiceAccounts collects the serviceAccount block from each known +// subchart that sets one, preserving v1ServiceAccountSubcharts order. +func readV1ServiceAccounts(values map[string]interface{}) ([]v1ServiceAccount, error) { + var out []v1ServiceAccount + for _, subchart := range v1ServiceAccountSubcharts { + saMap, found, err := unstructured.NestedMap(values, subchart, "serviceAccount") + if err != nil { + return nil, fmt.Errorf("spec.values.%s.serviceAccount: %w", subchart, err) + } + if !found || len(saMap) == 0 { + continue + } + + block := v1ServiceAccount{subchart: subchart} + + // Non-boolean create is treated as unset rather than failing, so a + // stringly-typed helm value can't make a v1 object unservable. + if raw, ok := saMap["create"]; ok { + if s, isScalar := scalarToString(raw); isScalar { + if parsed, parseErr := strconv.ParseBool(s); parseErr == nil { + block.create = ptr.To(parsed) + } + } + } + + if block.name, _, err = unstructured.NestedString(saMap, "name"); err != nil { + return nil, fmt.Errorf("spec.values.%s.serviceAccount.name: %w", subchart, err) + } + if block.annotations, _, err = unstructured.NestedStringMap(saMap, "annotations"); err != nil { + return nil, fmt.Errorf("spec.values.%s.serviceAccount.annotations: %w", subchart, err) + } + + out = append(out, block) + } + return out, nil +} + // mapInternalJWTIssuer pulls the first entry from global.internalJWTMap (or // app as fallback) into spec.wandb.internalServiceAuth.oidcIssuer. func mapInternalJWTIssuer(values map[string]interface{}, dst *appsv2.WeightsAndBiases) error { @@ -322,18 +424,6 @@ func readFirstInternalJWTIssuer(values map[string]interface{}, service string) ( return issuer, nil } -// readServiceAccountAnnotations reads values..serviceAccount.annotations. -func readServiceAccountAnnotations(values map[string]interface{}, service string) (map[string]string, error) { - anns, found, err := unstructured.NestedStringMap(values, service, "serviceAccount", "annotations") - if err != nil { - return nil, fmt.Errorf("spec.values.%s.serviceAccount.annotations: %w", service, err) - } - if !found { - return nil, nil - } - return anns, nil -} - func mapHostnameLicense(globalMap map[string]interface{}, dst *appsv2.WeightsAndBiases) error { host, _, err := unstructured.NestedString(globalMap, "host") if err != nil { diff --git a/api/v1/weightsandbiases_conversion_test.go b/api/v1/weightsandbiases_conversion_test.go index 014b614c..cf79aa04 100644 --- a/api/v1/weightsandbiases_conversion_test.go +++ b/api/v1/weightsandbiases_conversion_test.go @@ -309,6 +309,185 @@ func TestConvertTo_ServiceAccountAnnotationsEmptyAppFallsBackToApi(t *testing.T) "empty app annotations should not consume the slot; api fallback applies") } +// TestConvertTo_ServiceAccountCreateFalseAndName: v2 defaults create=true and +// serviceAccountName=wandb, so both must be carried or the operator stands up +// its own ServiceAccount and orphans the v1 cloud identity. +func TestConvertTo_ServiceAccountCreateFalseAndName(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{ + "create": false, + "name": "my-existing-sa", + "annotations": map[string]interface{}{ + "eks.amazonaws.com/role-arn": "arn:aws:iam::123456789012:role/wandb", + }, + }, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + sa := dst.Spec.Wandb.ServiceAccount + require.NotNil(t, sa.Create, "create must be set explicitly; nil defaults to true") + require.False(t, *sa.Create) + require.Equal(t, "my-existing-sa", sa.ServiceAccountName) + require.Equal(t, "arn:aws:iam::123456789012:role/wandb", + sa.Annotations["eks.amazonaws.com/role-arn"]) +} + +func TestConvertTo_ServiceAccountCreateTrueWithName(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{ + "create": true, + "name": "wandb-custom", + }, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.NotNil(t, dst.Spec.Wandb.ServiceAccount.Create) + require.True(t, *dst.Spec.Wandb.ServiceAccount.Create) + require.Equal(t, "wandb-custom", dst.Spec.Wandb.ServiceAccount.ServiceAccountName) +} + +// TestConvertTo_ServiceAccountFallsBackToApi: api supplies the identity when +// app has no serviceAccount block at all. +func TestConvertTo_ServiceAccountFallsBackToApi(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "api": map[string]interface{}{ + "serviceAccount": map[string]interface{}{ + "create": false, + "name": "api-sa", + }, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.False(t, *dst.Spec.Wandb.ServiceAccount.Create) + require.Equal(t, "api-sa", dst.Spec.Wandb.ServiceAccount.ServiceAccountName) +} + +// TestConvertTo_ServiceAccountAgreeingSubchartsOK: identical blocks are not a +// conflict. +func TestConvertTo_ServiceAccountAgreeingSubchartsOK(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"create": false, "name": "shared-sa"}, + }, + "api": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"create": false, "name": "shared-sa"}, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.False(t, *dst.Spec.Wandb.ServiceAccount.Create) + require.Equal(t, "shared-sa", dst.Spec.Wandb.ServiceAccount.ServiceAccountName) +} + +// TestConvertTo_ServiceAccountConflictingNamesFails: v2 has one application +// ServiceAccount, so two names are unrepresentable. +func TestConvertTo_ServiceAccountConflictingNamesFails(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"name": "app-sa"}, + }, + "api": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"name": "api-sa"}, + }, + }) + err := src.ConvertTo(dst) + require.Error(t, err) + require.Contains(t, err.Error(), "app-sa") + require.Contains(t, err.Error(), "api-sa") + require.Contains(t, err.Error(), "single spec.wandb.serviceAccount") +} + +func TestConvertTo_ServiceAccountConflictingCreateFails(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"create": false, "name": "shared-sa"}, + }, + "api": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"create": true, "name": "shared-sa"}, + }, + }) + err := src.ConvertTo(dst) + require.Error(t, err) + require.Contains(t, err.Error(), "serviceAccount.create") +} + +// TestConvertTo_ServiceAccountInfraSubchartsIgnored: infra serviceAccount +// blocks configure those workloads, not the application identity. +func TestConvertTo_ServiceAccountInfraSubchartsIgnored(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "mysql": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"create": false, "name": "mysql-sa"}, + }, + "redis": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"create": false, "name": "redis-sa"}, + }, + }) + require.NoError(t, src.ConvertTo(dst), "infra subcharts must not trigger a conflict") + + require.Nil(t, dst.Spec.Wandb.ServiceAccount.Create) + require.Empty(t, dst.Spec.Wandb.ServiceAccount.ServiceAccountName) +} + +// TestConvertTo_ServiceAccountCreateStringBool: helm values are frequently +// stringly-typed. +func TestConvertTo_ServiceAccountCreateStringBool(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"create": "false", "name": "my-sa"}, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.NotNil(t, dst.Spec.Wandb.ServiceAccount.Create) + require.False(t, *dst.Spec.Wandb.ServiceAccount.Create) +} + +// TestConvertTo_ServiceAccountCreateNonBoolIsUnset: an uninterpretable flag +// must not make a v1 object unservable. +func TestConvertTo_ServiceAccountCreateNonBoolIsUnset(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{ + "create": map[string]interface{}{"nested": "nonsense"}, + "name": "my-sa", + }, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.Nil(t, dst.Spec.Wandb.ServiceAccount.Create, "unparseable create is unset") + require.Equal(t, "my-sa", dst.Spec.Wandb.ServiceAccount.ServiceAccountName) +} + +// TestConvertTo_ServiceAccountNameOnlyNoCreate: name without create leaves +// create unset so the v2 default (true) applies — v1's own chart default. +func TestConvertTo_ServiceAccountNameOnlyNoCreate(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "app": map[string]interface{}{ + "serviceAccount": map[string]interface{}{"name": "named-sa"}, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.Nil(t, dst.Spec.Wandb.ServiceAccount.Create) + require.Equal(t, "named-sa", dst.Spec.Wandb.ServiceAccount.ServiceAccountName) +} + func TestConvertTo_ServiceAccountAnnotationsAbsent(t *testing.T) { dst := &appsv2.WeightsAndBiases{} src := newV1(map[string]interface{}{ diff --git a/internal/controller/reconciler/internal_service_auth.go b/internal/controller/reconciler/internal_service_auth.go new file mode 100644 index 00000000..a1a38700 --- /dev/null +++ b/internal/controller/reconciler/internal_service_auth.go @@ -0,0 +1,37 @@ +package reconciler + +import ( + v2 "github.com/wandb/operator/api/v2" + "github.com/wandb/operator/pkg/utils" +) + +// serviceAccountIssuerUnknownReason marks a CR that can't be reconciled because +// the cluster's service-account issuer is neither configured nor discoverable. +const serviceAccountIssuerUnknownReason = "ServiceAccountIssuerUnknown" + +const serviceAccountIssuerUnknownMessage = "could not determine the cluster service-account issuer: " + + "set spec.wandb.internalServiceAuth.oidcIssuer to the value of " + + "`kubectl get --raw /.well-known/openid-configuration`, or grant the operator get on that URL " + + "and restart it" + +// internalServiceAuthEnabled reports whether W&B services validate each other's +// projected ServiceAccount tokens. +func internalServiceAuthEnabled(wandb *v2.WeightsAndBiases) bool { + return wandb.Spec.Wandb.InternalServiceAuth.Enabled != nil && + *wandb.Spec.Wandb.InternalServiceAuth.Enabled +} + +// resolveInternalServiceAuthIssuer returns the issuer W&B services must validate +// projected ServiceAccount tokens against: the explicit CR value when set, else +// the issuer discovered from the cluster at start-up. +// +// Empty means unknown, and callers must not substitute a guess. The API server +// stamps its own --service-account-issuer as the token's `iss`, so any other +// value fails validation — which surfaces as a 401 and panics the API rather +// than degrading. +func resolveInternalServiceAuthIssuer(wandb *v2.WeightsAndBiases) string { + if wandb.Spec.Wandb.InternalServiceAuth.OIDCIssuer != "" { + return wandb.Spec.Wandb.InternalServiceAuth.OIDCIssuer + } + return utils.ServiceAccountIssuer() +} diff --git a/internal/controller/reconciler/internal_service_auth_test.go b/internal/controller/reconciler/internal_service_auth_test.go new file mode 100644 index 00000000..0ce0f6d7 --- /dev/null +++ b/internal/controller/reconciler/internal_service_auth_test.go @@ -0,0 +1,66 @@ +package reconciler + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/utils/ptr" + + apiv2 "github.com/wandb/operator/api/v2" + "github.com/wandb/operator/pkg/utils" +) + +func issuerWandb(enabled *bool, crIssuer string) *apiv2.WeightsAndBiases { + return &apiv2.WeightsAndBiases{ + Spec: apiv2.WeightsAndBiasesSpec{ + Wandb: apiv2.WandbAppSpec{ + InternalServiceAuth: apiv2.InternalServiceAuth{ + Enabled: enabled, + OIDCIssuer: crIssuer, + }, + }, + }, + } +} + +var _ = Describe("resolveInternalServiceAuthIssuer", func() { + AfterEach(func() { + utils.SetServiceAccountIssuer("") + }) + + It("returns the explicit CR issuer over the discovered one", func() { + utils.SetServiceAccountIssuer("https://oidc.eks.us-east-1.amazonaws.com/id/DISCOVERED") + wandb := issuerWandb(ptr.To(true), "https://issuer.example.com") + + Expect(resolveInternalServiceAuthIssuer(wandb)).To(Equal("https://issuer.example.com")) + }) + + It("falls back to the discovered cluster issuer", func() { + utils.SetServiceAccountIssuer("https://oidc.eks.us-east-1.amazonaws.com/id/ABC123") + wandb := issuerWandb(ptr.To(true), "") + + Expect(resolveInternalServiceAuthIssuer(wandb)). + To(Equal("https://oidc.eks.us-east-1.amazonaws.com/id/ABC123")) + }) + + // The pre-fix behavior substituted kubernetes.default.svc.cluster.local here, + // which 401s on any cluster that isn't kubeadm-defaulted. + It("returns empty when neither is set rather than guessing", func() { + wandb := issuerWandb(ptr.To(true), "") + + Expect(resolveInternalServiceAuthIssuer(wandb)).To(BeEmpty()) + }) +}) + +var _ = Describe("internalServiceAuthEnabled", func() { + It("is false when unset", func() { + Expect(internalServiceAuthEnabled(issuerWandb(nil, ""))).To(BeFalse()) + }) + + It("is false when explicitly disabled", func() { + Expect(internalServiceAuthEnabled(issuerWandb(ptr.To(false), ""))).To(BeFalse()) + }) + + It("is true when explicitly enabled", func() { + Expect(internalServiceAuthEnabled(issuerWandb(ptr.To(true), ""))).To(BeTrue()) + }) +}) diff --git a/pkg/utils/serviceaccount_issuer.go b/pkg/utils/serviceaccount_issuer.go new file mode 100644 index 00000000..20ebff68 --- /dev/null +++ b/pkg/utils/serviceaccount_issuer.go @@ -0,0 +1,28 @@ +package utils + +import "sync/atomic" + +// serviceAccountIssuer caches the cluster's OIDC issuer for projected +// ServiceAccount tokens, discovered once at manager start-up. +// +// This is the exact string the API server stamps as `iss` on those tokens, so +// W&B services must be told this value verbatim or internal token validation +// fails. It cannot be hardcoded: only kubeadm defaults to +// https://kubernetes.default.svc.cluster.local, while EKS, GKE and AKS each +// mint their own issuer URL. +var serviceAccountIssuer atomic.Pointer[string] + +// SetServiceAccountIssuer records the cluster's discovered service-account +// issuer. Also the test seam for the discovered value. +func SetServiceAccountIssuer(issuer string) { + serviceAccountIssuer.Store(&issuer) +} + +// ServiceAccountIssuer returns the discovered service-account issuer, or "" when +// discovery has not run or did not succeed. +func ServiceAccountIssuer() string { + if issuer := serviceAccountIssuer.Load(); issuer != nil { + return *issuer + } + return "" +}