-
Notifications
You must be signed in to change notification settings - Fork 6.8k
chore(deps): migrated aws sdk v1 to v2 for EKS with argocd-k8s-auth #26200
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: reggie-k <[email protected]>
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Signed-off-by: reggie-k <[email protected]>
| if profile != "" { | ||
| opts = append(opts, config.WithSharedConfigProfile(profile)) | ||
| } | ||
| cfg, err := config.LoadDefaultConfig(ctx, opts...) |
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.
| if roleARN != "" { | ||
| creds := stscreds.NewCredentials(sess, roleARN) | ||
| stsAPI = sts.New(sess, &aws.Config{Credentials: creds}) | ||
| appCreds := stscreds.NewAssumeRoleProvider(client, roleARN) |
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.
According to AWS Security Token Service Credentials in https://docs.aws.amazon.com/sdk-for-go/v2/developer-guide/migrate-gosdk.html
| request.HTTPRequest.Header.Add(clusterIDHeader, clusterName) | ||
| signed, err := request.Presign(requestPresignParam) | ||
|
|
||
| presignClient := sts.NewPresignClient(client) |
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.
According to Presigned Requests in the migration guide.
Another way to implement this was using middleware, according to HTTP request/response in the guide, but this way an invalid token was generated.
agaudreault
left a comment
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.
LGTM, but I haven't tested it with eks cluster
ppapapetrou76
left a comment
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.
LGTM I left a few optional code improvements
| if roleARN != "" { | ||
| creds := stscreds.NewCredentials(sess, roleARN) | ||
| stsAPI = sts.New(sess, &aws.Config{Credentials: creds}) | ||
| appCreds := stscreds.NewAssumeRoleProvider(client, roleARN) | ||
| cfg.Credentials = aws.NewCredentialsCache(appCreds) | ||
| client = sts.NewFromConfig(cfg) |
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 that this is not added entirely in this PR but it would be nice if we can add UT - optional ofc
Co-authored-by: Papapetrou Patroklos <[email protected]> Signed-off-by: Regina Voloshin <[email protected]>
Co-authored-by: Papapetrou Patroklos <[email protected]> Signed-off-by: Regina Voloshin <[email protected]>
Related to #12419
Previous PR:
#25222
cmd/argocd-k8s-auth/commands/aws.gokubectlusing anargocd-k8s-authgenerated-token against a live EKS cluster - with IAM role, AWS_PROFILE and AWS cred env vars.Checklist: