fix(middlewares): cache preset HTTP client; share TransportFactory in slack fallback#642
Conversation
…ugh TransportFactory Two defense-in-depth follow-ups from #624 surfaced in #630. A. PresetLoader now caches a single *http.Client constructed in NewPresetLoader from TransportFactory(), instead of building a fresh client (and a fresh *http.Transport with its own connection pool) on every loadFromURL call. Bursty preset fetches now share idle-conn reuse. Test ordering constraint: SetTransportFactoryForTest must run BEFORE NewPresetLoader to influence the cached client; this is documented on the constructor's godoc. B. Slack middleware (deprecated) now routes its fallback *http.Client through TransportFactory() instead of inheriting http.DefaultTransport. Both construction sites in slack.go (NewSlack and the nil-guard in pushMessage) are updated for consistency. Tests added (TDD, both initially failing): - TestPresetLoader_ClientReused: counts factory invocations across two loadFromURL calls and asserts pointer-identity of loader.httpClient. - TestSlack_FallbackClient_UsesTransportFactory: installs a sentinel transport via SetTransportFactoryForTest and asserts NewSlack wires it through to m.Client.Transport. Closes #630 Signed-off-by: Sebastian Mendel <[email protected]>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
documentation
|
![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.
|
There was a problem hiding this comment.
Pull request overview
Defense-in-depth follow-ups to PR #624: cache the HTTP client in PresetLoader for connection reuse, and route the deprecated Slack middleware's fallback client through the shared TransportFactory() for consistent TLS/proxy posture.
Changes:
- Cache a single
*http.ClientonPresetLoader, constructed once inNewPresetLoaderfromTransportFactory(). - Wire Slack's fallback
*http.Client(both inNewSlackand the nil-guard inpushMessage) throughTransportFactory(). - Add regression tests for client reuse and Slack transport wiring; document changes in CHANGELOG.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| middlewares/preset.go | Adds cached httpClient field and uses it in loadFromURL. |
| middlewares/slack.go | Routes fallback HTTP clients through TransportFactory(). |
| middlewares/preset_client_cache_test.go | New regression test asserting client/transport reuse. |
| middlewares/slack_test.go | New regression test for Slack fallback transport wiring. |
| CHANGELOG.md | Documents the 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 optimizes the PresetLoader by caching a single *http.Client initialized via TransportFactory(), which enables connection pooling and improves performance for remote preset fetches. Additionally, the deprecated Slack middleware has been updated to use TransportFactory() for its fallback HTTP client to ensure consistent TLS and proxy settings. New regression tests were introduced to verify these changes. I have no feedback to provide as there were no review comments to assess.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #642 +/- ##
=======================================
Coverage 87.86% 87.86%
=======================================
Files 88 88
Lines 10898 10898
=======================================
Hits 9576 9576
Misses 1081 1081
Partials 241 241
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:
|
Summary
Two defense-in-depth follow-ups from #624 tracked in #630.
PresetLoadernow caches a single*http.Client(constructed inNewPresetLoaderfromTransportFactory()) instead of building a fresh client and*http.Transporton everyloadFromURLcall. Bursty preset fetches now share the underlying connection pool / idle-conn reuse.middlewares/slack.go) routes its fallback*http.ClientthroughTransportFactory()instead of inheritinghttp.DefaultTransport. Both construction sites —NewSlack(line 51) and the nil-guard inpushMessage(line 94) — are updated.Caveats
SetTransportFactoryForTestonly takes effect on the cached client when called BEFORENewPresetLoader. Replacing the factory after construction has no effect. This is now documented onNewPresetLoader's godoc and on thePresetLoader.httpClientfield. The existing regression testTestPresetLoader_LoadFromURL_UsesTransportFactoryalready follows this ordering, so it remains green.slackpreset. The change inslack.gois explicitly defense-in-depth for the deprecation window — operators are still using it, and inheritinghttp.DefaultTransportwhile the rest of the webhook stack converges onTransportFactory()is the kind of latent posture drift this codebase has been actively closing.TDD
Both tests written first and verified failing (compile error: undefined field), then made to pass:
TestPresetLoader_ClientReused(new filemiddlewares/preset_client_cache_test.go) — installs a countingSetTransportFactoryForTestBEFORENewPresetLoader, performs twoloadFromURLcalls, asserts the factory was invoked exactly once, and pointer-checks thatloader.httpClientis the same instance across calls.TestSlack_FallbackClient_UsesTransportFactory(appended tomiddlewares/slack_test.go) — installs a sentinel*http.TransportviaSetTransportFactoryForTestand assertsNewSlack(...).Client.Transportis the same pointer.Test plan
go test ./middlewares/... -count=1 -short— passes (10.6s)golangci-lint run ./...— 0 issuesTestPresetLoader_LoadFromURL_UsesTransportFactorystill green (it already installs the factory beforeNewPresetLoader)preset_mutation_test.go,webhook_mutation_test.go,slack_mutation_test.go) included in the full middlewares runCloses #630