-
Notifications
You must be signed in to change notification settings - Fork 273
add cluster metadata to a route #598
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
add cluster metadata to a route #598
Conversation
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, small comment. Thanks!
envoy/api/v2/route/route.proto
Outdated
// For instance, if the metadata is intended for the Router filter, | ||
// the filter name should be specified as *envoy.router*. | ||
// | ||
// On runtime, this object will a merged with the cluster_metadata from the enclosing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please ref link to the enclosing "cluster_metadata" field.
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM will leave for a bit to see if anyone else has any comments.
Sounds good, thank you! |
LGTM with one question: does it make sense to treat this as cluster metadata or route metadata? |
its a bit of both - its the metadata of the cluster chosen for routing. specifically it's useful in the case of traffic splitting where you don't know in advance which cluster will be chosen. |
OK, seems legit. I had originally envisaged having all the metadata that factors into a particular route or cluster selection being merged anyway prior to hitting logging/stats. That didn't end up happening, but this would fit that model if we do go there some day. |
Per envoyproxy/envoy#2851, adding metadata object to the specific routed cluster. envoy patch is ready and a PR for envoy will be created when this PR is merged (so I can update the repository_locations.bzl)