Added AWS region provider by ivitjuk · Pull Request #7172 · envoyproxy/envoy

ivitjuk · 2019-06-04T23:55:54Z

Description: Added AWS region provider

This will be used by the AWS SigV4 signing Task 3 as described in:
https://docs.aws.amazon.com/general/latest/gr/sigv4-calculate-signature.html

Risk Level: Low
Testing: Added new unit tests
Docs Changes: N/A
Release Notes: N/A

ivitjuk · 2019-06-05T02:48:50Z

/retest

repokitteh-read-only · 2019-06-05T02:48:54Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

yanavlasov · 2019-06-05T17:45:05Z

This filter could benefit from some documentation.

ivitjuk · 2019-06-05T18:06:03Z

This filter could benefit from some documentation.

Documented the interface class.

yanavlasov · 2019-06-05T19:13:35Z

This filter could benefit from some documentation.

Documented the interface class.

Thanks. I was thinking of something more substantial along the lines of https://github.com/envoyproxy/envoy/pull/6470/files#diff-f17684fc3d777b8c62225f6c3c7694cf which was done recently for CSRF filter.

ivitjuk · 2019-06-05T19:45:16Z

This filter could benefit from some documentation.

Documented the interface class.

Thanks. I was thinking of something more substantial along the lines of https://github.com/envoyproxy/envoy/pull/6470/files#diff-f17684fc3d777b8c62225f6c3c7694cf which was done recently for CSRF filter.

I see. Let me add some clarification. This is first in a series of PRs breaking #5546 into smaller pieces and moving it to libcurl. It is implementation of #5215. As such, this will not be an HTTP filter, but a gRPC credentials factory implementation. This PR specifically only adds a helper class to discover the AWS region of the current execution environment. Discovered region be used in later PRs to sign HTTP requests.

The reason for including this in the extensions/filters/http/common/aws directory is to follow the prior work from #5580.

Regarding the documentation concern. Interface classes and proto API will be documented. I am not sure if any other documentation is needed for a new gRPC credentials implementation.

ivitjuk · 2019-06-05T19:48:51Z

/retest

repokitteh-read-only · 2019-06-05T19:48:53Z

🐴 hold your horses - no failures detected, yet.

🐱

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

see: more, trace.

jmarantz · 2019-06-06T14:13:34Z

I have a higher level question: even though this is being written as an extension, I feel like having a whole bunch of AWS-specific stuff in the common default build might not be a scalable strategy for Envoy. Would it be possible to achieve the goals of this PR with pure config, or by making fairly generic configuration hooks to existing filters, rather than adding a new set of custom filters per-cloud-provider?

@mattklein123 would love to hear your thoughts on this.

moderation · 2019-06-06T14:40:10Z

@jmarantz with the introduction of the OpenCensus there is now Google Cloud Platform Stackdriver specific code too. It is in an extension but with all extensions on by default it is an opt-out process.

jmarantz · 2019-06-06T14:50:12Z

Yeah I thought we had talked about having extensions off-by-default at some point. Anyone know what the status of that is?

ivitjuk · 2019-06-06T15:32:13Z

I think a lot of what we see in this PR, and will see in the following PRs, is mostly fetching data from the AWS execution environment (environment variables + HTTP metadata endpoints), which is then fed as input to the signing algorithm 040acac. I don't know if there is currently a generic solution for this "environment fetching" problem in Envoy.

However, I think generic fetcher could be done. It would consume protobuf message with a configuration telling it what variables to fetch and their sources (env, metadata, etc), and would produce a protobuf.Struct with the fetched values output. The result would be consumed by extensions such as AWS SigV4.

What worries me is the set of conditionals [1] we have in our case, that would require relatively rich configuration language for this hypothetical environment fetcher. Not sure if we currently have enough use cases to justify that complexity.

[1] https://github.com/envoyproxy/envoy/pull/5546/files#diff-35f9a4ce6cdaec629ca3e8ac69a74aa1R208

mattklein123 · 2019-06-06T16:17:34Z

@mattklein123 would love to hear your thoughts on this.

IMO it's fine to have vendor specific code in extensions as long as it can be compiled out. I view extensions no differently than Linux drivers, in which there is lots of vendor specific code.

With that said, I agree that there will come a time in which we probably need to provide a "standard" Envoy public image and a "kitchen sink" image. This is something we probably need to chat about separately though.

jmarantz

RE vendor code in extensions: makes sense, SGTM; we can sort out bare-bones vs kitchen sink later.

jmarantz · 2019-06-06T23:31:10Z

source/common/common/logger.h

can you add a TODO to figure out some way for extensions to register new logger IDs?

I haven't been looking at these PRs but just wanted to note that in #5580 we decided to not add aws: #5580 (comment)

So I'm surprised this got back in. @ivitjuk It would make sense to have the sigv4 signer use it again since the logging the signer does it very AWS-specific.

Makes sense to me. I'll make a PR to switch signer to the AWS logger.

ivitjuk · 2019-06-11T17:24:25Z

Ping

ivitjuk · 2019-06-12T17:01:59Z

/retest

repokitteh-read-only · 2019-06-12T17:02:06Z

🤷‍♀️ nothing to rebuild.

🐱

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

see: more, trace.

ivitjuk · 2019-06-12T18:46:54Z

@dio any ETA for the review of this PR?

dio · 2019-06-13T01:55:21Z

/azp run

azure-pipelines · 2019-06-13T01:55:36Z

Azure Pipelines successfully started running 2 pipeline(s).

dio

@ivitjuk sorry for the delay. I think code-wise this looks good. Just two nits and a question. Thanks!

dio · 2019-06-13T01:59:54Z

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

Should this be absl::nullopt instead? Unless we want this getRegion to always return std::string

Switched to absl::nullopt.

dio · 2019-06-13T02:05:31Z

test/test_common/environment.cc

nit: let's be consistent with others? ASSERT_EQ(0, rc);

It's consistent with TestEnvironment::setEnvVar implementation. I can fix that one too in the same time?

https://github.com/envoyproxy/envoy/blob/master/test/test_common/environment.cc#L324-L327

dio · 2019-06-13T02:09:12Z

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

Sorry, use-case wise, can you explain more about the difference or the need of this Static vs Environment RegionProvider?

Yes, please check the closed PR we are trying to break into smaller pieces here:

https://github.com/envoyproxy/envoy/pull/5546/files#diff-18af3435579912d71a19beaecd97d832R42

I mean, I think to provide inline comments here probably helpful?

Added documentation at the class level.

This will be used by the AWS SigV4 signing Task 3 as described in: https://docs.aws.amazon.com/general/latest/gr/sigv4-calculate-signature.html Signed-off-by: Ivan Vitjuk <ivan@vitjuk.com>

dio

Thanks!

dio · 2019-06-13T23:44:15Z

@lizan do you want to take a senior reviewer pass for this PR?

lizan · 2019-06-14T07:49:42Z

@ivitjuk from next PRs, please do not squash and force-push changes unless fixing DCO, it messes up review history on GH.

ivitjuk · 2019-06-14T15:30:27Z

@ivitjuk from next PRs, please do not squash and force-push changes unless fixing DCO, it messes up review history on GH.

ack, sorry about that.

ivitjuk marked this pull request as ready for review June 5, 2019 04:51

lizan assigned dio Jun 5, 2019

lizan requested a review from dio June 5, 2019 21:51

jmarantz reviewed Jun 6, 2019

View reviewed changes

dio suggested changes Jun 13, 2019

View reviewed changes

Added AWS region provider

c76ea18

This will be used by the AWS SigV4 signing Task 3 as described in: https://docs.aws.amazon.com/general/latest/gr/sigv4-calculate-signature.html Signed-off-by: Ivan Vitjuk <ivan@vitjuk.com>

dio approved these changes Jun 13, 2019

View reviewed changes

lizan approved these changes Jun 14, 2019

View reviewed changes

lizan merged commit baa9d2d into envoyproxy:master Jun 14, 2019

ivitjuk deleted the aws_sigv4 branch June 14, 2019 15:37

Conversation

ivitjuk commented Jun 4, 2019

Uh oh!

ivitjuk commented Jun 5, 2019

Uh oh!

repokitteh-read-only bot commented Jun 5, 2019

Uh oh!

yanavlasov commented Jun 5, 2019

Uh oh!

ivitjuk commented Jun 5, 2019

Uh oh!

yanavlasov commented Jun 5, 2019

Uh oh!

ivitjuk commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivitjuk commented Jun 5, 2019

Uh oh!

repokitteh-read-only bot commented Jun 5, 2019

Uh oh!

jmarantz commented Jun 6, 2019

Uh oh!

moderation commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Jun 6, 2019

Uh oh!

ivitjuk commented Jun 6, 2019

Uh oh!

mattklein123 commented Jun 6, 2019

Uh oh!

jmarantz 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!

ivitjuk Jun 20, 2019 • 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!

ivitjuk commented Jun 11, 2019

Uh oh!

ivitjuk commented Jun 12, 2019

Uh oh!

repokitteh-read-only bot commented Jun 12, 2019

Uh oh!

ivitjuk commented Jun 12, 2019

Uh oh!

dio commented Jun 13, 2019

Uh oh!

azure-pipelines bot commented Jun 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio left a comment

ivitjuk commented Jun 5, 2019 •

edited

Loading

moderation commented Jun 6, 2019 •

edited

Loading

ivitjuk Jun 20, 2019 •

edited

Loading

dio commented Jun 13, 2019 •

edited

Loading