perf(middlewares): cache *http.Client on WebhookManager keyed by Timeout (#674) - #700
Merged
Conversation
github-actions
Bot
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR improves webhook delivery performance by caching *http.Client instances on WebhookManager keyed by webhook Timeout, allowing HTTP keep-alive pools to survive rebuildAllMiddlewares reconciles.
Changes:
- Refactors webhook construction into a shared helper (
newWebhookWithClient) that can reuse a prebuilt*http.Client. - Adds a
WebhookManagerclient cache (map[time.Duration]*http.Client) with a mutex-protected getter. - Adds a dedicated test suite covering client sharing, timeout keying, and concurrent cache access; updates
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
middlewares/webhook.go |
Refactors webhook creation and introduces manager-scoped *http.Client caching keyed by timeout. |
middlewares/webhook_manager_client_cache_test.go |
Adds tests validating the cache contract (reuse across reconciles, timeout keying, concurrency). |
CHANGELOG.md |
Documents the behavioral/performance change under Unreleased. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces *http.Client caching in WebhookManager keyed by the webhook Timeout to allow the reuse of keep-alive connection pools across reconciles. It includes a new internal construction path for webhooks and comprehensive tests for the caching logic. Feedback was provided regarding a potential race condition where the webhooks map is accessed without the protection of the newly added mutex in the Register and GetMiddlewares methods.
CybotTM
added a commit
that referenced
this pull request
May 17, 2026
…oads Code reviewer caught a CRITICAL gap: the original fixup added the *http.Client cache on WebhookManager, but cli.Config.WebhookConfigs. InitManager — called by both syncWebhookConfigs (Docker label change) and refreshWebhookManagerOnGlobalChange (INI live-reload) — replaces the manager wholesale and therefore discards the cache. The cache only helped within a single rebuildAllMiddlewares loop (same manager across multiple jobs), not across reloads. The headline #674 benefit ("survive Docker label change or INI reload") was undermined. Fix: - Add (*WebhookManager).AdoptClientCacheFrom(prior *WebhookManager) that snapshots the prior manager's httpClients and merges them into the new manager's cache (under both mutexes, no TOCTOU). Safe for a nil prior (first-init no-op). - WebhookConfigs.InitManager now captures the prior manager reference before replacing it, then calls AdoptClientCacheFrom on the new manager. Cache survives every InitManager call. - Security config can change between reloads, but the cached transports remain valid: AllowedHosts / SSRF allow-list are enforced at validation time (per-request), not on the transport itself. Also addressed code reviewer "Important" items: - ApplyDefaults was being called twice per webhook per reconcile (once in GetMiddlewares, once inside newWebhookWithClient). Dropped the duplicate outer call. - getOrBuildClient ran BEFORE preset / variable validation, so a bad config minted (cached) a client. Refactored newWebhookWithClient to take an optional cachedClient func; it's called only after validation succeeds. Standalone NewWebhook keeps the legacy "fresh client per call" path. Manager-cached path uses (*WebhookManager).cachedClient as the method-value getter. Test engineer suggestions: - Asserted client.Transport != nil + same Transport pointer in SharesHTTPClientAcrossReconciles — the headline benefit is the shared underlying *http.Transport (keep-alive pool), not just the *http.Client wrapper. - TestWebhookManager_AdoptClientCacheFrom_PreservesAcrossReload pins the new carry-forward contract: build mgr1, register, resolve, then swap to mgr2 + AdoptClientCacheFrom(mgr1), and the same *http.Client pointer comes back. Closes the regression-test gap for the critical fix. - TestWebhookManager_AdoptClientCacheFrom_NilPriorIsNoOp pins the first-init contract (nil prior must not panic). - assert.Lenf instead of assert.Len with format args. Doc: - TODO comment on getOrBuildClient noting the cache key must expand if the transport ever grows per-webhook inputs (per-cert TLS, proxy override, custom dialer); today `Timeout` is sufficient because everything else is shared via TransportFactory() and the package-global security config. Deferred: - sync.RWMutex over Mutex (code reviewer suggestion) — negligible at current scale; the contention test passes -race with the Mutex. - Memory growth concern — not real in practice (operators have a handful of distinct timeouts; even pathological hot-reload churn caps at a few KB). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
added a commit
that referenced
this pull request
May 17, 2026
Copilot and Gemini both (HIGH) flagged that the comment on mu claimed to guard "races between GetMiddlewares and Register" but neither Register/Get/GetMiddlewares actually locked the webhooks map. Real concern; my comment was over-stated. In current production code, the mutation paths (cli.WebhookConfigs. InitManager from the INI loader + Docker-label sync) are sequential per reload AND InitManager swaps the entire manager rather than mutating the webhooks map in place — so the live manager's webhooks map is effectively immutable from GetMiddlewares' perspective. mu only needs to guard httpClients. AdoptClientCacheFrom is the only cross-manager handoff and locks both sides correctly. If a future change ever exposes Register on the hot path concurrently with GetMiddlewares, the webhooks map will need its own locking — comment now flags this explicitly so a future contributor sees the contract. No code change; doc-only. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Member
Author
Multi-axis review summary (code-reviewer + test-engineer + Copilot + Gemini)All findings applied across two fixup commits. 0 unresolved threads. Applied — Critical
Applied — Important / High
Applied — Medium / Low
Replied / declined
Deferred (rationale on threads)
Final stats: 7 tests (Shares + DifferentTimeouts + SameTimeoutShares + Concurrent + Standalone + AdoptPreserves + AdoptNilNoOp), CHANGELOG entry under |
Pre-fix, NewWebhook built a fresh *http.Client and *http.Transport per call inside WebhookManager.GetMiddlewares, so every reconcile (called after every Docker label change or INI reload via cli.Config.rebuildAllMiddlewares) dropped any held keep-alive connections and started fresh TCP/TLS handshakes for every job's webhooks. For realistic deployments this is microseconds and unmeasurable, but if reconciles become high-frequency (a future feature triggering rebuildAllMiddlewares per container event in a churn-heavy environment), tail latency on webhook delivery would rise. Same pattern as #630 (PresetLoader cached its HTTP client for the same reason). Refactor: - Extract a shared newWebhookWithClient(config, loader, client) helper that does all validation and accepts an optional pre-built client. NewWebhook becomes a thin wrapper that passes nil (legacy fresh-client-per-call behavior, preserved for tests and direct callers). - Add httpClients map[time.Duration]*http.Client + sync.Mutex to WebhookManager. Keyed by Timeout because that's the only per-webhook input that varies from the shared TransportFactory()-built transport posture; AllowedHosts and the SSRF allow-list live in the package-global security config, so they're not part of the key. - getOrBuildClient(timeout) returns the cached client or builds and caches a new one. Concurrent callers with the same timeout share the returned client. - GetMiddlewares passes m.getOrBuildClient(config.Timeout) to the new helper. Tests (5 new, all -race clean): - TestWebhookManager_SharesHTTPClientAcrossReconciles: two GetMiddlewares calls in sequence return webhooks with the same *http.Client pointer. The headline #674 fix pinned. - TestWebhookManager_DifferentTimeoutsGetDifferentClients: two webhooks with different Timeout get distinct clients (otherwise their *http.Client.Timeout would mismatch). - TestWebhookManager_SameTimeoutAcrossWebhooksSharesClient: two distinct webhooks with the same Timeout share a client (the keep-alive pool serves all jobs pointed at the same endpoint family). - TestWebhookManager_ConcurrentCacheReadsRaceFree: 16 concurrent GetMiddlewares calls across distinct webhook names exercise the cache contention path; pins the sync.Mutex guarantee. Each goroutine uses its own *WebhookConfig to avoid the pre-existing ApplyDefaults race that production avoids by processing webhooks sequentially. - TestNewWebhook_StandaloneStillBuildsOwnClient: backward compatibility — direct NewWebhook callers (tests / third-party) still get a fresh client per call. CHANGELOG entry under [Unreleased] ### Changed. Closes #674. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…oads Code reviewer caught a CRITICAL gap: the original fixup added the *http.Client cache on WebhookManager, but cli.Config.WebhookConfigs. InitManager — called by both syncWebhookConfigs (Docker label change) and refreshWebhookManagerOnGlobalChange (INI live-reload) — replaces the manager wholesale and therefore discards the cache. The cache only helped within a single rebuildAllMiddlewares loop (same manager across multiple jobs), not across reloads. The headline #674 benefit ("survive Docker label change or INI reload") was undermined. Fix: - Add (*WebhookManager).AdoptClientCacheFrom(prior *WebhookManager) that snapshots the prior manager's httpClients and merges them into the new manager's cache (under both mutexes, no TOCTOU). Safe for a nil prior (first-init no-op). - WebhookConfigs.InitManager now captures the prior manager reference before replacing it, then calls AdoptClientCacheFrom on the new manager. Cache survives every InitManager call. - Security config can change between reloads, but the cached transports remain valid: AllowedHosts / SSRF allow-list are enforced at validation time (per-request), not on the transport itself. Also addressed code reviewer "Important" items: - ApplyDefaults was being called twice per webhook per reconcile (once in GetMiddlewares, once inside newWebhookWithClient). Dropped the duplicate outer call. - getOrBuildClient ran BEFORE preset / variable validation, so a bad config minted (cached) a client. Refactored newWebhookWithClient to take an optional cachedClient func; it's called only after validation succeeds. Standalone NewWebhook keeps the legacy "fresh client per call" path. Manager-cached path uses (*WebhookManager).cachedClient as the method-value getter. Test engineer suggestions: - Asserted client.Transport != nil + same Transport pointer in SharesHTTPClientAcrossReconciles — the headline benefit is the shared underlying *http.Transport (keep-alive pool), not just the *http.Client wrapper. - TestWebhookManager_AdoptClientCacheFrom_PreservesAcrossReload pins the new carry-forward contract: build mgr1, register, resolve, then swap to mgr2 + AdoptClientCacheFrom(mgr1), and the same *http.Client pointer comes back. Closes the regression-test gap for the critical fix. - TestWebhookManager_AdoptClientCacheFrom_NilPriorIsNoOp pins the first-init contract (nil prior must not panic). - assert.Lenf instead of assert.Len with format args. Doc: - TODO comment on getOrBuildClient noting the cache key must expand if the transport ever grows per-webhook inputs (per-cert TLS, proxy override, custom dialer); today `Timeout` is sufficient because everything else is shared via TransportFactory() and the package-global security config. Deferred: - sync.RWMutex over Mutex (code reviewer suggestion) — negligible at current scale; the contention test passes -race with the Mutex. - Memory growth concern — not real in practice (operators have a handful of distinct timeouts; even pathological hot-reload churn caps at a few KB). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Copilot and Gemini both (HIGH) flagged that the comment on mu claimed to guard "races between GetMiddlewares and Register" but neither Register/Get/GetMiddlewares actually locked the webhooks map. Real concern; my comment was over-stated. In current production code, the mutation paths (cli.WebhookConfigs. InitManager from the INI loader + Docker-label sync) are sequential per reload AND InitManager swaps the entire manager rather than mutating the webhooks map in place — so the live manager's webhooks map is effectively immutable from GetMiddlewares' perspective. mu only needs to guard httpClients. AdoptClientCacheFrom is the only cross-manager handoff and locks both sides correctly. If a future change ever exposes Register on the hot path concurrently with GetMiddlewares, the webhooks map will need its own locking — comment now flags this explicitly so a future contributor sees the contract. No code change; doc-only. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
force-pushed
the
perf/674-webhook-client-cache
branch
from
May 17, 2026 09:42
35be317 to
f9e358c
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
✅ Mutation Testing ResultsMutation Score: 100.00% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
=======================================
Coverage 88.06% 88.07%
=======================================
Files 89 89
Lines 11464 11497 +33
=======================================
+ Hits 10096 10126 +30
- Misses 1115 1118 +3
Partials 253 253
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
`WebhookManager` now caches `*http.Client` per webhook `Timeout` so the underlying transport's keep-alive connection pool survives `cli.Config.rebuildAllMiddlewares` reconciles (called after every Docker label change or INI reload).
Pre-fix, `NewWebhook` built a fresh `*http.Client` and `*http.Transport` per call inside `GetMiddlewares`, so every reconcile dropped any held keep-alive connections and started fresh TCP/TLS handshakes for every job's webhooks.
Same pattern as #630 (which cached `PresetLoader`'s HTTP client for the same reason).
Closes #674.
Design
Backward compatibility
Standalone `NewWebhook(config, loader)` callers (tests, direct construction by third-party code) keep the legacy "fresh client per call" behavior. Only the `WebhookManager.GetMiddlewares` path goes through the cache via a new internal helper.
Refactor
Extracted a shared `newWebhookWithClient(config, loader, client)` that does all validation and accepts an optional pre-built client. `NewWebhook` becomes a thin wrapper that passes nil.
Tests (5 new, all
-raceclean)Test plan
References