revise auto-generation of MariaDBAccount for deletion to work

[zzzeek]

1. The previous addition of MariaDBAccount to Database lacked the ability to re-acquire the MariaDBAccount object based on a single "name" alone, as we named the mariadbaccount after the username field, not the name field. While we did fix this to at least swap out underscores with dashes, an operator that's built against the new API will already send usernames that are different from the "name" field in any case, so the lookup of this object later would fail, which is needed among other cases to remove finalizers from the objects. So use the same "name" for the MariaDBAccount as we do for the MariaDBDatabase; the username itself is still of course independent from the name of the object, and this will allow us to go forward within the legacy Database{} case for operators that dont yet refer to MariaDBAccount explicitly (which is all of them at the moment)
2. when we do get a Database with or without a username, and are asked to look up the MariaDBDatabase and MariaDBAccount for it, don't error out if we didnt find the MariaDBAccount. The legacy case of MariaDBDatabase with a direct "secret" and no MariaDBAccount is still supported, as would be the behavior of all controllers that are built against an older version of the mariadb controller. While it's unlikely that a controller would build out a MariaDBDatabase only and then later, be upgraded to be built against newer code where it then needs to be smarter about MariaDBAccount, at least cover
   this contingency. this will all be improved when all the
   controllers are updated to only refer to a Database{} object (or
   some other new API of some kind) where the MariaDBDatabase and
   MariaDBAccount are always named explicitly.
3. Add removal of finalizer for the MariaDBAccount to
   Database.DeleteFinalizer, which otherwise prevents the
   MariaDBDatabase / MariaDBAccount from being deleted.

[gibizer]

Thanks @zzzeek this makes openstack-k8s-operators/nova-operator#660 green for me.

[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

[booxter]

if username != "", then we'll look for a "databaseUser"-named account. But the code above creates an account that is named as the database. Which makes it so that if username is passed, then getDBWithName will always fail to find the account.

Thought?

[zzzeek]

that's correct. This legacy logic is relying on the current convention that services all name their DatabaseUser the same name as their database name. They have to do that because the mariadb "CREATE DATABASE / CREATE USER" commands that are run have been hardcoded to use the same name for the username as for the database. This guarantees that this convention is already in place for operators still using the legacy pattern, since otherwise, if they didnt match their database username to the database name itself, they would not be able to connect to the mariadb database at all. if we have observed failures in legacy code due to this change I would need to take a look.

This is not the only legacy/hardcoded thing that we face right now, the mariadb-operator is also hardcoded to use the DatabasePassword field from a given secret, which is what it uses in osp-secret when that's given. Individual operators then consume their database passwords from their own keys like GlanceDatabasePassword, KeystoneDatabasePassword, etc., but these aren't used. things "work" because all the keys in osp-secret have the same password 12345678 right now. if we put distinct passwords into osp-secret, then the services would not be able to connect.

The changes I'm doing to fix this can be seen in #184 where I am now making downstream operator PRs such as openstack-k8s-operators/keystone-operator#371 to use MariaDBAccount explicitly, consuming username and password from that.

[zzzeek]

note this also implies we can remove all the XYZDatabasePassword fields from osp-secret once that's done.

[booxter]

if we have observed failures in legacy code due to this change I would need to take a look.

The way this breaks neutron functional tests is:

• NewDatabase() is called with databaseUser=neutron. The function initializes database.name = "".
• CreateOrPatch() then resets database.name = neutronApi.name (which is neutron+UUID).
• It then creates database called neutronApi.name.
• It also creates account called neutronApi.name. (because of this change)
• Then getDBWithName is called at the end of CreateOrPatch routine.
• Since databaseUser=neutron (!=""), it attempts to find account named "neutron" (not neutron+UUID).

Once I switch to NewDatabaseWithNamespace(), as @gibizer suggested, it works fine (because now both database and account names are hardcoded to "neutron".) This is good enough for neutron, unless this hardcoding will also go away eventually (?).

But while NewDatabaseWithNamespace unblocks neutron adoption of the latest mariadb-operator libs, it still seems wrong to me that NewDatabase with databaseUser!="" won't ever produce a database that would be ready (as defined by WaitForDb routine.)

But, I think Neutron will adopt the WithNamespace flavor of the NewDatabase function. Technically this is not blocking neutron.

There may be some more details in the corresponding neutron-operator PR.

[zzzeek]

This is good enough for neutron, unless this hardcoding will also go away eventually (?).

I'm working to provide PRs to change all of these ASAP since what we have now is not sustainable, at least due to the osp-secret issue mentioned previously if not also the brittleness around instance names.

if we are not optimistic about the chances for #184 getting merged soon then I would say we revert the entire implicit generation of MariaDBAccount here if it was not specified.

[zzzeek]

cc @gibizer if you want to comment here....I think overall, my earlier patch to create MariaDBAccounts implicitly was probably not needed, we could have gone straight to what I'm doing in #184. however, having that change at least got us further with test suites and getting everyone generally annoyed and eager for me to fix what I broke :)

[zzzeek]

yeah I see now that the call to getDBWithName inside of CreateOrPatchDBByName is where this goes wrong. I think if I wanted to fix this right now before we move to #184 , I'd add an explicit flag inside of Database{} called "legacy", and have the functions that all the current controllers call all set up this flag. when that is set, all commands would assume a fixed mariadbdatabase/mariadbaccount of the same name where mariadbaccount username also matches mariadbdatabase database name. Only if controllers call new functions that I would add, does Database get the "non legacy" flag set up.

Or even have a new facade for Database completely called DatabaseAccount that comes in with the new functions.

I can actually do this and then have #184 's new functions be the ones that are "non legacy".

I might like to try making the new API based on DatabaseAccount and the old API fixed to Database. I'll see how that feels tomorrow

[gibizer]

We merged the "hardcoding" fix in neutron-operator. I think it is better to focus on #184 and the related service operator changes than to fix the legacy path.

[zzzeek]

OK great. I am halfway through nova which is gigantic. I'm hoping that after that I can likely whiz through the rest of them as nova is much more complicated