Added a set of AWS credential providers. by ivitjuk · Pull Request #7281 · envoyproxy/envoy

ivitjuk · 2019-06-14T18:15:25Z

Description:

Added a set of AWS credential providers.

EnvironmentCredentialsProvider fetches credentials from the environment
InstanceProfileCredentialsProvider fetches credentials from the instance metadata
TaskRoleCredentialsProvider fetches credentials from the ECS task metadata

Also added DefaultCredentialsProviderChain which is able to pick the
correct credential provider according to the execution environment it is
running in.

Signed-off-by: Ivan Vitjuk ivanvit@amazon.com

Risk Level: Low
Testing: Unit tests added
Docs Changes: Interface classes documented.
Release Notes: N/A
Fixes (partial): #5215

- EnvironmentCredentialsProvider fetches credentials from the environment - InstanceProfileCredentialsProvider fetches credentials from the instance metadata - TaskRoleCredentialsProvider fetches credentials from the ECS task metadata Also added DefaultCredentialsProviderChain which is able to pick the correct credential provider according to the execution environment it is running in. Signed-off-by: Ivan Vitjuk <ivanvit@amazon.com>

ivitjuk · 2019-06-20T16:00:45Z

Hi, this was opened almost a week ago, can we get the feedback?

dio

@ivitjuk thanks for your patience, was out last week. Looks good, but I see some potential improvements here:

source/extensions/filters/http/common/aws/credentials_provider.h

source/extensions/filters/http/common/aws/credentials_provider_impl.cc

source/extensions/filters/http/common/aws/credentials_provider_impl.h

- Replaced fromCString() and fromString() with a single Credential constructor accepting absl::string_view - Use constexpr for string constants - Pass std::string to credential constructors instead of absl::optional Signed-off-by: Ivan Vitjuk <ivanvit@amazon.com>

ivitjuk · 2019-06-24T20:05:25Z

/retest

repokitteh-read-only · 2019-06-24T20:05:29Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7281 (comment) was created by @ivitjuk.

see: more, trace.

ivitjuk · 2019-06-25T23:29:40Z

As far as I can see failing test is extensions/filters/http/lua/lua_filter_test which is unrelated to this change. Can we proceed with this review?

ivitjuk · 2019-06-25T23:31:23Z

/retest

repokitteh-read-only · 2019-06-25T23:31:27Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7281 (comment) was created by @ivitjuk.

see: more, trace.

dio · 2019-06-25T23:50:42Z

@ivitjuk thanks. Let me check...

ivitjuk · 2019-06-27T15:32:51Z

/wait-any

dio

Looks good. We need some clarifications and one coverage test. Thank you!

dio · 2019-06-27T23:08:00Z

source/extensions/filters/http/common/aws/credentials_provider.h

+  Credentials(absl::string_view access_key_id = absl::string_view(),
+              absl::string_view secret_access_key = absl::string_view(),
+              absl::string_view session_token = absl::string_view()) {
+    if (!access_key_id.empty()) {


Does this mean we always need access_key_id? and we require either secret_access_key or session_token to be there? This if statements seem not necessary. What is the implication if we don't have (or empty) access_key_id?

Credentials can be one of:

Empty

Have access key only

Have access key and secret key

Have access key, secret key and session token

Implication of an empty access key is that request will be made with anonymous identity. For example requests can be made to S3 with anonymous identity if bucket is configured in that way. Non-production environments can also accept anonymous requests.

dio · 2019-06-27T23:14:04Z

source/extensions/filters/http/common/aws/credentials_provider.h

+ * If a credential component was not found in the execution environment, it's getter method will
+ * return absl::nullopt. Credential components with the empty string value are treated as not found.
+ */
 class Credentials {


I felt like this should be a struct but not feeling strongly about it.

Interface of this type is not public by default. Data members specifically are not public and can not be changed once set by the constructor. In that sense it feels more like a type (class) than a collection of data (struct).

dio · 2019-06-27T23:15:31Z

source/extensions/filters/http/common/aws/credentials_provider.h

-              absl::string_view session_token)
-      : access_key_id_(access_key_id), secret_access_key_(secret_access_key),
-        session_token_(session_token) {}
+  Credentials(absl::string_view access_key_id = absl::string_view(),


Defaulting to absl::string_view() seems unnecessary? For convenience you always have the default constructor Credentials()?

As I already have this constructor, I wouldn't get the default one unless I request it with Credentials() = default. To me it looks simpler to have a single constructor, because relying on the default one would not lead to code reduction.

dio · 2019-06-27T23:19:03Z

source/extensions/filters/http/common/aws/credentials_provider_impl.cc

+  cached_credentials_ = Credentials(access_key_id, secret_access_key, session_token);
+}
+
+Credentials CredentialsProviderChain::getCredentials() {


Can we have coverage for this function? https://239673-65214191-gh.circle-artifacts.com/0/coverage/coverage.source_extensions_filters_http_common_aws_credentials_provider_impl.cc.html

Thanks to catching that. Added tests.

Signed-off-by: Ivan Vitjuk <ivanvit@amazon.com>

ivitjuk · 2019-06-29T00:50:18Z

/wait-any

dio

LGTM. Thanks @ivitjuk for sticking with this!

ivitjuk marked this pull request as ready for review June 14, 2019 21:25

mattklein123 assigned dio Jun 14, 2019

ivitjuk mentioned this pull request Jun 17, 2019

Aws sigv4 metadata fetcher #7303

Merged

dio suggested changes Jun 24, 2019

View reviewed changes

repokitteh-read-only bot added the waiting:any label Jun 27, 2019

dio suggested changes Jun 27, 2019

View reviewed changes

repokitteh-read-only bot removed waiting:any labels Jun 27, 2019

Added CredentialsProviderChain::getCredentials() tests

0403dcd

Signed-off-by: Ivan Vitjuk <ivanvit@amazon.com>

repokitteh-read-only bot added the waiting:any label Jun 29, 2019

dio approved these changes Jun 29, 2019

View reviewed changes

repokitteh-read-only bot removed the waiting:any label Jun 29, 2019

mattklein123 merged commit 188ae9f into envoyproxy:master Jul 1, 2019

rgs1 mentioned this pull request Jan 16, 2020

proxying requests to AWS authenticated services (e.g.: s3) #9708

Closed

Conversation

ivitjuk commented Jun 14, 2019

Uh oh!

ivitjuk commented Jun 20, 2019

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivitjuk commented Jun 24, 2019

Uh oh!

repokitteh-read-only bot commented Jun 24, 2019

Uh oh!

ivitjuk commented Jun 25, 2019

Uh oh!

ivitjuk commented Jun 25, 2019

Uh oh!

repokitteh-read-only bot commented Jun 25, 2019

Uh oh!

dio commented Jun 25, 2019

Uh oh!

ivitjuk commented Jun 27, 2019

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivitjuk commented Jun 29, 2019

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants