gRPC client to be used by ext_proc filter by gbrail · Pull Request #14283 · envoyproxy/envoy

gbrail · 2020-12-04T19:04:13Z

This is a set of classes that the ext_proc server will use to abstract talking to the remote server via a gRPC stream.

The ext_proc filter will use this set of classes to abstract handling the gRPC stream it'll use to talk to the remote server. Signed-off-by: Gregory Brail <gregbrail@google.com>

htuch

Exciting to see the implementation arriving :) Mostly just some Envoy-style comments..
/wait

htuch · 2020-12-07T01:23:11Z

source/extensions/filters/http/ext_proc/client.h

+public:
+  virtual ~ExternalProcessorCallbacks() = default;
+  virtual void onReceiveMessage(
+      std::unique_ptr<envoy::service::ext_proc::v3alpha::ProcessingResponse>&& response) = 0;


Nit: prefer using PURE in Envoy style

htuch · 2020-12-07T01:24:49Z

source/extensions/filters/http/ext_proc/client_impl.cc

+    : callbacks_(callbacks) {
+  client_ = std::move(client);
+  auto descriptor = Protobuf::DescriptorPool::generated_pool()->FindMethodByName(kExternalMethod);
+  assert(descriptor != nullptr);


No need for an assert here, since it will crash immediately if not true. Also, we don't use lower case assert in Envoy, prefer ASSERT, RELEASE_ASSERT or `ENVOY_BUG.

Ack. No assert needed for now then.

htuch · 2020-12-07T01:25:36Z

source/extensions/filters/http/ext_proc/client.h

+
+using ExternalProcessorStreamPtr = std::unique_ptr<ExternalProcessorStream>;
+
+class ExternalProcessorCallbacks {


Does this wrapper layer give you much over the existing stream callbacks? Maybe you are planning on doing more in here later?

Good point. In general I wanted to have a clean abstraction to ease unit testing. One difference is that I don't think that we'll be needing to handle the metadata callbacks and this abstraction takes care of them for me.

htuch · 2020-12-07T01:26:32Z

test/extensions/filters/http/ext_proc/client_test.cc

+  Grpc::Status::GrpcStatus grpc_status_ = Grpc::Status::WellKnownGrpcStatus::Ok;
+  bool grpc_closed_ = false;
+
+  std::unique_ptr<ExternalProcessorClientImpl> client_;


Nit: we usually setup an alias with using ExternalProcessClientImplPtr = std::unique.. for these smart pointer types in Envoy.

htuch · 2020-12-07T01:28:27Z

source/extensions/filters/http/ext_proc/client_impl.cc

+static constexpr char kExternalMethod[] =
+    "envoy.service.ext_proc.v3alpha.ExternalProcessor.Process";
+
+ExternalProcessorClientImpl::ExternalProcessorClientImpl(


FYI, we're going to want to have a thread-local cache of aync clients for ext_proc similar to what is done for the gRPC access loggers and ext_authz. This is what I referenced in my comment on your doc a while back. But, I'm hoping that we can land #12598 soon to avoid you needing to have to do this explicitly.

I see that in the ext_authz filter and I was planning to first get the basic thing working and then add that. However I'm watching that PR and if it comes in soon, then even better.

Signed-off-by: Gregory Brail <gregbrail@google.com>

htuch

LGTM modulo nits.
/wait

htuch · 2020-12-08T23:49:04Z

source/extensions/filters/http/ext_proc/BUILD

+)
+
+envoy_cc_library(
+    name = "client_impl",


Nit: Envoy convention is typically client_lib.

htuch · 2020-12-08T23:50:38Z

test/extensions/filters/http/ext_proc/client_test.cc

+TEST_F(ExtProcStreamTest, SendToStream) {
+  auto stream = client_->start(*this, 200ms);
+  // Send something and ensure that we get it. Doesn't really matter what.
+  EXPECT_CALL(stream_, sendMessageRaw_(_, false)).Times(1);


Nit: Times(1) is redundant.

Signed-off-by: Gregory Brail <gregbrail@google.com>

gbrail · 2020-12-09T20:15:21Z

Ready, I think, but Azure won't let me re-run the failed ARM build.

htuch · 2020-12-10T00:47:32Z

test/extensions/filters/http/ext_proc/client_test.cc

  auto stream = client_->start(*this, 200ms);
  // Send something and ensure that we get it. Doesn't really matter what.
-  EXPECT_CALL(stream_, sendMessageRaw_(_, false)).Times(1);
+  EXPECT_CALL(stream_, sendMessageRaw_(_, false));


Lots more of these in this PR, but I actually think I should just add a format check for this and stop harassing people :)

htuch

LGTM, thanks!

* master: buffer: Optimize the layout of Slices in Buffer::OwnedImpl by removing subclassing and storing slice info directly in the SliceDeque (envoyproxy#14282) gRPC client to be used by ext_proc filter (envoyproxy#14283) http2: Add integration tests for PRIORITY frame flood mitigation for upstream servers (envoyproxy#14328) event: touch watchdog before execution of each post callback and before deferred deletion (envoyproxy#14339) stale: more allowed ops (envoyproxy#14345) stale: more changes (envoyproxy#14344) test: TODO fixup making enable_half_close private envoyproxy#14330) event: Reduce potential for lock contention while executing dispatcher post callbacks. (envoyproxy#14289) stale: fix config (envoyproxy#14337) metrics service sink: generalize the sink and grpc streamer for external use (envoyproxy#13919) wasm: update V8 to v8.8.278.8. (envoyproxy#14298) repo: switch to actions based stale bot (envoyproxy#14335) buffer: Use WatermarkFactory to create most WatermarkBuffer instances (envoyproxy#14256) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

gbrail force-pushed the ext-proc-2 branch 2 times, most recently from 44a439b to cbe129f Compare December 4, 2020 21:17

gRPC client to be used by ext_proc filter

3c5fc0b

The ext_proc filter will use this set of classes to abstract handling the gRPC stream it'll use to talk to the remote server. Signed-off-by: Gregory Brail <gregbrail@google.com>

gbrail force-pushed the ext-proc-2 branch from cbe129f to 3c5fc0b Compare December 4, 2020 22:42

gbrail marked this pull request as ready for review December 5, 2020 00:41

gbrail requested a review from htuch as a code owner December 5, 2020 00:41

jmarantz assigned asraa Dec 5, 2020

htuch suggested changes Dec 7, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 7, 2020

Address code review comments

3dcd832

Signed-off-by: Gregory Brail <gregbrail@google.com>

repokitteh-read-only bot removed the waiting label Dec 7, 2020

htuch suggested changes Dec 8, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 8, 2020

Address code review nits

4cfb884

Signed-off-by: Gregory Brail <gregbrail@google.com>

repokitteh-read-only bot removed the waiting label Dec 9, 2020

asraa unassigned asraa Dec 9, 2020

dio assigned htuch Dec 9, 2020

htuch reviewed Dec 10, 2020

View reviewed changes

htuch approved these changes Dec 10, 2020

View reviewed changes

htuch merged commit 04b75b1 into envoyproxy:master Dec 10, 2020

gbrail deleted the ext-proc-2 branch December 11, 2020 23:10


		using ExternalProcessorStreamPtr = std::unique_ptr<ExternalProcessorStream>;

		class ExternalProcessorCallbacks {

Conversation

gbrail commented Dec 4, 2020

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!

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!

gbrail commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants