Skip to content
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

refactor external client to make single getAWSConfig call per connect #1003

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

erhancagirici
Copy link
Collaborator

@erhancagirici erhancagirici commented Dec 4, 2023

Description of your changes

Refactors getAWSConfig calls in the external client code, to make single call per connect. getAWSconfig calls involve resolving credentials and executing AWS STS calls when there are relevant AssumeRole or AssumeRoleWithWebIdentity operations needed. This was being called multiple times per connect, causing superfluous invocations.

This PR will improve the situation reported at #997

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Via uptest: https://github.com/upbound/provider-aws/actions/runs/7141372941

@erhancagirici erhancagirici changed the title Optimize sts calls refactor external client to make single getAWSConfig call per connect Dec 4, 2023
@haarchri
Copy link
Member

haarchri commented Dec 4, 2023

Is it possible to implement credential caching? Currently, it seems we make a single call per connect. However, when managing multiple resources, each resource triggers a separate call. Considering that IRSA credentials are valid for 15 minutes, it would be more efficient to perform the call only once every 15 minutes, even when dealing with numerous managed resources - what do you think ?

@erhancagirici erhancagirici force-pushed the optimize-sts-calls branch 3 times, most recently from 9529217 to 5328115 Compare December 6, 2023 13:51
@erhancagirici
Copy link
Collaborator Author

@haarchri This PR is an initial improvement for addressing the low-hanging fruit. Our plan is to make further improvements in iterations. This is especially motivated by the fact that those parts of the provider are currently fractured because we have a hybrid architecture for that provider (both the CLI-based & SDK-based upjet architectures are in use at the moment).
The plan is once we completely switch to the nofork arch for the remaining few resources , we will be able to simplify the provider setup logic there. And as we simplify provider setup logic, it will be easier for us to make further optimizations like credential caching, that is definitely in our scope.

@erhancagirici erhancagirici force-pushed the optimize-sts-calls branch 2 times, most recently from be58582 to dc4d6d1 Compare December 7, 2023 08:12
@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/iam/role.yaml"

@erhancagirici erhancagirici force-pushed the optimize-sts-calls branch 2 times, most recently from ef7740c to a990e46 Compare December 7, 2023 13:18
@ulucinar
Copy link
Collaborator

ulucinar commented Dec 8, 2023

/test-examples="examples/iam/role.yaml"

@ulucinar ulucinar marked this pull request as ready for review December 8, 2023 13:35
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @erhancagirici, lgtm.

@@ -199,7 +196,7 @@ func DefaultTerraformSetupBuilder(ctx context.Context, c client.Client, mg resou
if pc.Spec.Endpoint != nil {
if pc.Spec.Endpoint.URL.Static != nil {
if len(pc.Spec.Endpoint.Services) > 0 && *pc.Spec.Endpoint.URL.Static == "" {
return errors.Wrap(err, "endpoint is wrong")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Then, we were previously silently non-erroring :)

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @erhancagirici, LGTM.

@ulucinar
Copy link
Collaborator

ulucinar commented Dec 8, 2023

/test-examples="examples/iam/role.yaml"

@erhancagirici erhancagirici merged commit b8b5ae3 into crossplane-contrib:main Dec 8, 2023
8 checks passed
@erhancagirici erhancagirici deleted the optimize-sts-calls branch December 8, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants