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

MySQLContainer can't start the 8 MySQL version. #1168

Closed
wants to merge 2 commits into from

Conversation

DavaOps
Copy link

@DavaOps DavaOps commented Jan 20, 2019

Fix suggestion for issue #736.

mysql 8 container needs "mysqld --default-authentication-plugin=mysql_native_password" command to start
@guss77
Copy link
Contributor

guss77 commented Jan 21, 2019

This fix is supposed to address the problem of using a "tc" JDBC URL to start and connect to a test containers MySQL server.

When using the following JDBC URL, everything works fine: jdbc:tc:mysql://host/database, as testcontainers will use the default MySQL version in MySQLContainer that is set at 5.7.22 (which we probably want to update to 5.7.25 anyway).

But if the user wants to use MySQL 8, you'd expect them to use the following JDBC URL: jdbc:tc:mysql:8.0.13://host/database and this would not work because starting with MySQL 8.0, the default authentication mode for the database has changed from mysql_native_password to caching_sha2_password, and MySQL Connector/J that MySQLContainer uses to test that the container is up, does not support this authentication mode (at least before version 8.0.9). As a result the MySQLContainer fails to verify that the container is up and fails the connection.

This is all documented pretty well in issue #736 .

This PR attempts to fix the problem by modifying MySQLContainerProvider - which is responsible for creating a MySQLContainer for the testcontainers JDBC proxy driver ContainerDatabaseDriver - to detect if the user wants an 8-class (and supposedly 9 and above, if that would ever happen) database server and if so make sure that the container modifies the mysqld command line to use the old mysql_native_password authentication mechanism supported by all MySQL JDBC drivers. This basically automates the process suggested in issue #736 so that it works well in situation where the user cannot interact directly with MySQLContainer, such as jOOQ code generator configuration.

Another approach to solve issue #736, which I think would be better in the long run is to modify MySQLContainer to detect the MySQL server image version (instead of the container provider), and if an 8-class version is detected, check the version of the JDBC driver - if it is not compatible with caching_sha2_password then just fail the container immediately with an appropriate error message, but if the JDBC driver is compatible - configure the JDBC connection test URL to use the correct authentication mechanism.

@DavaOps DavaOps changed the title Patch 1 MySQLContainer can't start the 8 MySQL version. Jan 21, 2019
@rnorth
Copy link
Member

rnorth commented Jan 29, 2019

Sorry for not responding on this one yet, but I would like to spend a bit of time exploring whether there are other options. Relying on the image tag name to detect whether the v8-specific workaround needs to be applied seems a bit risky given that custom images can be used.

One interesting area I'm currently looking into is that it appears default-authentication-plugin=mysql_native_password is a setting that is usable on many versions of MySQL, going back to at least 5.7. I'm wondering if we could set that universally and rely on it only having an effect on v8+ instances.

@guss77
Copy link
Contributor

guss77 commented Jan 30, 2019

Could be a useful change - at least it will stop "out of the box" issues with MySQL 8.

Ideally, I would like to see MySQLContainer updated to use the correct MySQL Connector/J driver version (with the correct class name) and authentication plugin for MySQL 8.

Maybe the best solution is to fork the MySQLContainer implementation to MySQLContainer8 that supports only version 8.0 and above, and leave MySQLContainer to only support version 5. Custom image uses may then choose what they want to override.

@stale
Copy link

stale bot commented Apr 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Apr 30, 2019
@guss77
Copy link
Contributor

guss77 commented May 1, 2019

@rnorth - any updates regarding this issue?

@stale stale bot removed the stale label May 1, 2019
@rnorth
Copy link
Member

rnorth commented May 7, 2019

Sorry @guss77 - I was contemplating your previous comment and then got sidetracked at some point. I apologise.

Maybe the best solution is to fork the MySQLContainer implementation to MySQLContainer8 that supports only version 8.0 and above, and leave MySQLContainer to only support version 5. Custom image uses may then choose what they want to override.

I think this sounds like the best solution to be honest. Naming is a silly but hard thing here, as I'd not really want to end up with MySqlContainer9 etc in the future. I honestly can't thing of anything better than just the number, yet though.

I think I'd prefer to keep Container at the end of the name though (i.e. MySql8Container or similar).

@bsideup
Copy link
Member

bsideup commented May 7, 2019

I honestly can't thing of anything better than just the number, yet though.

Maybe we should change --default-authentication-plugin by default (MySQL 6+), and, for MySQL 5 users, document how to disable it?

@bsideup
Copy link
Member

bsideup commented May 7, 2019

Or we could run mysqld --version first, parse it and then make a decision

@guss77
Copy link
Contributor

guss77 commented May 7, 2019

@bsideup - the suggested patch basically does the version check but uses the image tag name to determine the version - which I believe to be as safe and much faster than running mysqld --version.

That being said, if using a new class is the fastest way forward, then lets do that @DavaOps

@bsideup
Copy link
Member

bsideup commented May 7, 2019

@guss77 the patch does not cover new MySQLContainer(), which is IMO critical to have if we aim to fix it. And we can't parse the image in MySQLContainer's constructor because the image can be anything, including my.cool.org/registry/mysql-custom:latest

@rnorth
Copy link
Member

rnorth commented May 7, 2019

Huh, this is interesting!

Did anyone consider (and rule out) approaching this from the driver angle?

i.e. If you want to use a mysql:8 image, ensure that you have v8 of the driver on the classpath?

Testcontainers has the driver as a runtime dependency, not transitive, so it's completely up to the user to choose which driver they need.

I've just tried this out, modifying our jdbc tests to use the newer driver, and found a few things:

  • The v8 driver seems to be backwards compatible (as you'd hope!) with 5.5, 5.6, 5.7.
  • With the v8 driver, MySqlContainer still chokes when connecting to mysql:8, throwing SQLNonTransientConnectionException: Public Key Retrieval is not allowed
  • However, this can be fixed by adding allowPublicKeyRetrieval=true to the query string params, as we already do with useSSL=false.
  • The allowPublicKeyRetrieval option goes back as far as MySql 5.1, (which predates Docker Hub) and seems to be compatible with any pre-8 image that we'd want to test.

Thus, I now have the following test code working without problems:

for (String tag : asList("5.5", "5.6", "5.7", "8")) {
    try (final MySQLContainer container = new MySQLContainer<>("mysql:" + tag)) {
         container.start();
    }
}

Have I missed some huge problem with this approach 🤔 😂 ?

@kiview
Copy link
Member

kiview commented May 8, 2019

But you also changed MySQLContainer to include allowPublicKeyRetrieval=true as query parameter by default?

@rnorth
Copy link
Member

rnorth commented May 8, 2019

But you also changed MySQLContainer to include allowPublicKeyRetrieval=true as query parameter by default?

@kiview yeah, sorry if that wasn’t clear.

That works with all relevant versions of MySQL.

Sent with GitHawk

@bsideup
Copy link
Member

bsideup commented May 8, 2019

@rnorth nice! I would say 5.1+ is a reasonable baseline 👍

@rnorth rnorth mentioned this pull request May 15, 2019
@rnorth
Copy link
Member

rnorth commented May 15, 2019

I'm going to close this ticket in deference to #1470, which uses the approach I outlined above.

Thanks @DavaOps for pushing this forward. Sorry that we're not able to use your hard work, but I hope the alternative solution resolves the problem in a simpler way.

@rnorth rnorth closed this May 15, 2019
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.

5 participants