Skip to content

ext_proc: Support per-route GrpcService configuration.#20757

Merged
htuch merged 3 commits intoenvoyproxy:mainfrom
mpwarres:ext_proc_grpc_service
Apr 19, 2022
Merged

ext_proc: Support per-route GrpcService configuration.#20757
htuch merged 3 commits intoenvoyproxy:mainfrom
mpwarres:ext_proc_grpc_service

Conversation

@mpwarres
Copy link
Copy Markdown
Contributor

This adds the ability to change the GrpcService used by the ext_proc filter on a per-route basis.

Signed-off-by: Michael Warres mpw@google.com

Commit Message: ext_proc: Support per-route GrpcService configuration
Additional Description:
Risk Level: Low. Not triggered unless configured.
Testing: New unit and integration tests added.
Docs Changes: Addition of new config field.
Release Notes:
Platform Specific Features:

Signed-off-by: Michael Warres <mpw@google.com>
@mpwarres mpwarres requested a review from snowp as a code owner April 10, 2022 19:34
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20757 was opened by mpwarres.

see: more, trace.

@mpwarres
Copy link
Copy Markdown
Contributor Author

Requesting review from ext_proc owners.

/assign @chaoqin-li1123 @pradeepcrao

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much scalability in # services do you need? I don't think this is going to be any different than regular Envoy cluster or gRPC client scalability concerns, but worth keeping in mind if you have many different services across routes.

@mpwarres
Copy link
Copy Markdown
Contributor Author

How much scalability in # services do you need? I don't think this is going to be any different than regular Envoy cluster or gRPC client scalability concerns, but worth keeping in mind if you have many different services across routes.

As discussed offline, we don't anticipate a large number of services in the near term, and will look to other approaches to address future scaling, such as having ExtProc set metadata based on per-route config that could then be read and acted upon by a (single) grpc_service's upstream filter.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @snowp can you take a non-Googler pass over this for general applicability? Thanks.

}

void processAndRespondImmediately(bool first_message,
void processAndRespondImmediately(FakeUpstream& grpc_upstream, bool first_message,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: do you think organizing these gRPC upstream methods inside a class would make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be cleaner, and allow for more flexible tests in the future with separate processor_connection_ and processor_stream_ for each FakeExternalProcessor.

I'd prefer to do that in a follow-on PR, though, to delineate the test refactor from the behavior additions in this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's fine.

chaoqin-li1123
chaoqin-li1123 previously approved these changes Apr 13, 2022
Signed-off-by: Michael Warres <mpw@google.com>
@htuch
Copy link
Copy Markdown
Member

htuch commented Apr 18, 2022

@snowp PTAL when you get a chance, thanks.

Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, thanks!

@snowp
Copy link
Copy Markdown
Contributor

snowp commented Apr 18, 2022

Needs main merge

/wait

Signed-off-by: Michael Warres <mpw@google.com>
@mpwarres
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews! Have merged from main.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@htuch htuch merged commit 0e8899c into envoyproxy:main Apr 19, 2022
@mpwarres mpwarres deleted the ext_proc_grpc_service branch April 19, 2022 03:31
vehre-x41 pushed a commit to vehre-x41/envoy that referenced this pull request Apr 19, 2022
)

This adds the ability to change the GrpcService used by the ext_proc filter on a per-route basis.

Risk Level: Low. Not triggered unless configured.
Testing: New unit and integration tests added.
Docs Changes: Addition of new config field.

Signed-off-by: Michael Warres <mpw@google.com>
Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
)

This adds the ability to change the GrpcService used by the ext_proc filter on a per-route basis.

Risk Level: Low. Not triggered unless configured.
Testing: New unit and integration tests added.
Docs Changes: Addition of new config field.

Signed-off-by: Michael Warres <mpw@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants