-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
aws: add metadata fetcher utility to use http async client #29880
aws: add metadata fetcher utility to use http async client #29880
Conversation
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
cc: @tonya11en for help reviewing this change |
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
/assign-from @envoyproxy/first-pass-reviewers |
@envoyproxy/first-pass-reviewers assignee is @tyxia |
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
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.
/assign @lavignes @tonya11en
This probably should be reviewed by aws extension owner as the code changes in this PR are aws extension specific
/assign @tonya11en @lavignes |
Removing myself from this as I'm on vacation. Opened #29911, since I haven't worked at AWS since 2021. |
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
/retest |
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.
Could this be broken into smaller pieces? It looks like metadata_fetcher could be prepared before the whole shebang, for example. If it's not possible that's okay, but this is a big chunk for a single review.
@ravenblackx thank you so much for your review
I am open for this suggestion. I will try to extract out Update: Reduced the PR size |
* move implementation from header to cc file * Don't create duplicate MockUpstream * Use named args - less error prone * Don't restrict runtime value read to be in just constructor Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
don't use loadFromYaml Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
@lavignes could probably still do with having an opinion as CODEOWNER, but it's been a month since that was originally requested so I don't know how likely that is. @mattklein123 is the remaining owner of the area and also a senior maintainer so could perhaps combine final-pass and area knowledge. |
/retest |
Following the merge of #29880 and #30626 we can mark the curl usage as deprecated. Meanwhile bazel/repositories.bzl had stale info that OpenCensus tracer was still using libcurl. We can continue to keep the Issue #11816 open until curl is removed entirely after the deprecation time (Probably for v1.31 release). Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>
Old title: Use http async client to fetch AWS credentials metadataCommit Message: aws: add metadata fetcher utility to use http async client
Additional Description:
This is
part #1
of set of changes to update common aws extension utility to make use of http async client to fetch aws credentials metadata instead of using libcurl. In this PR we are introducing a class MetadataFetcher (similar to JwksFetcher) and a function in utility.cc to add internal static cluster config.Risk Level: Low
Testing: Added unit testing to cover the new code path
Docs Changes: Updated
Release Notes: Updated
Platform Specific Features: NA
Runtime guard:
envoy.reloadable_features.use_libcurl_to_fetch_aws_credentials
Deprecated: libcurl
General Description about the overall effort:
For the effort to remove curl from Envoy #11816. This change will use http async client to make the call towards EC2 instance metadata & ECS task metadata service. To make http async client work it needs cluster manager with statically defined cluster configurations. If clusters with fixed names are not provided it will add the cluster during init.
There are 2 http filters with which this change works without issue
However, with AWS IAM gRPC credentials provider plugin there is a problem⚠️ The http async client will not support AWS IAM gRPC plugin because it needs Envoy server fully initialized before using cluster manager. When gRPC service tries to use the Cluster Manager the static clusters are not available for reasons explained on #27586. So in future changes if curl is removed entirely then we need to update docs to clearly state that AWS IAM gRPC plugin can only support Environment or File based credentials fetching. Other option is to add a curl version of AWS IAM gRPC credentials plugin into the contrib folder.
So far I am not aware if there are any Envoy users other than within AWS that uses AWS IAM gRPC plugin. Please let me know what is the best way to maintain it for everyone's convenience.
I have put a short doc to explain the implementation here https://docs.google.com/document/d/1m1KE_LGDnxrXwnUC1OEKYLmw8szy4r06OX_O3JcM-wM