From cedd0f642cc64fdf57bdd501e0d48326f67341b9 Mon Sep 17 00:00:00 2001 From: Dario Tranchitella Date: Mon, 9 Mar 2026 14:14:27 +0100 Subject: [PATCH] fix(datastore): consistent password update if user exists (#1097) Signed-off-by: Dario Tranchitella --- controllers/resources.go | 9 +- ...p_postgres_datastore_config_secret_test.go | 126 ++++++++++++++++++ internal/datastore/connection.go | 1 + internal/datastore/errors/errors.go | 4 + internal/datastore/etcd.go | 4 + internal/datastore/mysql.go | 9 ++ internal/datastore/nats.go | 4 + internal/datastore/postgresql.go | 10 ++ .../resources/datastore/datastore_setup.go | 4 + 9 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 e2e/tcp_postgres_datastore_config_secret_test.go diff --git a/controllers/resources.go b/controllers/resources.go index acec173c..a269a0f8 100644 --- a/controllers/resources.go +++ b/controllers/resources.go @@ -267,25 +267,24 @@ func getKubernetesStorageResources(c client.Client, dbConnection datastore.Conne func getKubernetesAdditionalStorageResources(c client.Client, dbConnections map[string]datastore.Connection, dataStoreOverrides []builder.DataStoreOverrides, threshold time.Duration) []resources.Resource { res := make([]resources.Resource, 0, len(dataStoreOverrides)) for _, dso := range dataStoreOverrides { - datastore := dso.DataStore res = append(res, &ds.MultiTenancy{ - DataStore: datastore, + DataStore: dso.DataStore, }, &ds.Config{ Client: c, ConnString: dbConnections[dso.Resource].GetConnectionString(), - DataStore: datastore, + DataStore: dso.DataStore, IsOverride: true, }, &ds.Setup{ Client: c, Connection: dbConnections[dso.Resource], - DataStore: datastore, + DataStore: dso.DataStore, }, &ds.Certificate{ Client: c, - DataStore: datastore, + DataStore: dso.DataStore, CertExpirationThreshold: threshold, }) } diff --git a/e2e/tcp_postgres_datastore_config_secret_test.go b/e2e/tcp_postgres_datastore_config_secret_test.go new file mode 100644 index 00000000..ef6fb6ce --- /dev/null +++ b/e2e/tcp_postgres_datastore_config_secret_test.go @@ -0,0 +1,126 @@ +// Copyright 2022 Clastix Labs +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "context" + "fmt" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/util/retry" + pointer "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + kamajiv1alpha1 "github.com/clastix/kamaji/api/v1alpha1" +) + +var _ = Describe("When the datastore-config Secret is corrupted for a PostgreSQL-backed TenantControlPlane", func() { + tcp := &kamajiv1alpha1.TenantControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "postgresql-secret-regeneration", + Namespace: "default", + }, + Spec: kamajiv1alpha1.TenantControlPlaneSpec{ + DataStore: "postgresql-bronze", + ControlPlane: kamajiv1alpha1.ControlPlane{ + Deployment: kamajiv1alpha1.DeploymentSpec{ + Replicas: pointer.To(int32(1)), + }, + Service: kamajiv1alpha1.ServiceSpec{ + ServiceType: "ClusterIP", + }, + }, + Kubernetes: kamajiv1alpha1.KubernetesSpec{ + Version: "v1.23.6", + }, + }, + } + + JustBeforeEach(func() { + Expect(k8sClient.Create(context.Background(), tcp)).NotTo(HaveOccurred()) + StatusMustEqualTo(tcp, kamajiv1alpha1.VersionReady) + }) + + JustAfterEach(func() { + Expect(k8sClient.Delete(context.Background(), tcp)).Should(Succeed()) + }) + + It("Should regenerate the Secret and restart the TCP pods successfully", func() { + By("recording the UIDs of the currently running TenantControlPlane pods") + initialPodUIDs := sets.New[types.UID]() + Eventually(func() int { + podList := &corev1.PodList{} + if err := k8sClient.List(context.Background(), podList, + client.InNamespace(tcp.GetNamespace()), + client.MatchingLabels{"kamaji.clastix.io/name": tcp.GetName()}, + ); err != nil { + return 0 + } + + initialPodUIDs.Clear() + for _, pod := range podList.Items { + initialPodUIDs.Insert(pod.GetUID()) + } + + return initialPodUIDs.Len() + }, time.Minute, time.Second).Should(Not(BeZero())) + + By("retrieving the current datastore-config Secret and its checksum") + secretName := fmt.Sprintf("%s-datastore-config", tcp.GetName()) + + var secret corev1.Secret + Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: tcp.GetNamespace()}, &secret)).To(Succeed()) + + originalChecksum := secret.GetAnnotations()["kamaji.clastix.io/checksum"] + Expect(originalChecksum).NotTo(BeEmpty(), "expected datastore-config Secret to carry a checksum annotation") + + By("corrupting the DB_PASSWORD in the datastore-config Secret") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(&secret), &secret); err != nil { + return err + } + + secret.Data["DB_PASSWORD"] = []byte("corrupted-password") + + return k8sClient.Update(context.Background(), &secret) + }) + Expect(err).ToNot(HaveOccurred()) + + By("waiting for the controller to detect the corruption and regenerate the Secret with a new checksum") + Eventually(func() string { + if err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(&secret), &secret); err != nil { + return "" + } + + return secret.GetAnnotations()["kamaji.clastix.io/checksum"] + }, 5*time.Minute, time.Second).ShouldNot(Equal(originalChecksum)) + + By("waiting for at least one new TenantControlPlane pod to replace the pre-existing ones") + Eventually(func() bool { + var podList corev1.PodList + if err := k8sClient.List(context.Background(), &podList, + client.InNamespace(tcp.GetNamespace()), + client.MatchingLabels{"kamaji.clastix.io/name": tcp.GetName()}, + ); err != nil { + return false + } + for _, pod := range podList.Items { + if !initialPodUIDs.Has(pod.GetUID()) { + return true + } + } + + return false + }, 5*time.Minute, time.Second).Should(BeTrue()) + + By("verifying the TenantControlPlane is Ready after the restart with the regenerated Secret") + StatusMustEqualTo(tcp, kamajiv1alpha1.VersionReady) + }) +}) diff --git a/internal/datastore/connection.go b/internal/datastore/connection.go index 49dd7728..9e5b1eb5 100644 --- a/internal/datastore/connection.go +++ b/internal/datastore/connection.go @@ -47,6 +47,7 @@ func NewStorageConnection(ctx context.Context, client client.Client, ds kamajiv1 type Connection interface { CreateUser(ctx context.Context, user, password string) error + UpdateUser(ctx context.Context, user, password string) error CreateDB(ctx context.Context, dbName string) error GrantPrivileges(ctx context.Context, user, dbName string) error UserExists(ctx context.Context, user string) (bool, error) diff --git a/internal/datastore/errors/errors.go b/internal/datastore/errors/errors.go index b7cf01fb..05162e3d 100644 --- a/internal/datastore/errors/errors.go +++ b/internal/datastore/errors/errors.go @@ -5,6 +5,10 @@ package errors import "fmt" +func NewUpdateUserError(err error) error { + return fmt.Errorf("cannot update user: %w", err) +} + func NewCreateUserError(err error) error { return fmt.Errorf("cannot create user: %w", err) } diff --git a/internal/datastore/etcd.go b/internal/datastore/etcd.go index 41ebdf4f..203f6c95 100644 --- a/internal/datastore/etcd.go +++ b/internal/datastore/etcd.go @@ -49,6 +49,10 @@ func (e *EtcdClient) CreateUser(ctx context.Context, user, password string) erro return nil } +func (e *EtcdClient) UpdateUser(ctx context.Context, user, password string) error { + return nil +} + func (e *EtcdClient) CreateDB(context.Context, string) error { return nil } diff --git a/internal/datastore/mysql.go b/internal/datastore/mysql.go index 6e907f8d..a2bb5966 100644 --- a/internal/datastore/mysql.go +++ b/internal/datastore/mysql.go @@ -29,6 +29,7 @@ const ( mysqlShowGrantsStatement = "SHOW GRANTS FOR `%s`@`%%`" mysqlCreateDBStatement = "CREATE DATABASE IF NOT EXISTS %s" mysqlCreateUserStatement = "CREATE USER `%s`@`%%` IDENTIFIED BY '%s'" + mysqlUpdateUserStatement = "ALTER USER `%s`@`%%` IDENTIFIED BY '%s'" mysqlGrantPrivilegesStatement = "GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, ALTER, INDEX ON `%s`.* TO `%s`@`%%`" mysqlDropDBStatement = "DROP DATABASE IF EXISTS `%s`" mysqlDropUserStatement = "DROP USER IF EXISTS `%s`" @@ -158,6 +159,14 @@ func (c *MySQLConnection) CreateUser(ctx context.Context, user, password string) return nil } +func (c *MySQLConnection) UpdateUser(ctx context.Context, user, password string) error { + if err := c.mutate(ctx, mysqlUpdateUserStatement, user, password); err != nil { + return errors.NewUpdateUserError(err) + } + + return nil +} + func (c *MySQLConnection) CreateDB(ctx context.Context, dbName string) error { if err := c.mutate(ctx, mysqlCreateDBStatement, dbName); err != nil { return errors.NewCreateDBError(err) diff --git a/internal/datastore/nats.go b/internal/datastore/nats.go index e37f064d..fa1d02e7 100644 --- a/internal/datastore/nats.go +++ b/internal/datastore/nats.go @@ -70,6 +70,10 @@ func (nc *NATSConnection) CreateUser(_ context.Context, _, _ string) error { return nil } +func (nc *NATSConnection) UpdateUser(_ context.Context, _, _ string) error { + return nil +} + func (nc *NATSConnection) CreateDB(_ context.Context, dbName string) error { _, err := nc.js.CreateKeyValue(&nats.KeyValueConfig{Bucket: dbName}) if err != nil { diff --git a/internal/datastore/postgresql.go b/internal/datastore/postgresql.go index 80ae41a1..24e6e4ce 100644 --- a/internal/datastore/postgresql.go +++ b/internal/datastore/postgresql.go @@ -20,6 +20,7 @@ const ( postgresqlCreateDBStatement = `CREATE DATABASE "%s"` postgresqlUserExists = "SELECT 1 FROM pg_roles WHERE rolname = ?" postgresqlCreateUserStatement = `CREATE ROLE "%s" LOGIN PASSWORD ?` + postgresqlUpdateUserStatement = `ALTER ROLE "%s" WITH PASSWORD ?` postgresqlShowGrantsStatement = "SELECT has_database_privilege(rolname, ?, 'create') from pg_roles where rolcanlogin and rolname = ?" postgresqlShowOwnershipStatement = "SELECT 't' FROM pg_catalog.pg_database AS d WHERE d.datname = ? AND pg_catalog.pg_get_userbyid(d.datdba) = ?" postgresqlShowTableOwnershipStatement = "SELECT 't' from pg_tables where tableowner = ? AND tablename = ?" @@ -142,6 +143,15 @@ func (r *PostgreSQLConnection) CreateUser(ctx context.Context, user, password st return nil } +func (r *PostgreSQLConnection) UpdateUser(ctx context.Context, user, password string) error { + _, err := r.db.ExecContext(ctx, fmt.Sprintf(postgresqlUpdateUserStatement, user), password) + if err != nil { + return errors.NewUpdateUserError(err) + } + + return nil +} + func (r *PostgreSQLConnection) DBExists(ctx context.Context, dbName string) (bool, error) { rows, err := r.db.ExecContext(ctx, postgresqlFetchDBStatement, dbName) if err != nil { diff --git a/internal/resources/datastore/datastore_setup.go b/internal/resources/datastore/datastore_setup.go index e8b36f5d..55b353eb 100644 --- a/internal/resources/datastore/datastore_setup.go +++ b/internal/resources/datastore/datastore_setup.go @@ -230,6 +230,10 @@ func (r *Setup) createUser(ctx context.Context, _ *kamajiv1alpha1.TenantControlP } if exists { + if updateErr := r.Connection.UpdateUser(ctx, r.resource.user, r.resource.password); updateErr != nil { + return controllerutil.OperationResultNone, fmt.Errorf("unable to update the user to : %w", updateErr) + } + return controllerutil.OperationResultNone, nil }