Add STS endpoint resolver #1332
Conversation
| log.Errorf("Failed to initialize AWS SDK session %v", err) | ||
| return nil, errors.Wrap(err, "instance metadata: failed to initialize AWS SDK session") | ||
| } | ||
| awsCfg := aws.NewConfig().WithRegion(region) |
There was a problem hiding this comment.
Can we move the "region" when we initialize "sess" ? That could avoid a copy.
There was a problem hiding this comment.
I thought of this. To move sess.copy to awssession and get region by querying ec2 metadata, get availability zone and then do some parsing did not seem efficient way, and more work.
There was a problem hiding this comment.
Can't afaics? We need to find the region somehow. Assuming we use IMDS to find the region, then we still need a 'regionless' session to initialise the IMDS client, so that we can find the region, so that we can configure a 'regionful' session to initialise the 'normal' AWS client. 😛
There was a problem hiding this comment.
you are right :D @anguslees
| sess, err := session.NewSession( | ||
| &aws.Config{ | ||
| Region: aws.String(cache.region), | ||
| MaxRetries: aws.Int(15), |
There was a problem hiding this comment.
Max retry was 15 here and 10 above. Do we know why we used different values for those two sessions?
There was a problem hiding this comment.
I am not sure why this discrepancy. I went ahead and assumed that 10 retries should be a good number to meet most of the use-cases. Let me know if you want me to increase it.
nithu0115
force-pushed
the
sts-regional-endpoint
branch
from
d7c5227
to
a7869e4
Compare
|
@couralex6 @achevuru @jayanthvn, thanks for the review. Addressed your comments :D |
nithu0115
force-pushed
the
sts-regional-endpoint
branch
from
a7869e4
to
c1e4bd4
Compare
nithu0115
force-pushed
the
sts-regional-endpoint
branch
from
c1e4bd4
to
3bb5f16
Compare
What type of PR is this?
Enhancement and Cleanup
Which issue does this PR fix: #647
What does this PR do / Why do we need it:
Add STS endpoint resolver
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Does this change require updates to the CNI daemonset config files to work?:
Does this PR introduce any user-facing change?: No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.