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

Add support for AWS SecretsManager as BK token provider #98

Merged
merged 3 commits into from
Oct 18, 2019
Merged

Add support for AWS SecretsManager as BK token provider #98

merged 3 commits into from
Oct 18, 2019

Conversation

alloveras
Copy link
Contributor

Intent

To allow retrieving the BK API token from AWS Secrets Manager.

Problem

The current approach is not flexible enough if we want to keep adding different methods to retrieve the BK API token.

Solution

To represent the behaviour of retrieving a BK API token using an interface and provide particular implementations to all the sources we currently support plus a new one for AWS SecretsManager.

@lox
Copy link
Contributor

lox commented Oct 17, 2019

Thanks for this @alloveras! This looks great. Looks like the token package would make for a great candidate for extraction into a library, but let's get this in place for now and we can consider next steps.

@alloveras
Copy link
Contributor Author

Thank you @lox . I absolutely agree about extracting it into a library but I can definitely do that in a separate PR if you guys find it useful 😄

@lox
Copy link
Contributor

lox commented Oct 18, 2019

This should be backwards compatible, right?

@alloveras
Copy link
Contributor Author

alloveras commented Oct 18, 2019

This should be backwards compatible, right?

I am pretty sure it is given that all the new code is behind new environment variables so they are opt-in features.

The only existing code that I slightly changed is the ssm.go file which I re-arranged and refactored a bit. However, its behaviour has remained unchanged if I am not mistaken.

@lox What do you think ?

Comment on lines +158 to +160
if err := checkMutuallyExclusiveEnvVars(mutuallyExclusiveEnvVars...); err != nil {
return nil, err
}
Copy link
Contributor Author

@alloveras alloveras Oct 18, 2019

Choose a reason for hiding this comment

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

This is maybe a non-backwards compatible part of the code. This is because, if someone is using previous versions and setting both BUILDKITE_AGENT_TOKEN and BUILDKITE_AGENT_TOKEN_SSM_KEY they will now get an error whereas before SSM will take precedence.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds to me like fixing a bug! :)

@lox
Copy link
Contributor

lox commented Oct 18, 2019

I reckon let's move the README.md from the token dir into a section of the main README so folks know how to use it, then let's merge this.

@alloveras
Copy link
Contributor Author

Sure, will do that ASAP and submit a new revision :D

@alloveras alloveras requested a review from lox October 18, 2019 03:38
@lox lox merged commit 719620d into buildkite:master Oct 18, 2019
@lox
Copy link
Contributor

lox commented Oct 18, 2019

Thanks for the contribution @alloveras!

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.

2 participants