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

Missing transport_api_version in rate limiting leads to crash #15083

Closed
howardjohn opened this issue Feb 17, 2021 · 3 comments · Fixed by #15548
Closed

Missing transport_api_version in rate limiting leads to crash #15083

howardjohn opened this issue Feb 17, 2021 · 3 comments · Fixed by #15548
Labels
api area/ratelimit bug help wanted Needs help!

Comments

@howardjohn
Copy link
Contributor

Title: Old envoyproxy/ratelimit leads to crash
Description:
Forwarding an issue from our repo: istio/istio#30900

Basically, it sounds like when missing transport_api_version in the rate limiting config, we get a crash

Expected behavior is some more graceful failure, probably a NACK

Repro steps:
See istio/istio#30900. I did not reproduce myself, just forwarding this
@howardjohn howardjohn added bug triage Issue requires triage labels Feb 17, 2021
@mattklein123
Copy link
Member

cc @htuch @adisuissa this is the same issue that keeps getting reported over in the ratelimit repo, so it seems like something is broken?

@htuch
Copy link
Member

htuch commented Feb 18, 2021

@howardjohn does this reproduce on main? I was previously unable to repro with the example from the ratelimit repo.

@howardjohn
Copy link
Contributor Author

I am not too familiar with this, just copied over from the Istio repo, but I did try their config on main and it didn't crash. I wasn't sure if this was because it's fixed or because it requires a request or something - I didn't set up the full rate limit service.

@junr03 junr03 added api area/ratelimit and removed triage Issue requires triage labels Feb 18, 2021
@mattklein123 mattklein123 mentioned this issue Mar 19, 2021
Merged
@mattklein123 mattklein123 added the help wanted Needs help! label Mar 19, 2021
@mattklein123 mattklein123 mentioned this issue Mar 29, 2021
Closed
htuch added a commit that referenced this issue Mar 30, 2021
@htuch
config: ensure Config::Utility::getAndCheckTransportVersion is called…
4cce00f 
… on main thread. (#15548)

During the v2 -> v3 migration, some uses of this were added inside filter factory lambdas and could
occur on worker threads, rejecting config on the data plane.

Relevant to #10943
Fixes #15083

Risk level: Low
Testing: New assertion in method caused existing tests to fail, fix addresses these. Also manually
audited via grep.

Signed-off-by: Harvey Tuch <htuch@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api area/ratelimit bug help wanted Needs help!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

config: ensure Config::Utility::getAndCheckTransportVersion is called on main thread. htuch/envoy
4 participants
@howardjohn @junr03 @mattklein123 @htuch