ext_authz: Allow to emit dynamic metadata #11820

dio · 2020-06-30T16:18:20Z

Commit Message:

This patch enables ext_authz backed with gRPC service to emit metadata. The gRPC authorization server can set the dynamic metadata (an opaque google.protobuf.Struct) as part of the CheckResponse when it is successful (i.e. when http_response is OkHttpResponse).

Risk Level: Low
Testing: Unit.
Docs Changes: Added.
Release Notes: Added.
Fixes #9049

Signed-off-by: Dhi Aurrahman [email protected]

This patch enables ext_authz backed with gRPC service to emit metadata. The authorization can set the dynamic metadata (an opaque google.protobuf.Struct) as part of the `CheckResponse` when it is successful (i.e. when `http_response` is `OkHttpResponse`). Signed-off-by: Dhi Aurrahman <[email protected]>

repokitteh-read-only · 2020-06-30T16:18:26Z

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

🐱

Caused by: #11820 was opened by dio.

see: more, trace.

dio · 2020-06-30T16:18:52Z

cc. @erikbos

dio · 2020-06-30T23:21:05Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-30T23:21:14Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Dhi Aurrahman <[email protected]>

htuch · 2020-07-01T16:12:32Z

api/envoy/service/auth/v3/external_auth.proto

@@ -69,6 +70,9 @@ message OkHttpResponse {
  // by Leaving `append` as false, the filter will either add a new header, or override an existing
  // one if there is a match.
  repeated config.core.v3.HeaderValueOption headers = 2;
+
+  // Optional response metadata that will be emitted as dynamic metadata.


Which part of the dynamic metadata keyspace does this live?

If I catch your question correctly: for HTTP filter, HttpFilterNames::get().ExtAuthorization (envoy.filters.http.ext_authz) while for the network one: NetworkFilterNames::get().ExtAuthorization (envoy.filters.network.ext_authz)

I think we should document these namespaces in the proto where the emit_dynamic_metadata can be enabled. And perhaps refer to those configs here?

Yeah, can we spell this out explicitly in the docs? Thanks. Otherwise LGTM for API.

zuercher

This looks good to me, from the dynamic metadata perspective.

cc/ @envoyproxy/api-shepherds for API review.

lizan · 2020-07-08T21:53:03Z

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

+
+  // A set of metadata returned by the authorization server, that will be emitted as filter's
+  // dynamic metadata that other filters can leverage.
+  ProtobufWkt::Struct dynamic_metadata{};


nit: no need of {}

Yeah. I was lazy to explicitly initialize some Response objects in source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc.

lizan · 2020-07-08T21:56:50Z

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

@@ -51,4 +51,7 @@ message ExtAuthz {
  // version of Check{Request,Response} used on the wire.
  config.core.v3.ApiVersion transport_api_version = 5
      [(validate.rules).enum = {defined_only: true}];
+
+  // Flag to specify whether dynamic metadata should be emitted. Defaults to false.


Do we really need this flag? What is the use case that you want configure this to false when an ext_authz server is returning metadata?

When there is no subsequent filter to actually consume the metadata (i.e. when ext_authz is the sole gatekeeper).

Then shouldn't the ext_authz server just be configured to not return metadata? (which also avoid the metadata on the wire for efficiency)

That’s a fair observation. I will remove this field and update the docs then. Thanks for the input!

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2020-07-09T08:05:02Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-09T08:05:12Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-07-09T11:46:43Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-09T11:46:51Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2020-07-10T16:53:24Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-10T16:53:33Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-07-14T22:39:00Z

@lizan, sorry, do I still need to introduce more fixes in this PR? Thanks!

lizan

LGTM, defer to @zuercher for non-tetrate final review/merge.

This patch enables ext_authz backed with gRPC service to emit metadata. The authorization can set the dynamic metadata (an opaque google.protobuf.Struct) as part of the `CheckResponse` when it is successful (i.e. when `http_response` is `OkHttpResponse`). Signed-off-by: Dhi Aurrahman <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>

This patch enables ext_authz backed with gRPC service to emit metadata. The authorization can set the dynamic metadata (an opaque google.protobuf.Struct) as part of the `CheckResponse` when it is successful (i.e. when `http_response` is `OkHttpResponse`). Signed-off-by: Dhi Aurrahman <[email protected]> Signed-off-by: scheler <[email protected]>

mailofapideveloper · 2020-08-18T18:29:29Z

@dio : Thanks for adding Dynamic Metadata in ext_auth response , but its still not available in
https://pkg.go.dev/github.com/envoyproxy/go-control-plane/envoy/service/auth/v3?tab=doc#OkHttpResponse
when I try building the container , its failing with
unknown field 'DynamicMetadata' in struct literal of type envoy_service_auth_v3.OkHttpResponse
can you please help in pushing the changes to pkg.go.dev
Thanks

erikbos · 2020-08-18T20:37:47Z

The current latest release of go-control-plane, 0.9.6, does not yet include the proto & structs definitions for this. You'll will have to use go-control-plane master. See #12615 for a code snippet with go-control-plane v0.9.7-0.20200811182123-112a4904c4b0 Don't forget to run envoy > 1.15.0 to run a version which actually supports this.

mailofapideveloper · 2020-08-19T01:06:01Z

Thanks much @erikbos , It worked .

repokitteh-read-only bot added the api label Jun 30, 2020

mattklein123 assigned zuercher Jun 30, 2020

dio added 2 commits July 1, 2020 06:28

Merge remote-tracking branch 'upstream/master'

c2ea373

Comments

1c3b670

Signed-off-by: Dhi Aurrahman <[email protected]>

htuch reviewed Jul 1, 2020

View reviewed changes

dio added 3 commits July 1, 2020 23:43

Merge remote-tracking branch 'upstream/master'

a2d0c77

Merge remote-tracking branch 'upstream/master'

2771757

Merge remote-tracking branch 'upstream/master'

9b5e4ba

zuercher previously approved these changes Jul 7, 2020

View reviewed changes

lizan reviewed Jul 8, 2020

View reviewed changes

dio added 2 commits July 9, 2020 06:16

Add explicit namespaces

8d0a68f

Signed-off-by: Dhi Aurrahman <[email protected]>

Explicit docs

1aa9b0c

Signed-off-by: Dhi Aurrahman <[email protected]>

dio dismissed zuercher’s stale review via 1aa9b0c July 8, 2020 23:33

dio added 2 commits July 9, 2020 06:33

Merge remote-tracking branch 'upstream/master'

12d5eaa

Current version history

fed8e48

Signed-off-by: Dhi Aurrahman <[email protected]>

dio added 5 commits July 10, 2020 05:45

Merge remote-tracking branch 'upstream/master'

af02c0f

Merge remote-tracking branch 'upstream/master'

7da6755

Fix ordering

d676a19

Signed-off-by: Dhi Aurrahman <[email protected]>

No flag

2c4d210

Signed-off-by: Dhi Aurrahman <[email protected]>

Remove flag

ff15096

Signed-off-by: Dhi Aurrahman <[email protected]>

dio added 3 commits July 11, 2020 07:04

Merge remote-tracking branch 'upstream/master'

c0b0d96

Merge remote-tracking branch 'upstream/master'

44bcc9c

Merge remote-tracking branch 'upstream/master'

cbe2fb2

lizan approved these changes Jul 14, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jul 14, 2020

mattklein123 approved these changes Jul 16, 2020

View reviewed changes

mattklein123 merged commit ff44d73 into envoyproxy:master Jul 16, 2020

This was referenced Aug 12, 2020

Access logging of emitted dynamic metadata by ext_authz? #12615

Closed

ext_authz: Support emit dynamic metadata in CheckResponse/DeniedHttpResponse #12671

Closed

rockspore mentioned this pull request Feb 11, 2021

Remove RBAC filter in Envoy and Istio templates apigee/apigee-remote-service-cli#165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext_authz: Allow to emit dynamic metadata #11820

ext_authz: Allow to emit dynamic metadata #11820

dio commented Jun 30, 2020

repokitteh-read-only bot commented Jun 30, 2020

dio commented Jun 30, 2020

dio commented Jun 30, 2020

azure-pipelines bot commented Jun 30, 2020

htuch Jul 1, 2020

dio Jul 1, 2020

zuercher Jul 7, 2020

htuch Jul 8, 2020

dio Jul 8, 2020

zuercher left a comment

lizan Jul 8, 2020

dio Jul 8, 2020

dio Jul 10, 2020

lizan Jul 8, 2020

dio Jul 8, 2020

lizan Jul 10, 2020 •

edited

Loading

dio Jul 10, 2020 •

edited

Loading

dio Jul 10, 2020

dio commented Jul 9, 2020

azure-pipelines bot commented Jul 9, 2020

dio commented Jul 9, 2020

azure-pipelines bot commented Jul 9, 2020

dio commented Jul 10, 2020

azure-pipelines bot commented Jul 10, 2020

dio commented Jul 14, 2020

lizan left a comment

mailofapideveloper commented Aug 18, 2020

erikbos commented Aug 18, 2020

mailofapideveloper commented Aug 19, 2020

ext_authz: Allow to emit dynamic metadata #11820

ext_authz: Allow to emit dynamic metadata #11820

Conversation

dio commented Jun 30, 2020

repokitteh-read-only bot commented Jun 30, 2020

dio commented Jun 30, 2020

dio commented Jun 30, 2020

azure-pipelines bot commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

dio Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Jul 9, 2020

azure-pipelines bot commented Jul 9, 2020

dio commented Jul 9, 2020

azure-pipelines bot commented Jul 9, 2020

dio commented Jul 10, 2020

azure-pipelines bot commented Jul 10, 2020

dio commented Jul 14, 2020

lizan left a comment

Choose a reason for hiding this comment

mailofapideveloper commented Aug 18, 2020

erikbos commented Aug 18, 2020

mailofapideveloper commented Aug 19, 2020

lizan Jul 10, 2020 •

edited

Loading

dio Jul 10, 2020 •

edited

Loading