-
Notifications
You must be signed in to change notification settings - Fork 616
api: add custom timeout for external auth service #6682
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
api: add custom timeout for external auth service #6682
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6682 +/- ##
=======================================
Coverage 71.09% 71.09%
=======================================
Files 225 225
Lines 39798 39810 +12
=======================================
+ Hits 28295 28304 +9
- Misses 9841 9844 +3
Partials 1662 1662 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fff7fd7 to
c45724a
Compare
c45724a to
50c024f
Compare
arkodg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
| }, | ||
| Timeout: &metav1.Duration{Duration: 500 * time.Millisecond}, | ||
| }, | ||
| expectedTimeout: 0, // 500ms = 0.5 seconds, rounds down to 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at issue #6679, it seems the user wants to set timeouts shorter than 1 second, such as 500 ms.
In this case, the fact that the timeout is truncated to 0s may be unintended behavior.
Since durationpb.Duration also has a nanoseconds field, should we support it?
https://pkg.go.dev/google.golang.org/[email protected]/types/known/durationpb#Duration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Yeah that would be a nice improvement. will update the PR shortly
c5f2939 to
8f386a0
Compare
Signed-off-by: sudipto baral <[email protected]>
8f386a0 to
97eabcc
Compare
|
LGTM thanks! |
Fixes #6679
E2E Test
I set the timeout to 1ms, which should cause an immediate timeout and return a 403 response, thereby verifying the effect of the custom timeout.
In ext_auth_http_service.go and ext_auth_http_service.go the default timeout of 10s is in effect, which allows these to pass.
Release Notes: Yes/No