Skip to content

Conversation

yuval-k
Copy link
Contributor

@yuval-k yuval-k commented Apr 3, 2018

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)

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Copy link
Member

@mattklein123 mattklein123 left a 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!

// 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
Copy link
Member

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>
Copy link
Member

@mattklein123 mattklein123 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 leave for a bit to see if anyone else has any comments.

@yuval-k
Copy link
Contributor Author

yuval-k commented Apr 3, 2018

Sounds good, thank you!

@htuch
Copy link
Member

htuch commented Apr 3, 2018

LGTM with one question: does it make sense to treat this as cluster metadata or route metadata?

@yuval-k
Copy link
Contributor Author

yuval-k commented Apr 3, 2018

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.

@htuch
Copy link
Member

htuch commented Apr 3, 2018

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.

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.

3 participants