chores: unify extra config behaviour, mount keeper extra config as a separate file (#156)

Author: GrigoryPervakov

go.mod

@@ -3,7 +3,6 @@ module github.com/ClickHouse/clickhouse-operator
 go 1.26.0
 
 require (
-	dario.cat/mergo v1.0.2
 	github.com/ClickHouse/clickhouse-go/v2 v2.43.0
 	github.com/blang/semver/v4 v4.0.0
 	github.com/cert-manager/cert-manager v1.20.1
@@ -30,6 +29,7 @@ require (
 
 require (
 	cel.dev/expr v0.25.1 // indirect
+	dario.cat/mergo v1.0.2 // indirect
 	github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c // indirect
 	github.com/ClickHouse/ch-go v0.71.0 // indirect
 	github.com/Masterminds/semver/v3 v3.4.0 // indirect

internal/controller/keeper/constants.go

@@ -16,8 +16,9 @@ const (
 	QuorumConfigPath     = "/etc/clickhouse-keeper/"
 	QuorumConfigFileName = "config.yaml"
 
-	ConfigPath     = QuorumConfigPath + "config.d/"
-	ConfigFileName = "00-config.yaml"
+	ConfigPath          = QuorumConfigPath + "config.d/"
+	ConfigFileName      = "00-config.yaml"
+	ExtraConfigFileName = "99-extra-config.yaml"
 
 	TLSConfigPath       = "/etc/clickhouse-keeper/tls/"
 	CABundleFilename    = "ca-bundle.crt"

internal/controller/keeper/controller.go

@@ -101,7 +101,6 @@ func (cc *ClusterController) Reconcile(ctx context.Context, req ctrl.Request) (c
 			v1.KeeperReplicaID,
 			replicaState,
 		](cc, cluster),
-		ExtraConfig: map[string]any{},
 	}
 
 	return reconciler.sync(ctx, logger)

internal/controller/keeper/controller_test.go

@@ -8,7 +8,6 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	"gopkg.in/yaml.v2"
 	appsv1 "k8s.io/api/apps/v1"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -234,11 +233,8 @@ var _ = When("reconciling standalone KeeperCluster resource", Ordered, func() {
 		}, &configmap)).To(Succeed())
 
 		Expect(configmap.Data).To(HaveKey(ConfigFileName))
-
-		var config confMap
-		Expect(yaml.Unmarshal([]byte(configmap.Data[ConfigFileName]), &config)).To(Succeed())
-		//nolint:forcetypeassert
-		Expect(config["keeper_server"].(confMap)["coordination_settings"].(confMap)["quorum_reads"]).To(BeTrue())
+		Expect(configmap.Data).To(HaveKey(ExtraConfigFileName))
+		Expect(configmap.Data[ExtraConfigFileName]).To(ContainSubstring("quorum_reads"))
 	})
 
 	It("should use security context overrides from spec", func(ctx context.Context) {

internal/controller/keeper/sync.go

@@ -2,7 +2,6 @@ package keeper
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"maps"
 	"math"
@@ -109,8 +108,6 @@ type keeperReconciler struct {
 	reconcilerBase
 
 	versionProbe chctrl.VersionProbeResult
-	// Should be populated after reconcileClusterRevisions with parsed extra config.
-	ExtraConfig map[string]any
 	// Computed by reconcileActiveReplicaStatus
 	HorizontalScaleAllowed bool
 	pvcRevision            string
@@ -207,16 +204,7 @@ func (r *keeperReconciler) reconcileClusterRevisions(ctx context.Context, log ct
 		log.Debug(fmt.Sprintf("observed new CR revision %q", updateRevision))
 	}
 
-	var extraConfig map[string]any
-	if len(r.Cluster.Spec.Settings.ExtraConfig.Raw) > 0 {
-		if err := json.Unmarshal(r.Cluster.Spec.Settings.ExtraConfig.Raw, &extraConfig); err != nil {
-			return nil, fmt.Errorf("unmarshal extra config: %w", err)
-		}
-
-		r.ExtraConfig = extraConfig
-	}
-
-	configRevision, err := getConfigurationRevision(r.Cluster, r.ExtraConfig)
+	configRevision, err := getConfigurationRevision(r.Cluster)
 	if err != nil {
 		return nil, fmt.Errorf("get configuration revision: %w", err)
 	}
@@ -761,7 +749,7 @@ func (r *keeperReconciler) updateReplica(ctx context.Context, log ctrlutil.Logge
 	log = log.With("replica_id", replicaID)
 	log.Info("updating replica")
 
-	configMap, err := templateConfigMap(r.Cluster, r.ExtraConfig, replicaID)
+	configMap, err := templateConfigMap(r.Cluster, replicaID)
 	if err != nil {
 		return nil, fmt.Errorf("template replica %q ConfigMap: %w", replicaID, err)
 	}

internal/controller/keeper/sync_test.go

@@ -210,7 +210,6 @@ func setupReconciler() (util.Logger, *keeperReconciler, context.CancelFunc) {
 					StatefulSetRevision:   "sts-v1",
 				},
 			}),
-		ExtraConfig: map[string]any{},
 	}
 
 	eventContext, cancel := context.WithCancel(context.Background())

internal/controller/keeper/templates.go

@@ -7,7 +7,6 @@ import (
 	"strconv"
 	"strings"
 
-	"dario.cat/mergo"
 	"gopkg.in/yaml.v2"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -204,13 +203,13 @@ type keeperServer struct {
 	HTTPControl          httpControl    `yaml:"http_control"`
 }
 
-func getConfigurationRevision(cr *v1.KeeperCluster, extraConfig map[string]any) (string, error) {
-	config, err := generateConfigForSingleReplica(cr, extraConfig, 0)
+func getConfigurationRevision(cr *v1.KeeperCluster) (string, error) {
+	cfg, err := generateConfigForSingleReplica(cr, 0)
 	if err != nil {
 		return "", fmt.Errorf("generate template configuration: %w", err)
 	}
 
-	hash, err := controllerutil.DeepHashObject(config)
+	hash, err := controllerutil.DeepHashObject(cfg)
 	if err != nil {
 		return "", fmt.Errorf("hash template configuration: %w", err)
 	}
@@ -234,8 +233,8 @@ func getStatefulSetRevision(cr *v1.KeeperCluster) (string, error) {
 	return hash, nil
 }
 
-func templateConfigMap(cr *v1.KeeperCluster, extraConfig map[string]any, id v1.KeeperReplicaID) (*corev1.ConfigMap, error) {
-	config, err := generateConfigForSingleReplica(cr, extraConfig, id)
+func templateConfigMap(cr *v1.KeeperCluster, id v1.KeeperReplicaID) (*corev1.ConfigMap, error) {
+	cfg, err := generateConfigForSingleReplica(cr, id)
 	if err != nil {
 		return nil, fmt.Errorf("generate configmap for replica %q: %w", id, err)
 	}
@@ -251,9 +250,7 @@ func templateConfigMap(cr *v1.KeeperCluster, extraConfig map[string]any, id v1.K
 			Labels:      controllerutil.MergeMaps(cr.Spec.Labels, replicaLabels(cr, id)),
 			Annotations: cr.Spec.Annotations,
 		},
-		Data: map[string]string{
-			ConfigFileName: config,
-		},
+		Data: cfg,
 	}, nil
 }
 
@@ -327,7 +324,7 @@ func replicaLabels(cr *v1.KeeperCluster, id v1.KeeperReplicaID) map[string]strin
 	return labels
 }
 
-func generateConfigForSingleReplica(cr *v1.KeeperCluster, extraConfig map[string]any, id v1.KeeperReplicaID) (string, error) {
+func generateConfigForSingleReplica(cr *v1.KeeperCluster, id v1.KeeperReplicaID) (map[string]string, error) {
 	config := config{
 		ListenHost: "0.0.0.0",
 		Path:       internal.KeeperDataPath,
@@ -370,26 +367,18 @@ func generateConfigForSingleReplica(cr *v1.KeeperCluster, extraConfig map[string
 
 	yamlConfig, err := yaml.Marshal(config)
 	if err != nil {
-		return "", fmt.Errorf("error marshalling config to yaml: %w", err)
+		return nil, fmt.Errorf("error marshalling config to yaml: %w", err)
 	}
 
-	if len(extraConfig) > 0 {
-		configMap := map[string]any{}
-		if err := yaml.Unmarshal(yamlConfig, &configMap); err != nil {
-			return "", fmt.Errorf("error unmarshalling config from yaml: %w", err)
-		}
-
-		if err := mergo.Merge(&configMap, extraConfig, mergo.WithOverride); err != nil {
-			return "", fmt.Errorf("error merging config with extraConfig: %w", err)
-		}
+	data := map[string]string{
+		ConfigFileName: string(yamlConfig),
+	}
 
-		yamlConfig, err = yaml.Marshal(configMap)
-		if err != nil {
-			return "", fmt.Errorf("error marshalling merged config to yaml: %w", err)
-		}
+	if len(cr.Spec.Settings.ExtraConfig.Raw) > 0 {
+		data[ExtraConfigFileName] = string(cr.Spec.Settings.ExtraConfig.Raw)
 	}
 
-	return string(yamlConfig), nil
+	return data, nil
 }
 
 func templatePodSpec(cr *v1.KeeperCluster, id v1.KeeperReplicaID) (corev1.PodSpec, error) {

internal/controller/keeper/templates_test.go

@@ -6,11 +6,11 @@ import (
 	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	"gopkg.in/yaml.v2"
 	corev1 "k8s.io/api/core/v1"
 	policyv1 "k8s.io/api/policy/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/randfill"
 
@@ -19,8 +19,6 @@ import (
 	"github.com/ClickHouse/clickhouse-operator/internal/controllerutil"
 )
 
-type confMap map[any]any
-
 var _ = Describe("ServerRevision", func() {
 	var (
 		baseCR          *v1.KeeperCluster
@@ -40,7 +38,7 @@ var _ = Describe("ServerRevision", func() {
 			},
 		}
 
-		baseCfgRevision, err = getConfigurationRevision(baseCR, nil)
+		baseCfgRevision, err = getConfigurationRevision(baseCR)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(baseCfgRevision).ToNot(BeEmpty())
 
@@ -52,7 +50,7 @@ var _ = Describe("ServerRevision", func() {
 	It("should not change config revision if only replica count changes", func() {
 		cr := baseCR.DeepCopy()
 		cr.Spec.Replicas = new(int32(3))
-		cfgRevisionUpdated, err := getConfigurationRevision(cr, nil)
+		cfgRevisionUpdated, err := getConfigurationRevision(cr)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(baseCfgRevision).ToNot(BeEmpty())
 		Expect(cfgRevisionUpdated).To(Equal(baseCfgRevision), "server config revision shouldn't depend on replica count")
@@ -66,7 +64,7 @@ var _ = Describe("ServerRevision", func() {
 	It("should not change sts revision if only config changes", func() {
 		cr := baseCR.DeepCopy()
 		cr.Spec.Settings.Logger.Level = "warning"
-		cfgRevisionUpdated, err := getConfigurationRevision(cr, nil)
+		cfgRevisionUpdated, err := getConfigurationRevision(cr)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(cfgRevisionUpdated).ToNot(BeEmpty())
 		Expect(cfgRevisionUpdated).ToNot(Equal(baseCfgRevision), "configuration change should update config revision")
@@ -79,64 +77,32 @@ var _ = Describe("ServerRevision", func() {
 })
 
 var _ = Describe("ExtraConfig", func() {
-	var (
-		cr             *v1.KeeperCluster
-		baseConfigYAML string
-		baseConfig     confMap
-	)
-
-	BeforeEach(func() {
-		cr = &v1.KeeperCluster{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "test",
-			},
+	It("should add extra config as a separate ConfigMap key", func() {
+		cr := &v1.KeeperCluster{
+			ObjectMeta: metav1.ObjectMeta{Name: "test"},
 			Spec: v1.KeeperClusterSpec{
 				Replicas: new(int32(1)),
-			},
-		}
-
-		var err error
-
-		baseConfigYAML, err = generateConfigForSingleReplica(cr, nil, 1)
-		Expect(err).NotTo(HaveOccurred())
-		Expect(yaml.Unmarshal([]byte(baseConfigYAML), &baseConfig)).To(Succeed())
-	})
-
-	It("should reflect config changes in generated config", func() {
-		configYAML, err := generateConfigForSingleReplica(cr, map[string]any{
-			"keeper_server": confMap{
-				"coordination_settings": confMap{
-					"quorum_reads": true,
+				Settings: v1.KeeperSettings{
+					ExtraConfig: runtime.RawExtension{Raw: []byte(`{"keeper_server": {"coordination_settings": {"quorum_reads": true}}}`)},
 				},
 			},
-		}, 1)
+		}
+		data, err := generateConfigForSingleReplica(cr, 1)
 		Expect(err).NotTo(HaveOccurred())
-
-		var config confMap
-		Expect(yaml.Unmarshal([]byte(configYAML), &config)).To(Succeed())
-		Expect(config).ToNot(Equal(baseConfig), cmp.Diff(config, baseConfig))
-		//nolint:forcetypeassert
-		Expect(config["keeper_server"].(confMap)["coordination_settings"].(confMap)["quorum_reads"]).To(BeTrue())
+		Expect(data).To(HaveKey(ConfigFileName))
+		Expect(data).To(HaveKey(ExtraConfigFileName))
+		Expect(data[ExtraConfigFileName]).To(ContainSubstring("quorum_reads"))
 	})
 
-	It("should override existing setting by extra config", func() {
-		configYAML, err := generateConfigForSingleReplica(cr, map[string]any{
-			"keeper_server": confMap{
-				"coordination_settings": confMap{
-					"compress_logs": true,
-				},
-			},
-		}, 1)
-		Expect(err).NotTo(HaveOccurred())
-
-		var config confMap
-
-		err = yaml.Unmarshal([]byte(configYAML), &config)
+	It("should not include extra config key when empty", func() {
+		cr := &v1.KeeperCluster{
+			ObjectMeta: metav1.ObjectMeta{Name: "test"},
+			Spec:       v1.KeeperClusterSpec{Replicas: new(int32(1))},
+		}
+		data, err := generateConfigForSingleReplica(cr, 1)
 		Expect(err).NotTo(HaveOccurred())
-
-		Expect(config).ToNot(Equal(baseConfig), cmp.Diff(config, baseConfig))
-		//nolint:forcetypeassert
-		Expect(config["keeper_server"].(confMap)["coordination_settings"].(confMap)["compress_logs"]).To(BeTrue())
+		Expect(data).To(HaveKey(ConfigFileName))
+		Expect(data).ToNot(HaveKey(ExtraConfigFileName))
 	})
 })
 

test/e2e/clickhouse_e2e_test.go

@@ -864,7 +864,15 @@ func WaitClickHouseUpdatedAndReady(
 		g.Expect(cluster.Status.CurrentRevision).To(Equal(cluster.Status.UpdateRevision))
 		g.Expect(cluster.Status.ReadyReplicas).To(Equal(cluster.Replicas() * cluster.Shards()))
 
-		for _, cond := range cluster.Status.Conditions {
+		for _, conditionType := range []v1.ConditionType{
+			v1.ConditionTypeReady,
+			v1.ConditionTypeHealthy,
+			v1.ConditionTypeClusterSizeAligned,
+			v1.ConditionTypeConfigurationInSync,
+			v1.ClickHouseConditionTypeSchemaInSync,
+		} {
+			cond := meta.FindStatusCondition(cluster.Status.Conditions, string(conditionType))
+			g.Expect(cond).ToNot(BeNil())
 			g.Expect(cond.Status).To(
 				Equal(metav1.ConditionTrue),
 				fmt.Sprintf("condition %s is false: %s", cond.Type, cond.Message),

test/e2e/keeper_e2e_test.go

@@ -314,7 +314,14 @@ func WaitKeeperUpdatedAndReady(ctx context.Context, cr *v1.KeeperCluster, timeou
 		g.Expect(cluster.Status.CurrentRevision).To(Equal(cluster.Status.UpdateRevision))
 		g.Expect(cluster.Status.ReadyReplicas).To(Equal(cluster.Replicas()))
 
-		for _, cond := range cluster.Status.Conditions {
+		for _, conditionType := range []v1.ConditionType{
+			v1.ConditionTypeReady,
+			v1.ConditionTypeHealthy,
+			v1.ConditionTypeClusterSizeAligned,
+			v1.ConditionTypeConfigurationInSync,
+		} {
+			cond := meta.FindStatusCondition(cluster.Status.Conditions, string(conditionType))
+			g.Expect(cond).ToNot(BeNil())
 			g.Expect(cond.Status).To(
 				Equal(metav1.ConditionTrue),
 				fmt.Sprintf("condition %s is false: %s", cond.Type, cond.Message),