ext_proc: template gRPC client and move it to common folder by botengyao · Pull Request #38898 · envoyproxy/envoy

botengyao · 2025-03-25T23:01:35Z

This PR will make the gRPC external processor client in l7_ext_proc share with the l4_ext_proc filter.

Commit Message:
Additional Description:
Risk Level: low
Testing:
Docs Changes:
Release Notes:

Signed-off-by: Boteng Yao <boteng@google.com>

repokitteh-read-only · 2025-03-25T23:01:41Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38898 was opened by botengyao.

see: more, trace.

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao · 2025-03-26T15:46:03Z

/retest

Signed-off-by: Boteng Yao <boteng@google.com>

repokitteh-read-only · 2025-03-26T17:55:31Z

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @alyssawilk

🐱

Caused by: #38898 was synchronize by botengyao.

see: more, trace.

RyanTheOptimist

coverage LGTM

yanjunxiang-google · 2025-03-27T18:51:02Z

LGTM in general. Thanks for working on this refactoring!
Left a few nit comments. It's okay if you want to address it in the follow up PR.

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao

thanks for the review, done.

tyxia · 2025-03-28T19:45:50Z

Templates are great to provide flexible interfaces but it also will make interface/code more complicated. As we can see in this PR , template arguments are passed around/specified through the multiple layers in the stack.

Some high level comments:

Do you have the e2e working prototype of using this interface in L4 ext_proc?
Can (or will it be possible that) this gRPC client be generic enough to be shared with clients other than ext_proc?
Have you explored the techniques like CTAD; default template argument etc to reduce template specification?

It seems to be a major change, I don't have strong opinion if 1 above is working

botengyao · 2025-03-28T20:09:04Z

Templates are great to provide flexible interfaces but it also will make interface/code more complicated. As we can see in this PR , template arguments are passed around/specified through the multiple layers in the stack.

Some high level comments:

Do you have the e2e working prototype of using this interface in L4 ext_proc?

Can (or will it be possible that) this gRPC client be generic enough to be shared with clients other than ext_proc?

Have you explored the techniques like CTAD; default template argument etc to reduce template specification?

It seems to be a major change, I don't have strong opinion if 1 above is working

Thanks Tianyu for the review. yes it will work with the new l4 ext_proc, and I think this can be shared for other cases as well for different message types.

Good to know CTAD, I will explore it, can it be a follow up? Thanks!

Signed-off-by: Boteng Yao <boteng@google.com>

tyxia

Follow-up actions are fine to me.
it will be great to reduce the code complexity (i.e., have easy to maintain code) while leveraging's template's power of writing generic code.

…xy#38898) This PR will make the gRPC external processor client in l7_ext_proc share with the l4_ext_proc filter. Commit Message: Additional Description: Risk Level: low Testing: Docs Changes: Release Notes: --------- Signed-off-by: Boteng Yao <boteng@google.com>

@botengyao

In envoyproxy#38898 @botengyao added an exception for the ext_proc tests for coverage. That's because clang source based coverage reported missing coverage for the comments (the desirable behavior here is that comments are not counted in the coverage reports). Digging through it, what I realized is that for only some tests report counters for the comments. And all the tests that report thos bogus counters don't actually exercise the affected code. I still don't fully understand what triggers the issue here, but that observation suggests a workaround that this PR implements. I basically remove dependency on the grpc_client_impl.h from the tests that don't need it. I started a thread on LLVM discussions, but given that I couldn't come up with a small enough reproducer, I don't think that we can resolve it quickly. That being said, removing unnecessary includes and moving functions from headers to cc files is in general a good strategy as it reduces depndencies between translation units and reduces the amount of work compiler has to do, so I think we can still push this workaround upstream, even though I don't have a root cause for the problem. Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>

@botengyao

Commit Message: In #38898 @botengyao added an exception for the ext_proc tests for coverage. That's because clang source based coverage reported missing coverage for the comments (the desirable behavior here is that comments are not counted in the coverage reports). Digging through it, what I realized is that for only some tests report counters for the comments. And all the tests that report thos bogus counters don't actually exercise the affected code. I still don't fully understand what triggers the issue here, but that observation suggests a workaround that this PR implements. I basically remove dependency on the grpc_client_impl.h from the tests that don't need it. I started a thread on LLVM discussions (https://discourse.llvm.org/t/llvm-source-based-coverage-sometimes-generates-counter-for-comments/85730), but given that I couldn't come up with a small enough reproducer, I don't think that we can resolve it quickly. That being said, removing unnecessary includes and moving functions from headers to cc files is in general a good strategy as it reduces depndencies between translation units and reduces the amount of work compiler has to do, so I think we can still push this workaround upstream, even though I don't have a root cause for the problem. Additional Description: It was reported in #37911, but other than that I don't think it's relevant. Risk Level: n/a Testing: regular CI tests, but with coverage targets changed to remove the exception for the ext_proc. Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a --------- Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>

…xy#38898) This PR will make the gRPC external processor client in l7_ext_proc share with the l4_ext_proc filter. Commit Message: Additional Description: Risk Level: low Testing: Docs Changes: Release Notes: --------- Signed-off-by: Boteng Yao <boteng@google.com>

Added the basic ext_proc gRPC callout functions without the data loss that was addressed in #38778. gRPC client was shared with HTTP ext_proc that was templated by #38898 Pending: * COUNTER will be in the followup RPs. * filter_metadata handling is pending. * log info is pending. * timeout setting is pending * flow control is pending Commit Message: Additional Description: Risk Level: low Testing: #38721 --------- Signed-off-by: Boteng Yao <boteng@google.com>

botengyao added 2 commits March 25, 2025 18:25

add stream initial

33a6776

Signed-off-by: Boteng Yao <boteng@google.com>

tempate ext_proc client

ed6b842

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao added 3 commits March 25, 2025 23:10

remove envoy package

6b66fed

Signed-off-by: Boteng Yao <boteng@google.com>

fix tests

7439a33

Signed-off-by: Boteng Yao <boteng@google.com>

clean up

6635983

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao marked this pull request as ready for review March 26, 2025 01:27

botengyao requested review from mattklein123, tyxia and yanavlasov as code owners March 26, 2025 01:27

botengyao mentioned this pull request Mar 26, 2025

Make Envoy build with newer versions of LLVM toolchain (e.g., clang-18) #37911

Closed

Merge remote-tracking branch 'upstream/main' into grpc_client_refactor

4102614

Signed-off-by: Boteng Yao <boteng@google.com>

ggreenway assigned yanjunxiang-google Mar 26, 2025

add coverage lift

1681c7b

Signed-off-by: Boteng Yao <boteng@google.com>

repokitteh-read-only Bot assigned alyssawilk Mar 26, 2025

botengyao assigned RyanTheOptimist and unassigned alyssawilk Mar 26, 2025

RyanTheOptimist previously approved these changes Mar 27, 2025

View reviewed changes

yanjunxiang-google approved these changes Mar 27, 2025

View reviewed changes

Comment thread source/extensions/filters/common/ext_proc/grpc_client.h Outdated

Comment thread source/extensions/filters/common/ext_proc/grpc_client.h Outdated

Comment thread source/extensions/filters/common/ext_proc/grpc_client.h Outdated

split the interface

0923503

Signed-off-by: Boteng Yao <boteng@google.com>

botengyao dismissed RyanTheOptimist’s stale review via 0923503 March 27, 2025 20:36

botengyao commented Mar 27, 2025

View reviewed changes

Comment thread source/extensions/filters/common/ext_proc/grpc_client.h Outdated

Comment thread source/extensions/filters/common/ext_proc/grpc_client.h Outdated

Comment thread source/extensions/filters/common/ext_proc/grpc_client.h Outdated

botengyao assigned tyxia Mar 27, 2025

botengyao added 2 commits March 28, 2025 20:59

Merge remote-tracking branch 'upstream/main' into grpc_client_refactor

7a367ef

Signed-off-by: Boteng Yao <boteng@google.com>

fix format

b5ef75e

Signed-off-by: Boteng Yao <boteng@google.com>

tyxia approved these changes Mar 30, 2025

View reviewed changes

botengyao merged commit ac05cf0 into envoyproxy:main Mar 31, 2025
24 checks passed

botengyao deleted the grpc_client_refactor branch March 31, 2025 19:54

krinkinmu mentioned this pull request Apr 7, 2025

Switch back to static linking for coverage tests #39030

Closed

krinkinmu mentioned this pull request Apr 8, 2025

Fix coverage reports for ext_proc tests #39041

Merged

botengyao mentioned this pull request Apr 22, 2025

network: added network ext proc basic callout functions #39185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext_proc: template gRPC client and move it to common folder#38898

ext_proc: template gRPC client and move it to common folder#38898
botengyao merged 10 commits intoenvoyproxy:mainfrom
botengyao:grpc_client_refactor

botengyao commented Mar 25, 2025 •

edited

Loading

Uh oh!

repokitteh-read-only Bot commented Mar 25, 2025

Uh oh!

botengyao commented Mar 26, 2025

Uh oh!

repokitteh-read-only Bot commented Mar 26, 2025

Uh oh!

RyanTheOptimist left a comment

Uh oh!

yanjunxiang-google commented Mar 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

botengyao left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tyxia commented Mar 28, 2025 •

edited

Loading

Uh oh!

botengyao commented Mar 28, 2025

Uh oh!

tyxia left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

botengyao commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Mar 25, 2025

Uh oh!

botengyao commented Mar 26, 2025

Uh oh!

repokitteh-read-only Bot commented Mar 26, 2025

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Mar 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

botengyao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tyxia commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

botengyao commented Mar 28, 2025

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

botengyao commented Mar 25, 2025 •

edited

Loading

tyxia commented Mar 28, 2025 •

edited

Loading