grpc: Allow to set parent context to a client to propagate stream info #13356

dio · 2020-10-01T15:05:35Z

Commit Message: This patch allows to set parent context which carries the current request stream info to a gRPC async client instance.

Risk Level: Low
Testing: Added
Docs Changes: Updated
Release Notes: Added
Fixes #13345

This patch allows to set parent context to an async client instance. Signed-off-by: Dhi Aurrahman <[email protected]>

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

dio · 2020-10-01T15:08:47Z

@htuch I'm not sure if this is the right way to solve #13345. If you have time could you please tell me your opinion on this? Thank you!

test/common/grpc/async_client_impl_test.cc

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

htuch

@dio makes sense to me!

source/common/grpc/async_client_impl.cc

source/common/grpc/async_client_impl.h

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

repokitteh-read-only · 2020-10-03T03:31:55Z

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: #13356 was synchronize by dio.

see: more, trace.

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

htuch

Looks good, main question is on whether header parser can be simplified..
/wait

htuch · 2020-10-07T00:03:51Z

source/common/router/header_parser.cc

@@ -287,5 +294,26 @@ void HeaderParser::evaluateHeaders(Http::HeaderMap& headers,
  }
 }

+void HeaderParser::evaluateHeaders(Http::HeaderMap& headers,
+                                   const StreamInfo::StreamInfo* stream_info) const {


Could we use a null StreamInfo rather than duplicate the add/remove logic?

Sorry, do you mean to have something like NullStreamInfoImpl?

The only problem with that probably if we have an input that creates StreamInfoHeaderFormatter (or CompoundHeaderFormatter) we need to always act as PlainHeaderFormatter.

For example: given the following HeaderValue:

- name: key value: %DOWNSTREAM_LOCAL_ADDRESS%

This creates StreamInfoHeaderFormatter, if we have NullStreamInfoImpl we need to provide a default value for stream_info.downstreamLocalAddress(), which probably could be misleading?

OK, fair enough, but could we merge the control flow so that we have only one internal evaluateHeadersInt and this can branch at the leaves on what to do?

Have done a bit of refactoring, hope it aligns with your expectation :).

htuch · 2020-10-07T00:05:17Z

test/common/grpc/google_async_client_impl_test.cc

@@ -94,6 +101,36 @@ TEST_F(EnvoyGoogleAsyncClientImplTest, StreamHttpStartFail) {
  EXPECT_TRUE(grpc_stream == nullptr);
 }

+TEST_F(EnvoyGoogleAsyncClientImplTest, MetadataIsInitialized) {


Same comment as above here.

htuch · 2020-10-07T00:07:04Z

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

@@ -57,6 +57,8 @@ void GrpcClientImpl::check(RequestCallbacks& callbacks, Event::Dispatcher& dispa
    }
  }

+  options.setParentContext(Http::AsyncClient::ParentContext{&stream_info});


It might be good one day to be able to have async clients automatically detect this information, maybe by introducing a StreamAsyncClient that can pick up on this context. In any case, I think what you have is fine for now.

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

htuch

LGTM, thanks!

dio added 2 commits October 1, 2020 12:23

async_client: Allow to set parent context

0e79e71

This patch allows to set parent context to an async client instance. Signed-off-by: Dhi Aurrahman <[email protected]>

Fix

beb3aab

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

dio commented Oct 1, 2020

View reviewed changes

test/common/grpc/async_client_impl_test.cc Outdated Show resolved Hide resolved

mattklein123 assigned htuch Oct 1, 2020

dio added 3 commits October 2, 2020 01:32

Fix

48d6b1a

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

Specific

384e0b7

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

Readability

f007431

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

htuch reviewed Oct 2, 2020

View reviewed changes

source/common/grpc/async_client_impl.cc Outdated Show resolved Hide resolved

source/common/grpc/async_client_impl.cc Outdated Show resolved Hide resolved

source/common/grpc/async_client_impl.h Show resolved Hide resolved

dio added 2 commits October 3, 2020 02:27

Google gRPC, ratelimit

b99e510

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

Update docs

4799f1d

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

repokitteh-read-only bot added the api label Oct 3, 2020

dio changed the title ~~WIP: async_client: Allow to set parent context to propagate stream info~~ async_client: Allow to set parent context to propagate stream info Oct 3, 2020

Proto format fix

3bacec8

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

dio changed the title ~~async_client: Allow to set parent context to propagate stream info~~ grpc: Allow to set parent context to a client to propagate stream info Oct 3, 2020

dio added 6 commits October 3, 2020 18:18

Revert

6464ca0

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

Superfluous newline

cbebf57

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

Fix

404b0de

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

Fix

3a8eb53

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

Fix common

15d2204

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

Comment

1ddaa81

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

dio marked this pull request as ready for review October 4, 2020 06:28

dio requested a review from zuercher as a code owner October 4, 2020 06:28

Merge remote-tracking branch 'upstream/master'

a8d188a

htuch suggested changes Oct 7, 2020

View reviewed changes

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

Alpha ordering

b4e491c

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

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

Merge remote-tracking branch 'upstream/master'

f5bbd80

dio added 11 commits October 7, 2020 01:21

Comments

71d1ba2

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

Remove

50240cc

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

Sort

4b4e6d4

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

Try to merge

c68ff48

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

Trigger StreamInfoHeaderFormatter

a98b6b0

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

Make it compact

a21890d

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

Merge remote-tracking branch 'upstream/master'

ce743c6

Fix

6b52ecf

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

Dedup

8add678

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

Rename

468eebf

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

Missing s

51793ab

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

htuch approved these changes Oct 9, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Oct 9, 2020

htuch merged commit e5aa696 into envoyproxy:master Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: Allow to set parent context to a client to propagate stream info #13356

grpc: Allow to set parent context to a client to propagate stream info #13356

dio commented Oct 1, 2020 •

edited

Loading

dio commented Oct 1, 2020

htuch left a comment

repokitteh-read-only bot commented Oct 3, 2020

htuch left a comment

htuch Oct 7, 2020

dio Oct 7, 2020

dio Oct 7, 2020 •

edited

Loading

htuch Oct 7, 2020

dio Oct 8, 2020 •

edited

Loading

htuch Oct 7, 2020

dio Oct 7, 2020

htuch Oct 7, 2020

htuch left a comment

grpc: Allow to set parent context to a client to propagate stream info #13356

grpc: Allow to set parent context to a client to propagate stream info #13356

Conversation

dio commented Oct 1, 2020 • edited Loading

dio commented Oct 1, 2020

htuch left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Oct 3, 2020

htuch left a comment

Choose a reason for hiding this comment

htuch Oct 7, 2020

Choose a reason for hiding this comment

dio Oct 7, 2020

Choose a reason for hiding this comment

dio Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

htuch Oct 7, 2020

Choose a reason for hiding this comment

dio Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

htuch Oct 7, 2020

Choose a reason for hiding this comment

dio Oct 7, 2020

Choose a reason for hiding this comment

htuch Oct 7, 2020

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

dio commented Oct 1, 2020 •

edited

Loading

dio Oct 7, 2020 •

edited

Loading

dio Oct 8, 2020 •

edited

Loading