feat!: support setting the username for the relational database (#891)

* Support setting the username for the relational database

fixes #889

* update crd+documentation

Author: toelke

api/v1alpha1/tenantcontrolplane_types.go

@@ -290,6 +290,7 @@ type AddonsSpec struct {
 // TenantControlPlaneSpec defines the desired state of TenantControlPlane.
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.dataStore) || has(self.dataStore)", message="unsetting the dataStore is not supported"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.dataStoreSchema) || has(self.dataStoreSchema)", message="unsetting the dataStoreSchema is not supported"
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.dataStoreUsername) || has(self.dataStoreUsername)", message="unsetting the dataStoreUsername is not supported"
 // +kubebuilder:validation:XValidation:rule="!has(self.networkProfile.loadBalancerSourceRanges) || (size(self.networkProfile.loadBalancerSourceRanges) == 0 || self.controlPlane.service.serviceType == 'LoadBalancer')", message="LoadBalancer source ranges are supported only with LoadBalancer service type"
 // +kubebuilder:validation:XValidation:rule="!has(self.networkProfile.loadBalancerClass) || self.controlPlane.service.serviceType == 'LoadBalancer'", message="LoadBalancerClass is supported only with LoadBalancer service type"
 // +kubebuilder:validation:XValidation:rule="self.controlPlane.service.serviceType != 'LoadBalancer' || (oldSelf.controlPlane.service.serviceType != 'LoadBalancer' && self.controlPlane.service.serviceType == 'LoadBalancer') || has(self.networkProfile.loadBalancerClass) == has(oldSelf.networkProfile.loadBalancerClass)",message="LoadBalancerClass cannot be set or unset at runtime"
@@ -307,8 +308,14 @@ type TenantControlPlaneSpec struct {
 	// to the user to avoid clashes between different TenantControlPlanes. If not set upon creation, Kamaji will default the
 	// DataStoreSchema by concatenating the namespace and name of the TenantControlPlane.
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="changing the dataStoreSchema is not supported"
-	DataStoreSchema string       `json:"dataStoreSchema,omitempty"`
-	ControlPlane    ControlPlane `json:"controlPlane"`
+	DataStoreSchema string `json:"dataStoreSchema,omitempty"`
+	// DataStoreUsername allows to specify the username of the database (for relational DataStores). This
+	// value is optional and immutable. Note that Kamaji currently doesn't ensure that DataStoreUsername values are unique. It's up
+	// to the user to avoid clashes between different TenantControlPlanes. If not set upon creation, Kamaji will default the
+	// DataStoreUsername by concatenating the namespace and name of the TenantControlPlane.
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="changing the dataStoreUsername is not supported"
+	DataStoreUsername string       `json:"dataStoreUsername,omitempty"`
+	ControlPlane      ControlPlane `json:"controlPlane"`
 	// Kubernetes specification for tenant control plane
 	Kubernetes KubernetesSpec `json:"kubernetes"`
 	// NetworkProfile specifies how the network is

charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml

@@ -6486,6 +6486,16 @@ spec:
                   x-kubernetes-validations:
                     - message: changing the dataStoreSchema is not supported
                       rule: self == oldSelf
+                dataStoreUsername:
+                  description: |-
+                    DataStoreUsername allows to specify the username of the database (for relational DataStores). This
+                    value is optional and immutable. Note that Kamaji currently doesn't ensure that DataStoreUsername values are unique. It's up
+                    to the user to avoid clashes between different TenantControlPlanes. If not set upon creation, Kamaji will default the
+                    DataStoreUsername by concatenating the namespace and name of the TenantControlPlane.
+                  type: string
+                  x-kubernetes-validations:
+                    - message: changing the dataStoreUsername is not supported
+                      rule: self == oldSelf
                 kubernetes:
                   description: Kubernetes specification for tenant control plane
                   properties:
@@ -6670,6 +6680,8 @@ spec:
                   rule: '!has(oldSelf.dataStore) || has(self.dataStore)'
                 - message: unsetting the dataStoreSchema is not supported
                   rule: '!has(oldSelf.dataStoreSchema) || has(self.dataStoreSchema)'
+                - message: unsetting the dataStoreUsername is not supported
+                  rule: '!has(oldSelf.dataStoreUsername) || has(self.dataStoreUsername)'
                 - message: LoadBalancer source ranges are supported only with LoadBalancer service type
                   rule: '!has(self.networkProfile.loadBalancerSourceRanges) || (size(self.networkProfile.loadBalancerSourceRanges) == 0 || self.controlPlane.service.serviceType == ''LoadBalancer'')'
                 - message: LoadBalancerClass is supported only with LoadBalancer service type

docs/content/reference/api.md

@@ -27049,6 +27049,16 @@ to the user to avoid clashes between different TenantControlPlanes. If not set u
 DataStoreSchema by concatenating the namespace and name of the TenantControlPlane.<br/>
         </td>
         <td>false</td>
+      </tr><tr>
+        <td><b>dataStoreUsername</b></td>
+        <td>string</td>
+        <td>
+          DataStoreUsername allows to specify the username of the database (for relational DataStores). This
+value is optional and immutable. Note that Kamaji currently doesn't ensure that DataStoreUsername values are unique. It's up
+to the user to avoid clashes between different TenantControlPlanes. If not set upon creation, Kamaji will default the
+DataStoreUsername by concatenating the namespace and name of the TenantControlPlane.<br/>
+        </td>
+        <td>false</td>
       </tr><tr>
         <td><b><a href="#tenantcontrolplanespecnetworkprofile">networkProfile</a></b></td>
         <td>object</td>

internal/resources/datastore/datastore_storage_config.go

@@ -6,7 +6,6 @@ package datastore
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
@@ -125,19 +124,6 @@ func (r *Config) mutate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.
 		default:
 			password = []byte(uuid.New().String())
 		}
-		// the coalesce function prioritizes the return value stored in the TenantControlPlane status,
-		// although this is going to be populated by the UpdateTenantControlPlaneStatus handler of the resource datastore-setup:
-		// the default value will be used for fresh new configurations, and preserving a previous one:
-		// this will keep us safe from naming changes cases as occurred with the following commit:
-		// https://github.com/clastix/kamaji/pull/203/commits/09ce38f489cccca72ab728a259bc8fb2cf6e4770
-		coalesceFn := func(fromStatus string) []byte {
-			if len(fromStatus) > 0 {
-				return []byte(fromStatus)
-			}
-			// The dash character (-) must be replaced with an underscore, PostgreSQL is complaining about it:
-			// https://github.com/clastix/kamaji/issues/328
-			return []byte(strings.ReplaceAll(fmt.Sprintf("%s_%s", tenantControlPlane.GetNamespace(), tenantControlPlane.GetName()), "-", "_"))
-		}
 
 		finalizersList := sets.New[string](r.resource.GetFinalizers()...)
 		finalizersList.Insert(finalizers.DatastoreSecretFinalizer)
@@ -161,7 +147,25 @@ func (r *Config) mutate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.
 			username = u
 			password = p
 		} else {
-			username = coalesceFn(tenantControlPlane.Status.Storage.Setup.User)
+			// prioritize the username stored in the TenantControlPlane status,
+			// although this is going to be populated by the UpdateTenantControlPlaneStatus handler of the resource datastore-setup:
+			// the default value will be used for fresh new configurations, and preserving a previous one:
+			// this will keep us safe from naming changes cases as occurred with the following commit:
+			// https://github.com/clastix/kamaji/pull/203/commits/09ce38f489cccca72ab728a259bc8fb2cf6e4770
+			switch {
+			case len(tenantControlPlane.Status.Storage.Setup.User) > 0:
+				// for existing TCPs, the dataStoreSchema will be adopted from the status,
+				// as the mutating webhook only takes care of TCP creations, not updates
+				username = []byte(tenantControlPlane.Status.Storage.Setup.User)
+				tenantControlPlane.Spec.DataStoreUsername = string(username)
+			case len(tenantControlPlane.Spec.DataStoreUsername) > 0:
+				// for new TCPs, the spec field will have been provided by the user
+				// or defaulted by the defaulting webhook
+				username = []byte(tenantControlPlane.Spec.DataStoreUsername)
+			default:
+				// this can only happen on TCP creations when the webhook is not installed
+				return fmt.Errorf("cannot build datastore storage config, username must either exist in Spec or Status")
+			}
 		}
 
 		var dataStoreSchema string

internal/resources/datastore/datastore_storage_config_test.go

@@ -60,16 +60,17 @@ var _ = Describe("DatastoreStorageConfig", func() {
 		}
 	})
 
-	When("TCP has no dataStoreSchema defined", func() {
+	When("TCP has neither dataStoreSchema nor dataStoreUsername defined", func() {
 		It("should return an error", func() {
 			_, err := resources.Handle(ctx, dsc, tcp)
 			Expect(err).To(HaveOccurred())
 		})
 	})
 
-	When("TCP has dataStoreSchema set in spec", func() {
+	When("TCP has dataStoreSchema and dataStoreUsername set in spec", func() {
 		BeforeEach(func() {
 			tcp.Spec.DataStoreSchema = "custom-prefix"
+			tcp.Spec.DataStoreUsername = "custom-user"
 		})
 
 		It("should create the datastore secret with the schema name from the spec", func() {
@@ -81,17 +82,19 @@ var _ = Describe("DatastoreStorageConfig", func() {
 			Expect(fakeClient.List(ctx, secrets)).To(Succeed())
 			Expect(secrets.Items).To(HaveLen(1))
 			Expect(secrets.Items[0].Data["DB_SCHEMA"]).To(Equal([]byte("custom-prefix")))
+			Expect(secrets.Items[0].Data["DB_USER"]).To(Equal([]byte("custom-user")))
 		})
 	})
 
-	When("TCP has dataStoreSchema set in status, but not in spec", func() {
+	When("TCP has dataStoreSchema and dataStoreUsername set in status, but not in spec", func() {
 		// this test case ensures that existing TCPs (created in a CRD version without
 		// the dataStoreSchema field) correctly adopt the spec field from the status.
 
 		It("should create the datastore secret with the correct schema name and update the TCP spec", func() {
 			By("updating the TCP status")
 			Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(tcp), tcp)).To(Succeed())
 			tcp.Status.Storage.Setup.Schema = "existing-schema-name"
+			tcp.Status.Storage.Setup.User = "existing-username"
 			Expect(fakeClient.Status().Update(ctx, tcp)).To(Succeed())
 
 			By("handling the resource")
@@ -104,12 +107,14 @@ var _ = Describe("DatastoreStorageConfig", func() {
 			Expect(fakeClient.List(ctx, secrets)).To(Succeed())
 			Expect(secrets.Items).To(HaveLen(1))
 			Expect(secrets.Items[0].Data["DB_SCHEMA"]).To(Equal([]byte("existing-schema-name")))
+			Expect(secrets.Items[0].Data["DB_USER"]).To(Equal([]byte("existing-username")))
 
 			By("checking the TCP spec")
 			// we have to check the modified struct here (instead of retrieving the object
 			// via the fakeClient), as the TCP resource update is not done by the resources.
 			// Instead, the TCP controller will handle TCP updates after handling all resources
 			tcp.Spec.DataStoreSchema = "existing-schema-name"
+			tcp.Spec.DataStoreUsername = "existing-username"
 		})
 	})
 })

internal/webhook/handlers/tcp_defaults.go

@@ -76,4 +76,11 @@ func (t TenantControlPlaneDefaults) defaultUnsetFields(tcp *kamajiv1alpha1.Tenan
 		dss := strings.ReplaceAll(fmt.Sprintf("%s_%s", tcp.GetNamespace(), tcp.GetName()), "-", "_")
 		tcp.Spec.DataStoreSchema = dss
 	}
+
+	if len(tcp.Spec.DataStoreUsername) == 0 {
+		// The dash character (-) must be replaced with an underscore, PostgreSQL is complaining about it:
+		// https://github.com/clastix/kamaji/issues/328
+		username := strings.ReplaceAll(fmt.Sprintf("%s_%s", tcp.GetNamespace(), tcp.GetName()), "-", "_")
+		tcp.Spec.DataStoreUsername = username
+	}
 }

internal/webhook/handlers/tcp_defaults_test.go

@@ -49,7 +49,7 @@ var _ = Describe("TCP Defaulting Webhook", func() {
 		It("should issue all required patches", func() {
 			ops, err := t.OnCreate(tcp)(ctx, admission.Request{})
 			Expect(err).ToNot(HaveOccurred())
-			Expect(ops).To(HaveLen(3))
+			Expect(ops).To(HaveLen(4))
 		})
 
 		It("should default the dataStore", func() {
@@ -60,19 +60,23 @@ var _ = Describe("TCP Defaulting Webhook", func() {
 			))
 		})
 
-		It("should default the dataStoreSchema to the expected value", func() {
+		It("should default the dataStoreSchema and dataStoreUsername to the expected value", func() {
 			ops, err := t.OnCreate(tcp)(ctx, admission.Request{})
 			Expect(err).ToNot(HaveOccurred())
 			Expect(ops).To(ContainElement(
 				jsonpatch.Operation{Operation: "add", Path: "/spec/dataStoreSchema", Value: "default_tcp"},
 			))
+			Expect(ops).To(ContainElement(
+				jsonpatch.Operation{Operation: "add", Path: "/spec/dataStoreUsername", Value: "default_tcp"},
+			))
 		})
 	})
 
 	Describe("fields are already set", func() {
 		BeforeEach(func() {
 			tcp.Spec.DataStore = "etcd"
 			tcp.Spec.DataStoreSchema = "my_tcp"
+			tcp.Spec.DataStoreUsername = "my_tcp"
 			tcp.Spec.ControlPlane.Deployment.Replicas = ptr.To(int32(2))
 		})