-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
config: add and populate metadata accessor in ClusterInfo #2345
Conversation
Signed-off-by: Matt Rice <mattrice@google.com>
2820f54
to
f223c07
Compare
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, thanks, small nits.
@@ -706,6 +707,34 @@ TEST(PrioritySet, Extend) { | |||
} | |||
} | |||
|
|||
// Resolve zero hosts, while using health checking. |
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: this is not really what this test is about. :)
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.
Haha, nice catch
)EOF"; | ||
|
||
auto cluster_config = parseClusterFromV2Yaml(yaml); | ||
Config::Metadata::mutableMetadataValue(*cluster_config.mutable_metadata(), "com.bar.foo", "baz") |
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: is it possible to add this to the YAML instead for clarity?
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.
Yep, I only did it this way because the yaml for metadata is a little obfuscated/ugly:
...
metadata: { filter_metadata: { com.bar.foo: { baz: test_value } } }
...
Totally cool doing it that way as long as that seems clearer.
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.
It would be clearer to me and I think more instructive for how static config would be written, but I'm fine either way.
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.
Gotcha. That's a good point - I didn't see another example of metadata in yaml or json.
Signed-off-by: Matt Rice <mattrice@google.com>
Signed-off-by: Matt Rice <mattrice@google.com>
include/envoy/upstream/upstream.h
Outdated
@@ -428,6 +430,11 @@ class ClusterInfo { | |||
* @return the configuration for load balancer subsets. | |||
*/ | |||
virtual const LoadBalancerSubsetInfo& lbSubsetInfo() const PURE; | |||
|
|||
/** | |||
* @return the configuration metadata for this cluster. |
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: @return const envoy::api::v2::Metadata& the configuration...
Signed-off-by: Matt Rice <mattrice@google.com>
* Fix postsubmit. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: use $(bazel info output_path). Signed-off-by: Piotr Sikora <piotrsikora@google.com>
We may be interested in experimenting with network monitoring & interface switching completely disabled to assess the impact of this feature. Disabling network switching is not something we recommend generally. Risk Level: Low, adds the ability to disable network switching on an opt-in basis Testing: Updated unit tests Docs Changes: Added Release Notes: Added Signed-off-by: JP Simard <jp@jpsim.com>
We may be interested in experimenting with network monitoring & interface switching completely disabled to assess the impact of this feature. Disabling network switching is not something we recommend generally. Risk Level: Low, adds the ability to disable network switching on an opt-in basis Testing: Updated unit tests Docs Changes: Added Release Notes: Added Signed-off-by: JP Simard <jp@jpsim.com>
Description: Made the cluster metadata field visible in ClusterInfo for access by filters and access logs.
Risk Level: Low
Testing: Added a small unit test to ensure that the metadata could be populated and extracted from ClusterInfo.
Docs Changes: envoyproxy/data-plane-api#408
Release Notes: Does this change require release notes?