refactor(service): extract shared SSRF options helper to pkg/ssrfopt#29
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
alexgarzao
changed the title
Adds pkg/ssrfopt with Options() and ResetCache() centralizing the SSRF_ALLOW_PRIVATE env-var lookup that was duplicated across three sites (command, midaz prober, tracer prober). Naming rationale: pkg/ssrfopt (over pkg/ssrf/options) because the package's whole purpose is a 2-symbol API and a flat path matches the existing pkg/clock, pkg/contextutil convention. ResetCache is exported unconditionally (not behind //go:build unit) rather than duplicated in three _test_helpers.go files. The package is plumbing-only with a tiny surface and the godoc clearly marks ResetCache as test-only.
Removes the local ssrfOptions helper, its ssrfAllowPrivate / ssrfAllowPrivateOnce cache vars, and the entire ssrf_test_helpers.go (resetMidazSSRFCache) from the midaz prober package. The prober now calls ssrfopt.Options() and the test resetSSRFCacheForTest delegates to ssrfopt.ResetCache(). Net result: one fewer file in the package (ssrf_test_helpers.go gone) and a single line where there used to be a ~30-line duplicate block. Pure refactor — no behaviour change. All 24 tests in ./pkg/executors/midaz/... pass under -tags=unit.
Removes the local ssrfOptions helper, its ssrfAllowPrivate / ssrfAllowPrivateOnce cache vars, and the entire ssrf_test_helpers.go (resetTracerSSRFCache) from the tracer prober package. The prober now calls ssrfopt.Options() and all 8 test sites that previously called resetTracerSSRFCache() now call ssrfopt.ResetCache() directly. This is the third (last) consumer to migrate to the shared package, closing out the rule-of-three TODO that the original tracer prober PR flagged in its source comments. Pure refactor — no behaviour change. All 14 tests in ./pkg/executors/tracer/... pass under -tags=unit.
alexgarzao
force-pushed
the
refactor/extract-ssrf-options
branch
from
e9a202d to
b88c723
Compare
alexgarzao
changed the base branch from
feature/connectivity-prober-tracer
to
develop
alexgarzao
added a commit
that referenced
this pull request
May 21, 2026
The skeleton on line 145 still used the old `ssrfOptions()` identifier, inconsistent with the surrounding text (line 161) that already directs readers to use the shared `pkg/ssrfopt.Options()` helper. After PR #29 merged, `ssrfOptions()` no longer exists in any prober — the canonical name is `ssrfopt.Options()`. Update the example so it matches both the explanatory text below it and the real implementations in midaz/tracer.
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
1 participant
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
Extracts the duplicated
ssrfOptions()helper into a new shared packagepkg/ssrfopt. Originally targeted three call sites (command + midaz + tracer); after rebasing on develop, the command path no longer usesssrfOptions()directly — PR #25 migrated it tosafehttp.Validate, which encapsulates SSRF policy internally. This refactor now consolidates the remaining duplication between midaz and tracer probers.Pure refactor, zero behavior change.
What changed
New package
pkg/ssrfoptwith two exported symbols:Options() []libSSRF.Option— lazily-cached env-var lookup (SSRF_ALLOW_PRIVATE).ResetCache()— test-only escape hatch (godoc-tagged), so test files can flipSSRF_ALLOW_PRIVATEviat.Setenvbetween scenarios.Two call sites updated:
pkg/executors/midaz/connectivity_prober.gopkg/executors/tracer/connectivity_prober.goTwo helper files deleted:
pkg/executors/midaz/ssrf_test_helpers.go— wrappedresetMidazSSRFCache, now folded into the shared package.pkg/executors/tracer/ssrf_test_helpers.go— same, for tracer.Net impact
Single source of truth for SSRF policy across the remaining outbound probes. Future providers that add their own
ConnectivityProber(e.g., S3) consumessrfopt.Options()instead of cloning the helper.Rebase / retarget notes (2026-05-21)
PR was originally based on
feature/connectivity-prober-tracer(PR #25's branch). After #25 merged to develop:develop.feature/connectivity-prober-tracer→develop.test_provider_config_connectivity.gono longer referencesssrfOptions()at all).probeCtxwall-time cap from PR feat(service): implement ConnectivityProber for Tracer with X-API-Key auth #25 AND usesssrfopt.Options()from this PR.So the original "rule-of-three" premise narrowed to rule-of-two (midaz + tracer), but the deduplication still earns its keep —
ssrfOptions()was a non-trivial helper (env-var caching + lazy init) duplicated verbatim across both probers.Naming and design decisions
pkg/ssrfopt(flat, notpkg/ssrf/options)pkg/clock,pkg/contextutil); only 2 exported symbols, nesting would be over-structured.ResetCacheexposure//go:build unittag would force tag interactions in every consumer test. Godoc is clear enough that no production caller will reach for it.Test plan
go build ./...— clean.go test ./...— 240 pass across 53 packages (the newpkg/ssrfoptadds 1 package).make lint— 0 issues.make sec— 0 issues.libSSRF.WithAllowPrivateNetworkoption, same caching semantics — just centralized.Reviewer checklist
pkg/ssrfopt/options.gomatches the semantics of the old per-packagessrfOptions()(env-var name, libSSRF option set, lazy caching viasync.Once).ResetCachecorrectly resets between subtests in both midaz and tracer suites).