Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: add NonForwardingAction route action type #16144

Merged
merged 2 commits into from
Apr 28, 2021

Conversation

markdroth
Copy link
Contributor

Signed-off-by: Mark D. Roth [email protected]

Commit Message: api: add NonForwardingAction route action type
Additional Description: Adds a new type of route action that is used in cases when not forwarding the request to an upstream server. This will be used by xDS-aware gRPC servers. It could also be used in the future by Envoy if there is a filter that directly generates responses to requests.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A

Continuing discussion from #13366.

CC @htuch @ejona86 @dfawley

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16144 was opened by markdroth.

see: more, trace.

// without forwarding to an upstream host. This will be used in non-proxy
// xDS clients like the gRPC server. It could also be used in the future
// in Envoy for a filter that directly generates responses for requests.
NonForwardingAction non_forwarding_action = 18;
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense. Just thinking loudly, and probably not worth the churn, shall other 'non-forwarding' actions (e.g. direct response and redirect) to be included here in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they'll probably remain separate. The main difference between those existing action types and this one is that those existing action types explicitly define everything that's needed to process the request (e.g., "reply with this specific HTTP status" or "redirect to this specific URL"), whereas this new type of action is basically saying "we won't be forwarding the request, but we're leaving it undefined how it will actually be handled". Some external code (e.g., the gRPC server application or an Envoy filter) will have to actually decide how to respond to the request.

Copy link
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, will defer to @lizan for the rest of the review.

@@ -229,11 +229,18 @@ message Route {
DirectResponseAction direct_response = 7;

// [#not-implemented-hide:]
// If true, a filter will define the action (e.g., it could dynamically generate the
// A filter-defined action (e.g., it could dynamically generate the
Copy link
Member

Choose a reason for hiding this comment

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

Nit: weird word wrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Signed-off-by: Mark D. Roth <[email protected]>
@yanavlasov yanavlasov merged commit f51e927 into envoyproxy:main Apr 28, 2021
@markdroth markdroth deleted the route_non_forwarding_action branch April 28, 2021 16:18
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
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.

4 participants