From 2e650c77ccd39b2d9cf004e4ac6ddc4c9d374007 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Gro=C3=9Fmann?= Date: Mon, 10 Aug 2026 15:00:07 +0200 Subject: [PATCH] feat(pass): preflight engine liveness ping before resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run a single bounded Version() ping before resolving secrets, so an unreachable engine fails `pass run` fast instead of hanging resolution indefinitely. The ping only runs when the client request timeout is indefinite; a timeout configured via WithTimeout already bounds the wait. The Docker CLI bounds its daemon connection ping the same way (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652). Also expose WithResponseTimeout and WithSocketPath run options so embedders can override the client's 1s response-header timeout and the engine socket path; the socket option also lets tests point the preflight ping at a dead socket. Signed-off-by: Johannes Großmann --- plugins/pass/commands/run.go | 100 +++++++++++++++++++++--------- plugins/pass/commands/run_test.go | 90 ++++++++++++++++++++++++++- 2 files changed, 158 insertions(+), 32 deletions(-) diff --git a/plugins/pass/commands/run.go b/plugins/pass/commands/run.go index c5e8f0ad..3b321a88 100644 --- a/plugins/pass/commands/run.go +++ b/plugins/pass/commands/run.go @@ -37,11 +37,10 @@ import ( const sePrefix = "se://" -// ExitCodeError is returned from RunCommand when the executed child process -// terminated with a non-zero status. It carries the exit code the wrapper -// should exit with. Returning this instead of calling os.Exit directly lets -// the surrounding OTel span wrapper finish recording metrics and span data -// before the process exits. +const defaultPreflightPingTimeout = 3 * time.Second + +// ExitCodeError is returned when the child process exits non-zero, letting the +// OTel span wrapper finish before the process exits. type ExitCodeError struct { Code int } @@ -57,21 +56,35 @@ var runExample string var runLong string type runOpts struct { - envFiles []string - timeout *time.Duration + envFiles []string + timeout *time.Duration + responseTimeout *time.Duration + socketPath string } type RunOption func(*runOpts) -// WithTimeout sets the request timeout forwarded to the secrets-engine -// client. A timeout of 0 disables the request timeout; when the option is -// not provided, the client's default applies. +// WithTimeout sets the client request timeout; 0 disables it. func WithTimeout(timeout time.Duration) RunOption { return func(o *runOpts) { o.timeout = &timeout } } +// WithResponseTimeout sets the client response header timeout; 0 disables it. +func WithResponseTimeout(responseTimeout time.Duration) RunOption { + return func(o *runOpts) { + o.responseTimeout = &responseTimeout + } +} + +// WithSocketPath overrides the engine socket path; empty means the default. +func WithSocketPath(socketPath string) RunOption { + return func(o *runOpts) { + o.socketPath = socketPath + } +} + func RunCommand(options ...RunOption) *cobra.Command { opts := runOpts{} for _, o := range options { @@ -89,36 +102,33 @@ func RunCommand(options ...RunOption) *cobra.Command { return err } - copts := []client.Option{client.WithSocketPath(api.DefaultSocketPath())} - if opts.timeout != nil { - copts = append(copts, client.WithTimeout(*opts.timeout)) - } - c, err := client.New(copts...) + c, err := newRunClient(opts) if err != nil { return err } + if opts.timeout == nil || *opts.timeout == 0 { + if err := preflightPing(cmd.Context(), c, defaultPreflightPingTimeout); err != nil { + return err + } + } + env, err := resolveEnv(cmd.Context(), c, merged) if err != nil { return err } - // No CommandContext: the signal forwarder owns the child's - // lifecycle. Tying the child to cmd.Context() would let cobra's - // ctx cancellation SIGKILL the child out from under the forwarder. + // No CommandContext: cobra's ctx cancellation would SIGKILL the + // child out from under the signal forwarder. child := exec.Command(args[0], args[1:]...) child.Env = env child.Stdin = os.Stdin child.Stdout = os.Stdout child.Stderr = os.Stderr - // Isolate the child in its own process group so that - // terminal-generated signals (Ctrl-C) are delivered to us alone; - // the forwarder is then the sole path that reaches the child. - configureChildProcGroup(child) - - // Install the signal handler before Start so a signal arriving in - // the window between fork and the forwarder goroutine cannot kill - // the parent and orphan the child. + configureChildProcGroup(child) // own process group; Ctrl-C goes to us only + + // Install before Start to avoid orphaning the child if a signal + // arrives between fork and the forwarder goroutine. sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, forwardableSignals()...) defer signal.Stop(sigCh) @@ -157,9 +167,6 @@ func RunCommand(options ...RunOption) *cobra.Command { return cmd } -// mergeEnv folds the process environment and any --env-file inputs into a -// single deterministic KEY=VALUE slice. Precedence: process env first, then -// each file in order; later entries override earlier ones. func mergeEnv(processEnv, files []string) ([]string, error) { merged := make(map[string]string, len(processEnv)) for _, kv := range processEnv { @@ -182,6 +189,38 @@ func mergeEnv(processEnv, files []string) ([]string, error) { return out, nil } +func newRunClient(opts runOpts) (client.Client, error) { + socketPath := opts.socketPath + if socketPath == "" { + socketPath = api.DefaultSocketPath() + } + copts := []client.Option{client.WithSocketPath(socketPath)} + if opts.timeout != nil { + copts = append(copts, client.WithTimeout(*opts.timeout)) + } + if opts.responseTimeout != nil { + copts = append(copts, client.WithResponseTimeout(*opts.responseTimeout)) + } + return client.New(copts...) +} + +// preflightPing fails fast when the engine is unreachable, instead of letting +// an unbounded client block resolution indefinitely. +// +// The Docker CLI bounds its daemon connection ping the same way +// (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652). +func preflightPing(ctx context.Context, c client.Client, timeout time.Duration) error { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + if _, err := c.Version(ctx); err != nil { + if !errors.Is(err, client.ErrSecretsEngineNotAvailable) { + err = fmt.Errorf("%w: %w", client.ErrSecretsEngineNotAvailable, err) + } + return fmt.Errorf("preflight ping: %w", err) + } + return nil +} + func resolveEnv(ctx context.Context, r secrets.Resolver, env []string) ([]string, error) { out := make([]string, 0, len(env)) for _, kv := range env { @@ -201,8 +240,7 @@ func resolveEnv(ctx context.Context, r secrets.Resolver, env []string) ([]string func resolveRef(ctx context.Context, r secrets.Resolver, key, value string) (string, error) { name := strings.TrimPrefix(value, sePrefix) - // Validate as an ID first so wildcards in the reference are rejected - // instead of silently broadening the lookup. + // ParseID rejects wildcards before ParsePattern broadens the lookup. if _, err := secrets.ParseID(name); err != nil { return "", fmt.Errorf("resolving %s: %w", key, err) } diff --git a/plugins/pass/commands/run_test.go b/plugins/pass/commands/run_test.go index f0ef069a..10baac9c 100644 --- a/plugins/pass/commands/run_test.go +++ b/plugins/pass/commands/run_test.go @@ -16,6 +16,7 @@ package commands import ( "bufio" + "bytes" "context" "errors" "fmt" @@ -32,6 +33,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/docker/secrets-engine/client" "github.com/docker/secrets-engine/x/secrets" "github.com/docker/secrets-engine/x/testhelper" ) @@ -46,6 +48,10 @@ const ( helperActiveEnv = "GO_PASS_RUN_HELPER_ACTIVE" helperExitEnv = "GO_PASS_RUN_HELPER_EXIT" helperSleepEnv = "GO_PASS_RUN_HELPER_SLEEP" + // helperSocketEnv switches the wrapper to preflight mode: RunCommand is + // built with WithSocketPath(value) and no request timeout, so the + // preflight ping must run and fail against the dead socket. + helperSocketEnv = "GO_PASS_RUN_HELPER_SOCKET" ) func TestMain(m *testing.M) { @@ -83,7 +89,13 @@ func runAsWrapper() { if err != nil { os.Exit(2) } - cmd := RunCommand() + // A bounded timeout skips the preflight ping, so these subprocess tests + // exercise child-process mechanics without needing a running engine. + ropts := []RunOption{WithTimeout(time.Second)} + if socket := os.Getenv(helperSocketEnv); socket != "" { + ropts = []RunOption{WithSocketPath(socket)} + } + cmd := RunCommand(ropts...) cmd.SetArgs([]string{exe}) cmd.SetContext(context.Background()) cmd.SilenceUsage = true @@ -94,6 +106,7 @@ func runAsWrapper() { os.Exit(exitErr.Code) } if err != nil { + _, _ = fmt.Fprintln(os.Stderr, err) os.Exit(2) } os.Exit(0) @@ -270,6 +283,23 @@ func TestRunCommand(t *testing.T) { assert.Equal(t, 42, exitErr.ExitCode()) }) + t.Run("preflight ping fails fast on a dead socket", func(t *testing.T) { + sub := exec.CommandContext(t.Context(), exe) + sub.Env = append(os.Environ(), + helperWrapperEnv+"=1", + helperActiveEnv+"=1", + helperExitEnv+"=0", + helperSocketEnv+"="+filepath.Join(t.TempDir(), "dead.sock"), + ) + var stderr bytes.Buffer + sub.Stderr = &stderr + err := sub.Run() + var exitErr *exec.ExitError + require.True(t, errors.As(err, &exitErr), "expected ExitError, got %v", err) + assert.Equal(t, 2, exitErr.ExitCode()) + assert.Contains(t, stderr.String(), "preflight ping") + }) + t.Run("forwards SIGINT and exits 130", func(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("SIGINT cross-process semantics differ on Windows") @@ -319,6 +349,64 @@ func waitForReady(t *testing.T, r io.Reader) { go func() { _, _ = io.Copy(io.Discard, r) }() } +// pingClient adapts a Version func to client.Client. The embedded +// MockResolver supplies GetSecrets, so no hand-rolled resolver mock can drift +// from the shared one. +type pingClient struct { + testhelper.MockResolver + ping func(context.Context) (client.DaemonVersion, error) +} + +func (p pingClient) Version(ctx context.Context) (client.DaemonVersion, error) { + return p.ping(ctx) +} + +func TestPreflightPing(t *testing.T) { + t.Parallel() + + t.Run("passes when the engine responds", func(t *testing.T) { + t.Parallel() + c := pingClient{ping: func(_ context.Context) (client.DaemonVersion, error) { + return client.DaemonVersion{}, nil + }} + require.NoError(t, preflightPing(t.Context(), c, time.Second)) + }) + + t.Run("fails when the engine is unreachable", func(t *testing.T) { + t.Parallel() + engineErr := errors.New("connection refused") + c := pingClient{ping: func(_ context.Context) (client.DaemonVersion, error) { + return client.DaemonVersion{}, engineErr + }} + err := preflightPing(t.Context(), c, time.Second) + require.Error(t, err) + assert.ErrorContains(t, err, "preflight ping") + assert.ErrorIs(t, err, engineErr) + assert.ErrorIs(t, err, client.ErrSecretsEngineNotAvailable) + }) + + t.Run("gives up after the timeout when the engine hangs", func(t *testing.T) { + t.Parallel() + c := pingClient{ping: func(ctx context.Context) (client.DaemonVersion, error) { + <-ctx.Done() + return client.DaemonVersion{}, ctx.Err() + }} + // Watchdog parent: if preflightPing loses its own deadline, ping + // unblocks here and the elapsed assertion fails fast, instead of the + // package hanging until the go test panic. A parent deadline alone is + // not enough — the regressed path would still surface + // DeadlineExceeded, just later, and pass spuriously. + watchdogCtx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + start := time.Now() + err := preflightPing(watchdogCtx, c, 50*time.Millisecond) + require.Error(t, err) + require.Less(t, time.Since(start), 2*time.Second) + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.ErrorIs(t, err, client.ErrSecretsEngineNotAvailable) + }) +} + // testWriter forwards cobra output to t.Log so it does not leak onto stderr. type testWriter struct{ t *testing.T }