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

[NET-5455] Allow disabling request and idle timeouts with -1 in service router and resolver #19992

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Dec 19, 2023

Description

Adds the ability to disable request and idle timeouts in a service router and service resolver.

Previous Behavior:
user sets 0s or doesn’t set anything -> results in envoy defaults of 15s (requestTimeout) or 1hr (idleTimeout)
user sets >0s -> results in setting that same value in envoy

Behavior with this change:
user sets -1s -> results in setting 0s in envoy (disabling the timeout)
user sets 0s or doesn’t set anything -> result in envoy defaults of 15s or 1hr
user sets >0s -> results in setting that same value in envoy

  • Made changes to xds/routes.go and proxystateconverter/routes.go to support this
  • Adds to the chain-with-splitter test cases to test the timeout (picked an arbitrary case with disco chains to add coverage for the timeouts)

Links

https://hashicorp.atlassian.net/browse/NET-5455

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Dec 19, 2023
"commonLbConfig": {
"healthyPanicThreshold": {}
},
"connectTimeout": "25s",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line shows that the service resolver connect timeout works

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This db cluster was added because it has been added as a destination on the splitter

@ndhanushkodi ndhanushkodi changed the title disable request and idle timeouts with -1 in service router and resolver [NET-5455] Allow disabling request and idle timeouts with -1 in service router and resolver Dec 19, 2023
"commonLbConfig": {
"healthyPanicThreshold": {}
},
"connectTimeout": "25s",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ingress cases are updated in the same way that connect proxy cases are

"route": {
"cluster": "big-side.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
"idleTimeout": "0s",
"timeout": "10s"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shows disabling idle timeout and positive value for request timeout

"prefix": "/lil-bit-side"
},
"route": {
"cluster": "lil-bit-side.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shows zero values for both timeouts result in them being unset in envoy

@ndhanushkodi ndhanushkodi requested review from hashi-derek, a team and zalimeni and removed request for a team December 19, 2023 07:21
Copy link
Member

@zalimeni zalimeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM

@ndhanushkodi ndhanushkodi merged commit 9975b8b into main Dec 19, 2023
87 of 88 checks passed
@ndhanushkodi ndhanushkodi deleted the nd/net-5455-router-resolver-timeouts branch December 19, 2023 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-backport theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants