authz_filter: extended ext_authz to support v2alpha api by gsagula · Pull Request #3162 · envoyproxy/envoy

gsagula · 2018-04-23T05:57:58Z

Description: This PR extends the current Ext_Authz filter to allow optional HTTP attributes being passed from the Authorization service down to client or, to the upstream services. I would like to get some feedback on the changes to the current gRPC async client and filter before moving to implementation of HTTP part of this extension and tests.

*issue: #2828

Risk Level: Medium

Testing: Manual, unit testing.

Docs Changes: envoyproxy/data-plane-api#563

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2018-04-23T15:33:31Z

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

+  // Call status.
+  CheckStatus status;
+  // Optional http headers attribute.
+  KeyValueHeaders headers;


I think this could be HeaderMapPtr.

If it makes thing easier or more efficient, sure.

I think both if possible to move it later. I'll need to take a closer look at the HeaderMap api though.

Signed-off-by: Gabriel <gsagula@gmail.com>

htuch

Seems a totally reasonable approach, thanks for getting this rolling. @saumoh for further thoughts here.

htuch · 2018-04-23T21:17:12Z

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

+// downstream client or, to modify/add request headers being dispatched to the upstream.
+message HttpResponse {
+  // Http status code.
+  uint32 status_code = 1 [(validate.rules).uint32 = {gte: 100, lt: 600}];


Non-actionable, but if we were to redo the APIs from the ground up, I think I would add a dedicated HttpStatusCode object with the related constraints. We've done this now in a lot of places.

Sounds good. Should I place it here in a separated file like the http_uri.proto? https://github.com/gsagula/envoy/tree/e6d6df09c84a189a43762a1e6839103e8d02c51e/api/envoy/api/v2/core

htuch · 2018-04-23T21:38:20Z

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

+  // Optional http body attribute.
+  Buffer::InstancePtr body;
+  // Extra http status code attribute.
+  uint32_t status_code;


Nit: prefer to zero initialize this.

htuch · 2018-04-23T21:50:09Z

source/extensions/filters/http/ext_authz/ext_authz.cc

  using Filters::Common::ExtAuthz::CheckStatus;

-  switch (status) {
+  const bool update_status_code = response->status_code >= enumToInt(Http::Code::Continue) &&


Not sure I follow the logic here, can you add some comments?

may be save the specific code we do want to use and base rest of the logic off it? (in case u think that will help)

Agreed! That's super confusing and, I definitely change it. The idea is that when the authz service defines a new status code for "reject/denied", this logic will checks whether the code is valid and not equal to 403. If that evaluates to true, set a custom Status Code header and ResponseStatInfo. I will simplify this.

htuch · 2018-04-23T21:51:49Z

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

+  // Call status.
+  CheckStatus status;
+  // Optional http headers attribute.
+  KeyValueHeaders headers;


If it makes thing easier or more efficient, sure.

htuch · 2018-04-23T21:53:18Z

source/extensions/filters/http/ext_authz/ext_authz.cc

+  if (response->status == CheckStatus::Denied ||
+      (response->status == CheckStatus::Error && !config_->failureModeAllow())) {
+    Http::HeaderMapPtr response_headers;
+    if (update_status_code) {


This logic is a bit hard to follow due to the multiple reasons we can get here combined with the above code. Comments and/or restructure would be appreciated.

See my comment above. I will simplify this.

spikecurtis · 2018-04-24T20:21:13Z

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

-    string body = 3;
-  }
+  // A field that supplies the attributes for an HTTP response.
+  HttpResponse http_response = 2;


The comment here needs to explain whether this affects the response to the downstream client, or upstream server.

It also needs to explain the relationship between this field and the status field. For example, can I set the status to "OK" and then also provide this field? What should Envoy do in that case?

Good point too! I will add comments to explain the difference and when to use them.

spikecurtis · 2018-04-24T20:22:43Z

CC @kflynn is this moving us toward what you wanted to accomplish?

saumoh

Thanks for the PR. Some minor comments.

saumoh · 2018-04-24T23:26:19Z

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

-    status = CheckStatus::Denied;
    span.setTag(Constants::get().TraceStatus, Constants::get().TraceUnauthz);
+    authz_response->status = CheckStatus::Denied;
+    authz_response->status_code = response->http_response().status_code();


response may not have http_response ?

saumoh · 2018-04-24T23:34:10Z

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

-    // Http entity body.
-    string body = 3;
-  }
+  // A field that supplies the attributes for an HTTP response.


could u please add a comment about how this is not going to be used for TCP filters since the CheckResponse is used by both filters.

Good call! I will do that.

saumoh · 2018-04-24T23:46:58Z

source/extensions/filters/http/ext_authz/ext_authz.cc

  using Filters::Common::ExtAuthz::CheckStatus;

-  switch (status) {
+  const bool update_status_code = response->status_code >= enumToInt(Http::Code::Continue) &&


may be save the specific code we do want to use and base rest of the logic off it? (in case u think that will help)

saumoh · 2018-04-24T23:54:32Z

source/extensions/filters/http/ext_authz/ext_authz.cc

+      callbacks_->encodeData(*response->body.get(), true);
+    } else {
+      callbacks_->encodeHeaders(std::move(response_headers), true);
+    }


may be wrapping the encoding of response headers and body into a function would help with parsing of the logic here.

I think so too. I will try to simplify this logic, but first I will make sure that we have tests for all the permutations.

saumoh · 2018-04-24T23:58:41Z

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

  client_.check(request_callbacks_, request, Tracing::NullSpan::instance());

-  EXPECT_CALL(request_callbacks_, onComplete(CheckStatus::Error));
+  EXPECT_CALL(request_callbacks_, onComplete_(_));


we should be checking the Response's CheckStatus in this file.

I will address the tests after all the design changes.

gsagula · 2018-04-25T06:24:45Z

@saumoh @htuch @spikecurtis Thank you for the reviews. They are very helpful! I will do some work this week, but mostly over the weekend. I let you know as soon as it's ready.

Signed-off-by: Gabriel <gsagula@gmail.com>

htuch

This looks great, a few comments. @saumoh for another pass.

htuch · 2018-05-10T21:50:24Z

api/envoy/api/v2/core/http_status.proto

@@ -0,0 +1,12 @@
+syntax = "proto3";
+
+package envoy.api.v2.core;


Should this go in envoy.types?

htuch · 2018-05-10T21:53:13Z

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

+// Http attributes for ok responses.
+message OkHttpResponse {
+  // Http entity headers.
+  map<string, string> headers = 2;


NIt: this should be field number 1. How come you decided to duplicate headers rather than factor it out?

this should be field number 1

👍

How come you decided to duplicate headers rather than factor it out?

Sorry, I'm not sure if I follow your question. I decided to separate the content into two distinct messages and then use them inside of a oneof block in CheckResponse. Both DeniedHttpResponse & OkHttpResponse need headers.

htuch · 2018-05-10T21:54:54Z

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

+      }
+      // Only reassign http status if it's greater than 0 and different than 403.
+      if (response->denied_response().status().code() &&
+          response->denied_response().status().code() != authz_response->status_code) {


Nit: You can probably just drop the second conjunct here.

htuch · 2018-05-10T21:55:03Z

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

+            std::make_pair(Http::LowerCaseString(header.first), header.second));
+      }
+      // Only reassign http status if it's greater than 0 and different than 403.
+      if (response->denied_response().status().code() &&


Nit: code() > 0

htuch · 2018-05-10T21:57:59Z

source/extensions/filters/http/ext_authz/ext_authz.cc

+  } else {
+    header_map = std::make_unique<Http::HeaderMapImpl>(*getDeniedHeader());
+  }
+  for (const auto& header : reponse->headers) {


I wonder if it make sense to have a utility or method on header map to handle these copies, this pattern comes up a fair bit.

htuch · 2018-05-10T22:01:33Z

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

+    authz_response->status_code = enumToInt(Http::Code::Forbidden);
+
+    if (response->has_denied_response()) {
+      for (const auto& header : response->denied_response().headers()) {


There's some missing coverage, check out https://49526-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_extensions_filters_common_ext_authz_ext_authz_impl.cc.html.

Haven't addressed all the tests yet. Will do.

saumoh · 2018-05-11T00:07:34Z

ill do another pass soon.

saumoh

Thanks for the changes. This is looking good. Just some minor comments.

saumoh · 2018-05-11T18:00:08Z

source/extensions/filters/http/ext_authz/ext_authz.cc

+
    // We can get completion inline, so only call continue if that isn't happening.
    if (!initiating_call_) {
+      if (response->status == CheckStatus::OK) {


The conditional check and addition of headers should be outside the check for initiating_call_.
We want to add those headers whether the callback is invoked on the stack or async.

Oh, I see. Thanks!

saumoh · 2018-05-11T18:00:34Z

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

+
+// Http attributes for ok responses.
+message OkHttpResponse {
+  // Http entity headers.


These headers are in addition to existing headers or in addition to existing headers. A comment would help. Thanks.

saumoh · 2018-05-11T18:01:58Z

source/extensions/filters/http/ext_authz/ext_authz.cc

  }
 }

+Http::HeaderMapPtr Filter::getHeaderMap(const Filters::Common::ExtAuthz::ResponsePtr& reponse) {


nit: s/reponse/response/

gsagula · 2018-05-12T07:37:17Z

@htuch @saumoh Thanks for looking at the code. I'm finalizing the HTTP client, and I will address your comments as soon as I finish it.

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2018-05-21T08:34:43Z

I still need to work on some tests and documentation, but that's it. Grpc and the HTTP clients.

gsagula · 2018-06-22T18:31:58Z

Not sure what is causing ASAN errors. It seems not related to my changes. It will require more investigation.

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2018-06-25T08:23:04Z

@htuch I think all the previous comments have been addressed. Please take another look whenever you get a chance. Thanks!

I've brought all tests up to a 100%, except for this one https://66526-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_extensions_filters_common_ext_authz_ext_authz_http_impl.cc.html which is 95%.

https://66526-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_extensions_filters_common_ext_authz_ext_authz_grpc_impl.cc.html

https://66526-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_extensions_filters_http_ext_authz_ext_authz.cc.html

htuch · 2018-06-26T21:33:56Z

@gsagula thanks! Do you think you can manage that last 5%? I will take a look at this PR today.

gsagula · 2018-06-26T21:38:48Z

@htuch In order to get the last 5% I will need this to return false: StringUtil::atoul(response->headers().Status()->value().c_str(), status_code)

I couldn't think of an easy way of doing that, but I can try.

htuch

@gsagula not sure if I see the issue you describe in the code fragment. This PR LGTM overall and is awesome, I'll leave it open for that final fix if you think it's doable.

htuch · 2018-06-26T21:35:26Z

api/envoy/config/filter/http/ext_authz/v2alpha/ext_authz.proto

+// External Authorization filter calls out to an external service over either:
+//
+//  1. gRPC Authorization API defined by :ref:`CheckRequest <envoy_api_msg_service.auth.v2alpha.CheckRequest>`.
+//  2. raw HTTP Authorization server by passing the request headers to the service.


Nit: s/raw/Raw/

gsagula · 2018-06-26T22:51:31Z

@htuch Thanks, I will take a look at this later today.

htuch · 2018-06-27T14:11:01Z

@gsagula reach out to me on Slack if you need any assistance with debug.

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2018-06-27T17:45:29Z

@htuch The sanitizer failures seem to be unrelated to my changes. I will try to merge master later and try it again. Thanks.

htuch · 2018-06-27T18:02:26Z

@gsagula can you merge master to pick up #3742?

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2018-06-27T19:59:48Z

@htuch All tests are in 100% now.

https://67507-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_extensions_filters_common_ext_authz_ext_authz_http_impl.cc.html

htuch

Awesome, thanks @gsagula for the contribution!

gsagula · 2018-06-27T20:58:29Z

@htuch Thank you for all the reviews and the patience.

Gabriel added 2 commits April 22, 2018 21:36

authz_v2alpha: initial commit

9501403

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: fixed format

8419745

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula commented Apr 23, 2018

View reviewed changes

authz_v2alpha: fixed format with right version

e6d6df0

Signed-off-by: Gabriel <gsagula@gmail.com>

mattklein123 assigned htuch Apr 23, 2018

gsagula changed the title ~~wip_authz_v2alpha: extended ext_authz gRPC implementation to support V2alpha api~~ [WIP] Authz V2alpha: extended ext_authz gRPC implementation to support V2alpha api Apr 23, 2018

htuch suggested changes Apr 23, 2018

View reviewed changes

htuch assigned saumoh Apr 24, 2018

spikecurtis reviewed Apr 24, 2018

View reviewed changes

saumoh reviewed Apr 25, 2018

View reviewed changes

gsagula mentioned this pull request May 1, 2018

Ext_Authz: Allow extension to append or modify original query string params #3266

Closed

Gabriel added 4 commits May 7, 2018 00:35

authz_v2alpha: code review changes

af3925b

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into authz_grpc_v2alpha

33e7eba

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: added missing proto

0ed4639

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: fixed response initialization

ab5cf4c

Signed-off-by: Gabriel <gsagula@gmail.com>

htuch reviewed May 10, 2018

View reviewed changes

saumoh suggested changes May 11, 2018

View reviewed changes

This was referenced May 14, 2018

extauth documentation #3369

Closed

Add an explicit state for whether filter chain should continue or not. #3334

Merged

Gabriel added 2 commits May 16, 2018 17:24

authz_v2alpha: implemented http client

615b64c

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: merged master

050f75f

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula changed the title ~~[WIP] Authz V2alpha: extended ext_authz gRPC implementation to support V2alpha api~~ Authz V2alpha: extended ext_authz gRPC to support V2alpha api May 21, 2018

gsagula changed the title ~~Authz V2alpha: extended ext_authz gRPC to support V2alpha api~~ authz_filter: extended ext_authz to support v2alpha api May 21, 2018

Gabriel added 2 commits May 20, 2018 23:15

authz_v2alpha: renamed authz implementation files

7e9cf7c

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: move http_status into envoy type

09e4125

Signed-off-by: Gabriel <gsagula@gmail.com>

Gabriel added 9 commits June 22, 2018 22:16

Merge remote-tracking branch 'upstream/master' into authz_grpc_v2alpha

f8155ab

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: improved ext_authz HTTP filter branch test coverage

0250a49

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: improved ext_authz HTTP client test coverage

14889bc

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: fixed sanitizer error

e17a6fc

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into authz_grpc_v2alpha

a760b45

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: improved gRPC authz client test coverage

e2ba046

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: improved gRPC authz client branch test coverage

298a696

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: improved http authz client branch test coverage

8d05a5d

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: improved http authz filter test

b7aaef1

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula mentioned this pull request Jun 25, 2018

Add response field to CheckResponse in external_auth.proto #3711

Closed

htuch previously approved these changes Jun 26, 2018

View reviewed changes

authz_v2alpha: added test for unknown HTTP status code use case

6058248

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula dismissed htuch’s stale review via 6058248 June 27, 2018 17:16

Gabriel added 2 commits June 27, 2018 10:20

Merge remote-tracking branch 'upstream/master' into authz_grpc_v2alpha

91db962

Signed-off-by: Gabriel <gsagula@gmail.com>

authz_v2alpha: fixed format

5c320bb

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into authz_grpc_v2alpha

a5fcd62

Signed-off-by: Gabriel <gsagula@gmail.com>

htuch approved these changes Jun 27, 2018

View reviewed changes

htuch merged commit 5244597 into envoyproxy:master Jun 27, 2018

danielhochman mentioned this pull request Jun 28, 2018

build: include vector in header_map.h #3746

Merged

gsagula mentioned this pull request Jul 20, 2018

Users want to have extauth talk to its auth server with gRPC emissary-ingress/emissary#63

Closed

		@@ -0,0 +1,12 @@
		syntax = "proto3";

		package envoy.api.v2.core;

Conversation

gsagula commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

Choose a reason for hiding this comment

Uh oh!

spikecurtis commented Apr 24, 2018

Uh oh!

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

gsagula Apr 25, 2018 • 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!

gsagula commented Apr 25, 2018

Uh oh!

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

gsagula May 12, 2018 • 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

gsagula commented Apr 23, 2018 •

edited

Loading

gsagula Apr 25, 2018 •

edited

Loading

gsagula May 12, 2018 •

edited

Loading