Skip to content

Commit

Permalink
feat: make datastore schema (prefix) configurable (#554)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>

* Update goDoc on DataStore field

* make apidoc

---------

Co-authored-by: Dario Tranchitella <[email protected]>
  • Loading branch information
SimonKienzler and prometherion authored Oct 2, 2024
1 parent 489e0e1 commit 8b71843
Show file tree
Hide file tree
Showing 11 changed files with 325 additions and 44 deletions.
9 changes: 7 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 12 additions & 3 deletions api/v1alpha1/tenantcontrolplane_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 17 additions & 1 deletion charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
13 changes: 12 additions & 1 deletion docs/content/reference/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,18 @@ such as the number of Pod replicas, the Service resource, or the Ingress.<br/>
<td>
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.<br/>
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.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>dataStoreSchema</b></td>
<td>string</td>
<td>
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.<br/>
</td>
<td>false</td>
</tr><tr>
Expand Down
18 changes: 17 additions & 1 deletion internal/resources/datastore/datastore_storage_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down
115 changes: 115 additions & 0 deletions internal/resources/datastore/datastore_storage_config_test.go
Original file line number Diff line number Diff line change
@@ -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"
})
})
})
23 changes: 23 additions & 0 deletions internal/resources/datastore/datastore_suite_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
16 changes: 16 additions & 0 deletions internal/webhook/handlers/handlers_suite_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
50 changes: 22 additions & 28 deletions internal/webhook/handlers/tcp_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package handlers
import (
"context"
"fmt"
"strings"

"github.com/pkg/errors"
"gomodules.xyz/jsonpatch/v2"
Expand All @@ -23,47 +24,40 @@ 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
}
}

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
}
}
Loading

0 comments on commit 8b71843

Please sign in to comment.