[envoy/admin] Scaffolding for structured admin and config_dump endpoint #532

jsedgwick · 2018-03-09T21:00:21Z

I will add documentation once the interfaces and placement of these things is settled. Current location (new admin package) was agreed upon as a good start with @htuch but i don't feel strongly about that or naming.

Signed-off-by: James Sedgwick jsedgwick@lyft.com

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

htuch

Thanks for starting this initiative.

htuch · 2018-03-09T21:16:05Z

envoy/admin/admin.proto

@@ -0,0 +1,17 @@
+syntax = "proto3";


This should be envoy/admin/v2/admin.proto as per versioning elsewhere in data-plane-api.

Kk. So basically the whole repo is v2?

At this point, yep. In the future we may have independent subtrees evolve at different rates, but this is probably medium-long term.

htuch · 2018-03-09T21:17:35Z

envoy/admin/admin.proto

+import "gogoproto/gogo.proto";
+
+message ConfigDump {
+  map<string, google.protobuf.Any> configs = 1 [(gogoproto.nullable) = false];


Can you add comments? It's hard to know what this very general type is representing.

+1. In general, we should add full comments here for which admin endpoints these map to, how they are used, etc. It's not completely clear to me whether we want strongly typed output (maybe we do) vs. something general like this type.

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

jsedgwick · 2018-03-12T03:19:08Z

Moved to v2/ and added some comments.

Btw, I tried to put in the protodoc :ref:s but I don't think I had the format right. Besides, not sure where we would want these hooked in. Is our doc generator setup documented anywhere besides what's in CONTRIBUTING.md now?

mattklein123

Nice. LGTM. Just a few comments from my perspective.

mattklein123 · 2018-03-12T16:02:28Z

envoy/admin/v2/admin.proto

@@ -0,0 +1,31 @@
+syntax = "proto3";


Can we move this to config_dump.proto? I think we should have a discrete proto file for each endpoint in the admin package.

Yeah, agreed, was just using a catch-all to punt on naming. Will do.

mattklein123 · 2018-03-12T16:03:55Z

envoy/admin/v2/admin.proto

+
+import "gogoproto/gogo.proto";
+
+// [#protodoc-title: ConfigDump]


I wouldn't worry about RST docs right now for this. There is a generally open item for how to document APIs in RST. Right now we don't really do it. We just document configuration. (In any case I don't think it's critical that these protos have HTML docs right now). We can figure it out in the context of https://github.com/envoyproxy/data-plane-api/issues/351.

Sounds good. I didn't realize we weren't generating docs for the APIs, good that's it's on the list.

mattklein123 · 2018-03-12T16:05:27Z

envoy/admin/v2/admin.proto

+
+// The /config_dump admin endpoint uses this wrapper message to maintain and serve arbitrary
+// configuration information from any component in Envoy.
+message ConfigDump {


I don't think we need to do it right now, but in the future I think we want to make each endpoint a full RPC with a request and response message. This will allow us to serve both binary gRPC/REST and take parameters, etc. Can you potentially add a TODO here to that effect?

htuch · 2018-03-13T17:33:28Z

envoy/admin/v2/admin.proto

+  // routing subsystem might use "routes" as the key for its config, for which it uses the message
+  // RouteConfigDump as defined below. In the future, the key will also be used to filter the output
+  // of the /config_dump endpoint.
+  map<string, google.protobuf.Any> configs = 1 [(gogoproto.nullable) = false];


@louiscryan advocated that we maintain these in a format suitable for replay. For that to work, I think the string here should be the type URL. We need to think how this is going to work with #527. We should have a doc for this by later this week. I think we might want this to be something like type URL -> resource name -> (resource proto x version) or something like this. It depends what we settle on for versioning in incremental xDS.

Yeah +1 it would be nice to just have version be part of this, then we can also solve envoyproxy/envoy#2172.

I originally had that but saw that Any has the type_url, as in https://gist.github.com/jsedgwick/3a46408a9728ccef9a63d32b4c463c2b
Now, it's missing the version - is that a deficiency of Any?
So currently it's more like resource name -> type URL -> resource proto'. Does it make more sense to group type_url` and the actual resource, just like with Any? The keep the resource name top level and bundle in the version at the end.

Anyways, bikeshedding, we can just hold on that until you guys settle on versioning.

You don't need to wait on the versioning. Just update the map target to be a struct which contains a string version and an Any object.

Hold on, what exactly is the version we're encoding? Or is that what's pending resolution? Shall I just fill the field with empty strings?

I see. But in this case, those belong to RouteConfiguration, not RouteConfigDump, no? Which version would pair with the RouteConfigDump in the Any?

RDS is a bit of a tricky one, since technically without ADS there can be multiple RDS streams, 1 per HTTP connection manager. In that case, yes, the version likely needs to go with the route configuration. For the other cases it could go at the top level (CDS, LDS, etc.).

Another way of doing it, probably better, would be to track the discovery response version that carried a discrete cluster, route, listener, etc. and print that along with each object. This would work for incremental updates better. So maybe that's that we should do. In light of that I would take a look at the message structure and figure out a nesting that works. I don't think you need to wait until the incremental stuff is decided.

@mattklein123 One of the possible incremental xDS option is to have a version at resource level. Let's discuss there and circle back here.

@htuch OK SGTM.

htuch · 2018-03-13T17:33:53Z

envoy/admin/v2/admin.proto

+message ConfigDump {
+  // This map is serialized and dumped in its entirety at the /config_dump endpoint.
+  //
+  // Keys should be a short descriptor of the config object they map to. For example, envoy's HTTP


Nit: s/envoy/Envoy/

htuch · 2018-03-13T17:34:41Z

envoy/admin/v2/admin.proto

+// their RouteConnfiguration objects. Static routes configured in the bootstrap configuration are
+// separated from those configured dynamically via RDS. This message is available at the
+// /config_dump admin endpoint.
+message RouteConfigDump {


This is a good point; we want to be able to dump static and dynamic information. What if we have an endpoint to dump the entire bootstrap? Would that be sufficient for covering the static requirements?

Yes we should definitely have a bootstrap dump option. I could go either way on whether that covers though, because for example a route table can come as part of a static LDS route. I think it's probably OK for "routes" to dump both static and dynamic.

Yes, the Bootstrap should definitely have a dump. Is that the sort of thing I should be opening issues for? I'm keeping notes locally so I don't forget.

More to the point, it seems helpful to have this distinction available without going for the bootstrap dump. IMO RouteConfiguration could use a flag indicating its static/dynamic origin but maybe that ship has sailed.

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

mattklein123 · 2018-03-14T03:43:55Z

@jsedgwick my advice is to mark this proto as "draft" type (see other examples) and go ahead and finish up the envoy side PR. We can add in version info next as we are saying the proto is not finalized.

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

mattklein123

LGTM. Thanks. @htuch any further comments? We can add version info in a follow up once we finalize that proposal.

htuch

LGTM to unblock initial implementation work. We should do another API design review before moving from draft status, while the protos are fully flexible.

htuch · 2018-03-15T20:55:12Z

envoy/admin/v2/config_dump.proto

+  // This map is serialized and dumped in its entirety at the /config_dump endpoint.
+  //
+  // Keys should be a short descriptor of the config object they map to. For example, envoy's HTTP
+  // routing subsystem might use "routes" as the key for its config, for which it uses the message


I think we should consider making this a type URL in the future. I'm still after a config that could be effectively replayed via an xDS management server.

This is a step towards dumping configs in proto form. There is a new config_dump endpoint, which spits out all registered configs. It's pretty-printed JSON for now; obviously we can play with format/serialization. Anyone can add a config via the ConfigTracker object that hangs off Admin. ConfigTracker makes the whole thing RAII-y, so that the list of config providers is maintained opaquely. This means the end user doesn't have to worry about ownership, capturing this, etc. Contrast with current unenforced pair of addHandler/removeHandler. This sort of managed weak ownership can be useful all over envoy; just in this diff I left a comment where something like ConfigTracker would delete a bunch of fragile code and confusion. I will genericize it in a future diff though, so let's focus on the functionality here and try to punt on extended discussion of this component and its possibilities. To demonstrate all the structured admin/config dumping scaffolding, I implemented it all for routes. See the data-plane-api PR (envoyproxy/data-plane-api#532) for context. But you can see how other config_dump handlers and, hopefully, all admin endpoints can be turned into structured, arbitrarily consumable proto via this setup. Risk Level: Medium. New feature on admin subsystem, intended to be minimally intrusive into rest of server Testing: New + existing unit. Tested locally with sudo ./bazel-bin/source/exe/envoy-static -c examples/front-proxy/front-envoy.yaml Output looks like: https://gist.github.com/jsedgwick/3a46408a9728ccef9a63d32b4c463c2b Docs Changes: I put TODO doxme everywhere I plan to add docs once the code/API is agreed upon. I won't merge this PR until docs are in. Release Notes: TODO pending consensus on this PR Issues: Makes progress on #2421, #2172 API Changes:] envoyproxy/data-plane-api#532 Deprecated: Current routes endpoint should be considered marked for deletion, but I'll do that in a subsequent PR. There should be some nice cleanup in addition to code deletion with this change. Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

tmp

3269cd6

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

jsedgwick mentioned this pull request Mar 9, 2018

[admin] config_dump scaffolding and routes endpoint envoyproxy/envoy#2771

Merged

htuch suggested changes Mar 9, 2018

View reviewed changes

James Sedgwick added 2 commits March 11, 2018 19:59

add docs

a8d954e

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

move to a v2/ subdir

01f2d00

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

mattklein123 reviewed Mar 12, 2018

View reviewed changes

jsedgwick mentioned this pull request Mar 13, 2018

bazel: better support for temporarily using a local dependency repository envoyproxy/envoy#2781

Merged

htuch reviewed Mar 13, 2018

View reviewed changes

add TODO, fix package, fix BUILD

2b9a9ce

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

mark as draft

3084be8

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

mattklein123 approved these changes Mar 14, 2018

View reviewed changes

htuch approved these changes Mar 15, 2018

View reviewed changes

htuch merged commit ed9e1a1 into envoyproxy:master Mar 15, 2018

jsedgwick deleted the config_dump branch March 15, 2018 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[envoy/admin] Scaffolding for structured admin and config_dump endpoint #532

[envoy/admin] Scaffolding for structured admin and config_dump endpoint #532

jsedgwick commented Mar 9, 2018

htuch left a comment

htuch Mar 9, 2018

jsedgwick Mar 9, 2018

htuch Mar 11, 2018

htuch Mar 9, 2018

mattklein123 Mar 10, 2018

jsedgwick commented Mar 12, 2018

mattklein123 left a comment

mattklein123 Mar 12, 2018

jsedgwick Mar 12, 2018

mattklein123 Mar 12, 2018

jsedgwick Mar 12, 2018

mattklein123 Mar 12, 2018

jsedgwick Mar 12, 2018

htuch Mar 13, 2018

mattklein123 Mar 13, 2018

jsedgwick Mar 13, 2018

mattklein123 Mar 13, 2018

jsedgwick Mar 13, 2018

jsedgwick Mar 13, 2018 •

edited

Loading

mattklein123 Mar 13, 2018

jsedgwick Mar 13, 2018

htuch Mar 13, 2018

mattklein123 Mar 14, 2018

htuch Mar 13, 2018

jsedgwick Mar 13, 2018

htuch Mar 13, 2018

mattklein123 Mar 13, 2018

jsedgwick Mar 13, 2018

mattklein123 commented Mar 14, 2018

mattklein123 left a comment

htuch left a comment

htuch Mar 15, 2018


		import "gogoproto/gogo.proto";

		// [#protodoc-title: ConfigDump]

[envoy/admin] Scaffolding for structured admin and config_dump endpoint #532

[envoy/admin] Scaffolding for structured admin and config_dump endpoint #532

Conversation

jsedgwick commented Mar 9, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsedgwick commented Mar 12, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsedgwick Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Mar 14, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsedgwick Mar 13, 2018 •

edited

Loading