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.

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_dump returns output in random order #4122

Closed
ramaraochavali opened this issue Aug 13, 2018 · 15 comments
Closed

config_dump returns output in random order #4122

ramaraochavali opened this issue Aug 13, 2018 · 15 comments
Assignees
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@ramaraochavali
Copy link
Contributor

Currently config_dump returns configs in random as they are defined as a map in the proto.
Is there any better way to to make output more deterministic via protos? Otherwise can we enforce some ordering while displaying?

@mattklein123 mattklein123 added the question Questions that are neither investigations, bugs, nor enhancements label Aug 13, 2018
@mattklein123
Copy link
Member

IIRC there is a way to have proto do consistent serialization but I forget the details right now and I don't have time to go poking through the code (check proto utilities). @htuch do you remember?

@htuch
Copy link
Member

htuch commented Aug 13, 2018

I'm not sure this is possible in the presence of maps, e.g. https://gist.github.com/kchristidis/39c8b310fd9da43d515c4394c3cd9510. I think for this reason, maps should be avoided in our admin API endpoint output.

@mandarjog
Copy link
Contributor

Maps keys can be optionally sorted to make diffing easier. The question is should this be done inside the config_dump endpoint or is it better left to the client that is consuming the output.

If it is not too expensive, determnistic json output is preferred.

@htuch
Copy link
Member

htuch commented Aug 13, 2018

@mandarjog where is the option to have proto do this? I don't see it in JsonPrintOptions.

@htuch
Copy link
Member

htuch commented Aug 13, 2018

Also, I'd question what value maps bring to any of our endpoint output, can someone articulate this?

@mattklein123
Copy link
Member

I don't think the maps add any value. I would just switch them all to lists.

@ramaraochavali
Copy link
Contributor Author

@mattklein123 assign this to me. I will work on it

@mattklein123 mattklein123 added enhancement Feature requests. Not bugs or questions. and removed question Questions that are neither investigations, bugs, nor enhancements labels Aug 15, 2018
@mattklein123 mattklein123 added the help wanted Needs help! label Aug 15, 2018
@lizan
Copy link
Member

lizan commented Aug 15, 2018

Even we make them lists, the output won't be deterministic in many places where the config is dumped from std::unordered_{set,map} internally, (I'm sure routes are backed by unordered_set of pointers, not sure for others). It is still better the client consuming the output do a semantic comparison.

@htuch
Copy link
Member

htuch commented Aug 15, 2018

@lizan from the client perspective, this is unsatisfying, since we use these protos to serialize to JSON and output for humans. Humans aren't very good at comparing large unordered lists 💩

@lizan
Copy link
Member

lizan commented Aug 15, 2018

Right, so I would suggest use (or should we make one?) a client side tool to sort them instead of doing admin output, to keep admin endpoints lightweight.

@yangminzhu
Copy link
Contributor

I'm trying to sync Istio with the latest envoy API, Is it always guaranteed that the [bootstrap, listener, cluster, routes] will always appear in the Configs in this specific order?

cc @liamawhite

@ramaraochavali
Copy link
Contributor Author

@yangminzhu It is always guaranteed to return in the order [bootstrap, cluster, listener, routes] (lexicographic order) as documented here

@ramaraochavali
Copy link
Contributor Author

@htuch I guess this issue can closed via #4197 unless you would like some thing else to be done here.

@htuch
Copy link
Member

htuch commented Aug 27, 2018

Well, there are other maps for stats (https://github.com/envoyproxy/envoy/blob/master/api/envoy/admin/v2alpha/clusters.proto#L49), but I think this is a reasonable step and we can open new issues if folks object.

@htuch htuch closed this as completed Aug 27, 2018
@ramaraochavali
Copy link
Contributor Author

Yeah. I saw that. Let me see if I can fix that as well this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

6 participants