diff --git a/api/v1/weightsandbiases_conversion_mapping.go b/api/v1/weightsandbiases_conversion_mapping.go index d1c94b76..3ad1317b 100644 --- a/api/v1/weightsandbiases_conversion_mapping.go +++ b/api/v1/weightsandbiases_conversion_mapping.go @@ -89,6 +89,10 @@ func applyValueMappings(src *WeightsAndBiases, dst *appsv2.WeightsAndBiases) err if err := mapLegacyOverrides(values, dst); err != nil { return err } + // Peer-of-global: reads both the Altinity subchart section and global.clickhouse. + if err := mapClickHouse(values, dst); err != nil { + return err + } globalMap, found, err := unstructured.NestedMap(values, "global") if err != nil { @@ -126,9 +130,6 @@ func applyGlobalMappings(globalMap map[string]interface{}, dst *appsv2.WeightsAn if err := mapBucket(globalMap, dst); err != nil { return err } - if err := mapClickHouse(globalMap, dst); err != nil { - return err - } return nil } @@ -535,66 +536,115 @@ var clickHouseFields = []struct { {"password", func(c *appsv2.ClickHouseConnection, s corev1.SecretKeySelector) { c.Password = s }}, } -// mapClickHouse routes v1 global.clickhouse to externalClickhouse (like -// mapMySQL); external is asserted only when a connection field is present. -func mapClickHouse(globalMap map[string]interface{}, dst *appsv2.WeightsAndBiases) error { - chMap, found, err := unstructured.NestedMap(globalMap, "clickhouse") +// clickHouseInstallPaths are the v1 locations of the ClickHouse install flag, +// in precedence order: the Altinity subchart section wins over global. +var clickHouseInstallPaths = [][]string{ + {"clickhouse", "install"}, + {"global", "clickhouse", "install"}, +} + +// clickHouseSource pairs a v1 values section with its path, so errors name the +// section the offending key actually came from. +type clickHouseSource struct { + path string + m map[string]interface{} +} + +// mapClickHouse routes v1 ClickHouse config to externalClickhouse. Connection +// fields come from global.clickhouse, falling back to the top-level clickhouse +// section (the Altinity subchart values) for installs that configured them +// there. +func mapClickHouse(values map[string]interface{}, dst *appsv2.WeightsAndBiases) error { + globalCH, _, err := unstructured.NestedMap(values, "global", "clickhouse") if err != nil { return fmt.Errorf("spec.values.global.clickhouse: %w", err) } - if !found || len(chMap) == 0 { + topCH, _, err := unstructured.NestedMap(values, "clickhouse") + if err != nil { + return fmt.Errorf("spec.values.clickhouse: %w", err) + } + if len(globalCH) == 0 && len(topCH) == 0 { + return nil + } + + install, installSet := firstClickHouseInstallFlag(values) + if installSet && install { return nil } + sources := []clickHouseSource{ + {"spec.values.global.clickhouse", globalCH}, + {"spec.values.clickhouse", topCH}, + } + conn := &appsv2.ClickHouseConnection{} remaining := map[string]string{} sawField := false for _, f := range clickHouseFields { - raw, ok := chMap[f.v1Key] + for _, src := range sources { + raw, ok := src.m[f.v1Key] + if !ok { + continue + } + ref, literal, classifyErr := classifyValueFromOrLiteral(raw) + if classifyErr != nil { + return fmt.Errorf("%s.%s: %w", src.path, f.v1Key, classifyErr) + } + if ref != nil { + f.setRef(conn, *ref) + sawField = true + break + } + // An empty value is no value: let the next source supply it. + if literal != "" { + remaining[f.v1Key] = literal + sawField = true + break + } + } + } + + for _, src := range sources { + ps, ok, err := unstructured.NestedMap(src.m, "passwordSecret") + if err != nil { + return fmt.Errorf("%s.passwordSecret: %w", src.path, err) + } if !ok { continue } - ref, literal, classifyErr := classifyValueFromOrLiteral(raw) - if classifyErr != nil { - return fmt.Errorf("spec.values.global.clickhouse.%s: %w", f.v1Key, classifyErr) + name, _, err := unstructured.NestedString(ps, "name") + if err != nil { + return fmt.Errorf("%s.passwordSecret.name: %w", src.path, err) } - switch { - case ref != nil: - f.setRef(conn, *ref) - sawField = true - case literal != "": - remaining[f.v1Key] = literal - sawField = true + if name == "" || conn.Password.Name != "" { + continue } - } - - if ps, ok, err := unstructured.NestedMap(chMap, "passwordSecret"); err != nil { - return fmt.Errorf("spec.values.global.clickhouse.passwordSecret: %w", err) - } else if ok { - name, _, err := unstructured.NestedString(ps, "name") + key, _, err := unstructured.NestedString(ps, "passwordKey") if err != nil { - return fmt.Errorf("spec.values.global.clickhouse.passwordSecret.name: %w", err) + return fmt.Errorf("%s.passwordSecret.passwordKey: %w", src.path, err) } - alreadyHasPassword := conn.Password.Name != "" - if name != "" && !alreadyHasPassword { - key, _, err := unstructured.NestedString(ps, "passwordKey") - if err != nil { - return fmt.Errorf("spec.values.global.clickhouse.passwordSecret.passwordKey: %w", err) - } - if key == "" { - key = defaultClickHousePasswordSecretKey - } - conn.Password = corev1.SecretKeySelector{ - LocalObjectReference: corev1.LocalObjectReference{Name: name}, - Key: key, - } - delete(remaining, "password") - sawField = true + if key == "" { + key = defaultClickHousePasswordSecretKey + } + conn.Password = corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: name}, + Key: key, } + delete(remaining, "password") + sawField = true + break } if !sawField { + if installSet && !install { + return fmt.Errorf( + "spec.values: ClickHouse install=false but no connection found under "+ + "global.clickhouse or clickhouse (%s, passwordSecret); set "+ + "spec.clickhouse.%s.externalClickhouse explicitly", + clickHouseFieldNames(), appsv2.DefaultInstanceName, + ) + } return nil } @@ -608,6 +658,35 @@ func mapClickHouse(globalMap map[string]interface{}, dst *appsv2.WeightsAndBiase return nil } +// firstClickHouseInstallFlag returns the install flag from the first v1 +// location that sets it. Non-boolean values are treated as unset rather than failing +func firstClickHouseInstallFlag(values map[string]interface{}) (install bool, found bool) { + for _, path := range clickHouseInstallPaths { + raw, ok, err := unstructured.NestedFieldNoCopy(values, path...) + if err != nil || !ok { + continue + } + s, isScalar := scalarToString(raw) + if !isScalar { + continue + } + parsed, parseErr := strconv.ParseBool(s) + if parseErr != nil { + continue + } + return parsed, true + } + return false, false +} + +func clickHouseFieldNames() string { + names := make([]string, 0, len(clickHouseFields)) + for _, f := range clickHouseFields { + names = append(names, f.v1Key) + } + return strings.Join(names, ", ") +} + // redisFields maps each v1 global.redis. to a *RedisConnection setter. var redisFields = []struct { v1Key string diff --git a/api/v1/weightsandbiases_conversion_test.go b/api/v1/weightsandbiases_conversion_test.go index 014b614c..2795501e 100644 --- a/api/v1/weightsandbiases_conversion_test.go +++ b/api/v1/weightsandbiases_conversion_test.go @@ -1986,8 +1986,8 @@ func TestConvertTo_NoClickHouseLeavesEmpty(t *testing.T) { require.NotContains(t, dst.Annotations, ClickHousePendingAnnotation) } -// TestConvertTo_ClickHouseOnlyNonConnectionKeys: keys like replicated/install -// must not be misread as an external connection. +// TestConvertTo_ClickHouseOnlyNonConnectionKeys: keys like replicated must not +// be misread as an external connection. (install is meaningful — see below.) func TestConvertTo_ClickHouseOnlyNonConnectionKeys(t *testing.T) { dst := &appsv2.WeightsAndBiases{} src := newV1(map[string]interface{}{ @@ -2003,3 +2003,135 @@ func TestConvertTo_ClickHouseOnlyNonConnectionKeys(t *testing.T) { "only non-connection keys must not assert an external clickhouse") require.NotContains(t, dst.Annotations, ClickHousePendingAnnotation) } + +// TestConvertTo_ClickHouseTopLevelSectionConnection: the Altinity subchart +// section supplies the connection when global.clickhouse doesn't. +func TestConvertTo_ClickHouseTopLevelSectionConnection(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "clickhouse": map[string]interface{}{ + "install": false, + "host": "ch.example.com", + "database": "wandb", + "password": map[string]interface{}{ + "valueFrom": map[string]interface{}{ + "secretKeyRef": map[string]interface{}{ + "name": "ch-secret", + "key": "password", + }, + }, + }, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + conn := dst.Spec.ClickHouse[appsv2.DefaultInstanceName].ExternalClickHouse + require.NotNil(t, conn, "install=false must assert an external clickhouse") + require.Equal(t, "ch-secret", conn.Password.Name) + require.Equal(t, "password", conn.Password.Key) + + var pending map[string]string + require.NoError(t, json.Unmarshal([]byte(dst.Annotations[ClickHousePendingAnnotation]), &pending)) + require.Equal(t, "ch.example.com", pending["host"]) + require.Equal(t, "wandb", pending["database"]) +} + +// TestConvertTo_ClickHouseInstallFalseGlobalConnection: install lives in the +// subchart section while the connection lives under global. +func TestConvertTo_ClickHouseInstallFalseGlobalConnection(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "clickhouse": map[string]interface{}{"install": false}, + "global": map[string]interface{}{ + "clickhouse": map[string]interface{}{"host": "ch.example.com"}, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.NotNil(t, dst.Spec.ClickHouse[appsv2.DefaultInstanceName].ExternalClickHouse) +} + +// TestConvertTo_ClickHouseGlobalWinsOverTopLevel: global.clickhouse is the +// app-facing connection, so it takes precedence per field. +func TestConvertTo_ClickHouseGlobalWinsOverTopLevel(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "clickhouse": map[string]interface{}{ + "install": false, + "host": "subchart.example.com", + "user": "subchart-user", + }, + "global": map[string]interface{}{ + "clickhouse": map[string]interface{}{"host": "global.example.com"}, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + var pending map[string]string + require.NoError(t, json.Unmarshal([]byte(dst.Annotations[ClickHousePendingAnnotation]), &pending)) + require.Equal(t, "global.example.com", pending["host"], "global.clickhouse wins") + require.Equal(t, "subchart-user", pending["user"], "subchart fills what global omits") +} + +// TestConvertTo_ClickHouseInstallTrueStaysManaged: v1 owned ClickHouse, so the +// spec is left empty for the defaulter even though a connection is present. +func TestConvertTo_ClickHouseInstallTrueStaysManaged(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "clickhouse": map[string]interface{}{"install": true}, + "global": map[string]interface{}{ + "clickhouse": map[string]interface{}{ + "host": "clickhouse.default.svc.cluster.local", + "user": "wandb", + }, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.Empty(t, dst.Spec.ClickHouse, + "install=true must not assert external; the defaulter makes it managed") + require.NotContains(t, dst.Annotations, ClickHousePendingAnnotation) +} + +// TestConvertTo_ClickHouseInstallFalseNoConnectionFails: falling through to the +// defaulter here would silently provision managed ClickHouse. +func TestConvertTo_ClickHouseInstallFalseNoConnectionFails(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "clickhouse": map[string]interface{}{ + "install": false, + "replicated": true, + }, + }) + err := src.ConvertTo(dst) + require.Error(t, err) + require.Contains(t, err.Error(), "install=false but no connection found") +} + +// TestConvertTo_ClickHouseInstallStringBool: helm values are frequently +// stringly-typed; "false" must behave like false. +func TestConvertTo_ClickHouseInstallStringBool(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "clickhouse": map[string]interface{}{ + "install": "false", + "host": "ch.example.com", + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.NotNil(t, dst.Spec.ClickHouse[appsv2.DefaultInstanceName].ExternalClickHouse) +} + +// TestConvertTo_ClickHouseInstallNonBoolIsUnset: an uninterpretable flag must +// not make a v1 object unservable; it falls back to connection presence. +func TestConvertTo_ClickHouseInstallNonBoolIsUnset(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "clickhouse": map[string]interface{}{ + "install": map[string]interface{}{"nested": "nonsense"}, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + require.Empty(t, dst.Spec.ClickHouse) +}