-
-
Notifications
You must be signed in to change notification settings - Fork 957
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 AssumeRoleWithWebIdentity #2997
base: master
Are you sure you want to change the base?
Add support for AssumeRoleWithWebIdentity #2997
Conversation
FYI: I also tested this using gitlab's OIDC integration and it worked correctly, both with |
a6dae29
to
1363712
Compare
Sorry, I have no idea how this got closed. I certainly did not intend to. |
|
Thanks for the review! I have replaced calls to |
@denis256 any chance I could get a followup review? I addressed the comments you left previously. Thanks in advance! |
Hi,
|
8c7c168
to
36dd31a
Compare
Hi @denis256 thanks for picking this up again. I have resolved the merge conflicts (and updated the new flag name per the new naming convention). Please take another look. |
36dd31a
to
4d69332
Compare
27501fd
to
3c89638
Compare
Synced this branch against master again, resolving a small merge conflict. Hey @denis256 I would really appreciate a followup review so I don't have to keep checking syncing this to avoid merge conflicts. Please and thank you! |
aws_helper/config.go
Outdated
return nil, errors.WithStackTrace(err) | ||
} | ||
return resp.Credentials, nil | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think else is not required since return
is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I have removed the unnecessary else clause
It is possible to implement integration tests to track that this functionality will work over time? |
Yes, depending on your CI environment. For example, we will be using this functionality to replace shell script calls to Do you have integration tests for the current In order to do this, you need to
Then point Here are the docs for setting this up in gitlab: https://docs.gitlab.com/ee/ci/cloud_services/aws/ Here are the docs for setting this up in github: https://docs.github.com/en/actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-amazon-web-services Here are the docs for setting this up in circleci: https://circleci.com/docs/openid-connect-tokens/ |
Hello, In CircleCI we can add later required env variable. |
To be clear, @partcyborg , we don't need these tests to be running in our CircleCI configuration for this pull request. This integration test only has to work in the context of your CI system. Run it there, show the configurations you used (masking any account identifiers, etc), and share the test output with us so that we can consider this functionality tested. They should require the minimum set of dependencies like an environment variable and an IAM role. It should also be accompanied with example CI setups in the docs that we can share with the community. Once we have these, we can independently verify the tests, then merge this PR. In a subsequent PR, we can setup the integration tests into the CI configurations for this repository. |
Thanks for the more detailed explanation @yhakbar Just to be sure I understand, you want me to
Is this correct? |
That's basically it! If you could also provide some documentation updates to help other users set up their usage of the feature, with example workflows, etc that would be great. |
9c3e3c3
to
e941367
Compare
I have added two integration tests for
I also updated the documentation to include instructions for how to set this up in GitHub, GitLab, and CircleCI. To run the tests, I uploaded my fork of the terragrunt repo to my company's private gitlab project. I was then able to successfully execute both integration tests using an IAM role configured for web identity using the documentation referenced in my changes to the terragrunt documentation in this pull request. I have included the GitLab pipeline config config used to run both tests, along with the pipeline's output (showing that the tests passed) in the following gist: https://gist.github.com/partcyborg/370d66dfea927052e27fa00f2e378019 One thing I will point out however is that I had to add support for using the given IAM role ARN and WebIdentity token to the Please let me know if there is anything else I can do here. While I can easily help with configuring GitLab pipelines in this manner, unfortunately I do not have experience with configuring GitHub Actions or CircleCI in this manner. |
Add support for STS [AssumeRoleWithWebIdentity](https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRoleWithWebIdentity.html). Includes new config option `iam_web_identity_token` which takes either a WebIdentity token (designed to be passed in with `get_env()`), or a path to a file containing a WebIdentity token.
21053a8
to
ab21e33
Compare
|
I was wondering why the integration test passed with WebIdentityFile
I also found in internal tests that usage of WebIdentity file fails:
Added fix in partcyborg#1 |
Found that in internal tests, only with WebIdentityToken, Terragrunt fails with: ``` time=2024-06-05T18:11:01Z level=error msg=Error finding AWS credentials (did you set the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables?): NoCredentialProviders: no valid providers in chain. Deprecated. For verbose messaging see aws.Config.CredentialsChainVerboseErrors time=2024-06-05T[18](https://github.com/gruntwork-test/testing-terragrunt-with-web-identity/actions/runs/9389092410/job/25855946545#step:6:19):11:01Z level=error msg=Unable to determine underlying exit code, so Terragrunt will exit with error code 1 ``` Fixed by updating AssumeIamRole
Interesting that it works in our environment but not yours, as when initially setting up our CI environment to run these tests I was able to execute a static rendering of Regardless, I accepted the PR and updated our private fork and the tests still pass, so the change LGTM |
Quality Gate passedIssues Measures |
Added a blank line before the start of the list. Can you point me at instructions on how to run this test? A google search for "MD032/blanks-around-lists" points me at markdownlint, but running that against the docs returns a lot of unrelated issues. |
Different markdownlint. It looks good to me! $ markdownlint --disable 'MD013' -- docs I will circle back to this tomorrow to review if Denis doesn't beat me to it. I think we're close to merging this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Will give Denis a final chance to object before merging.
This would be an awesome addition. Mind taking a look @denis256 so it can be released? 🤞🏼 |
|
||
// Implements the stscreds.TokenFetcher interface | ||
// Supports providing a token value or the path to a token on disk | ||
func (f tokenFetcher) FetchToken(ctx credentials.Context) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment should have the following format 'FetchToken ...'
} | ||
token, err := os.ReadFile(string(f)) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.WithStack(err)
https://terragrunt.gruntwork.io/docs/community/contributing/#error-handling
@@ -188,6 +235,35 @@ func AssumeIamRole(iamRoleOpts options.IAMRoleOptions) (*sts.Credentials, error) | |||
sessionDurationSeconds = iamRoleOpts.AssumeRoleDuration | |||
} | |||
|
|||
if iamRoleOpts.WebIdentityToken != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nested block can be simplified
if iamRoleOpts.WebIdentityToken == "" {
// return
}
// continue flow when WebIdentityToken is set
Description
Add support for STS AssumeRoleWithWebIdentity.
Includes new config option
iam_web_identity_token
which takes either a WebIdentity token (designed to be passed in withget_env()
), or a path to a file containing a WebIdentity token.Fixes #2585.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added attribute
iam_web_identity_token
to support AssumeRoleWithWebIdentityMigration Guide