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

Add stronger hash or comparison to xDS APIs #1452

Closed
htuch opened this issue Aug 11, 2017 · 9 comments · Fixed by #1627
Closed

Add stronger hash or comparison to xDS APIs #1452

htuch opened this issue Aug 11, 2017 · 9 comments · Fixed by #1627
Assignees

Comments

@htuch
Copy link
Member

htuch commented Aug 11, 2017

Some xDS APIs (e.g. LDS, RDS and EDS) ignore updates that hash to the value of the existing config. This is to avoid churn and overhead when there is no actual update. However, we use std::hash today for this and no further checks, which is probably not strong enough. We should either keep the original value of the config proto around for direct comparison or use a stronger hash that will guarantee improbable collisions.

@dnoe
Copy link
Contributor

dnoe commented Aug 17, 2017

The actively loaded version of each xDS API can also be exposed as a stat or at an admin URL as part of this.

@htuch
Copy link
Member Author

htuch commented Aug 17, 2017

@dnoe Yeah, this will make writing an integration tests that validate version easier as well.

@mattklein123
Copy link
Member

+1 for hash as a stat for all the different APIs (as well as via admin). FYI we need this in admin URL imminently so @junr03 and @danielhochman will be working on this in the general case. They can follow up.

@htuch
Copy link
Member Author

htuch commented Aug 17, 2017

@mattklein123 Do you want hash in addition to version_info or instead of?

@htuch htuch removed the help wanted Needs help! label Aug 17, 2017
@htuch
Copy link
Member Author

htuch commented Aug 17, 2017

Assigned to @dnoe for triage.

@mattklein123
Copy link
Member

Version info is most important (like we already do with stat for git sha) but hash wouldn't hurt either.

@htuch
Copy link
Member Author

htuch commented Sep 15, 2017

@mattklein123 @danielhochman should we also add the versionInfo() for all APIs to stats/admin? @kmyerson can take a look at this.

@htuch htuch reopened this Sep 15, 2017
@mattklein123
Copy link
Member

@htuch @kmyerson This is related to #1453. Optimally, we would have version info and full dump for all the xDS endpoints, as well as a gauge for each version (hash of version) in stats.

This is a little complicated because we have an existing /listeners endpoint which we use only for integration tests currently, as well as the existing /clusters endpoint which dumps host info. So we need to sort this out.

@kmyerson if you want to generally work on ^ I think that would be very valuable for lots of people.

@mattklein123
Copy link
Member

Fixed by #1805

jpsim pushed a commit that referenced this issue Nov 28, 2022
Signed-off-by: Alan Chiu <[email protected]>

For an explanation of how to fill out the fields, please see the relevant section
in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md)

Description: kotlin: add receive error test
Risk Level: low
Testing: unit
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this issue Nov 29, 2022
Signed-off-by: Alan Chiu <[email protected]>

For an explanation of how to fill out the fields, please see the relevant section
in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md)

Description: kotlin: add receive error test
Risk Level: low
Testing: unit
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants