-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Centralize building of the aws.Config object #14048
Conversation
Commit 9772f8519b5b99107f2403f43c09a08ecde47b72 does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
9772f85
to
65b5aef
Compare
test-me-please |
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.
Nice! Just a minor nit below. Also, could you add the PR context into the commit body itself?
When running the ENI IPAM an operator does not need to pass any region env var flag for the AWS SDK to configure it's client because we read it from the ec2 metadata endpoint. If the same operator wants to run with --update-ec2-apdater-limit-via-api=true then the cilium-operator will fail to construct the ec2 client because a valid region was not passed. Signed-off-by: Vlad Ungureanu <vladu@palantir.com>
65b5aef
to
1180663
Compare
test-me-please |
When running the ENI IPAM an operator does not need to pass any region env var flag for the AWS SDK to configure it's client because we read it from the ec2 metadata endpoint. If the same operator wants to run with
--update-ec2-apdater-limit-via-api=true
then thecilium-operator
will fail to construct the ec2 client because a valid region was not passed.I removed the logging that was identifying that we connected to the ec2 metadata (mostly because we do not have a logger in the package where I moved this). Happy to add it back and construct a logger there but did not think it provides a lot of value IMO.
I think in the future we should use the
ec2.NewClient
method to get the client for the ENI limits updater so we get bought into the same metrics + QPS pipeline as well.