Skip to content
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

Support MySQL 8 #72

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

fernandes
Copy link
Contributor

Thankfully now GH has the Draft Pull Request option, so no need to explicit it here 😉

Some premises:

  • I couldn't manage the driver to connect without password on MySQL 8, so I had to change the spec_helper to accept a password
  • I changed the driver_spec to make the test more robust, with some DROP IF EXISTS
  • As secret is considered a weak password on MySQL 8, I set the 3 globals to allow secret as a password for the user
  • MySQL 8 does not allow creating a user on GRANT call, so I split in two, first CREATE USER then GRANT

The workaround fix for #62 is the removal on src/mysql/packets; not sure about the (likely) bad impact on MySQL 5 versions

Here on my machine, to run the tests, I created the user on DB

CREATE USER 'crystal_mysql_spec'@'localhost' IDENTIFIED WITH mysql_native_password BY 'crystal_mysql_spec';
GRANT ALL PRIVILEGES ON *.* TO 'crystal_mysql_spec'@'localhost' WITH GRANT OPTION;
FLUSH PRIVILEGES;

then

DATABASE_USER="crystal_mysql_spec" crystal spec

So yeah, here is what I could manage to do, any idea where we can go next? 😄

@bcardiff
Copy link
Member

bcardiff commented Mar 2, 2019

I would not drop the empty password support. Maybe the default installation of MySQL 8 has non-empty root password and can be tweaked (https://medium.com/@benmorel/remove-the-mysql-root-password-ba3fcbe29870, https://stackoverflow.com/questions/48824572/mysql-8-0-unable-to-reset-root-password).

Please update the travis.yml to include MySQL 8.

If "secret" bothers as a password, use any other constant value and that's it. Let's avoid changing the rules. Not everybody will run an isolated db server.

@fernandes
Copy link
Contributor Author

@bcardiff thanks for the reply! :)

my mysql root user has no password, the problem happens when use crystal-mysql with the following connection URI:

mysql://root@#{database_host}/#{initial_db}

MySQL driver does not play well with MySQL 8 when using no password, so I got this error:

Unhandled exception: Client does not support authentication protocol requested by server; consider upgrading MySQL client (Exception) as reported on #56 , no idea how to solve here

For the secret, just used a "stronger" password, and MySQL was happy

ps: I'm using a default MySQL 8 install on OSX

@@ -65,8 +65,6 @@ module MySql::Protocol
def write(packet : MySql::WritePacket)
caps = CLIENT_PROTOCOL_41 | CLIENT_SECURE_CONNECTION | CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA

caps |= CLIENT_PLUGIN_AUTH if @password
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, thanks for the link, I was looking for something like this hahahha

After some research (aka stack overflow hahaha) I found this PR mysqljs/mysql#1962 that is adding support for MySQL 8, I was reading trying to figure out this auth mechanims, but as this is the very first time I'm dealing with db driver, and using mysql after a few years, I'm having a hard time 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is the main point of the PR... the other modifications are just some 💅 to fix the specs (like the password stuff)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are on the same page. This was the first binary protocol I implemented for a db driver. I mostly read the docs of mysql internals and compared other source code.

Is hard to have in mind all the evolution of the protocol and even harders try all the possibilities. But the end result is worth it 🚀.

After adding mysql 8 to travis and whenever you feel the PR ready let us know 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are on the same page. This was the first binary protocol I implemented for a db driver. I mostly read the docs of mysql internals and compared other source code.

awesome!! :D

After adding mysql 8 to travis and whenever you feel the PR ready let us know 😃

Sorry but I think I missed something, what about the password for mysql 5.6 and 5.7? Should we remove both versions from travis, or add password to them? hehehe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None 🙈. I would expect that mysql 8 with a blank root password is added.

I think is common for developers to have root with empty passwords for mysql intallations. I would not drop the support for that.

I've just found a bit more on information regarding the new auth mechanism in mysql 8: caching_sha2_password

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahahah sure!! that's why I asked, I misunderstood... now I need to figure out how to enable this support in mysql 8

How do you think we should handle this in the design of the driver, leave the current HandshakeResponse41 and use other classes (that inherits from this one), based on protocol_version / version? Because do some "ifs" on the #write method will fastly become a monster hahah

anyway, I'm gonna draft something here when I figure out how to do this and commit here 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants