-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: master
Are you sure you want to change the base?
Support MySQL 8 #72
Conversation
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 |
@bcardiff thanks for the reply! :) my mysql root user has no password, the problem happens when use
MySQL driver does not play well with MySQL 8 when using no password, so I got this error:
For the 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After double checking https://dev.mysql.com/doc/internals/en/determining-authentication-method.html it seems we are fine removing this and force Secure Password Authentication instead of Old Password Authentication.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 😃
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
-
https://github.com/go-sql-driver/mysql/blob/6be42e0ff99645d7d9626d779001a46e39c5f280/auth.go#L242
-
from Fix MySQL 8.0.x incompatibilities mysqljs/mysql#1962 it seems there are some differences between 8.0.3 and 8.0.4
The handshake process remains unchanged when connecting to any server with version lower or equal to 8.0.3. Whereas for 8.0.4 or above, the process is now the following:
There was a problem hiding this comment.
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 😉
Thankfully now GH has the Draft Pull Request option, so no need to explicit it here 😉
Some premises:
spec_helper
to accept a passworddriver_spec
to make the test more robust, with someDROP IF EXISTS
secret
is considered a weak password on MySQL 8, I set the 3 globals to allowsecret
as a password for the userGRANT
call, so I split in two, firstCREATE USER
thenGRANT
The
workaroundfix for #62 is the removal onsrc/mysql/packets
; not sure about the (likely) bad impact on MySQL 5 versionsHere on my machine, to run the tests, I created the user on DB
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? 😄