Skip to content
This repository has been archived by the owner on Jan 12, 2021. It is now read-only.

Adding authentication plugin support, mainly for AWSAuthenticationPlu… #26

Merged
merged 4 commits into from
Mar 12, 2018

Conversation

keith-miller
Copy link
Contributor

…gin use with Aurora in RDS, issue #16175

@apparentlymart
Copy link
Contributor

Thanks for the continued work on this, @doyouevensunbro.

For some reason the Travis-CI build didn't trigger for this PR, and I'm not sure why... for this provider in particular we actually run the acceptance tests in Travis using the Travis-provided MySQL service, so we might need to tweak the settings for that in order to enable the test auth plugin. When I get a moment I'll get up to speed with how the Travis-CI MySQL service works and see what we need to do here.

@keith-miller
Copy link
Contributor Author

Instead of using the test plugin, I used the mysql_no_login plugin which should come with base MySQL installations. That said, you do have to run the following to enable it:

INSTALL PLUGIN mysql_no_login SONAME 'mysql_no_login.so';

Hope that helps.

@cliles
Copy link

cliles commented Jan 10, 2018

@apparentlymart - Any thoughts? This is an excellent feature to the provider.

@keith-miller
Copy link
Contributor Author

Just let me know if I need to do anything else.

@apparentlymart
Copy link
Contributor

Hi again, @doyouevensunbro!

I just wanted to check in and say that I've not forgotten about this but unfortunately I've not had an opportunity to properly look at this yet due to us being deep in a planning and design phase for Terraform Core. I do appreciate your work on this, and we'll get it reviewed and merged as soon as we can.

Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

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

Hey @doyouevensunbro, sorry for the really late review here, but I have some changes that I'd like to see before we merge this. If you can look into these that would be great, otherwise we'd be happy to make the changes as we merge!

Thanks!

"password": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ConflictsWith: []string{"plaintext_password"},
Sensitive: true,
Deprecated: "Please use plaintext_password instead",
},

"auth": &schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be modified to auth_plugin, and make to be a flat TypeString value? I'm not too sure if we will ever be putting anything else in here - grants are already handled by mysql_grant, and we already have separate fields for plaintext passwords. So having the extra layer here is not entirely useful as it stands right now.

The field should also be made to conflict with all other authentication fields (including password as well, even though it's deprecated, it can still be used).

@@ -26,6 +26,16 @@ resource "mysql_user" "jdoe" {
}
```

```hcl
Copy link
Contributor

Choose a reason for hiding this comment

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

Could a heading be added here denoting that this example is a an example using an authentication plugin?

@keith-miller
Copy link
Contributor Author

Sure thing. Just got back into town and will make those changes.

@keith-miller
Copy link
Contributor Author

Fixing...

@keith-miller
Copy link
Contributor Author

For the tests to complete, the Travis CI needs to have the following run in MySQL:

INSTALL PLUGIN mysql_no_login SONAME 'mysql_no_login.so';

@apparentlymart apparentlymart removed their request for review February 16, 2018 19:24
@vancluever
Copy link
Contributor

Hey @doyouevensunbro, looks like there's still this comment to address. Once this is done I'll do another check and work on merging.

Thanks!

…d the docs to reflect the new auth changes.
@vancluever
Copy link
Contributor

Hey @doyouevensunbro, thanks for the updates - I want to make a couple of updates to the docs and fix Travis so all tests run but I can do that in master. Thanks for your work!

@vancluever vancluever closed this Mar 12, 2018
@vancluever vancluever reopened this Mar 12, 2018
@vancluever vancluever merged commit 51c5bcb into hashicorp:master Mar 12, 2018
@keith-miller
Copy link
Contributor Author

No problem!

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

Successfully merging this pull request may close these issues.

4 participants