From 8b7184332545dd771dc63357c2f108d461322b29 Mon Sep 17 00:00:00 2001 From: Simon Kienzler Date: Wed, 2 Oct 2024 17:33:28 +0200 Subject: [PATCH] feat: make datastore schema (prefix) configurable (#554) * feat: Add DataStoreSchema field to TCP spec * feat: Read DB_SCHEMA from TCP spec field * feat: Default DataStoreSchema in webhook * fix: Catch unsetting the dataStore via CEL * fix: Apply all patches, not only the first This also includes converting OnUpdate() to a no-op, as the existence and immutability of the fields are already checked by the API server, thanks to kubebuilder markers. The webhook ensures that fields like dataStore, dataStoreSchema are defaulted during creation (if unset), and the CEL expressions prohibit unsetting them during update. * test: Add tests for defaulting webhook * fix: typo * fix: Linter issues * fix: make apidoc * Update TCP CRD in charts folder * fix: Don't run E2E tests during `make test` * fix: Use proper `metav1` import name * feat: Handle updates of TCPs without dataStoreSchema (+ tests) * fix: Prioritize Status over Spec Co-authored-by: Dario Tranchitella * Update goDoc on DataStore field * make apidoc --------- Co-authored-by: Dario Tranchitella --- Makefile | 9 +- api/v1alpha1/tenantcontrolplane_types.go | 15 ++- ...kamaji.clastix.io_tenantcontrolplanes.yaml | 18 ++- docs/content/reference/api.md | 13 +- .../datastore/datastore_storage_config.go | 18 ++- .../datastore_storage_config_test.go | 115 ++++++++++++++++++ .../datastore/datastore_suite_test.go | 23 ++++ .../webhook/handlers/handlers_suite_test.go | 16 +++ internal/webhook/handlers/tcp_defaults.go | 50 ++++---- .../webhook/handlers/tcp_defaults_test.go | 78 ++++++++++++ internal/webhook/utils/jsonpatch.go | 14 +-- 11 files changed, 325 insertions(+), 44 deletions(-) create mode 100644 internal/resources/datastore/datastore_storage_config_test.go create mode 100644 internal/resources/datastore/datastore_suite_test.go create mode 100644 internal/webhook/handlers/handlers_suite_test.go create mode 100644 internal/webhook/handlers/tcp_defaults_test.go diff --git a/Makefile b/Makefile index 345e1ca2..a1f32841 100644 --- a/Makefile +++ b/Makefile @@ -121,8 +121,13 @@ generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and golint: golangci-lint ## Linting the code according to the styling guide. $(GOLANGCI_LINT) run -c .golangci.yml -test: - go test ./... -coverprofile cover.out +.PHONY: test +test: ## Run unit tests (all tests except E2E). + @go test \ + ./api/... \ + ./cmd/... \ + ./internal/... \ + -coverprofile cover.out _datastore-mysql: $(MAKE) NAME=$(NAME) -C deploy/kine/mysql mariadb diff --git a/api/v1alpha1/tenantcontrolplane_types.go b/api/v1alpha1/tenantcontrolplane_types.go index 329dea11..78affad0 100644 --- a/api/v1alpha1/tenantcontrolplane_types.go +++ b/api/v1alpha1/tenantcontrolplane_types.go @@ -249,12 +249,21 @@ 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" type TenantControlPlaneSpec struct { // DataStore allows to specify a DataStore that should be used to store the Kubernetes data for the given Tenant Control Plane. // This parameter is optional and acts as an override over the default one which is used by the Kamaji Operator. - // Migration from a different DataStore to another one is not yet supported and the reconciliation will be blocked. - DataStore string `json:"dataStore,omitempty"` - ControlPlane ControlPlane `json:"controlPlane"` + // Migration from one DataStore to another backed by the same Driver is possible. See: https://kamaji.clastix.io/guides/datastore-migration/ + // Migration from one DataStore to another backed by a different Driver is not supported. + DataStore string `json:"dataStore,omitempty"` + // DataStoreSchema allows to specify the name of the database (for relational DataStores) or the key prefix (for etcd). This + // value is optional and immutable. Note that Kamaji currently doesn't ensure that DataStoreSchema values are unique. It's up + // 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"` // 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 cc2c141f..a7042078 100644 --- a/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml +++ b/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml @@ -6415,8 +6415,19 @@ spec: description: |- DataStore allows to specify a DataStore that should be used to store the Kubernetes data for the given Tenant Control Plane. This parameter is optional and acts as an override over the default one which is used by the Kamaji Operator. - Migration from a different DataStore to another one is not yet supported and the reconciliation will be blocked. + Migration from one DataStore to another backed by the same Driver is possible. See: https://kamaji.clastix.io/guides/datastore-migration/ + Migration from one DataStore to another backed by a different Driver is not supported. type: string + dataStoreSchema: + description: |- + DataStoreSchema allows to specify the name of the database (for relational DataStores) or the key prefix (for etcd). This + value is optional and immutable. Note that Kamaji currently doesn't ensure that DataStoreSchema values are unique. It's up + 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. + type: string + x-kubernetes-validations: + - message: changing the dataStoreSchema is not supported + rule: self == oldSelf kubernetes: description: Kubernetes specification for tenant control plane properties: @@ -6563,6 +6574,11 @@ spec: - controlPlane - kubernetes type: object + x-kubernetes-validations: + - message: unsetting the dataStore is not supported + rule: '!has(oldSelf.dataStore) || has(self.dataStore)' + - message: unsetting the dataStoreSchema is not supported + rule: '!has(oldSelf.dataStoreSchema) || has(self.dataStoreSchema)' status: description: TenantControlPlaneStatus defines the observed state of TenantControlPlane. properties: diff --git a/docs/content/reference/api.md b/docs/content/reference/api.md index 4cc35913..0d99b2a0 100644 --- a/docs/content/reference/api.md +++ b/docs/content/reference/api.md @@ -839,7 +839,18 @@ such as the number of Pod replicas, the Service resource, or the Ingress.
DataStore allows to specify a DataStore that should be used to store the Kubernetes data for the given Tenant Control Plane. This parameter is optional and acts as an override over the default one which is used by the Kamaji Operator. -Migration from a different DataStore to another one is not yet supported and the reconciliation will be blocked.
+Migration from one DataStore to another backed by the same Driver is possible. See: https://kamaji.clastix.io/guides/datastore-migration/ +Migration from one DataStore to another backed by a different Driver is not supported.
+ + false + + dataStoreSchema + string + + DataStoreSchema allows to specify the name of the database (for relational DataStores) or the key prefix (for etcd). This +value is optional and immutable. Note that Kamaji currently doesn't ensure that DataStoreSchema values are unique. It's up +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.
false diff --git a/internal/resources/datastore/datastore_storage_config.go b/internal/resources/datastore/datastore_storage_config.go index cc623f0f..813f827e 100644 --- a/internal/resources/datastore/datastore_storage_config.go +++ b/internal/resources/datastore/datastore_storage_config.go @@ -155,9 +155,25 @@ func (r *Config) mutate(ctx context.Context, tenantControlPlane *kamajiv1alpha1. username = coalesceFn(tenantControlPlane.Status.Storage.Setup.User) } + var dataStoreSchema string + switch { + case len(tenantControlPlane.Status.Storage.Setup.Schema) > 0: + // for existing TCPs, the dataStoreSchema will be adopted from the status, + // as the mutating webhook only takes care of TCP creations, not updates + dataStoreSchema = tenantControlPlane.Status.Storage.Setup.Schema + tenantControlPlane.Spec.DataStoreSchema = dataStoreSchema + case len(tenantControlPlane.Spec.DataStoreSchema) > 0: + // for new TCPs, the spec field will have been provided by the user + // or defaulted by the defaulting webhook + dataStoreSchema = tenantControlPlane.Spec.DataStoreSchema + default: + // this can only happen on TCP creations when the webhook is not installed + return fmt.Errorf("cannot build datastore storage config, schema name must either exist in Spec or Status") + } + r.resource.Data = map[string][]byte{ "DB_CONNECTION_STRING": []byte(r.ConnString), - "DB_SCHEMA": coalesceFn(tenantControlPlane.Status.Storage.Setup.Schema), + "DB_SCHEMA": []byte(dataStoreSchema), "DB_USER": username, "DB_PASSWORD": password, } diff --git a/internal/resources/datastore/datastore_storage_config_test.go b/internal/resources/datastore/datastore_storage_config_test.go new file mode 100644 index 00000000..84754c08 --- /dev/null +++ b/internal/resources/datastore/datastore_storage_config_test.go @@ -0,0 +1,115 @@ +// Copyright 2022 Clastix Labs +// SPDX-License-Identifier: Apache-2.0 + +package datastore_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + kamajiv1alpha1 "github.com/clastix/kamaji/api/v1alpha1" + "github.com/clastix/kamaji/internal/resources" + "github.com/clastix/kamaji/internal/resources/datastore" +) + +var _ = Describe("DatastoreStorageConfig", func() { + var ( + ctx context.Context + dsc *datastore.Config + tcp *kamajiv1alpha1.TenantControlPlane + ds *kamajiv1alpha1.DataStore + ) + + BeforeEach(func() { + ctx = context.Background() + + tcp = &kamajiv1alpha1.TenantControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tcp", + Namespace: "default", + }, + Spec: kamajiv1alpha1.TenantControlPlaneSpec{}, + } + + ds = &kamajiv1alpha1.DataStore{ + ObjectMeta: metav1.ObjectMeta{ + Name: "datastore", + Namespace: "default", + }, + } + + Expect(kamajiv1alpha1.AddToScheme(scheme)).To(Succeed()) + Expect(corev1.AddToScheme(scheme)).To(Succeed()) + }) + + JustBeforeEach(func() { + fakeClient = fake.NewClientBuilder(). + WithScheme(scheme).WithObjects(tcp).WithStatusSubresource(tcp).Build() + + dsc = &datastore.Config{ + Client: fakeClient, + ConnString: "", + DataStore: *ds, + } + }) + + When("TCP has no dataStoreSchema 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() { + BeforeEach(func() { + tcp.Spec.DataStoreSchema = "custom-prefix" + }) + + It("should create the datastore secret with the schema name from the spec", func() { + op, err := resources.Handle(ctx, dsc, tcp) + Expect(err).ToNot(HaveOccurred()) + Expect(op).To(Equal(controllerutil.OperationResultCreated)) + + secrets := &corev1.SecretList{} + 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"))) + }) + }) + + When("TCP has dataStoreSchema 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" + Expect(fakeClient.Status().Update(ctx, tcp)).To(Succeed()) + + By("handling the resource") + op, err := resources.Handle(ctx, dsc, tcp) + Expect(err).ToNot(HaveOccurred()) + Expect(op).To(Equal(controllerutil.OperationResultCreated)) + + By("checking the secret") + secrets := &corev1.SecretList{} + 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"))) + + 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" + }) + }) +}) diff --git a/internal/resources/datastore/datastore_suite_test.go b/internal/resources/datastore/datastore_suite_test.go new file mode 100644 index 00000000..5cff8155 --- /dev/null +++ b/internal/resources/datastore/datastore_suite_test.go @@ -0,0 +1,23 @@ +// Copyright 2022 Clastix Labs +// SPDX-License-Identifier: Apache-2.0 + +package datastore_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ( + fakeClient client.Client + scheme *runtime.Scheme = runtime.NewScheme() +) + +func TestDatastore(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Datastore Suite") +} diff --git a/internal/webhook/handlers/handlers_suite_test.go b/internal/webhook/handlers/handlers_suite_test.go new file mode 100644 index 00000000..afbb1479 --- /dev/null +++ b/internal/webhook/handlers/handlers_suite_test.go @@ -0,0 +1,16 @@ +// Copyright 2022 Clastix Labs +// SPDX-License-Identifier: Apache-2.0 + +package handlers_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestHandlers(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Handlers Suite") +} diff --git a/internal/webhook/handlers/tcp_defaults.go b/internal/webhook/handlers/tcp_defaults.go index 2de905d4..6274d13c 100644 --- a/internal/webhook/handlers/tcp_defaults.go +++ b/internal/webhook/handlers/tcp_defaults.go @@ -6,6 +6,7 @@ package handlers import ( "context" "fmt" + "strings" "github.com/pkg/errors" "gomodules.xyz/jsonpatch/v2" @@ -23,24 +24,17 @@ type TenantControlPlaneDefaults struct { func (t TenantControlPlaneDefaults) OnCreate(object runtime.Object) AdmissionResponse { return func(ctx context.Context, req admission.Request) ([]jsonpatch.JsonPatchOperation, error) { - tcp := object.(*kamajiv1alpha1.TenantControlPlane) //nolint:forcetypeassert + original := object.(*kamajiv1alpha1.TenantControlPlane) //nolint:forcetypeassert - if len(tcp.Spec.DataStore) == 0 { - operations, err := utils.JSONPatch(tcp, func() { - tcp.Spec.DataStore = t.DefaultDatastore - }) - if err != nil { - return nil, errors.Wrap(err, "cannot create patch responses upon Tenant Control Plane creation") - } + defaulted := original.DeepCopy() + t.defaultUnsetFields(defaulted) - return operations, nil + operations, err := utils.JSONPatch(original, defaulted) + if err != nil { + return nil, errors.Wrap(err, "cannot create patch responses upon Tenant Control Plane creation") } - if tcp.Spec.ControlPlane.Deployment.Replicas == nil { - tcp.Spec.ControlPlane.Deployment.Replicas = pointer.To(int32(2)) - } - - return nil, nil + return operations, nil } } @@ -48,22 +42,22 @@ func (t TenantControlPlaneDefaults) OnDelete(runtime.Object) AdmissionResponse { return utils.NilOp() } -func (t TenantControlPlaneDefaults) OnUpdate(object runtime.Object, oldObject runtime.Object) AdmissionResponse { - return func(ctx context.Context, req admission.Request) ([]jsonpatch.JsonPatchOperation, error) { - newTCP, oldTCP := object.(*kamajiv1alpha1.TenantControlPlane), oldObject.(*kamajiv1alpha1.TenantControlPlane) //nolint:forcetypeassert - - if oldTCP.Spec.DataStore == newTCP.Spec.DataStore { - return nil, nil - } +func (t TenantControlPlaneDefaults) OnUpdate(runtime.Object, runtime.Object) AdmissionResponse { + // all immutability requirements are handled trough CEL annotations on the TenantControlPlaneSpec type + return utils.NilOp() +} - if len(newTCP.Spec.DataStore) == 0 { - return nil, fmt.Errorf("DataStore is a required field") - } +func (t TenantControlPlaneDefaults) defaultUnsetFields(tcp *kamajiv1alpha1.TenantControlPlane) { + if len(tcp.Spec.DataStore) == 0 { + tcp.Spec.DataStore = t.DefaultDatastore + } - if newTCP.Spec.ControlPlane.Deployment.Replicas == nil { - newTCP.Spec.ControlPlane.Deployment.Replicas = pointer.To(int32(2)) - } + if tcp.Spec.ControlPlane.Deployment.Replicas == nil { + tcp.Spec.ControlPlane.Deployment.Replicas = pointer.To(int32(2)) + } - return nil, nil + if len(tcp.Spec.DataStoreSchema) == 0 { + dss := strings.ReplaceAll(fmt.Sprintf("%s_%s", tcp.GetNamespace(), tcp.GetName()), "-", "_") + tcp.Spec.DataStoreSchema = dss } } diff --git a/internal/webhook/handlers/tcp_defaults_test.go b/internal/webhook/handlers/tcp_defaults_test.go new file mode 100644 index 00000000..4bc62e0c --- /dev/null +++ b/internal/webhook/handlers/tcp_defaults_test.go @@ -0,0 +1,78 @@ +// Copyright 2022 Clastix Labs +// SPDX-License-Identifier: Apache-2.0 + +package handlers_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "gomodules.xyz/jsonpatch/v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + kamajiv1alpha1 "github.com/clastix/kamaji/api/v1alpha1" + "github.com/clastix/kamaji/internal/webhook/handlers" +) + +var _ = Describe("TCP Defaulting Webhook", func() { + var ( + ctx context.Context + t handlers.TenantControlPlaneDefaults + tcp *kamajiv1alpha1.TenantControlPlane + ) + + BeforeEach(func() { + t = handlers.TenantControlPlaneDefaults{ + DefaultDatastore: "etcd", + } + tcp = &kamajiv1alpha1.TenantControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tcp", + Namespace: "default", + }, + Spec: kamajiv1alpha1.TenantControlPlaneSpec{}, + } + ctx = context.Background() + }) + + Describe("fields missing", 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)) + }) + + It("should default the dataStore", func() { + ops, err := t.OnCreate(tcp)(ctx, admission.Request{}) + Expect(err).ToNot(HaveOccurred()) + Expect(ops).To(ContainElement( + jsonpatch.Operation{Operation: "add", Path: "/spec/dataStore", Value: "etcd"}, + )) + }) + + It("should default the dataStoreSchema 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"}, + )) + }) + }) + + Describe("fields are already set", func() { + BeforeEach(func() { + tcp.Spec.DataStore = "etcd" + tcp.Spec.DataStoreSchema = "my_tcp" + tcp.Spec.ControlPlane.Deployment.Replicas = ptr.To(int32(2)) + }) + + It("should not issue any patches", func() { + ops, err := t.OnCreate(tcp)(ctx, admission.Request{}) + Expect(err).ToNot(HaveOccurred()) + Expect(ops).To(BeEmpty()) + }) + }) +}) diff --git a/internal/webhook/utils/jsonpatch.go b/internal/webhook/utils/jsonpatch.go index 17927ca2..8f25c6e5 100644 --- a/internal/webhook/utils/jsonpatch.go +++ b/internal/webhook/utils/jsonpatch.go @@ -10,18 +10,16 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func JSONPatch(obj client.Object, modifierFunc func()) ([]jsonpatch.Operation, error) { - original, err := json.Marshal(obj) +func JSONPatch(original, modified client.Object) ([]jsonpatch.Operation, error) { + originalJSON, err := json.Marshal(original) if err != nil { - return nil, errors.Wrap(err, "cannot marshal input object") + return nil, errors.Wrap(err, "cannot marshal original object") } - modifierFunc() - - patched, err := json.Marshal(obj) + modifiedJSON, err := json.Marshal(modified) if err != nil { - return nil, errors.Wrap(err, "cannot marshal patched object") + return nil, errors.Wrap(err, "cannot marshal modified object") } - return jsonpatch.CreatePatch(original, patched) + return jsonpatch.CreatePatch(originalJSON, modifiedJSON) }