-
Notifications
You must be signed in to change notification settings - Fork 338
Add support for consuming web identity credentials #238
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 consuming web identity credentials #238
Conversation
1d5aa14
to
0f787df
Compare
pkg/providers/v1/aws.go
Outdated
@@ -1194,7 +1195,16 @@ func init() { | |||
} | |||
|
|||
var provider credentials.Provider | |||
if cfg.Global.RoleARN == "" { | |||
if os.Getenv("AWS_WEB_IDENTITY_TOKEN_FILE") != "" { |
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 know we already have this unfortunate if statement, but it seems like the purpose of credentials.NewChainCredentials
is to allow an order to be specified -- can we simply add the NewWebIdentityRoleProvider
to the chain?
I think what we should have done instead of if cfg.Global.RoleARN == "" ...
, is to just create a custom provider similar to AssumeRoleProvider which takes an arn from the config file, and just add it to the NewChainCredentials call (if the role is not provided it should return an error and the next provider will be called).
Let me know if you already tried something like that--perhaps I'm misunderstanding why it was originally done this way.
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.
Actually, the identity session should have been picked up from the original session that we pass on with:
&credentials.StaticProvider{
Value: sess.Config.Credentials.Get(),
}
Maybe it is as simple as enabling the shared config:
sess, err = session.NewSessionWithOptions(session.Options{
Config: *cfg,
SharedConfigState: session.SharedConfigEnable,
})
Let me try that.
0f787df
to
16f63cc
Compare
@nckturner I changed the PR to obtain credentials from the default credentials chain instead. The effect should be similar, but doesn't duplicate the logic for building the web identity credentials. |
CCM builds its own credential providers list and configures them directly instead of using those from the session. This PR will only pass on the list if an IAM role from cloudconfig is used. If not, we pass on nil so that the default credential chain is used instead.
16f63cc
to
0a181c2
Compare
Just realised the problem with this approach is that credentials probably will not renew itself. I tweaked this approach a bit to only pass on the list of credentials providers if we are actually using the role from cloudconfig. Passing on nil otherwise will make sessions just use the default credential list otherwise, which will now also support web identity credentials. |
Agreed, using the default chain is a better approach. Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nckturner, olemarkus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
CCM builds its own credential providers list and configures them directly instead of using those from the session.
This PR checks the presence of the web identity env vars and adds a web identity role provider.
What type of PR is this?
/kind feature
What this PR does / why we need it:
I have been working on IRSA support for the AWS CCM for kOps, and I had to add something like this in order for CCM to pick up the AWS credentials. Without this, it would just use the instance role, which no longer has the necessary AWS permissions to run CCM.