Add tcp and http filters for external authorization service. by saumoh · Pull Request #2359 · envoyproxy/envoy

saumoh · 2018-01-12T17:32:15Z

Title
Support TCP and HTTP filter for external authorization service.

Description
Add support for filters for authorization by an external gRPC based authorization service. This PR implements the discussing we had in #2291.

Testing
I have tested this on my local setup with an external gRPC authorization server. I have also added UT's for both the http and tcp filters.

Risk
Low: Because only the users of this filter will be impacted. It should not impact general stability of Envoy it self.

Caveats

I did not test with a Google gRPC service configuration.
The v1 to v2 conversion of the configuration does not directly map to the way gRPC services protobuf is defined. I can make that change either as a follow up PR or within this PR if we feel that is important to cover here.
There may be a change required to data-plane-api accesslog. Once there is general aggrement on the change set here then I can submit a PR to make the changes to data-plane-api repo.

Notes to Reviewer

I'd really appreciate a closer look at how filter_config is being used in the file server/config/http/ext_auth.cc

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch

Thanks for this very significant contribution. I'd like to ask two things to make the review work better:

Can you apply the general style feedback (and (re)read the Envoy style guide at https://github.com/envoyproxy/envoy/blob/master/STYLE.md) to the entire set of files?
Is it possible to break up this into at least 2 PRs? One for the gRPC client, and another for the auth filter? In general, we try to discourage such large PRs. Thanks.

htuch · 2018-01-12T17:39:53Z

source/common/config/filter_json.cc

                                      *proto_config.mutable_ip_white_list());
 }

+void FilterJson::translateTcpExtAuthzFilter(


Do you need v1 configuration support? If not, please drop this as we're trying to deprecate v1.

I am happy to remove it. It's useful in ut's though, like here
What it the recommended way of filling out the protobuf in UT's?

There are some examples, e.g. see https://github.com/envoyproxy/envoy/blob/master/test/common/access_log/grpc_access_log_impl_test.cc for an example of a unit test that takes protobuf config. I don't think it's worth keeping just for unit tests, although admittedly a lot of the legacy examples do still use v1 JSON. Protobufs are easy to build programatticaly or via JSON/YAML parsing.

htuch · 2018-01-12T17:40:34Z

bazel/repositories.bzl

+    ]
+    for t in sub_bind_targets:
+        native.bind(
+            name = "envoy_api_sub_" + t[0],


What is the goal here?

This is for config(s) under directory auth in data-plane-api
Need to be able to specify dependency on the proto's in that directory. for example here
I'll re-write it using the idioms already in place for filters.

htuch · 2018-01-12T17:40:49Z

include/envoy/ext_authz/BUILD

+    "//bazel:envoy_build_system.bzl",
+    "envoy_cc_library",
+    "envoy_package",
+    "envoy_proto_library",


htuch · 2018-01-12T17:41:45Z

include/envoy/ext_authz/ext_authz.h

+namespace Envoy {
+namespace ExtAuthz {
+
+using envoy::api::v2::auth::CheckRequest;


using isn't allowed in headers by the style guide. It's also more readable IMHO to have the envoy::api::v2:: prefix in the type, indicating that this is a proto.

htuch · 2018-01-12T17:42:59Z

include/envoy/ext_authz/ext_authz.h

+/**
+ * An interface for creating ext_authz.proto (authorization) request.
+ */
+class CheckRequestGenIntf {


Hard to understand what this is from either the comment or type name. Please avoid abbreviations like Intf as they make readability more difficult.

will rename

htuch · 2018-01-12T17:50:06Z

source/common/ext_authz/ext_authz_impl.cc

+  Ssl::Connection* ssl =
+      const_cast<Ssl::Connection*>(connection->ssl());
+  if (ssl != nullptr) {
+    if (local == false) {


htuch · 2018-01-12T17:50:51Z

source/common/ext_authz/ext_authz_impl.cc

+  Envoy::Http::StreamDecoderFilterCallbacks *sdfc = const_cast<Envoy::Http::StreamDecoderFilterCallbacks *>(callbacks);
+  httpreq->set_id(std::to_string(sdfc->streamId()));
+
+  #define SET_HDR_IN_HTTPREQ(_hq, _api, mname)                          \


Is there no other way to do this? Not a huge fan of macro magic (but it's OK if there's really no other way).

Let me try to make it a function call.

htuch · 2018-01-12T17:51:07Z

source/common/ext_authz/ext_authz_impl.cc

+  httpreq->set_protocol(proto2str(sdfc->requestInfo().protocol().value()));
+
+  // Fill in the headers
+  ::google::protobuf::Map< ::std::string, ::std::string >* mhdrs = httpreq->mutable_headers();


auto* hdrs =

htuch · 2018-01-12T17:51:16Z

source/common/ext_authz/ext_authz_impl.cc

+
+
+  AttributeContext_Request *req = new AttributeContext_Request();
+  ASSERT(req);


Same here with ownership.

htuch · 2018-01-12T17:51:46Z

source/common/filter/ext_authz.cc

+void Instance::setCheckReqGen(CheckRequestGenIntf *crg)
+{
+  ASSERT(crg_ == nullptr);
+  crg_ = CheckRequestGenIntfPtr{std::move(crg)};


Please use meaningful field names.

saumoh · 2018-01-12T18:33:17Z

@htuch thanks for the quick turn around. I'll followup on the style guide. Still learning and coming up to speed on the Envoy way.
Let me try to see how to break it into two pr's.

mattklein123 · 2018-01-12T19:13:27Z

@saumoh please break into 3 PRs:

client
HTTP filter
TCP filter

Thank you!

saumoh · 2018-01-12T20:58:57Z

sounds good. Will break it into 3. thx

saumoh · 2018-01-17T00:30:53Z

@htuch As i am breaking this into three pr's can u confirm if naming files ext_authz is fine? thx

htuch · 2018-01-17T21:57:43Z

@saumoh FYI, you'll need to adapt this PR to work with the new pattern in #2393 once that merges.

saumoh · 2018-01-19T17:14:19Z

Closing this PR and broken down the change set into three PR's:
#2415 (gRPC client)
#2416 (TCP filter)
#2417 (HTTP filter)

PS: Have incorporated the feedback here into the new PR's.
cc: @htuch @mattklein123

devdinu · 2018-02-08T10:55:42Z

@saumoh Thanks for the contribution. we want to use this to authenticate the request with another gRPC server. could you share the documentation or steps to use this filter,

service proto definition for authentication server
how do we configure envoy to use this (Is it part of filters in http_connection_manager?) or sample config if you've.
can we customise what we want to do by extending this? eg: custom errors

saumoh · 2018-02-08T23:59:41Z

@Dineshkumar-cse The proto definitions for the auth server, the tcp filter and http filter are defined in the data-plane-api repo.
The protobuf for gRPC server is at:
https://github.com/envoyproxy/data-plane-api/tree/master/envoy/service/auth/v2
For the tcp filter config see:
https://github.com/envoyproxy/data-plane-api/tree/master/envoy/config/filter/network/ext_authz/v2
And for the http filter config see:
https://github.com/envoyproxy/data-plane-api/tree/master/envoy/config/filter/http/ext_authz/v2

The http filters are configured under the http_connection_manager.
hope this helps.

devdinu · 2018-02-12T13:41:57Z

@saumoh we've built the new envoy binary from master branch, and used the following config (v1), we just wanted to test whether it loads the filter.

{
"listeners": [
....
 "filters": [
 { "name": "http_connetion_manager", ... },
 {
   "name": "ext_authz",
   "config": {} 
 }] ...
]

But it errors out with the following,

envoy_bin.sh[8884]: [2018-02-12 20:32:03.100][8886][debug][config] source/server/config/network/http_connection_manager.cc:238]     filter #0
 envoy_bin.sh[8884]: [2018-02-12 20:32:03.101][8886][debug][config] source/server/config/network/http_connection_manager.cc:239]       name: envoy.router
 envoy_bin.sh[8884]: [2018-02-12 20:32:03.101][8886][debug][config] source/server/config/network/http_connection_manager.cc:243]     config: {"value":{},"deprecated_v1":true}
 envoy_bin.sh[8884]: [2018-02-12 20:32:03.101][8886][debug][config] source/server/listener_manager_impl.cc:29]   filter #1:
 envoy_bin.sh[8884]: [2018-02-12 20:32:03.101][8886][debug][config] source/server/listener_manager_impl.cc:30]     name: envoy.ext_authz
 envoy_bin.sh[8884]: [2018-02-12 20:32:03.101][8886][debug][config] source/server/listener_manager_impl.cc:33]   config: {"value":{},"deprecated_v1":true}
 envoy_bin.sh[8884]: [2018-02-12 20:32:03.102][8886][critical][main] source/server/server.cc:71] error initializing configuration '/etc/default/envoy.json': Didn't find a registered implementation for name
: 'envoy.ext_authz'
 envoy_bin.sh[8884]: [2018-02-12 20:32:03.102][8886][debug][upstream] source/common/upstream/cluster_manager_impl.cc:582] shutting down thread local cluster manager

Is there anything else we're missing?

saumoh · 2018-02-13T05:17:11Z

@Dineshkumar-cse the filter's are under review still.
u can follow them and try it out once they are merged.
#2416
and
#2417

As per the feedback on #2359 I am breaking it up into three PR's. The patch in this PR adds support for the TCP filter. This PR implements the discussing we had in #2291. Testing I have tested this on my local setup with an external gRPC authorization server. I have also added UT's for the TCP filter. Risk Low: Because only the users of this filter will be impacted. It should not impact general stability of Envoy it self. Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Authorization TCP and HTTP filters using an external gRPC service. Patch 2 of 3: HTTP filter. Description As per the feedback on #2359 I am breaking it up into three PR's. The patch in this PR adds support for the HTTP filter. This PR implements the discussing we had in #2291. Testing I have tested this on my local setup with an external gRPC authorization server. I have also added UT's for the HTTP filter. Risk Low: Because only the users of this filter will be impacted. It should not impact general stability of Envoy it self. Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

* metric record and integration test * make test run * make circle pass * update go path * try fix prow * update sd filter and test * clean * remove direction from stackdriver config * cleanup comment * address comment * fix build * recover fake sd server * add test prefix to field name * update test config

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

Add tcp and http filters for external authorization service.

4c3e58f

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch reviewed Jan 12, 2018

View reviewed changes

mattklein123 assigned htuch Jan 12, 2018

This was referenced Jan 19, 2018

Support for external authorization grpc service. #2415

Merged

Support for external authorization tcp filter. #2416

Merged

Support for external authorization http filter. #2417

Merged

saumoh closed this Jan 19, 2018

jpsim pushed a commit that referenced this pull request Nov 28, 2022

cleanup: convert some more uses of NULL to nullptr (#2359)

236e81c

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

cleanup: convert some more uses of NULL to nullptr (#2359)

9ba1920

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>



		AttributeContext_Request *req = new AttributeContext_Request();
		ASSERT(req);

Conversation

saumoh commented Jan 12, 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!

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!

Choose a reason for hiding this comment

Uh oh!

saumoh commented Jan 12, 2018

Uh oh!

mattklein123 commented Jan 12, 2018

Uh oh!

saumoh commented Jan 12, 2018

Uh oh!

saumoh commented Jan 17, 2018

Uh oh!

htuch commented Jan 17, 2018

Uh oh!

saumoh commented Jan 19, 2018

Uh oh!

devdinu commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saumoh commented Feb 8, 2018

Uh oh!

devdinu commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saumoh commented Feb 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devdinu commented Feb 8, 2018 •

edited

Loading

devdinu commented Feb 12, 2018 •

edited

Loading