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

dynamic aws credentials support #2135

Closed
wants to merge 3 commits into from
Closed

dynamic aws credentials support #2135

wants to merge 3 commits into from

Conversation

kush-patel-hs
Copy link

DO NOT MERGE. This is a proposal design and needs work.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Will watch for aws credential changes

Verification

If we are good with this design then we can work on verification

creds = credentials.NewChainCredentials(chain)

// We will watch for credential changes for non-static credentials
filename := os.Getenv("AWS_SHARED_CREDENTIALS_FILE")
Copy link
Author

@kush-patel-hs kush-patel-hs Feb 13, 2020

Choose a reason for hiding this comment

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

We should probably also get filename from config if we can. Then provide that to FileAWSCredentials. I really hate having to duplicate the FileAWSCredentials logic to determine filename here (I guess we would have to anyways if there's no filename provided in config).

}

// Launch the watch which will expire the credentials when fired
go func() {
Copy link
Author

Choose a reason for hiding this comment

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

We need to kill this go routine when thanos quit or dies. Is there a standard way thanos does this atm for other go routines?

Copy link
Member

@bwplotka bwplotka 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! It's indeed not that much of code, but I would love to focus on specifying first if this is what we want.

This only fixes AWS, so might introduce some snowflake. I think there is quite easy long term solution. Proposed here

@kush-patel-hs
Copy link
Author

kush-patel-hs commented Feb 14, 2020

Thanks for taking a look! For our use case it is what we want. What we do is we have our service. Then we have a "Vault Sidecar" attached to our service which refreshes AWS credentials every x minutes and will write the refreshed credentials (an ~/.aws/credentials file to a directory). So we wouldn't be changing the Thanos config when this happens and would be a lot of work for us to do. So we would rely on this else block https://github.com/thanos-io/thanos/pull/2135/files#diff-bef8b149473cb313205c67f945d249ceR131 and reloadable thanos config would not help unless we reload ~/.aws/credentials (or the file path in the env var) as well.

@stale
Copy link

stale bot commented Mar 15, 2020

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

@stale stale bot added the stale label Mar 15, 2020
@kush-patel-hs
Copy link
Author

closing

@kush-patel-hs kush-patel-hs deleted the dynamic-aws-credentials-support branch March 15, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants