-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Additional features for the Route Table Check Tool #730
Comments
Automatic merge from submit-queue. add port to discovery address **What this PR does / why we need it**: **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note NONE ```
@mattklein123 We are now using router check tool to check the sanity of routing changes we do internally. |
@jyotimahapatra can you describe in more detail what is missing? What do the extensions have to do with the route check tool? You want to verify per-route filter config? If so, that sounds good to me. At a higher layer, it would be good to completely get rid of the JSON schema and move to a proto defined schema as we have done every else. If you want to do that, that would be great. cc @hennna @htuch. |
@mattklein123 Yes per_filter_config is exactly what i have in mind right now. We are adding a few more filters and there's no way to test them, hence the idea. Would you like to get it done as an extra validation in the current code? Or should we try to make the validation part extensible in OSS and add the validation piece outside OSS? I understand the point of getting rid of json schema, but I would look at it as a separate task, given i am looking for a starter task. Would that make sense? |
I would add this as an OSS feature. It should be straight forward to do this generically, and then just link the tool against any needed extensions. However, I don't want to invest anymore in the JSON schema, so please convert it to proto first. It should be a fairly trivial transform as proto is directly convertable back and forth to/from JSON/YAML. |
Sounds good. I will try it out. |
+1, proto first is the way to go, thanks for the cleanup here as first step. |
Slightly related, in the future we were thinking of changing the route validation tool from depending on mocks to fakes so the tool could potentially be run as a service. |
I am trying convert the json schema to proto, today the tool is run like this
Any suggestions about how to deal with this and still have backward compatibility with the existing json test file schemas being used out there? Or we can introduce a new schema and a parallel code path and then deprecate json schema ? |
I would just do ^. I think we could probably just switch it, but it would be nicer to deprecate. Thank you! |
There has been so much work on the router checker tool recently that I'm going to close this generic issue. At this point let's open specific issues for feature requests, bugs, etc. |
Fixes an [issue](envoyproxy/envoy-mobile#729) where the library could potentially race and crash when calling `flushStats` when `server_` has not yet been assigned. @junr03 astutely pointed out that this could be caused by the fact that `server_` is not assigned to anything, so the `if (server_)` check does not perform as expected. To fix this, we should assign it to `nullptr` immediately. I was able to simulate the race by calling `flushStats()` at the top of `Engine::run()` (before `server_` is assigned). With this patch, the crash disappears. Resolves envoyproxy/envoy-mobile#729. Signed-off-by: Michael Rebello <[email protected]> Signed-off-by: JP Simard <[email protected]>
Fixes an [issue](envoyproxy/envoy-mobile#729) where the library could potentially race and crash when calling `flushStats` when `server_` has not yet been assigned. @junr03 astutely pointed out that this could be caused by the fact that `server_` is not assigned to anything, so the `if (server_)` check does not perform as expected. To fix this, we should assign it to `nullptr` immediately. I was able to simulate the race by calling `flushStats()` at the top of `Engine::run()` (before `server_` is assigned). With this patch, the crash disappears. Resolves envoyproxy/envoy-mobile#729. Signed-off-by: Michael Rebello <[email protected]> Signed-off-by: JP Simard <[email protected]>
v1 of the route table check tool can be found at #682. The original issue discussion can be found at #538.
The features that have been identified to be included in v2 are:
The text was updated successfully, but these errors were encountered: