diff --git a/api/v1alpha1/tenantcontrolplane_types.go b/api/v1alpha1/tenantcontrolplane_types.go index 7e4a37b..b101d98 100644 --- a/api/v1alpha1/tenantcontrolplane_types.go +++ b/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 diff --git a/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml b/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml index 4a4fb94..95c2d14 100644 --- a/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml +++ b/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 diff --git a/docs/content/reference/api.md b/docs/content/reference/api.md index ac73a24..de0740d 100644 --- a/docs/content/reference/api.md +++ b/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.
false + + dataStoreUsername + string + + 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.
+ + false networkProfile object diff --git a/internal/resources/datastore/datastore_storage_config.go b/internal/resources/datastore/datastore_storage_config.go index 3065d51..73e7dc2 100644 --- a/internal/resources/datastore/datastore_storage_config.go +++ b/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 diff --git a/internal/resources/datastore/datastore_storage_config_test.go b/internal/resources/datastore/datastore_storage_config_test.go index 84754c0..a71383a 100644 --- a/internal/resources/datastore/datastore_storage_config_test.go +++ b/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,10 +82,11 @@ 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. @@ -92,6 +94,7 @@ var _ = Describe("DatastoreStorageConfig", 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" }) }) }) diff --git a/internal/webhook/handlers/tcp_defaults.go b/internal/webhook/handlers/tcp_defaults.go index 5d71f1e..06fd184 100644 --- a/internal/webhook/handlers/tcp_defaults.go +++ b/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 + } } diff --git a/internal/webhook/handlers/tcp_defaults_test.go b/internal/webhook/handlers/tcp_defaults_test.go index 43e848e..868c08b 100644 --- a/internal/webhook/handlers/tcp_defaults_test.go +++ b/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,12 +60,15 @@ 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"}, + )) }) }) @@ -73,6 +76,7 @@ var _ = Describe("TCP Defaulting Webhook", 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)) })