Support for external authorization tcp filter. by saumoh · Pull Request #2416 · envoyproxy/envoy

saumoh · 2018-01-19T17:09:24Z

Title
Authorization TCP and HTTP filters using an external gRPC service.
Patch 2 of 3: TCP 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 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.

Caveats

The merge of this PR is dependent on the commit of Support for external authorization grpc service. #2415 into master. Once that is merged and this branch is in sync the change set here will build succesfully.

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

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

htuch · 2018-02-07T16:09:02Z

@junr03 could you take a first pass on this?

saumoh · 2018-02-09T17:10:16Z

@junr03, @htuch Not in any particular rush from my end but just want to make sure it is in your radar. :-)
Thanks.

junr03 · 2018-02-09T20:08:47Z

@saumoh yes still on my radar. Sorry for the delay. I have been sick this week, so I had to prioritize my screen time. I will take a first pass this afternoon.

saumoh · 2018-02-09T22:30:59Z

@junr03 hope you feel better soon...this can wait till then!

junr03

thanks for splitting this work into 3 easily digestible steps. Some comments to get started.

junr03 · 2018-02-09T20:42:27Z

source/server/config/network/ext_authz.h

+/**
+ * Config registration for the  external authorization filter. @see NamedNetworkFilterConfigFactory.
+ */
+class ExtAuthzConfigFactory : public NamedNetworkFilterConfigFactory {


@envoyproxy/senior-maintainers I noticed we are inconsistent about the naming of these filter config classes, i.e some have "filter" in them some dont.

I think this is fine, but if we want to have some standard to make things consistent we can put it in STYLE.md.

junr03 · 2018-02-09T20:48:09Z

source/server/config/network/ext_authz.cc

+    FactoryContext& context) {
+
+  ASSERT(!proto_config.stat_prefix().empty());
+  ASSERT(proto_config.grpc_service().has_envoy_grpc());


I would update the documentation with an attention blob that says that the filter only works with envoy grpc https://github.com/envoyproxy/data-plane-api/blob/master/envoy/config/filter/network/ext_authz/v2/ext_authz.proto#L18

junr03 · 2018-02-09T21:09:40Z

source/common/filter/ext_authz.h

+  bool failOpen() const { return failure_mode_allow_; }
+
+private:
+  static InstanceStats generateStats(const std::string& name, Stats::Scope& scope);


nit: newline

junr03 · 2018-02-09T21:28:07Z

source/common/filter/ext_authz.cc

+
+Network::FilterStatus Instance::onData(Buffer::Instance&) {
+  if (status_ == Status::NotStarted) {
+    // If the ssl handshake was not done and data is the next event!


please expand this comment. If I understand correctly, what you are trying to express is that there are two ways to initiate the external authz check, either when the filter gets an onEvent callback with a ConnectionEvent::Connected after a succesful ssl handshake or after an onData callback if the connection is not over ssl. I would make this more clear in this comment

junr03 · 2018-02-09T22:04:02Z

source/common/filter/ext_authz.cc

+  // request already completed.
+  if (event == Network::ConnectionEvent::RemoteClose ||
+      event == Network::ConnectionEvent::LocalClose) {
+    if (status_ == Status::Calling) {


move the comment above here

junr03 · 2018-02-09T22:48:02Z

test/common/filter/ext_authz_test.cc

+          Invoke([&](RequestCallbacks& callbacks) -> void { request_callbacks_ = &callbacks; })));
+
+  EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection());
+  // Called by SSL when the handshake is done.


same as above

junr03 · 2018-02-09T22:56:24Z

test/common/filter/ext_authz_test.cc

+  EXPECT_CALL(*client_, cancel());
+  filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
+
+  EXPECT_EQ(1U, stats_store_.counter("ext_authz.name.total").value());


I would set expectations on the other stats to be 0

added those.

junr03 · 2018-02-09T22:57:01Z

test/common/filter/ext_authz_test.cc

+namespace ExtAuthz {
+namespace TcpFilter {
+
+class ExtAuthzFilterTest : public testing::Test {


comment for all the tests: can you set expectations on the active gauge

The destructor checks for the gauge. But if that isn't read-able then i can add a check for the gauge.

junr03 · 2018-02-09T23:02:46Z

test/common/filter/ext_authz_test.cc

+      "grpc_service": {
+          "envoy_grpc": { "cluster_name": "ext_authz_server" }
+      },
+      "failure_mode_allow": true,


there is no test with default (false)

Thanks for catching this. Added a test.

junr03 · 2018-02-09T23:05:11Z

test/common/filter/ext_authz_test.cc

+  EXPECT_EQ(0U, stats_store_.counter("ext_authz.name.cx_closed").value());
+}
+
+TEST_F(ExtAuthzFilterTest, Error) {


Not a very descriptive test name. And I am actually having a tough time groking what it is that you want to test here. Do you mind clarifying

I also like the idea of adding a leading comment line for tests that aren't obvious/trivial. E.g.:

// Validate error handling when foo does bar with baz. TEST_F(ExtAuthzFilterTest, Error) {

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

saumoh · 2018-02-12T23:30:36Z

@junr03 Thanks for the detailed review comments. I think I have addressed all of them.
I created the PR in data-plane-api also: envoyproxy/data-plane-api#491

Fix up the comments for failure_mode_allow that was incorrectly worded and caught during review envoyproxy/envoy#2416 Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch

Looks good, a few comments.

htuch · 2018-02-13T23:17:20Z

source/common/filter/ext_authz.cc

+namespace TcpFilter {
+
+InstanceStats Config::generateStats(const std::string& name, Stats::Scope& scope) {
+  std::string final_prefix = fmt::format("ext_authz.{}.", name);


Nit: const.

htuch · 2018-02-13T23:19:38Z

source/common/filter/ext_authz.cc

+
+void Instance::callCheck() {
+  envoy::service::auth::v2::CheckRequest request;
+  CreateCheckRequest::createTcpCheck(filter_callbacks_, request);


I should have picked up on this at the time, but can you rename from CreateCheckRequest to CheckRequestUtils or something? Package and class name should be nouns, not verbs.

htuch · 2018-02-13T23:21:09Z

source/common/filter/ext_authz.cc

+  config_->stats().active_.inc();
+  config_->stats().total_.inc();
+
+  calling_check_ = true;


Can you integrate this state variable with status_ somehow? Having multiple state variables makes it harder to reason about.

@htuch thanks for the review. I've addressed other changes. For this one I really found it more difficult to combine calling_check_ with status_.
calling_check_ is specifically to check for on stack callback; i.e when onComplete is invoked in the same call stack. When I tried to combine it into status_ it ended up with status_ CallingOnStack, CallingOffStack; which made it more confusing to me.

To me these two states feel disjoint. Any suggestions on how to address it?

htuch · 2018-02-14T14:28:03Z

source/common/filter/ext_authz.cc

+  config_->stats().total_.inc();
+
+  calling_check_ = true;
+  client_->check(*this, request, Tracing::NullSpan::instance());


I think you might want to retain ownership of request until it has been sent, the gRPC client doesn't make a copy and only guarantees the send happens after you receive a response.

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

htuch

Thanks for addressing last round of feedback, a few more comments/questions.

htuch · 2018-02-16T04:28:18Z

source/common/filter/ext_authz.cc

+  } else {
+    // SSL connection is post TCP newConnection. Therefore the ext_authz check in onEvent.
+    // if the ssl handshake was successful then it will invoke the
+    // Network::ConnectionEvent::Connected.


Should we assert or if condition on this event type?

htuch · 2018-02-16T04:29:17Z

source/common/filter/ext_authz.cc

+    // Network::ConnectionEvent::Connected.
+    // Thus invoke the authorization service check if not done yet.
+    if (status_ == Status::NotStarted) {
+      callCheck();


Why do we treat SSL and non-SSL differently? Why not, in both cases, wait until we get the first onData? Then we can pause the pipe and perform auth check. Seems like that would have simpler flow, but I may well be missing something here, so interested to hear your thoughts.

That is a very good point/observation. Deferring to onData() would definitely simplify the call logic.

nice, I didnt think about that

htuch · 2018-02-16T04:29:58Z

source/common/filter/ext_authz.cc

+  if (status_ == Status::NotStarted) {
+    // If there was no ssl handshake there will be no event Network::ConnectionEvent::Connected
+    // and onData() will be the first event into the filter.
+    // Therefore, if the authorization service hasn not been invoked then we must do so now.


Nit: s/hasn/hasn't/

htuch · 2018-02-16T04:34:04Z

source/common/filter/ext_authz.cc

+
+void Instance::onComplete(CheckStatus status) {
+  status_ = Status::Complete;
+  filter_callbacks_->connection().readDisable(false);


Should this be deferred until we know we are going to be successful and want to continue reading?

htuch · 2018-02-16T04:36:34Z

source/server/config/network/ext_authz.cc

+  };
+}
+
+NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactory(const Json::Object& json_config,


We only need this for v1 config; are we supporting v1 ext_authz?

we are doing only v2. This helps in specifying the configuration as v2 json and still use it without rigging up a xDS server.

Not sure this is strictly needed for v2 JSON, this should only apply to v1 JSON placed in deprecated_v1. Does the test fail if you don't have this?

afaik this function is the entry point when I give envoy executable a configuration file envoy.conf and I can then specify the filter's configuration like:

"filters": [ { "type": "read", "name": "ext_authz", "config": { "stat_prefix": "authz", "grpc_service": { "envoy_grpc": { "cluster_name": "authz_server" } }, "failure_mode_allow": true } },

And that way I can run an end-2-end system.
i'll remove it since it seems to be not what Envoy natively expects to support.

htuch · 2018-02-16T04:43:42Z

source/common/filter/ext_authz.cc

+  } else {
+    // We can get completion inline, so only call continue if that isn't happening.
+    if (!calling_check_) {
+      filter_callbacks_->continueReading();


I wonder if readDisable(false) above is sufficient, and you don't need to do this; it's docs say it will restart the read pipeline, and redispatch pending data. Does this work for this filter? That could eliminate the need to maintain calling_check_.

Moving readDisable(false) here -which is what u were asking about at the top of this function-makes sense.
From what I understood-which may be totally wrong-is that readDisable(false) was to get the socket buffer pipeline moving and the filter_callbacks_->continueReading() is to get the filter chain iteration moving forward. The filter iteration may have been stopped iff the callCheck()and the onComplete() callback happened asynchronously.
But if they both are invoked in the same call stack then the filter chain was never stopped, hence it doesn't need to be kick started again by invoking filter_callbacks_->continueReading().

I think that filter_callback->connection().readDisable(false|true) will no longer be necessary since now we will invoke the external authz service from onData() which clearly returns either a StopIteration or Continue and NOT from onEvent().

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

htuch

Last couple of things and we can ship. This is looking great, thanks for your patience through the review process.

htuch · 2018-02-16T20:20:12Z

source/server/config/network/ext_authz.cc

+  };
+}
+
+NetworkFilterFactoryCb ExtAuthzConfigFactory::createFilterFactory(const Json::Object& json_config,


Not sure this is strictly needed for v2 JSON, this should only apply to v1 JSON placed in deprecated_v1. Does the test fail if you don't have this?

htuch · 2018-02-16T20:26:32Z

source/server/config/network/ext_authz.cc

+  ASSERT(!proto_config.stat_prefix().empty());
+  ASSERT(!proto_config.grpc_service().envoy_grpc().cluster_name().empty());
+
+  ExtAuthz::TcpFilter::ConfigSharedPtr ext_authz_config(


This file is missing coverage, see https://27311-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_server_config_network_ext_authz.cc.html. Can you add tests? See https://github.com/envoyproxy/envoy/tree/master/test/server/config for examples.

my bad...thanks for catching that. Let me address this.

saumoh · 2018-02-16T22:22:39Z

@htuch Quick clarification please. The NamedNetworkFilterFactory expects an implementation for createFilterFactory.
Should v2 only implementations throw an exception in this case?

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

saumoh · 2018-02-17T01:01:08Z

@htuch Thanks for all your feedback and patience in working through these PR's.
I've added the missing test cases. Based off your recommendation for what to do for v2 only createFilterFactory(). (throw exception/or something else). I'll modify the code and the corresponding test.

htuch · 2018-02-18T04:09:28Z

@saumoh re: NamedNetworkFilterFactory question, see https://github.com/envoyproxy/envoy/blob/master/source/server/config/http/gzip.cc#L23 for example.

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

htuch

Thanks, appreciate all the changes, this is rad.

htuch · 2018-02-20T10:01:13Z

test/server/config/network/config_test.cc

+    )EOF";
+
+  envoy::config::filter::network::ext_authz::v2::ExtAuthz proto_config{};
+  MessageUtil::loadFromJson(json, proto_config);


FYI, we now generally prefer YAML examples and using MessageUtil::loadFromYaml, but we can fix this later or leave it as is, I'd like to unblock your other PR review.

htuch · 2018-02-20T10:42:06Z

@saumoh I'm seeing build failures on master following the merge here, e.g. https://circleci.com/gh/envoyproxy/envoy/27658?utm_campaign=build-failed&utm_content=failing-command&utm_medium=email#tests/containers/0/actions/103

Can you take a look? Thanks. CC @danielhochman who is on-call for maintainer rotation this week.

Root cause: PR was not up-to-date when merging. Signed-off-by: Harvey Tuch <htuch@google.com>

saumoh · 2018-02-20T17:06:30Z

@htuch thanks for addressing the post merge issue. I just got in and was starting to look!

htuch · 2018-02-20T17:09:22Z

@saumoh NP. A good habit when working on PRs with rounds of reviews is to fetch/merge upstream each time you do a push. That way, the chance of this happening is reduced (but not eliminated). Try it out in your next PR review :)

Description: Integrate existing upstream Envoy support for proxies (#21942) with Android APIs that are responsible for getting the information about the configured proxy settings from user's device. There are a few things that we will need to address before we fully productionize this - they are listed in envoyproxy/envoy-mobile#2533. Risk Level: Medium, it's supposed to be a no-op if not enabled explicitly. Testing: Manual and integration Docs Changes: Done Release Notes: Done Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Fix up the comments for failure_mode_allow that was incorrectly worded and caught during review envoyproxy/envoy#2416 Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Support for external authroization tcp filter.

98197b8

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

saumoh mentioned this pull request Jan 19, 2018

Add tcp and http filters for external authorization service. #2359

Closed

mattklein123 assigned htuch Jan 19, 2018

Saurabh Mohan added 17 commits January 23, 2018 14:44

Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp

6c88bd1

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

Merge with gRPC singleton manager.

981ae9d

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

Format fixes for tcp files.

3952f68

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

Fix test tcp filter format.

9da8b85

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

Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp

4fe6572

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

Merge with data-plane-api for tcp.

9c02776

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

Review feedback for tcp files.

25d37c6

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

Fix up network config build.

5dc5495

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

Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp

38d8799

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

Remove mock for proto generation: tcp changes.

ce9c1e9

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

Review comments tcp.

40746cd

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

Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp

8823360

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

Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp

36053cd

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

Merge tcp changes with data-plane-api reorg #2495.

a675ec2

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

Merge remote-tracking branch 'upstream/master' into ext-auth-review-tcp

fa371c3

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

Fixup default timeout.

c8910ad

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

Include network filter config in build.

945ac1c

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

htuch assigned junr03 Feb 7, 2018

junr03 reviewed Feb 9, 2018

View reviewed changes

saumoh mentioned this pull request Feb 12, 2018

External authorization review comments envoyproxy/data-plane-api#491

Merged

Address review comments.

5b41b32

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

htuch suggested changes Feb 14, 2018

View reviewed changes

Saurabh Mohan added 2 commits February 14, 2018 16:39

Review feedback.

a649490

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

Fix tests class name.

d1392ff

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

htuch suggested changes Feb 16, 2018

View reviewed changes

Move callCheck to onData.

8f5db76

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

htuch suggested changes Feb 16, 2018

View reviewed changes

Saurabh Mohan added 3 commits February 16, 2018 16:09

Add tcp filter server configuration tests.

b3dc7bf

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

Fix formatting issue.

4143a3c

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

Formatting fix up with clang-5 not 3.8

a02d949

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

Remove createFilterFactor implementation.

4d13223

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

htuch approved these changes Feb 20, 2018

View reviewed changes

htuch merged commit bae55f9 into envoyproxy:master Feb 20, 2018

htuch mentioned this pull request Feb 20, 2018

Support for external authorization http filter. #2417

Merged

htuch added a commit to htuch/envoy that referenced this pull request Feb 20, 2018

build: fix merge break on master in envoyproxy#2416.

6adf8e9

Root cause: PR was not up-to-date when merging. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch added a commit that referenced this pull request Feb 20, 2018

build: fix merge break on master in #2416. (#2646)

38d8271

Root cause: PR was not up-to-date when merging. Signed-off-by: Harvey Tuch <htuch@google.com>

saumoh deleted the ext-auth-review-tcp branch May 4, 2018 17:28

Conversation

saumoh commented Jan 19, 2018

Uh oh!

htuch commented Feb 7, 2018

Uh oh!

saumoh commented Feb 9, 2018

Uh oh!

junr03 commented Feb 9, 2018

Uh oh!

saumoh commented Feb 9, 2018

Uh oh!

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

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

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!