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

Loading credentials from env_file parameter #107

Closed
wants to merge 8 commits into from

Conversation

ndajr
Copy link
Contributor

@ndajr ndajr commented Nov 25, 2019

Use Cases

  • As a developer, I would like to have a way to ignore assumeRole authentication if I already have the credentials from another custom plugin. Now the behaviour is always run the assumeRole function.

  • The single responsibility of drone-terraform should be running terraform as a drone plugin not authenticating behind the scenes. I want to pass the credentials somehow to drone-terraform plugin.

  • Due the limitations on the host machine, I can't store the ~/.aws/credentials file and without the admin keys the drone-terraform will fail. This is a real case with Cloud Drone because we can't store files there, the only way to run the drone-terraform is reading the credentials from a temporary file created by another custom plugin. Without this PR is not possible to run the drone-terraform on Cloud Drone.

  • Due security reasons, I don't want to pass the Admin AWS tokens for drone plugins, I want to use my custom plugin to generate the session tokens and load the .env file on drone-terraform with the temporary credentials.

Solutions

Actually I needed to change 2 things:

  1. add the env_file as a plugin param
  2. skip the assumeRole if the aws tokens are already set (by the load env)

I also introduced a breaking change by changing CLI param env-file to env_file

I did it mainly for 3 reasons:

  • Make it consistent with other params that uses underscore. There is no parameter with dash actually;
  • No one using the plugin will be impacted, I assume most users will not be impacted;
  • Since the property was introduced two years ago and not implemented as plugin parameter, is not a feature with so much demand since then, the impact should not be big for most users.

@jmccann does that make sense to you? I'm happy to discuss and also revert that breaking change if you have a concern.

Core Changes

  • Extending the CLI env-file to a plugin parameter
  • Skipping the assumeRole function if credentials have been created before

@jmccann jmccann self-requested a review November 25, 2019 18:09
@ndajr
Copy link
Contributor Author

ndajr commented Nov 26, 2019

Just in case you need a real example, this is a small project using my fork:
https://github.com/neemiasjnr/golang-microservice-example

Copy link
Owner

@jmccann jmccann left a comment

Choose a reason for hiding this comment

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

I had several questions and comments for you. But I do understand the use case you have and agree we should be able to setup auth "outside" of this plugin.

CHANGELOG.md Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
plugin.go Show resolved Hide resolved
plugin.go Show resolved Hide resolved
plugin.go Outdated Show resolved Hide resolved
@ndajr
Copy link
Contributor Author

ndajr commented Nov 26, 2019

@jmccann I did the changes you suggested, thank you for your comments, it was really helpful to me. Now the PR is more clean and focused in the core changes I mentioned. Please let me know if you see something else I can improve on this PR

Copy link
Owner

@jmccann jmccann left a comment

Choose a reason for hiding this comment

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

Sorry, I think I may have found another issue. Let me know your thoughts.

Thanks also for the updates. The PR is a lot easier for me to read now as a result! 💯

plugin.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
@caioquirino
Copy link
Contributor

Hello @jmccann, I hope you are starting well this year and had good resolutions for 2019 :)
Is there anything else we could do to help you with the code review for this PR and make it to work for a public/cloud drone pipeline? Please let us know :) Thank you.

Copy link
Owner

@jmccann jmccann left a comment

Choose a reason for hiding this comment

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

Code looks better, now I have some questions/comments around the testing ... sorry! 🙇

Also, sorry it took so long to take another look.

plugin_test.go Outdated Show resolved Hide resolved
plugin_test.go Outdated Show resolved Hide resolved
plugin_test.go Outdated Show resolved Hide resolved
plugin.go Show resolved Hide resolved
@ndajr
Copy link
Contributor Author

ndajr commented Feb 9, 2020

@jmccann Thank you for the comments, I really don't why I did what I did to test the credsSet function 😅. I simplified the plugin_test.go as you suggested and now I'm not using the Unmarshal function from godotenv anymore, although I still find it interesting to migrate to version 1.0+ (more stable), what do you think? I'm looking forward to ship it 🥳

@jmccann
Copy link
Owner

jmccann commented Feb 12, 2020

This has been "merged". I rebased from master locally and added some testing fixes and pushed to master. Thanks!

@jmccann jmccann closed this Feb 12, 2020
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.

3 participants