Skip to content

config: add and populate metadata accessor in ClusterInfo#2345

Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
mrice32:cluster_metadata
Jan 11, 2018
Merged

config: add and populate metadata accessor in ClusterInfo#2345
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
mrice32:cluster_metadata

Conversation

@mrice32
Copy link
Copy Markdown
Member

@mrice32 mrice32 commented Jan 10, 2018

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?

Signed-off-by: Matt Rice <mattrice@google.com>
Copy link
Copy Markdown
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, thanks, small nits.

}
}

// Resolve zero hosts, while using health checking.
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.

nit: this is not really what this test is about. :)

Copy link
Copy Markdown
Member Author

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")
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.

nit: is it possible to add this to the YAML instead for clarity?

Copy link
Copy Markdown
Member Author

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.

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.

It would be clearer to me and I think more instructive for how static config would be written, but I'm fine either way.

Copy link
Copy Markdown
Member Author

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>
htuch
htuch previously approved these changes Jan 11, 2018
virtual const LoadBalancerSubsetInfo& lbSubsetInfo() const PURE;

/**
* @return the configuration metadata for this cluster.
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.

Nit: @return const envoy::api::v2::Metadata& the configuration...

Signed-off-by: Matt Rice <mattrice@google.com>
@mattklein123 mattklein123 merged commit cb132c1 into envoyproxy:master Jan 11, 2018
@mrice32 mrice32 deleted the cluster_metadata branch May 23, 2018 01:30
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* Fix postsubmit.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

* review: use $(bazel info output_path).

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
jpsim added a commit that referenced this pull request Nov 28, 2022
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>
jpsim added a commit that referenced this pull request Nov 29, 2022
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>
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