p2-inc / keycloak-orgs

Single realm, multi-tenancy for SaaS apps
https://phasetwo.io
Other
389 stars 66 forks source link

Add support for MySQL #41

Closed phamann closed 1 year ago

phamann commented 1 year ago

Fixes: #42

This PR adds MySQL support by refactoring the existing database migrations to add MySQL-specific changesets. This has been tested on MySQL 5.7 and 8 (the two most prominent releases).

MySQL is generally more strict when it comes to altering or dropping columns with existing foreign key and uniqueness constraints attached to them; therefore, you need to be more explicit and also drop the constraints before your operation. Postgres is more "intelligent" when it comes to this and knows when to cascade FK relationships down and thus does this for you implicitly, hence why the migrations in their current form work.

This also follows the same pattern used by Keycloak core, an example of which can be found here. However, while these changes are considered "safe", I opted not to impact the existing migrations and wrapped any mysql-specfic operations in their own chanegSet's guarded with preCondition's, much like you've already done for cockroach in a few places.

Let me know if you want anything changed, and once again, thank you for the great extension :)


For further context, here's a couple of example errors thrown by mysql/liquibase without this changes:

Reason: liquibase.exception.DatabaseException: ALGORITHM=COPY is not supported. Reason: Columns participating in a foreign key are renamed. Try ALGORITHM=INPLACE. 
Reason: liquibase.exception.DatabaseException: Cannot drop index 'UK_S5FNFVSRU9I4QPH9GB8V4FXFM': needed in a foreign key constraint [Failed SQL: (1553) ALTER TABLE keycloak.USER_ORGANIZATION_ROLE_MAPPING DROP KEY UK_S5FNFVSRU9I4QPH9GB8V4FXFM]
xgp commented 1 year ago

Thanks for the great PR! We will need to test these against existing Postgres installations to make sure the preConditions are working properly. This might take a few days.

phamann commented 1 year ago

Hey @xgp, have you had a chance to test this out on your own internal builds yet? Let me know if there is anything else I can do to help. (sorry to chase)

xgp commented 1 year ago

@phamann We got delayed with updating to the current (21.0.1) Keycloak release. It should be by the weekend that we'll be able to test this. Apologies for the delay.

phamann commented 1 year ago

No worries, thanks for the update 👍🏻

xgp commented 1 year ago

Tested from docker with postgres, h2 crdb and mysql types. First with 0.9 release version of keycloak-orgs, then with patched version on same DB. Also from empty schema with patched version. Appears that the conditions work properly, don't change anything in postgres, h2 or crdb, only activate on mysql. Liquibase seems to not hash problems with altering old migration xml files.

xgp commented 1 year ago

@phamann This passed all the tests for us. As we roll this out to our fleet (all postgres and crdb), I'll post here if we find any problems. Please do the same if you are deploying with mysql.

jar should be in maven central shortly. https://repo1.maven.org/maven2/io/phasetwo/keycloak/keycloak-orgs/0.10/