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 for multiple secrets manager secrets #233

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

lucylura
Copy link
Contributor

Context

#227 added support for Clusters (@DrJosh9000), with agent registration tokens provided as a comma separated list.

The assumption for secrets manager secrets was that it would be a single secret with a comma separated list of values. This is not ideal, as it means a new secret needs to be created just for this lambda. If instead a comma separated list of secrets is used, we can keep the registration tokens in their own secrets which is referenced in multiple places.

Change

This PR supports multiple secrets provided to BUILDKITE_AGENT_SECRETS_MANAGER_SECRET_ID (comma-seperated) and queries secrets manager for each value.

Testing

I've built the lambda zip locally and tested on AWS with 2 agent registration tokens across 2 clusters.

@lucylura
Copy link
Contributor Author

hey @DrJosh9000 @triarius are you able to take a look at this?
seems I am unable to add reviewers

Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

Thanks for this @lucylura. I think the change for SSM parameters is fine as they cannot contain commas in their name, but it looks like secrets manager secrets can.

While I think there's a low risk of this causing an issue, I think we should eliminate it by using a new env var, like BUILDKITE_AGENT_SECRETS_MANAGER_SECRET_IDS and make it clear that they will be split by comma. We may as well make it symmetrical for SSM and create a BUILDKITE_AGENT_TOKEN_SSM_KEYS while we are at it. The old env vars should stay, which the old behaviour that they split the value by comma after looking up a single secret/parameter by name.

@@ -93,7 +93,8 @@ supported.
#### Option 3 - Retrieve token from AWS Secrets Manager

- `BUILDKITE_AGENT_SECRETS_MANAGER_SECRET_ID`: The id of the secret which
contains the token value in AWS Secrets Manager.
contains the token value in AWS Secrets Manager. You can supply
multiple secrets comma-separated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Give you're making this change to SSM as well, can we get it documented there too pls?

@triarius triarius self-requested a review November 27, 2023 03:36
Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

Huh, I misread it. 🤦‍♂️ LGTM ✅

@triarius triarius enabled auto-merge November 27, 2023 03:39
@triarius triarius merged commit c867087 into buildkite:master Nov 27, 2023
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