Skip to content

api: Add mechanism for filters to define route action.#8956

Merged
htuch merged 6 commits intoenvoyproxy:masterfrom
markdroth:route_action_override
Nov 21, 2019
Merged

api: Add mechanism for filters to define route action.#8956
htuch merged 6 commits intoenvoyproxy:masterfrom
markdroth:route_action_override

Conversation

@markdroth
Copy link
Copy Markdown
Contributor

@markdroth markdroth commented Nov 8, 2019

Description: Add a mechanism for a filter to define the action for a route.
Risk Level: Low
Testing: N/A
Docs Changes: Inline with proto change.
Release Notes: N/A
Fixes #8953.

@htuch and @mattklein123, I'm proposing this as a simple bool, because I'm not sure what else we'd need to put in this field. But I'm also fine with changing it to an Any, as we discussed. Discussion welcome.

Signed-off-by: Mark D. Roth <roth@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8956 was opened by markdroth.

see: more, trace.


// [#not-implemented-hide:]
// If true, a filter will overwrite some or all of the RouteAction proto.
bool filter_overwrites_route_action = 17;
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.

I thought we had discussed adding a FilterAction into the action oneof below with an Any?

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.

Ah, okay -- that makes sense. I think I misunderstood your suggestion earlier.

Is this more like what you had in mind?

Signed-off-by: Mark D. Roth <roth@google.com>
@markdroth markdroth changed the title api: Add filter_overwrites_route_action field in Route proto. api: Add mechanism for filters to define route action. Nov 8, 2019
message FilterAction {
oneof action {
google.protobuf.Any filter_action = 1;
google.protobuf.Struct typed_filter_action = 2;
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.

Yep, this is what I was thinking. The nifty thing is that it's possible to stuff a serialized RouteAction in here, with missing cluster for your use case, so no need to redefine any protos for that.

@htuch htuch added the waiting label Nov 8, 2019
@htuch htuch self-assigned this Nov 11, 2019
Signed-off-by: Mark D. Roth <roth@google.com>
DirectResponseAction direct_response = 7;

// [#not-implemented-hide:]
// If true, a filter will define the RouteAction.
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.

Question: Should this have the ability to define just RouteAction, or could it also generate a RedirectAction or a DirectResponseAction?

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.

Yeah, maybe leave this as "the action" and we can offer multiple internal interfaces. We'll still only have a single Any as config though.

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.

Done.

Signed-off-by: Mark D. Roth <roth@google.com>
htuch
htuch previously approved these changes Nov 17, 2019
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, will hand off to @mattklein123 to merge.

Signed-off-by: Mark D. Roth <roth@google.com>
@mattklein123
Copy link
Copy Markdown
Member

LGTM, thanks. Can you merge master to hopefully fix CI?

/wait

…ride

Signed-off-by: Mark D. Roth <roth@google.com>
@htuch htuch merged commit a907cff into envoyproxy:master Nov 21, 2019
@markdroth markdroth deleted the route_action_override branch November 22, 2019 16:40
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.

Need a way for RouteAction to indicate that a filter will choose the cluster

4 participants