ext_authz: add dynamic_metadata_matchers and use them in the ext_authz HTTP implementation by esmet · Pull Request #14843 · envoyproxy/envoy

esmet · 2021-01-27T23:02:07Z

Adds support for emitting dynamic metadata when using an HTTP authorization service. This is already possible when using a gRPC authorization service, so this change just connects the pipes on the HTTP side.

Commit Message: ext_authz: add dynamic_metadata_matchers and use them in the ext_authz HTTP implementation
Additional Description:
Risk Level: Low
Testing: New unit test
Docs Changes: Added documentation to API protos, similar to sibling fields.
Release Notes: ext_authz: added :ref:dynamic_metadata_from_headers <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.AuthorizationResponse.dynamic_metadata_from_headers> to support emitting dynamic metadata from headers returned by an external authorization service via HTTP.
Fixes #14622

HTTP implementation Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only · 2021-01-27T23:02:13Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14843 was opened by esmet.

see: more, trace.

api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto

lizan · 2021-01-27T23:16:52Z

/lgtm api

esmet · 2021-01-28T01:47:47Z

api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto

  // *Status*, *Content-Length*, *WWWAuthenticate* and *Location* are automatically added.
  type.matcher.v3.ListStringMatcher allowed_client_headers = 2;
+
+  // When this :ref:`list <envoy_api_msg_type.matcher.v3.ListStringMatcher>`. is set, authorization


typo near the .

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2021-02-02T05:38:53Z

/retest

repokitteh-read-only · 2021-02-02T05:38:57Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14843 (comment) was created by @esmet.

see: more, trace.

lizan · 2021-02-02T05:47:55Z

@dio can you do a first pass of review?

dio

Thank you for working on this! Looks good. Requesting for a better variable naming, and fixing some comments. Thank you!

dio · 2021-02-09T13:01:17Z

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc

  const Http::HeaderMap& headers_;
  const MatcherSharedPtr& matchers_;
  const MatcherSharedPtr& append_matchers_;
+  const MatcherSharedPtr& dynamic_metadata_matchers_;


Sorry, can we rename this to to_dynamic_metadata_matchers_ or similar, headers_to_dynamic_metadata_matchers_? A bit confused if we look at only this. Probably add a comment that all of these matchers (matchers, append_matchers, ...) work against headers_.

good idea, this code is indeed getting hairy

I went with to_dynamic_metadata_matchers

dio · 2021-02-09T13:01:34Z

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc

+        std::string key{header.key().getStringView()};
+        std::string value{header.value().getStringView()};


Nit. const for both of it.

dio · 2021-02-09T13:03:05Z

source/extensions/filters/common/ext_authz/ext_authz_http_impl.h

+  /**
+   * Returns a list of matchers used for selecting the headers to emit as dynamic metadata.
+   */
+  const MatcherSharedPtr& dynamicMetadataMatchers() const { return dynamic_metadata_matchers_; }


The same as above, let's rename it to something more explicit (?).

dio · 2021-02-09T13:04:22Z

source/extensions/filters/common/ext_authz/ext_authz_http_impl.h


  const MatcherSharedPtr request_header_matchers_;
  const MatcherSharedPtr client_header_matchers_;
+  const MatcherSharedPtr dynamic_metadata_matchers_;


Ditto with above.

dio · 2021-02-09T13:11:52Z

test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc

+  (*metadata_fields)["x-metadata-header-2"] = ValueUtil::stringValue("4");
+
+  // When we call onSuccess() at the bottom of the test we expect that all the
+  // headers-to-remove in that http response to have been correctly extracted


Sorry, I confused about this "headers-to-remove" comment. Do you mean all headers matched with the pattern defined by dynamic_metadata_from_headers?

s/http/HTTP.

I think this is bad copy paste. Let me fix it.

…data Signed-off-by: John Esmet <john.esmet@gmail.com>

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2021-02-22T03:25:19Z

@dio I addressed the code review feedback and I think this should be ready to go now.

esmet · 2021-02-22T21:52:35Z

@lizan friendly poke to take a pass and provide an API approval

dio

Looks good. Thank you!

esmet · 2021-03-09T14:22:14Z

@envoyproxy/api-shepherds sorry to bother, but can I ask for one more pass at the API portions and an approval if all looks good?

htuch · 2021-03-10T04:23:54Z

api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto

+  // response headers that have a correspondent match will be emitted as dynamic metadata, whose keys
+  // correspond to the name of the matched header and whose values are the respective values. When this
+  // list is *not* set, no additional dynamic metadata will be emitted. This metadata lives in a namespace
+  // specified by the canonical name of extension filter that requires it:


@lizan @kyessenov I'm little concerned that we've moved to a model where type URL is the canonical name for extension filters but we continue to use these names in the API for other purposes. What exists in this PR is consistent with other places we do this, e.g. RLS, but curious what to do going forward. Do we need some API style guidance on which one to use where here?

CC @envoyproxy/api-shepherds @phlax

Yeah, we need guidance what keys to use for metadata (and filter state). It's wild west right now. As long as the name is unique, it should be OK to use any name, but we need to phrase it this way and avoid canonical name.

+1 anything we can do here to centralize this in TypedExtensionConfig and have common documentation would be hugely helpful. This is incredibly confusing IMO.

Right, this PR uses the same metadata key name that is documented in the proto used by the gRPC authorization server. What can I do to help here, and is it something that should be addressed in this PR or in a followup PR?

For now can you potentially just add an example to these docs? It's pretty hard to understand for the casual user. Thank you!

/wait

github-actions · 2021-04-27T20:01:14Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-05-04T20:01:21Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

ext_authz: add dynamic_metadata_matchers and use them in the ext_authz

de34f7a

HTTP implementation Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only bot added the api label Jan 27, 2021

repokitteh-read-only bot assigned lizan Jan 27, 2021

esmet commented Jan 27, 2021

View reviewed changes

api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto Show resolved Hide resolved

repokitteh-read-only bot removed the api label Jan 27, 2021

esmet commented Jan 28, 2021

View reviewed changes

Cleanup

817eabd

Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only bot added the api label Jan 28, 2021

esmet added 4 commits January 28, 2021 16:27

Cleanup

ee02f0e

Signed-off-by: John Esmet <john.esmet@gmail.com>

Dynamic metadata unit test

30b2e3c

Signed-off-by: John Esmet <john.esmet@gmail.com>

Changelog

a40e361

Signed-off-by: John Esmet <john.esmet@gmail.com>

Merge remote-tracking branch 'origin/main' into ext-authz-http-metadata

455a532

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet marked this pull request as ready for review January 29, 2021 20:25

esmet requested a review from dio as a code owner January 29, 2021 20:25

esmet added 2 commits January 29, 2021 20:33

Merge remote-tracking branch 'origin/main' into ext-authz-http-metadata

843e721

Signed-off-by: John Esmet <john.esmet@gmail.com>

Merge remote-tracking branch 'origin/main' into ext-authz-http-metadata

2713c5a

Signed-off-by: John Esmet <john.esmet@gmail.com>

dio self-assigned this Feb 7, 2021

dio suggested changes Feb 9, 2021

View reviewed changes

esmet added 5 commits February 9, 2021 16:26

Merge remote-tracking branch 'upstream/main' into ext-authz-http-meta…

d827253

…data Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix merge conflicts

e77a399

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix the merge conflict fixes

fe7a974

Signed-off-by: John Esmet <john.esmet@gmail.com>

Formatting

6b3d057

Signed-off-by: John Esmet <john.esmet@gmail.com>

CR feedback

f6b75e3

Signed-off-by: John Esmet <john.esmet@gmail.com>

dio approved these changes Mar 4, 2021

View reviewed changes

htuch reviewed Mar 10, 2021

View reviewed changes

esmet mentioned this pull request Mar 22, 2021

FR - Ability to log headers from authorization response *without* forwarding to upstream service #14622

Closed

repokitteh-read-only bot added the waiting label Mar 28, 2021

mattklein123 self-assigned this Mar 28, 2021

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 27, 2021

github-actions bot closed this May 4, 2021

		std::string key{header.key().getStringView()};
		std::string value{header.value().getStringView()};

Conversation

esmet commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 27, 2021

Uh oh!

Uh oh!

lizan commented Jan 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esmet commented Feb 2, 2021

Uh oh!

repokitteh-read-only bot commented Feb 2, 2021

Uh oh!

lizan commented Feb 2, 2021

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!

Choose a reason for hiding this comment

Uh oh!

esmet commented Feb 22, 2021

Uh oh!

esmet commented Feb 22, 2021

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

esmet commented Mar 9, 2021

Uh oh!

htuch Mar 10, 2021 • 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!

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!

github-actions bot commented Apr 27, 2021

Uh oh!

github-actions bot commented May 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

esmet commented Jan 27, 2021 •

edited

Loading

htuch Mar 10, 2021 •

edited

Loading