admin: complete /config_dump #3340
Conversation
This completes initial implementation of the /config_dump admin endpoint, allowing for detailed display of the currently loaded Envoy configuration, including versions of each resource. Fixes #2421 Fixes #2172 Signed-off-by: Matt Klein <[email protected]>
| auto& dynamic_listener = *config_dump->mutable_dynamic_draining_listeners()->Add(); | ||
| dynamic_listener.set_version_info(listener.listener_->versionInfo()); | ||
| dynamic_listener.mutable_listener()->MergeFrom(listener.listener_->config()); | ||
| } |
There was a problem hiding this comment.
Can we factor out some of these copy loops in LDS/CDS?
There was a problem hiding this comment.
I thought about that, but the loops are subtly different, and I'm really not sure it's worth it to have a bunch of small functions here.
| @@ -79,15 +78,17 @@ message ListenersConfigDump { | |||
| // configuration information can be used to recreate an Envoy configuration by populating all | |||
| // clusters as static clusters or by returning them in a CDS response. | |||
| message ClustersConfigDump { | |||
| // This is the *version_info* in the last processed CDS discovery response. If there | |||
| // are only static bootstrap clusters, this field will be "". | |||
| // This is the :ref:`version_info <envoy_api_field_DiscoveryResponse.version_info>` in the | |||
There was a problem hiding this comment.
Is EDS supported? This would be needed for a complete dump I think, right?
There was a problem hiding this comment.
It's not supported currently. Host information is available via the /clusters endpoint and how EDS is handled via the config tracker interface is not super simple. I would rather have someone do this in a follow up if this is important to them. I don't plan on doing this as part of this change. Happy to document that somewhere if you like.
There was a problem hiding this comment.
If you can add some TODOs and file an issue, that fine. Maybe also change the PR description to indicate this is mostly there but not EDS yet.
| virtual ~LdsApi() {} | ||
|
|
||
| /** | ||
| * @return the last received version by the xDS API for LDS. |
There was a problem hiding this comment.
Nit on return type in @return. I think we're at the point that this is not being enforced consistently..
| /** | ||
| * Instruct the listener manager to create an LDS API provider. This is a separate operation | ||
| * during server initialization because the listener manager is created prior to several core | ||
| * pieces of the server existing. |
| : server_(server), factory_(listener_factory), stats_(generateStats(server.stats())) { | ||
| : server_(server), factory_(listener_factory), stats_(generateStats(server.stats())), | ||
| config_tracker_entry_(server.admin().getConfigTracker().add( | ||
| "listeners", [this] { return dumpListenerConfigs(); })) { |
There was a problem hiding this comment.
Where are all these names, e.g. "listeners", documented?
|
|
||
| envoy::admin::v2alpha::ClustersConfigDump expected_clusters_config_dump; | ||
| MessageUtil::loadFromYaml(expected_dump_yaml, expected_clusters_config_dump); | ||
| EXPECT_EQ(expected_clusters_config_dump.DebugString(), clusters_config_dump.DebugString()); |
There was a problem hiding this comment.
FWIW, we already have in test/test_common/utility.h protoEqual(). I seem to recall there is a way to compare protos with order invariance, but maybe that is only internal.
There was a problem hiding this comment.
The nice thing about comparing DebugString() directly is EXPECT_EQ will show you a diff of what is different which is pretty useful...
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
|
@htuch EDS issue opened, PR description updated, comments addressed. |
|
Oops nevermind sorry going to add more docs. Hold on review. |
Signed-off-by: Matt Klein <[email protected]>
|
@htuch OK ready for another pass. |
This completes initial implementation of the /config_dump admin endpoint,
allowing for detailed display of the currently loaded Envoy configuration,
including versions of each resource. Note that this does not include output
for EDS. That work is tracked in #3362.
Fixes #2421
Fixes #2172
Risk Level: Low
Testing: Unit/integration
Docs Changes: Added
Release Notes: Added