MySQL storage - Take 2 #1485

bonifaido · 2019-07-13T12:28:53Z

This PR tries to continue where #924 dropped off, namely on the work of @pborzenkov, I have changed the following stuff:

rebased his branch on the current master
compile-time issues and tests fixed
Travis based MySQL conformance test enabled (I have also changed to Xenial, which is a good starting point IMHO, but if you are not ok with it I change to MySQL 5.7 the other way)
added basic documentation for MySQL storage

Tested with MySQL 5.7 and 8.0.

I know that maintaining a storage backend takes a lot of time, I can offer my help (becoming a maintainer if needed/acceptable).

srenatus

Thanks for your contribution, and thanks for picking up where this was left off. (Also thanks to @pborzenkov)

I know that maintaining a storage backend takes a lot of time, I can offer my help (becoming a maintainer if needed/acceptable).

I think that might be a good idea -- @JoelSpeed, what do you think?

srenatus · 2019-07-18T13:55:49Z

storage/sql/config_test.go

+}
+
+func testDB(t *testing.T, o opener, withTransactions bool) {
+	// t.Fatal has a bad habbit of not actually printing the error


IIRC the problem is the timeout. If a test times out, the t.Fatal(...) output will be lost. If it doesn't timeout, you'll see it just fine.

Should I just change it back? TBH I haven't had issues with Fatal.

It's fine, we can revisit that some other time 👍

srenatus · 2019-07-18T14:00:10Z

storage/sql/migrate.go

@@ -61,14 +63,14 @@ func (c *conn) migrate() (int, error) {
 }

 type migration struct {
-	stmt string
+	stmts []string


Why do we need this? I haven't dug into this code in a while, but I'd think we could wrap stuff in BEGIN; ... COMMIT;, maybe? What's wrong with the ; concatenation? 🤔I guess it's difficult to spot what's wrong; transactions-wise, I think it should not matter, should it?

The issue here is that MySQL driver by default doesn't support multiple statements in one execution, in my original implementation I have used https://github.com/go-sql-driver/mysql#multistatements and this change is not needed. If you think that using that is better I can use that version.

srenatus · 2019-07-18T14:02:08Z

storage/sql/migrate.go

@@ -77,8 +79,8 @@ var migrations = []migration{
 				public boolean not null,
 				name text not null,
 				logo_url text not null
-			);
-
+			);`,


[nit] If we do this as multiple calls to tx.Exec, we don't need the ending ;, I think.

srenatus · 2019-07-18T14:02:49Z

storage/sql/migrate.go

@@ -38,8 +38,10 @@ func (c *conn) migrate() (int, error) {



Correct me if I'm wrong, but if this change has nothing to do with the mysql backend, I'd rather split it off... 😃

This is because of the multistatement stuff :)

srenatus · 2019-07-18T14:03:13Z

.travis.yml

 matrix:
  include:
    - go: '1.12.x'

 env:
-  global: DEX_POSTGRES_DATABASE=postgres DEX_POSTGRES_USER=postgres DEX_POSTGRES_HOST="localhost" DEX_ETCD_ENDPOINTS=http://localhost:2379 DEX_LDAP_TESTS=1 DEBIAN_FRONTEND=noninteractive DEX_KEYSTONE_URL=http://localhost:5000 DEX_KEYSTONE_ADMIN_URL=http://localhost:35357 DEX_KEYSTONE_ADMIN_USER=demo DEX_KEYSTONE_ADMIN_PASS=DEMO_PASS
+  global:


👍 It's about time this happens.

bonifaido · 2019-07-18T17:16:23Z

The Postgres backend uses strict SSL verification by default, should this backend have the same semantics?

srenatus · 2019-07-22T09:26:19Z

The Postgres backend uses strict SSL verification by default, should this backend have the same semantics?

I think that would be fair. 🤔

bonifaido · 2019-07-23T10:52:41Z

I made MySQL tls=true default, which the strictest available option for the MySQL Go driver.

srenatus · 2019-07-23T11:12:21Z

@bonifaido would you mind squashing these? maybe one commit per author?

It will be shared by both Postgres and MySQL configs. Signed-off-by: Pavel Borzenkov <[email protected]>

bonifaido · 2019-07-23T12:32:29Z

Rebase done! 👍

srenatus

Thank you, @pborzenkov and @bonifaido! 🎉 👏

MySQL storage - Take 2

forsberg · 2019-09-19T11:00:12Z

Question: Is there anything that specifically requires MySQL 5.7, or would 5.6 work?

I'd like to run it against an Amazon Aurora Serverless database, and they claim they are compatible with MySQL 5.6.

bonifaido · 2019-09-23T06:47:26Z

Hi @forsberg, the only issue that comes into my mind is the transaction_isolation system variable
renaming issue (I just recompiled the code with tx_isolation and it works fine on MySQL 5.6):

https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_transaction_isolation

Note
transaction_isolation was added in MySQL 5.7.20 as an alias for tx_isolation, which is now deprecated and is removed in MySQL 8.0. Applications should be adjusted to use transaction_isolation in preference to tx_isolation.

So we need to add some kind of logic to detect MySQL 5.6 and use tx_isolation instead of transaction_isolation, or add it explicitly in the config. I'm checking what is possible right now with the driver.

But, I just checked and AWS Aurora supports 5.7 today: https://aws.amazon.com/about-aws/whats-new/2018/02/amazon-aurora-is-compatible-with-mysql-5-7/

It would be nice to run some tests on Aurora first since they say that SERIALIZABLE isolation level is not supported, but I have no idea if it is or not, and that is a hard requirement by Dex currently, please see: #1370 (comment)

bonifaido · 2019-09-23T07:13:41Z

Okay, I just created an AWS Aurora instance for a quick check with MySQL 5.7 (Aurora version of the db was 2.04.6) and the transaction_isolation variable is still unavailable, probably this version is not MySQL 5.7.20.

bonifaido · 2019-09-23T07:46:07Z

#1550

forsberg · 2019-09-23T18:29:45Z

AWS Aurora does indeed support what they call 5.7, but AWS Aurora Serverless (where you don't have to run instances yourself) only support 5.6. This is pretty hard to find in the documentation, but is documented in the FAQ.

srenatus reviewed Jul 18, 2019

View reviewed changes

srenatus mentioned this pull request Jul 23, 2019

MAINTAINERS: add @bonifaido #1492

Merged

pborzenkov and others added 2 commits July 23, 2019 14:26

storage/sql: initial MySQL storage implementation

e53bdfa

It will be shared by both Postgres and MySQL configs. Signed-off-by: Pavel Borzenkov <[email protected]>

storage/sql: rework of the original MySQL PR

a572ad8

srenatus approved these changes Jul 23, 2019

View reviewed changes

srenatus merged commit bc27a61 into dexidp:master Jul 23, 2019

bonifaido deleted the mysql-storage branch July 23, 2019 13:25

srenatus mentioned this pull request Jul 23, 2019

Add MySQL support for storage #200

Closed

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019

Merge pull request dexidp#1485 from bonifaido/mysql-storage

f1d9e24

MySQL storage - Take 2

bonifaido self-assigned this Sep 23, 2019

bonifaido mentioned this pull request Sep 23, 2019

storage/mysql: support pre-5.7.20 instances with tx_isolation only #1550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MySQL storage - Take 2 #1485

MySQL storage - Take 2 #1485

bonifaido commented Jul 13, 2019 •

edited

Loading

srenatus left a comment

srenatus Jul 18, 2019

bonifaido Jul 18, 2019

srenatus Jul 23, 2019

srenatus Jul 18, 2019

bonifaido Jul 18, 2019 •

edited

Loading

srenatus Jul 18, 2019

srenatus Jul 18, 2019

bonifaido Jul 18, 2019

srenatus Jul 18, 2019

bonifaido commented Jul 18, 2019

srenatus commented Jul 22, 2019

bonifaido commented Jul 23, 2019

srenatus commented Jul 23, 2019

bonifaido commented Jul 23, 2019 •

edited

Loading

srenatus left a comment

forsberg commented Sep 19, 2019

bonifaido commented Sep 23, 2019 •

edited

Loading

bonifaido commented Sep 23, 2019

bonifaido commented Sep 23, 2019

forsberg commented Sep 23, 2019

MySQL storage - Take 2 #1485

MySQL storage - Take 2 #1485

Conversation

bonifaido commented Jul 13, 2019 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonifaido Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonifaido commented Jul 18, 2019

srenatus commented Jul 22, 2019

bonifaido commented Jul 23, 2019

srenatus commented Jul 23, 2019

bonifaido commented Jul 23, 2019 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

forsberg commented Sep 19, 2019

bonifaido commented Sep 23, 2019 • edited Loading

bonifaido commented Sep 23, 2019

bonifaido commented Sep 23, 2019

forsberg commented Sep 23, 2019

bonifaido commented Jul 13, 2019 •

edited

Loading

bonifaido Jul 18, 2019 •

edited

Loading

bonifaido commented Jul 23, 2019 •

edited

Loading

bonifaido commented Sep 23, 2019 •

edited

Loading