Remove secret from MariaDBDatabase and use MariaDBAccount instead

[zzzeek]

this change proposes an initial step to transition to the use of MariaDBAccount to manage DB username/password combinations, rather than using MariaDBDatabase as is the case right now.

Downstream services make use of the Database{} API as an abstraction layer on top of MariaDBDatabase in order to ensure their database exists. They call upon 1. mariadbv1.NewDatabase to create the object and then 2. CreateOrPatchDB to perform the work to make this database available.

Prior to this PR, CreateOrPatchDB creates a MariaDBDatabase CR; MariaDBDatabase does both CREATE DATABASE as well as CREATE USER. MariaDBDatabase includes a secret parameter that is combined with the name of the database itself as a hardcoded username to create a database plus a login. the login cannot be changed independently of the database itself. Even though Database has a separate username parameter Database.databaseUser, this parameter is currently ignored.

The change made to this PR is that CreateOrPatchDB creates a MariaDBDatabase CR and a MariaDBAccount CR, making use of the Database.databaseUser parameter to create both a database and a username/password that is not hardcoded and can vary independently of the database. Appropriate finalizers are applied to both the MariaDBDatabase and MariaDBAccount CRs from the downstream controller.

Towards this change, the secret parameter is deprecated inMariaDBDatabase, defaulting to non-present; MariaDBDatabase no longer creates a username/password by itself in the new database unless "secret" is present. By doing so, MariaDBAccount is free to create database accounts where the username matches the name of the database, which is currently required by downstream controllers that all establish Database.databaseName andDatabase.databaseUser using the same name.

The "secret" parameter continues to be supported by MariaDBDatabase in an optional way so that downstream operators built on previous versions of the mariadb-operator will still continue to function, since they are still generating MariaDBDatabase objects with "secrets" and they are not generating MariaDBAccount objects. Once all downstream operators are built against this version of the code, where the Database construct creates a separate MariaDBAccount, then the "secret" parameter can be fully removed from MariaDBDatabase and the logic to generate and drop mariadb accounts can be removed from database.go.

These steps are intended only to integrate MariaDBAccount and independent username/passwords into the existing flow, without changing the flow itself, as this would be a breaking change in downstream controllers. The downstream controllers right now pass an explicit "databaseuser" and "secret" into Database, not the other way around. There is also no mechanism for downstream controllers to respond to changes in database username / password. this can't be implemented until MariaDBAccount is integrated as the controllers would need to be watching and consuming their corresponding MariaDBAccount objects directly.

Introduction of an independent "account / password generation service" would be done in a subsequent step. Subsequent steps would be:

1. downstream operators will build against this version of mariadb-operator or greater.
2. "secret" logic is removed from MariaDBDatabase.
3. downstream operators would need to be modified to become responsive to changes in MariaDBAccount CRs so that they can rewrite their configs and re-deploy pods when the MariaDBAccount they rely upon is to be replaced with a new one.
4. a new flow would need to be introduced that can generate as well as rotate username/passwords which can be deployed using the MariaDBAccount CR. The nature of this flow is TBD.
5. downstream operators would no longer have any kind of "database username" or "database secret" in their own CRs - this would be created dynamically by an account rotation service.

[zzzeek]

/ok-to-test

[zzzeek]

suggestions welcome how to set these defaults for the MariaDBDatabaseSpec

[gibizer]

Unfortunately golang and json defaulting is not working nicely together. When you instantiate MariaDBDatabaseSpec struct here, golang will fill all the undefined fields of the struct with the golang default value of the field type. For string it is "". Then when you send this struct to the json serializer (when send it to the k8s API) the json serializer sees the "" value and puts that into the json payload. Then when that is deserialized by the API from json to golang, it will see that the field is specified so no defaulting is applied.

You can work around this by telling the json serializer not to emit the field to the json payload if the value of the field is the golang default value of the field type. The keyword is omitempty

The following def will consider "" as a value that means no value is provided:

	// +kubebuilder:default=utf8
	// Default character set for this database
	DefaultCharacterSet string `json:"defaultCharacterSet,omitempty"`

If the empty string "" is a valid field value for the CR then you can do an additional step to keep "" valid. You can make the field as a string pointer in golang. This way "" is a valid value and nil will be used to signal unspecified value instead:

	// +kubebuilder:default=utf8
	// Default character set for this database
	DefaultCharacterSet *string `json:"defaultCharacterSet,omitempty"`

[zzzeek]

/retest

[gibizer]

Unfortunately golang and json defaulting is not working nicely together. When you instantiate MariaDBDatabaseSpec struct here, golang will fill all the undefined fields of the struct with the golang default value of the field type. For string it is "". Then when you send this struct to the json serializer (when send it to the k8s API) the json serializer sees the "" value and puts that into the json payload. Then when that is deserialized by the API from json to golang, it will see that the field is specified so no defaulting is applied.

You can work around this by telling the json serializer not to emit the field to the json payload if the value of the field is the golang default value of the field type. The keyword is omitempty

The following def will consider "" as a value that means no value is provided:

	// +kubebuilder:default=utf8
	// Default character set for this database
	DefaultCharacterSet string `json:"defaultCharacterSet,omitempty"`

If the empty string "" is a valid field value for the CR then you can do an additional step to keep "" valid. You can make the field as a string pointer in golang. This way "" is a valid value and nil will be used to signal unspecified value instead:

	// +kubebuilder:default=utf8
	// Default character set for this database
	DefaultCharacterSet *string `json:"defaultCharacterSet,omitempty"`

[zzzeek]

/retest

[gibizer]

Excellent PR summary message @zzzeek . It explained lot of my questions about the intentions of this and the next steps.

[zzzeek]

still looks like CI issues

error: build error: Failed to push image: trying to reuse blob sha256:07a64a71e01156f8f99039bc246149925c6d1480d3957de78510bbec6ec68f7a at destination: pinging container registry quay.rdoproject.org: Get "https://quay.rdoproject.org/v2/": net/http: TLS handshake timeout

[zzzeek]

/retest

[zzzeek]

/retest

[zzzeek]

I just realized why this isn't building. will have to pull back the changes to mariadbdatabase and have it work "both" ways for cross-compatibility

[zzzeek]

I had to re-introduce "secret" to MariaDBDatabase since in the build, the downstream operators still build against the older version of the operator. this complicates the MariaDBDatabase operator since "secret" is now optional on that end.

[gibizer]

I had to re-introduce "secret" to MariaDBDatabase since in the build, the downstream operators still build against the older version of the operator. this complicates the MariaDBDatabase operator since "secret" is now optional on that end.

It is fine by me.
I think we are close to the inevitable point of breaking the service operators and just go and fix them (I know it is a lot of work)

[zzzeek]

@dciabrin this should be ready to push

[dciabrin]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dciabrin, gibizer, zzzeek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dciabrin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment