fix/opv2-fix-dropped-SA

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.<service>.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 {

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{}{