aws: add metadata fetcher utility to use http async client by suniltheta · Pull Request #29880 · envoyproxy/envoy

suniltheta · 2023-09-30T00:50:11Z

~~Old title: Use http async client to fetch AWS credentials metadata~~

Commit 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

AwsRequestSigning ✅
AwsLambda ✅

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

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only · 2023-09-30T00:50:17Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #29880 was opened by suniltheta.

see: more, trace.

suniltheta · 2023-09-30T00:51:19Z

cc: @tonya11en for help reviewing this change

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

zuercher · 2023-10-02T18:59:29Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2023-10-02T18:59:35Z

@envoyproxy/first-pass-reviewers assignee is @tyxia

🐱

Caused by: a #29880 (comment) was created by @zuercher.

see: more, trace.

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

tyxia

/assign @lavignes @tonya11en

This probably should be reviewed by aws extension owner as the code changes in this PR are aws extension specific

tyxia · 2023-10-02T21:41:21Z

/assign @tonya11en @lavignes

tonya11en · 2023-10-02T21:58:57Z

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>

suniltheta · 2023-10-12T14:37:31Z

/retest

ravenblackx

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.

suniltheta · 2023-10-12T21:15:23Z

@ravenblackx thank you so much for your review

Could this be broken into smaller pieces?

I am open for this suggestion. I will try to extract out metadata_fetcher and other associated changes into this PR. And follow up with introducing the changes in credentials_provider_impl.* in the next PR. This should give me enough time to move out too many implementations inside header file out to .cc file. I will make sure all your review comments are addressed in this or next PRs.

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>

don't use loadFromYaml Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

ravenblackx · 2023-10-27T20:56:53Z

@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.

suniltheta · 2023-10-30T22:38:54Z

/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>

Sunil Narasimhamurthy added 2 commits September 29, 2023 23:05

use http async client instead of libcurl

e7e29cc

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

docs and release notes update

63b4d18

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

suniltheta requested a review from mattklein123 as a code owner September 30, 2023 00:50

Sunil Narasimhamurthy added 3 commits September 30, 2023 03:08

ignore instance metadata for aws lambda integ test

7ca9a64

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

ignore expected_duration_ when it is not available yet

3ab3360

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

improve code coverage

773c178

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only bot assigned tyxia Oct 2, 2023

fix constant expression failure

d3732b6

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

tyxia reviewed Oct 2, 2023

View reviewed changes

repokitteh-read-only bot assigned lavignes and tonya11en Oct 2, 2023

tyxia removed their assignment Oct 2, 2023

tonya11en removed their assignment Oct 2, 2023

suniltheta mentioned this pull request Oct 2, 2023

owners: Remove tonya11en from AWS files #29911

Merged

fix asan memory leak in test

bbf86e8

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

suniltheta mentioned this pull request Oct 11, 2023

deps: Bump com_github_curl -> 8.4.0 #30088

Merged

Honor host value from AWS_CONTAINER_CREDENTIALS_FULL_URI

5ce6bf8

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

jmarantz assigned ravenblackx Oct 12, 2023

ravenblackx requested changes Oct 12, 2023

View reviewed changes

Sunil Narasimhamurthy added 3 commits October 13, 2023 06:48

address review comments

0284a53

* 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>

update access modifier

be5eced

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

reduce PR 29880 to include metadata fetcher changes

08dc2ed

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

Add Metadatafetcher failureToString conversion coverage

1e5f1bf

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

suniltheta changed the title ~~Use http async client to fetch AWS credentials metadata~~ aws: add metadata fetcher utility to use http async client Oct 13, 2023

ravenblackx requested changes Oct 16, 2023

View reviewed changes

ravenblackx added the waiting label Oct 17, 2023

code review f/b

da3a2bb

don't use loadFromYaml Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only bot removed the waiting label Oct 19, 2023

suniltheta mentioned this pull request Oct 19, 2023

Remove curl as an Envoy dependency #11816

Closed

Use MATCHER_P helpers for checking options

56ec673

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

ravenblackx requested changes Oct 23, 2023

View reviewed changes

Comment thread test/extensions/common/aws/metadata_fetcher_test.cc Outdated

Comment thread source/extensions/common/aws/metadata_fetcher.cc

ravenblackx added the waiting label Oct 23, 2023

Avoid Invoke by using Matcher

5433e87

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only bot removed the waiting label Oct 24, 2023

ravenblackx added the waiting label Oct 26, 2023

ravenblackx approved these changes Oct 27, 2023

View reviewed changes

ravenblackx removed the waiting label Oct 27, 2023

ravenblackx assigned mattklein123 and unassigned ravenblackx Oct 27, 2023

mattklein123 approved these changes Oct 30, 2023

View reviewed changes

mattklein123 enabled auto-merge (squash) October 30, 2023 19:23

mattklein123 disabled auto-merge October 30, 2023 23:09

mattklein123 merged commit 60b2867 into envoyproxy:main Oct 30, 2023

This was referenced Oct 31, 2023

aws: use http async client to fetch metadata credentials #30626

Merged

deps: update the deprecation status of libcurl #30731

Merged

Conversation

suniltheta commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 30, 2023

Uh oh!

suniltheta commented Sep 30, 2023

Uh oh!

zuercher commented Oct 2, 2023

Uh oh!

repokitteh-read-only bot commented Oct 2, 2023

Uh oh!

tyxia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia commented Oct 2, 2023

Uh oh!

tonya11en commented Oct 2, 2023

Uh oh!

suniltheta commented Oct 12, 2023

Uh oh!

ravenblackx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suniltheta commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravenblackx commented Oct 27, 2023

Uh oh!

suniltheta commented Oct 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

suniltheta commented Sep 30, 2023 •

edited

Loading

tyxia left a comment •

edited

Loading

suniltheta commented Oct 12, 2023 •

edited

Loading