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

Additional features for the Route Table Check Tool #730

Closed
hennna opened this issue Apr 11, 2017 · 10 comments
Closed

Additional features for the Route Table Check Tool #730

hennna opened this issue Apr 11, 2017 · 10 comments
Assignees
Labels
beginner Good starter issues! enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Milestone

Comments

@hennna
Copy link
Contributor

hennna commented Apr 11, 2017

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:

  1. auto_host_rewrite
  2. valid runtime value support
  3. allow users to load a full config and extract the route configuration from it
@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Apr 11, 2017
@mattklein123 mattklein123 added help wanted Needs help! beginner Good starter issues! labels Oct 28, 2017
rshriram pushed a commit to rshriram/envoy that referenced this issue Oct 30, 2018
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
```
@jyotimahapatra
Copy link
Contributor

jyotimahapatra commented May 10, 2019

@mattklein123 We are now using router check tool to check the sanity of routing changes we do internally.
However, i found we could include more validations, say the validations for the filters. Is this a good item to pick up?
If so, I would like to understand the right approach. Could we link the OSS filters with the router check binary and add code to perform checks?
Or should we try making the validation piece extensible, so that those checks stay outside of OSS?

@mattklein123
Copy link
Member

@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.

@jyotimahapatra
Copy link
Contributor

jyotimahapatra commented May 10, 2019

@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?

@mattklein123
Copy link
Member

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 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.

@jyotimahapatra
Copy link
Contributor

Sounds good. I will try it out.

@htuch
Copy link
Member

htuch commented May 13, 2019

+1, proto first is the way to go, thanks for the cleanup here as first step.

@hennna
Copy link
Contributor Author

hennna commented May 14, 2019

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.

@mattklein123 mattklein123 added this to the 1.11.0 milestone May 14, 2019
@jyotimahapatra
Copy link
Contributor

jyotimahapatra commented May 16, 2019

I am trying convert the json schema to proto, today the tool is run like this router_check_tool config.json test.json
The test.json is a headless json array, which causes the proto conversion to fail with this message

Unable to parse JSON as proto (INVALID_ARGUMENT:: Root element must be a message.)

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 ?

@mattklein123
Copy link
Member

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!

@mattklein123 mattklein123 modified the milestones: 1.12.0, 1.13.0 Oct 10, 2019
@mattklein123
Copy link
Member

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.

jpsim pushed a commit that referenced this issue Nov 28, 2022
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 <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this issue Nov 29, 2022
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 <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner Good starter issues! enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

4 participants