ec2 client credential init fix for AWS IRSA #29784
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Greyeye
force-pushed
the
ec2-cred-fix
branch
2 times, most recently
from
f2670b6 to
c1203db
Compare
louis-cqrl
reviewed
Nov 4, 2024
| log.Warnf("unable to get aws configurations: %s", err) | ||
| return nil | ||
| } | ||
| return ec2.NewFromConfig(cfg) |
There was a problem hiding this comment.
❓ maybe add a log such as: log.Info("AWS configurations successfully fetched")
What do you think ?
There was a problem hiding this comment.
@louis-cqrl I believe that will populate unnecessary log output. How about log.Debug() instead.
| return ec2.NewFromConfig(cfg) | ||
| } | ||
|
|
||
| func getTagsWithCreds(ctx context.Context, instanceIdentity *EC2Identity, connection ec2ClientInterface) ([]string, error) { |
There was a problem hiding this comment.
❓ if ec2Cconnection fails then connection.DescribeTags will unreference a nil pointer, isn't an issue ?
There was a problem hiding this comment.
That's a good find, I've added the check and updated the test
pkg/util/ec2/ec2_tags.go
Outdated
| } | ||
|
|
||
| // ec2Cconnection creates an ec2 client with the given region and credentials | ||
| func ec2Cconnection(ctx context.Context, region string, awsCreds aws.CredentialsProvider) ec2ClientInterface { |
There was a problem hiding this comment.
❓ Why the function name is ec2Cconnection and not ec2Connection ?
drichards-87
approved these changes
Nov 4, 2024
current use of ec2.New() does not work with IRSA. (not tested but most like same problem with EKS Pod Identity as well) ``` (pkg/util/ec2/ec2_tags.go:104 in fetchEc2TagsFromAPI) | unable to get tags using default credentials (falling back to instance role): operation error EC2: DescribeTags, https response error StatusCode: 400, RequestID: 1234-1234-1234, api error MissingParameter: The request must contain the parameter AWSAccessKeyId ``` This change is to change to use aws config package to help initialise aws ec2 client with the built-in credentials such as AWS IRSA tokens.
1. rename function name ec2Cconnection to ec2Connection 2. add nil check for connection 3. updated the unit test to cover nil test 4. add debug log print for aws config setup
Co-authored-by: DeForest Richards <[email protected]>
Greyeye
force-pushed
the
ec2-cred-fix
branch
from
268bf2d to
a814946
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR modifies the initialisation of the AWS EC2 client to use the AWS configuration package. This change enables support for built-in credentials such as AWS IAM Roles for Service Accounts (IRSA). The current use of ec2.New() does not work with IRSA, and although not tested, a similar issue likely occurs with EKS Pod Identity as well.
When a pod is assigned an IRSA with the correct permissions to perform DescribeTags, it encounters an error and logs the following message:
fixes issue 29916
Motivation
We are currently running the Datadog Agent pod in AWS EKS and are aiming to clean up the instance role by eliminating unnecessary permissions attached to it. Datadog is one of many services that currently use the instance role for the ec2:DescribeTags permission, and we would like to switch to using the IRSA-assigned role.
After extensive debugging and tracing, we discovered that when the pod is assigned an IRSA token but the instance is running IMDSv2, it fails to use IRSA and always falls back to using the instance role.
Describe how to test/QA your changes
collect_ec2_tags: true
collect_ec2_tags_use_imds: true
ec2_prefer_imdsv2: true
Possible Drawbacks / Trade-offs
none
Additional Notes
none