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 RDS IAM authentication for MySQL #186

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fortra-cloudops-platform

Description of your changes

Fixes #106, provides the ability to create MySQL users with the AWSAuthenticationPlugin

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Honestly I have had issues testing as I don't know how to get the provider to run my image successfully, any help appreciated.

@mcanevet
Copy link

mcanevet commented Jul 3, 2024

I was about to rebase #133, but it's a bit complicated because a lot of things have changed since that PR have been submitted.
So I tested that one and it works fine. Although, the default is mysql_native_password but this value throws errAuthPluginNotSupported error. Except form that it works fine.

@cten
Copy link

cten commented Jul 3, 2024

I found that setting the default to mysql_native_password fails, documentation makes me believe it should work but failed on the MySQL version I tried. I can try to replicate the changes from #133 on current master. I think those might be cleaner code. Only difference would be not setting a default plugin name.

@mcanevet
Copy link

mcanevet commented Jul 3, 2024

At least I can confirm that this PR works fine. I'm using it right now.
Thanks a lot for this work!

@pierluigilenoci
Copy link

@chlunde @Duologic, could someone from you review the PR?

if pw == "" {
pw, err = password.Generate()
switch authplugin {
case "":
Copy link
Contributor

Choose a reason for hiding this comment

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

default:"mysql_native_password"

is not handled here?

// AuthPlugin defines the MySQL auth plugin (ie. AWSAuthenticationPlugin for AWS IAM authentication when using AWS RDS )
// See https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.DBAccounts.html
// +optional
AuthPlugin string `json:"authPlugin,omitempty" default:"mysql_native_password"`
Copy link
Contributor

Choose a reason for hiding this comment

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

since the code only accepts specific values here, I think you should use an enum to limit to the valid types. Alternatively, let the code handle any value, if that possible

@chlunde
Copy link
Contributor

chlunde commented Aug 14, 2024

I'm not a maintainer of this project (and not a MySQL user), but as you pinged me I left some comments :)

A couple more:

  1. this code needs unit tests so we cover the new branches

  2. It might be cleaner if you keep the query building part inside executeCreateUserQuery instead of partially building it outside, because now we get SQL quoting/escaping in different layers of the code

@pierluigilenoci
Copy link

@fortra-cloudops-platform, can you check/react to PR comments?

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.

Support RDS IAM authentication
5 participants