-
Notifications
You must be signed in to change notification settings - Fork 132
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
Unify TF AWS provider configuration logic #1204
Conversation
Signed-off-by: Erhan Cagirici <[email protected]>
These copies are not needed anymore after refactoring the AWS configuration logic to not use native provider Configure() Signed-off-by: Erhan Cagirici <[email protected]>
22ae56a
to
1fabbe6
Compare
The issue with the |
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.
Thank you very much @erhancagirici for cleaning up the AWS ProviderConfig code base and improving the situation with the number of per-MR STS calls the provider makes . Could you please also update the PR description with:
- The details of the changes this PR proposes
- The manual tests you did to verify these changes
- STS calls saved with these changes
We can also discuss briefly in the PR description the next iteration of planned improvements, where we would like to introduce a credential cache to further decrease the number of STS calls we make.
internal/clients/provider_config.go
Outdated
// specified managed resource and produces a config that can be used to | ||
// authenticate to AWS and tracks the ProviderConfigUsage. Useful for obtaining | ||
// AWS config for non-upjet based MR controllers. | ||
func GetAWSConfigWithProviderUsage(ctx context.Context, c client.Client, mg resource.Managed) (*aws.Config, error) { // nolint:gocyclo |
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 feel like it will help simplifying this package if we just move GetAWSConfigWithProviderUsage
to the ClusterAuth.eks
controller package as currently, it's the only controller who uses this. If, in the future, we add another controller that needs this, then we can move this to a command module. What do you think?
f148c02
to
50a5d83
Compare
Signed-off-by: Erhan Cagirici <[email protected]>
50a5d83
to
481dde7
Compare
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.
Thank you @erhancagirici, lgtm.
The |
Description of your changes
refactors AWS client configuration logic with a single path.
Previously, the AWS config was following various paths due to:
eks.clusterAuth
resource has a manual controller implementation, since it is not terraform-basedTo handle these paths, a native AWS configuration was being constructed along with the terraform configuration attributes.
These were causing in a non-unified configuration path, and duplicated AWS STS API calls due to configuration being processed by the native method and Terraform provider configure for no-fork.
Since the TF CLI is removed all relevant configuration paths can be removed to simplify.
This PR:
As a result this leads to 50% reduction in STS calls made per reconcile. Further improvement to STS calls will follow with a cache implementation.
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Tested manually with following ProviderConfig specifications: