-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Make DataStore Schema (prefix) Configurable #554
feat: Make DataStore Schema (prefix) Configurable #554
Conversation
✅ Deploy Preview for kamaji-documentation ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -49,21 +43,21 @@ func (t TenantControlPlaneDefaults) OnDelete(runtime.Object) AdmissionResponse { | |||
} | |||
|
|||
func (t TenantControlPlaneDefaults) OnUpdate(object runtime.Object, oldObject runtime.Object) AdmissionResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (t TenantControlPlaneDefaults) OnUpdate(object runtime.Object, oldObject runtime.Object) AdmissionResponse { | |
func (t TenantControlPlaneDefaults) OnUpdate(runtime.Object, runtime.Object) AdmissionResponse { |
@@ -249,12 +249,20 @@ 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A CEL policy here would create a breaking change since it requires Kubernetes >= 1.25, and Kamaji has a loose requirement of +1.22 (https://kamaji.clastix.io/reference/versioning/)
@bsctl WDYT here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About version, frankly I don’t care. 1.22 is a very old release and we can move to 1.25 as baseline for support. If CEL usage provides benefits over webhook and if we want push in this direction for the future, I think it is a good idea, but I would avoid a mix and match of different techniques unless it provides clear benefits, just for sake of maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my experience, it's convenient to handle as many validation/defaulting tasks as possible using the kubebuilder annotations, i.e. letting the K8s API Server handle that for you. Except for the things that need a more sophisticated logic, then resort to webhooks
Are you referring to changes introduced with 071e679? It seems fine, wondering why we never had a report, maybe everyone's hardly setting values. |
Regarding the failing E2E test: The Helm Chart ( |
Another question that came to my mind: What would be the preferred way of handling existing TCP CRDs? Should the controller handle this silently (adopting the spec from the status)? |
Reading from the status is the way to preserve consistency since it's our state machine declaration, and saved us from many issues. Users will need to patch their TCP with the newer version of the CRD just to be sure to have a consistent status. |
It depends on your dev environment, I'm used to developing with Debugging from your IDE is much more complicated since it requires some hacking of the webhooks with tunnelling, as well as having Kamaji with a TLS certificate: I'm used to |
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.
4539724
to
d02c580
Compare
Okay, with the changes from #565 incorporated, the E2E test suite passed. Nice 🎉 Regarding the adoption of existing TCP resources, this is less complicated than I thought. As the status will always be the same pattern as what the webhook will default to (
|
Just reviewed and suggested some prioritizations.
That's enough, thanks.
Let's implement the CEL language as you suggested, I'll open a PR once we have it merged to document this change.
LGTM! |
Co-authored-by: Dario Tranchitella <dario@tranchitella.eu>
@@ -6415,8 +6415,18 @@ 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 a different DataStore to another one is supported, see: https://kamaji.clastix.io/guides/datastore-migration/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect; rather, this sentence should explain we do not offer migration between Datastore backed by different drivers.
Fixes #548
As agreed in the aforementioned discussion, this PR introduces a new
TenantControlPlaneSpec
field calledDataStoreSchema
. The field name could also be different, if desired. It allows users to control the database name or etcd prefix. If the field is unset during creation, the defaulting webhook sets the field to<TCPNamespace>_<TCPName>
, which is the current behavior. The field is immutable.Some remarks:
operations
in theOnCreate
function, therefore the replicas were never defaulted.OnUpdate
function also had some early returns, so same problem there.DataStore
andDataStoreSchema
fields can't be unset after creation. This makes theOnUpdate
function a NoOp.DataStore
field. According to the docs, datastore migrations are in fact possible.