feat(client): preflight the engine before unbounded secret resolution - #616
feat(client): preflight the engine before unbounded secret resolution#616joe0BAB wants to merge 1 commit into
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity issue was confirmed in the new preflight function introduced by this PR.
| Severity | File | Finding |
|---|---|---|
| 🟡 MEDIUM | client/client.go:162 |
Non-dial engine errors misclassified as ErrSecretsEngineNotAvailable |
The high-severity hypothesis (logic inversion on requestTimeout == 0) was dismissed: api.DefaultClientRequestTimeout is 0, so default clients correctly trigger preflight. The per-call preflight overhead finding was also dismissed as an intentional, explicitly-tested design decision.
GetSecrets on a client without a request timeout now pings the engine's version endpoint first, bounded to 2 seconds to match the Docker CLI's defaultInitTimeout (docker/cli#3722), so an unreachable engine fails fast instead of blocking resolution indefinitely. Clients with a request timeout skip the ping and fail on their own. This moves the preflight out of the pass run command into the client, so every unbounded consumer gets the fail-fast behavior. As a side effect, pass run no longer contacts the engine when the environment holds no se:// references. Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
4a87e95 to
2775918
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The preflight-in-client approach works correctly for the fail-fast goal and the error-wrapping changes are intentional (the test explicitly documents that non-availability errors are no longer blanketed under ErrSecretsEngineNotAvailable). One medium-severity design regression was confirmed: the ping now fires once per GetSecrets call rather than once per run invocation, producing O(N) version round-trips for N secrets.
| } | ||
|
|
||
| func (c client) GetSecrets(ctx context.Context, pattern secrets.Pattern) ([]secrets.Envelope, error) { | ||
| if c.requestTimeout == 0 { |
There was a problem hiding this comment.
[MEDIUM] Preflight ping fires on every GetSecrets call — O(N) round-trips for N secrets
preflight() is called unconditionally inside GetSecrets whenever requestTimeout == 0. Because resolveEnv (in run.go) loops over every se:// environment variable and calls r.GetSecrets(ctx, pattern) for each one, a process with N secret references now performs N version-endpoint pings instead of the single ping that the old preflightPing call in run.go performed before entering the loop.
The test "unbounded client pings before every resolution" explicitly confirms 2 pings for 2 calls — the naming signals awareness of the pattern, but the implementation doesn't guard against it.
Trigger path:
resolveEnv (run.go) ──loop N times──▶ resolveRef ──▶ r.GetSecrets (client.go:136)
└──▶ preflight() [version ping × N]
Impact: Each preflight call opens a round-trip over the Unix socket and sleeps up to 2 s if the engine is slow to respond. For a process with 10 se:// variables, this can add up to 10 × RTT of unnecessary serialised latency compared to the previous design (O(1) ping per run invocation). It also increases load on the version endpoint proportional to the number of secrets, not the number of client instances.
Suggestion: Use a sync.Once (or an atomic boolean guarded by the first successful preflight) inside the client struct so the ping fires at most once per client lifetime, matching the old O(1) behaviour while still giving every unbounded consumer the fail-fast guarantee.
GetSecrets on a client without a request timeout now pings the engine's version endpoint first, bounded to 2 seconds to match the Docker CLI's defaultInitTimeout (docker/cli#3722), so an unreachable engine fails fast instead of blocking resolution indefinitely. Clients with a request timeout skip the ping and fail on their own.
This moves the preflight out of the pass run command into the client, so every unbounded consumer gets the fail-fast behavior. As a side effect, pass run no longer contacts the engine when the environment holds no se:// references.