fix(auth): add timeout to prevent proxy auth requests from hanging#23105
fix(auth): add timeout to prevent proxy auth requests from hanging#23105ipsitapp8 wants to merge 3 commits intogoharbor:mainfrom
Conversation
ipsitapp8
force-pushed
the
fix-auth-timeout
branch
from
69b398a to
f4d04fb
Compare
Signed-off-by: ipsitapp8 <ipsitapp8@gmail.com>
ipsitapp8
force-pushed
the
fix-auth-timeout
branch
from
0e0dfa1 to
a726d63
Compare
|
This will not solve the problem. The timeout needs to be way lower. e.g. 30 seconds. The reason is if there is a problem, the default 30 min will have the same effect as "unlimited" resulting in connections piling up, until Harbor becomes unresponsive or the system runs out of file descriptors. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #23105 +/- ##
===========================================
- Coverage 66.04% 46.69% -19.35%
===========================================
Files 1074 253 -821
Lines 116571 14326 -102245
Branches 2941 2941
===========================================
- Hits 76987 6690 -70297
+ Misses 35330 7279 -28051
+ Partials 4254 357 -3897
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses Harbor proxy-cache hangs by ensuring auth-related HTTP requests (auth scheme probing and bearer token fetching) don’t wait indefinitely when an upstream registry/auth service becomes unresponsive.
Changes:
- Add an
http.Client.Timeout(30 minutes) to the registry auth scheme probing client. - Add an
http.Client.Timeout(30 minutes) to the bearer token fetching client.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/pkg/registry/auth/authorizer.go |
Adds a request timeout to the client used for /v2/ challenge probing to avoid indefinite hangs. |
src/pkg/registry/auth/bearer/authorizer.go |
Adds a request timeout to the client used for bearer token retrieval to avoid indefinite hangs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client: &http.Client{ | ||
| Transport: transport, | ||
| Timeout: 30 * time.Minute, // prevent hanging auth requests if upstream is unresponsive | ||
| }, |
There was a problem hiding this comment.
The timeout is hardcoded to 30 minutes here, but the registry client timeout is already configurable via REGISTRY_HTTP_CLIENT_TIMEOUT. With the current change, operators who tune REGISTRY_HTTP_CLIENT_TIMEOUT will still see auth scheme probing/token requests use a fixed 30m, which is inconsistent and can be surprising operationally. Consider reusing the same env-driven value (or a shared helper/constant) instead of duplicating the literal 30*time.Minute.
There was a problem hiding this comment.
Consider reusing the same env-driven value (or a shared helper/constant) instead of duplicating the literal 30*time.Minute.
30 minutes is a very high limit - better than nothing, but it would be great to have the ability to lower it (make it configurable). However, REGISTRY_HTTP_CLIENT_TIMEOUT may not be a good choice: auth requests are generally light and fast, while downloading binaries can take much more time. I have no good ideas for a name for the new option, though. 😕
| return &authorizer{ | ||
| username: username, | ||
| password: password, | ||
| client: &http.Client{ | ||
| Transport: transport, | ||
| Timeout: 30 * time.Minute, // prevent hanging auth requests if upstream is unresponsive | ||
| }, | ||
| } |
There was a problem hiding this comment.
Add a unit test asserting NewAuthorizer sets a non-zero client timeout (ideally the same value as the configured registry HTTP client timeout). This guards against regressions back to an unbounded http.Client and makes the hanging-request fix verifiable in CI.
| authorizer.client = &http.Client{ | ||
| Transport: transport, | ||
| Timeout: 30 * time.Minute, // prevent hanging auth requests if upstream is unresponsive | ||
| } |
There was a problem hiding this comment.
Same as in auth/authorizer.go: this hardcodes 30 minutes and bypasses the existing REGISTRY_HTTP_CLIENT_TIMEOUT setting used by the main registry client. To keep behavior consistent and configurable, consider sourcing the timeout from the same env-driven value (or a shared helper) rather than duplicating 30*time.Minute here.
| // NewAuthorizer return a bearer token authorizer | ||
| // The parameter "a" is an authorizer used to fetch the token | ||
| func NewAuthorizer(realm, service string, a lib.Authorizer, transport http.RoundTripper) lib.Authorizer { | ||
| authorizer := &authorizer{ | ||
| realm: realm, | ||
| service: service, | ||
| authorizer: a, | ||
| cache: newCache(cacheCapacity), | ||
| } | ||
|
|
||
| authorizer.client = &http.Client{Transport: transport} | ||
| authorizer.client = &http.Client{ | ||
| Transport: transport, | ||
| Timeout: 30 * time.Minute, // prevent hanging auth requests if upstream is unresponsive | ||
| } | ||
| return authorizer |
There was a problem hiding this comment.
Add/extend unit tests to assert the bearer authorizer's underlying http.Client has a timeout set by NewAuthorizer. Since this package already has tests, a simple assertion via type-asserting to *authorizer would prevent future regressions to an infinite timeout.
Thank you for contributing to Harbor!
Fixes #23104
Summary
Fixes auth requests hanging indefinitely due to missing timeout.
What changed
Why
The HTTP clients were created without a timeout, so requests could hang indefinitely when upstream auth services stop responding.
Since these calls are inside a mutex-protected path, this can block other requests as well.
What I did NOT change
Validation