Skip to content
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.

Sign up for GitHub

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

Jump to bottom

True xDS version in admin interface #2172

Closed
htuch opened this issue Dec 7, 2017 · 4 comments
Closed

True xDS version in admin interface #2172

htuch opened this issue Dec 7, 2017 · 4 comments
Assignees
@mattklein123
Labels
enhancement Feature requests. Not bugs or questions.
Milestone
1.7.0

Comments

@htuch
Copy link
Member

htuch commented Dec 7, 2017

While we added config hash for versioning of xDS in #1805, it would be useful for integration testing to have the actual version info as understood by the management server and xDS exposed on an admin interface handler. This won't be available via stats, since version info is an arbitrary length string which we can't represent as a gauge, so just an admin handler.
@htuch
Copy link
Member Author

htuch commented Dec 7, 2017 •
edited
Loading

@rfaulk @mattklein123 @junr03 I'm thinking of adding for ADS a JSON/proto handler for this that returns some message defined in data-plane-api describing each of the muxed protocols. For non-ADS, it's a bit more complicated for RDS/EDS, since each of these xDS might actually have multiple versions, as resources may fan out to multiple servers individually. This is in aid of Google integration testing, I think others will find it helpful.

LMK if the above proposal makes sense and I can implement.

@mattklein123
Copy link
Member

SGTM. Should this be the first admin API that we define a proto response for instead of hand rolling JSON output? Incrementally more work but would be cool.

@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Dec 8, 2017
@junr03
Copy link
Member

junr03 commented Dec 8, 2017

For posterity: right now we have the version info string of different xDS APIs surfaced in a couple places:

  1. RDS: in the /routes admin endpoint, for each route configuration. You are right that it would be a little bit more complicated to do so centrally for RDS. Which is why I did the handler work in the scope of the router.
  2. CDS: in the /clusters admin endpoint, for the cluster manager.

@htuch what do you think about removing these from their separate places and unifying them under the one /xds(?) admin endpoint? I would love to see the admin endpoint output converted into proto messages, as I have already been burned by bad JSON in the /routes output.

@htuch
Copy link
Member Author

htuch commented Dec 14, 2017

@junr03 Yep, I think moving to a single handler would be great. Let's make this happen, hopefully I get cycles this year unless someone is interested in this before Q1.

@mattklein123 mattklein123 added this to the 1.6.0 milestone Dec 20, 2017
@mattklein123 mattklein123 mentioned this issue Jan 13, 2018
Closed
@junr03 junr03 mentioned this issue Jan 25, 2018
Merged
@mattklein123 mattklein123 modified the milestones: 1.6.0, 1.7.0 Mar 5, 2018
@htuch htuch mentioned this issue Mar 8, 2018
Open
This was referenced Mar 9, 2018
Merged
Merged
@mattklein123 mattklein123 mentioned this issue Mar 13, 2018
Merged
htuch pushed a commit that referenced this issue Mar 27, 2018
@htuch
[admin] config_dump scaffolding and routes endpoint (#2771)
2429797 
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 <[email protected]>
mattklein123 added a commit that referenced this issue Apr 24, 2018
@mattklein123
admin/xDS: prepare for full /config_dump and version support
df81052 
This change does several things:
1) Clarifies how we handle xDS version_info in responses and sets us up
   for both top-level/transactional versions as well as per-resource
   versions in the future.
2) Moves the config_dump admin endpoint to the v2alpha namespace so that
   we can iterate on it in the future.
3) Fills out the config dump proto for the remaining resource types.
   These are not implemented but are here to force a discussion about
   how we want to handle versions moving forward.
4) Fixes RDS static config dump to actually work and add better tests.
5) Wire up version for the RDS config dump on a per-resource basis.

Once we agree on the general version semantics I will be following up
with dump capability of the remaining resource types.

Part of #2421
Part of #2172
Fixes #3141

Signed-off-by: Matt Klein <[email protected]>
@mattklein123 mattklein123 mentioned this issue Apr 24, 2018
Merged
mattklein123 added a commit that referenced this issue May 8, 2018
@mattklein123
admin/xDS: prepare for full /config_dump and version support (#3199)
ada7587 
This change does several things:
1) Clarifies how we handle xDS version_info in responses and sets us up
   for both top-level/transactional versions as well as per-resource
   versions in the future.
2) Moves the config_dump admin endpoint to the v2alpha namespace so that
   we can iterate on it in the future.
3) Fills out the config dump proto for the remaining resource types.
   These are not implemented but are here to force a discussion about
   how we want to handle versions moving forward.
4) Fixes RDS static config dump to actually work and add better tests.
5) Wire up version for the RDS config dump on a per-resource basis.

Once we agree on the general version semantics I will be following up
with dump capability of the remaining resource types.

Part of #2421
Part of #2172
Fixes #3141

Signed-off-by: Matt Klein <[email protected]>
mattklein123 pushed a commit to envoyproxy/data-plane-api that referenced this issue May 8, 2018
admin/xDS: prepare for full /config_dump and version support (#3199)
fcd3ca7 
This change does several things:
1) Clarifies how we handle xDS version_info in responses and sets us up
   for both top-level/transactional versions as well as per-resource
   versions in the future.
2) Moves the config_dump admin endpoint to the v2alpha namespace so that
   we can iterate on it in the future.
3) Fills out the config dump proto for the remaining resource types.
   These are not implemented but are here to force a discussion about
   how we want to handle versions moving forward.
4) Fixes RDS static config dump to actually work and add better tests.
5) Wire up version for the RDS config dump on a per-resource basis.

Once we agree on the general version semantics I will be following up
with dump capability of the remaining resource types.

Part of envoyproxy/envoy#2421
Part of envoyproxy/envoy#2172
Fixes envoyproxy/envoy#3141

Signed-off-by: Matt Klein <[email protected]>

Mirrored from https://github.com/envoyproxy/envoy @ ada758739907628b50079b9adfccf5481ec9fc5f
mattklein123 added a commit that referenced this issue May 10, 2018
@mattklein123
admin: complete /config_dump
0eb8756 
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]>
@mattklein123 mattklein123 mentioned this issue May 10, 2018
Merged
mattklein123 added a commit that referenced this issue May 11, 2018
@mattklein123
admin: complete /config_dump (#3340)
a2a5fc1 
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

Signed-off-by: Matt Klein <[email protected]>
mattklein123 pushed a commit to envoyproxy/data-plane-api that referenced this issue May 11, 2018
admin: complete /config_dump (#3340)
2bbed54 
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 envoyproxy/envoy#3362.

Fixes envoyproxy/envoy#2421
Fixes envoyproxy/envoy#2172

*Risk Level*: Low
*Testing*: Unit/integration
*Docs Changes*: Added
*Release Notes*: Added

Signed-off-by: Matt Klein <[email protected]>

Mirrored from https://github.com/envoyproxy/envoy @ a2a5fc1df1541ebe0b0cd761f3e77212dbf70e5f
@htuch htuch mentioned this issue Apr 10, 2020
Closed
jpsim added a commit that referenced this issue Nov 28, 2022
@jpsim
Fix CI setup command for main_android_dist_ci GitHub Action (#2172)
2b5ef44 
Signed-off-by: JP Simard <[email protected]>
jpsim added a commit that referenced this issue Nov 29, 2022
@jpsim
Fix CI setup command for main_android_dist_ci GitHub Action (#2172)
75cbbf1 
Signed-off-by: JP Simard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mattklein123 mattklein123

Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Milestone
1.7.0
Development

No branches or pull requests
4 participants
@jsedgwick @junr03 @mattklein123 @htuch