From 819b57a33a102c12137d088b1a5575d1badf35c0 Mon Sep 17 00:00:00 2001 From: "Matt, Matthias" Date: Tue, 13 Oct 2026 16:40:31 +0200 Subject: [PATCH 1/6] fix(kubernetes): make terminal resize shutdown race-safe Signal terminal-size queue shutdown separately from its data channel so late or concurrent window requests cannot panic the gateway. Release blocked operations, reject future resizes, and make Stop idempotent without locking across a send. Add regression coverage through real execution completion and OnWindow in connection, persistent, and session modes. Reproduced the original send-on-closed-channel panic; 50 race-enabled repetitions, vet, and gateway build pass. Signed-off-by: Matt, Matthias --- internal/kubernetes/pushSizeQueueImpl.go | 47 +++- internal/kubernetes/resize_shutdown_test.go | 247 ++++++++++++++++++++ 2 files changed, 290 insertions(+), 4 deletions(-) create mode 100644 internal/kubernetes/resize_shutdown_test.go diff --git a/internal/kubernetes/pushSizeQueueImpl.go b/internal/kubernetes/pushSizeQueueImpl.go index e942ecec..8b73dcfa 100644 --- a/internal/kubernetes/pushSizeQueueImpl.go +++ b/internal/kubernetes/pushSizeQueueImpl.go @@ -2,31 +2,70 @@ package kubernetes import ( "context" + "errors" + "sync" "k8s.io/client-go/tools/remotecommand" ) type pushSizeQueueImpl struct { resizeChan chan remotecommand.TerminalSize + lock sync.Mutex + stopped bool + stopChan chan struct{} } func (s *pushSizeQueueImpl) Push(ctx context.Context, size remotecommand.TerminalSize) error { + s.lock.Lock() + stopChan := s.getStopChanLocked() + stopped := s.stopped + s.lock.Unlock() + if stopped { + return errors.New("terminal size queue is stopped") + } + select { case <-ctx.Done(): return ctx.Err() + case <-stopChan: + return errors.New("terminal size queue is stopped") case s.resizeChan <- size: return nil } } func (s *pushSizeQueueImpl) Next() *remotecommand.TerminalSize { - size, ok := <-s.resizeChan - if !ok { + s.lock.Lock() + stopChan := s.getStopChanLocked() + stopped := s.stopped + s.lock.Unlock() + if stopped { + return nil + } + + select { + case <-stopChan: return nil + case size := <-s.resizeChan: + return &size } - return &size } func (s *pushSizeQueueImpl) Stop() { - close(s.resizeChan) + s.lock.Lock() + defer s.lock.Unlock() + if s.stopped { + return + } + s.stopped = true + // Window requests may still be sending when execution finishes. Signal + // shutdown without closing their data channel or blocking behind a send. + close(s.getStopChanLocked()) +} + +func (s *pushSizeQueueImpl) getStopChanLocked() chan struct{} { + if s.stopChan == nil { + s.stopChan = make(chan struct{}) + } + return s.stopChan } diff --git a/internal/kubernetes/resize_shutdown_test.go b/internal/kubernetes/resize_shutdown_test.go new file mode 100644 index 00000000..d1eb6548 --- /dev/null +++ b/internal/kubernetes/resize_shutdown_test.go @@ -0,0 +1,247 @@ +package kubernetes + +import ( + "context" + "errors" + "fmt" + "io" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.containerssh.io/containerssh/config" + "go.containerssh.io/containerssh/internal/metrics" + "go.containerssh.io/containerssh/log" + "k8s.io/client-go/tools/remotecommand" + "k8s.io/client-go/util/exec" +) + +func TestResizeQueueShutdown(t *testing.T) { + for _, mode := range []config.KubernetesExecutionMode{ + config.KubernetesExecutionModeConnection, + config.KubernetesExecutionModePersistent, + config.KubernetesExecutionModeSession, + } { + t.Run(string(mode), func(t *testing.T) { + tty := true + pod := &kubernetesPodImpl{ + config: config.KubernetesConfig{}, + tty: &tty, + wg: &sync.WaitGroup{}, + } + pod.config.Pod.Mode = mode + // PID framing is independent of resize shutdown. Bypass the agent so + // this regression also runs against versions with different framing. + pod.config.Pod.DisableAgent = true + pod.config.Timeouts.PodStop = time.Second + if mode == config.KubernetesExecutionModeConnection { + pod.wg.Add(1) + } + + collector := metrics.New(nil) + stream := &resizeExecutor{ + entered: make(chan struct{}), + received: make(chan remotecommand.TerminalSize, 1), + finish: make(chan struct{}), + } + execution := &kubernetesExecutionImpl{ + pod: pod, + exec: stream, + terminalSizeQueue: &pushSizeQueueImpl{resizeChan: make(chan remotecommand.TerminalSize)}, + logger: log.NewTestLogger(t), + doneChan: make(chan struct{}), + backendRequestsMetric: collector.MustCreateCounter( + "resize-shutdown-requests", "", "test"), + backendFailuresMetric: collector.MustCreateCounter( + "resize-shutdown-failures", "", "test"), + } + network := &networkHandler{ + mutex: &sync.Mutex{}, + config: config.KubernetesConfig{}, + logger: log.NewTestLogger(t), + } + network.config.Timeouts.Window = 2 * time.Second + handler := &channelHandler{networkHandler: network, exec: execution} + + exitStatus := make(chan int, 1) + streamDone := make(chan struct{}) + go func() { + execution.handleStream(nil, io.Discard, io.Discard, func() error { return nil }, func(code int) { + exitStatus <- code + }) + close(streamDone) + }() + select { + case <-stream.entered: + case <-time.After(time.Second): + t.Fatal("remotecommand executor did not enter StreamWithContext") + } + + // A live window request must reach the real execution's stream queue. + active := captureWindow(handler, 80, 24) + if active.panicValue != nil { + t.Fatalf("active OnWindow panicked: %v", active.panicValue) + } + require.NoError(t, active.err) + select { + case size := <-stream.received: + require.Equal(t, remotecommand.TerminalSize{Width: 80, Height: 24}, size) + case <-time.After(time.Second): + t.Fatal("active resize was not delivered to the remotecommand executor") + } + + // The executor stops reading sizes. Leave an OnWindow push pending + // while actual stream completion stops its queue. + const concurrentResizes = 4 + results := make(chan resizeCall, concurrentResizes) + started := make(chan struct{}, concurrentResizes) + for i := 0; i < concurrentResizes; i++ { + go func() { + started <- struct{}{} + results <- captureWindow(handler, 100, 40) + }() + } + for i := 0; i < concurrentResizes; i++ { + select { + case <-started: + case <-time.After(time.Second): + t.Fatal("concurrent resize caller did not start") + } + } + select { + case result := <-results: + t.Fatalf("resize completed before the stream stopped: %+v", result) + case <-time.After(25 * time.Millisecond): + } + + close(stream.finish) + select { + case <-streamDone: + case <-time.After(time.Second): + t.Fatal("execution did not finish after the remote stream returned") + } + require.Equal(t, 7, <-exitStatus) + + var failures []string + for i := 0; i < concurrentResizes; i++ { + select { + case result := <-results: + if result.panicValue != nil { + failures = append(failures, fmt.Sprintf("concurrent OnWindow panicked: %v", result.panicValue)) + } else if result.err == nil { + failures = append(failures, "concurrent resize succeeded after stream shutdown") + } + case <-time.After(time.Second): + failures = append(failures, "concurrent OnWindow remained blocked after stream shutdown") + } + } + + // A delayed window notification must return an error, never panic. + delayed := captureWindow(handler, 120, 50) + if delayed.panicValue != nil { + failures = append(failures, fmt.Sprintf("post-exit OnWindow panicked: %v", delayed.panicValue)) + } else if delayed.err == nil { + failures = append(failures, "post-exit OnWindow unexpectedly succeeded") + } + require.Empty(t, failures, "%s execution resize shutdown failures", mode) + }) + } + + t.Run("queue shutdown contract", func(t *testing.T) { + const prompt = 250 * time.Millisecond + + readerQueue := &pushSizeQueueImpl{resizeChan: make(chan remotecommand.TerminalSize)} + readResult := make(chan *remotecommand.TerminalSize, 1) + go func() { readResult <- readerQueue.Next() }() + select { + case size := <-readResult: + t.Fatalf("Next returned before shutdown: %v", size) + case <-time.After(25 * time.Millisecond): + } + readerQueue.Stop() + readerQueue.Stop() + select { + case size := <-readResult: + require.Nil(t, size, "Next must return nil after shutdown") + case <-time.After(prompt): + t.Fatal("Stop did not release a blocked Next") + } + + writerQueue := &pushSizeQueueImpl{resizeChan: make(chan remotecommand.TerminalSize)} + writeResult := make(chan resizePushResult, 1) + go func() { + err, panicValue := captureResizePush(writerQueue, context.Background()) + writeResult <- resizePushResult{err: err, panicValue: panicValue} + }() + select { + case result := <-writeResult: + t.Fatalf("Push returned before shutdown: %+v", result) + case <-time.After(25 * time.Millisecond): + } + writerQueue.Stop() + writerQueue.Stop() + select { + case result := <-writeResult: + require.Nil(t, result.panicValue, "Stop must not panic a blocked Push") + require.Error(t, result.err, "shutdown must release a pending Push with an error") + case <-time.After(prompt): + t.Fatal("Stop did not release a blocked Push") + } + + futureErr, futurePanic := captureResizePush(writerQueue, context.Background()) + require.Nil(t, futurePanic, "Push after Stop must not panic") + require.Error(t, futureErr, "Push after Stop must return an error") + + cancelledQueue := &pushSizeQueueImpl{resizeChan: make(chan remotecommand.TerminalSize)} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + cancelledErr, cancelledPanic := captureResizePush(cancelledQueue, ctx) + require.Nil(t, cancelledPanic) + require.ErrorIs(t, cancelledErr, context.Canceled, "caller cancellation must be preserved") + cancelledQueue.Stop() + }) +} + +type resizeCall struct { + err error + panicValue interface{} +} + +func captureWindow(handler *channelHandler, columns, rows uint32) (result resizeCall) { + defer func() { result.panicValue = recover() }() + result.err = handler.OnWindow(1, columns, rows, 0, 0) + return result +} + +type resizePushResult struct { + err error + panicValue interface{} +} + +func captureResizePush(queue pushSizeQueue, ctx context.Context) (err error, panicValue interface{}) { + defer func() { panicValue = recover() }() + err = queue.Push(ctx, remotecommand.TerminalSize{Width: 80, Height: 24}) + return err, panicValue +} + +type resizeExecutor struct { + entered chan struct{} + received chan remotecommand.TerminalSize + finish chan struct{} +} + +func (s *resizeExecutor) Stream(options remotecommand.StreamOptions) error { + return s.StreamWithContext(context.Background(), options) +} + +func (s *resizeExecutor) StreamWithContext(_ context.Context, options remotecommand.StreamOptions) error { + close(s.entered) + size := options.TerminalSizeQueue.Next() + if size == nil { + return fmt.Errorf("terminal size queue stopped before active resize") + } + s.received <- *size + <-s.finish + return exec.CodeExitError{Err: errors.New("resize fixture exit"), Code: 7} +} From a176225266c69d5c82564d0a1ee11b91efee8341 Mon Sep 17 00:00:00 2001 From: "Matt, Matthias" Date: Tue, 13 Oct 2026 16:53:21 +0200 Subject: [PATCH 2/6] test(kubernetes): align resize regression with lint rules Return the capture helper error last and explain the internal-package test annotation, following existing regression-test conventions. Signed-off-by: Matt, Matthias --- internal/kubernetes/resize_shutdown_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/kubernetes/resize_shutdown_test.go b/internal/kubernetes/resize_shutdown_test.go index d1eb6548..678e4dcc 100644 --- a/internal/kubernetes/resize_shutdown_test.go +++ b/internal/kubernetes/resize_shutdown_test.go @@ -1,4 +1,4 @@ -package kubernetes +package kubernetes //nolint:testpackage // Regression exercises the unexported execution and queue lifecycle. import ( "context" @@ -171,7 +171,7 @@ func TestResizeQueueShutdown(t *testing.T) { writerQueue := &pushSizeQueueImpl{resizeChan: make(chan remotecommand.TerminalSize)} writeResult := make(chan resizePushResult, 1) go func() { - err, panicValue := captureResizePush(writerQueue, context.Background()) + panicValue, err := captureResizePush(writerQueue, context.Background()) writeResult <- resizePushResult{err: err, panicValue: panicValue} }() select { @@ -189,14 +189,14 @@ func TestResizeQueueShutdown(t *testing.T) { t.Fatal("Stop did not release a blocked Push") } - futureErr, futurePanic := captureResizePush(writerQueue, context.Background()) + futurePanic, futureErr := captureResizePush(writerQueue, context.Background()) require.Nil(t, futurePanic, "Push after Stop must not panic") require.Error(t, futureErr, "Push after Stop must return an error") cancelledQueue := &pushSizeQueueImpl{resizeChan: make(chan remotecommand.TerminalSize)} ctx, cancel := context.WithCancel(context.Background()) cancel() - cancelledErr, cancelledPanic := captureResizePush(cancelledQueue, ctx) + cancelledPanic, cancelledErr := captureResizePush(cancelledQueue, ctx) require.Nil(t, cancelledPanic) require.ErrorIs(t, cancelledErr, context.Canceled, "caller cancellation must be preserved") cancelledQueue.Stop() @@ -219,10 +219,10 @@ type resizePushResult struct { panicValue interface{} } -func captureResizePush(queue pushSizeQueue, ctx context.Context) (err error, panicValue interface{}) { +func captureResizePush(queue pushSizeQueue, ctx context.Context) (panicValue interface{}, err error) { defer func() { panicValue = recover() }() err = queue.Push(ctx, remotecommand.TerminalSize{Width: 80, Height: 24}) - return err, panicValue + return panicValue, err } type resizeExecutor struct { From 7f8f7a2ce0035f910815f0989c1f87910d1d799d Mon Sep 17 00:00:00 2001 From: "Matt, Matthias" Date: Wed, 14 Oct 2026 12:44:45 +0200 Subject: [PATCH 3/6] fix: preserve Settings gateway guarantees and publish internal PR images Port regression-tested single-start, signal-policy, connection readiness rollback, and mode-aware PID framing fixes from dev.box. Keep the resize shutdown fix and validate all cases with the race detector. Publish same-repository PR images even when this repository is itself a fork; continue excluding external PR publication, with matching attestation and summary conditions. Signed-off-by: Matt, Matthias --- .github/workflows/docker.yml | 7 +- internal/kubernetes/kubernetesClientImpl.go | 12 +- .../kubernetes/kubernetesExecutionImpl.go | 10 +- internal/kubernetes/settings_pid_test.go | 146 ++++++++++++++++++ internal/kubernetes/settings_rollback_test.go | 114 ++++++++++++++ internal/security/handler_session.go | 10 +- internal/security/settings_signal_test.go | 51 ++++++ internal/sshserver/serverImpl.go | 1 + internal/sshserver/settings_start_test.go | 126 +++++++++++++++ .../startOnceSessionChannelHandler.go | 42 +++++ 10 files changed, 506 insertions(+), 13 deletions(-) create mode 100644 internal/kubernetes/settings_pid_test.go create mode 100644 internal/kubernetes/settings_rollback_test.go create mode 100644 internal/security/settings_signal_test.go create mode 100644 internal/sshserver/settings_start_test.go create mode 100644 internal/sshserver/startOnceSessionChannelHandler.go diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 53af737b..b63c9fae 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -59,14 +59,15 @@ jobs: platforms: linux/amd64,linux/arm64 provenance: false context: . - push: ${{ github.event.pull_request.head.repo.fork != true }} + # Our repository is itself a fork; publish trusted same-repository PRs. + push: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} cache-from: type=gha cache-to: type=gha,mode=max - name: Generate artifact attestation - if: ${{ github.event.pull_request.head.repo.fork != true }} + if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} uses: actions/attest@508db95dd578ae2727ebd6217d5ba78e4fbda05d # v4.2.1 with: subject-name: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} @@ -74,7 +75,7 @@ jobs: push-to-registry: true - name: Job summary - if: ${{ github.event.pull_request.head.repo.fork != true }} + if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} env: DIGEST: ${{ steps.push.outputs.digest }} REF_NAME: ${{ github.ref_name }} diff --git a/internal/kubernetes/kubernetesClientImpl.go b/internal/kubernetes/kubernetesClientImpl.go index 1bb12224..661465bd 100644 --- a/internal/kubernetes/kubernetesClientImpl.go +++ b/internal/kubernetes/kubernetesClientImpl.go @@ -151,7 +151,17 @@ func (k *kubernetesClientImpl) attemptPodCreate( wg: &sync.WaitGroup{}, removeLock: &sync.Mutex{}, } - return createdPod.wait(ctx) + readyPod, waitErr := createdPod.wait(ctx) + if waitErr != nil && k.config.Pod.Mode == containerSSHConfig.KubernetesExecutionModeConnection { + // Startup may have exhausted ctx. Roll back this creation before the + // retry loop loses its identity, with a separate, bounded stop budget. + // remove logs failures; preserve the original readiness error. + cleanupCtx, cancel := context.WithTimeout(context.Background(), k.config.Timeouts.PodStop) + _ = createdPod.remove(cleanupCtx) + cancel() + return nil, waitErr + } + return readyPod, waitErr } k.backendFailuresMetric.Increment() logger.Debug( diff --git a/internal/kubernetes/kubernetesExecutionImpl.go b/internal/kubernetes/kubernetesExecutionImpl.go index 35b958d9..411005e1 100644 --- a/internal/kubernetes/kubernetesExecutionImpl.go +++ b/internal/kubernetes/kubernetesExecutionImpl.go @@ -214,7 +214,7 @@ type stdoutProxyWriter struct { pidRead bool lock *sync.Mutex buf *bytes.Buffer - tty bool + waitEcho bool } func (s *stdoutProxyWriter) Write(p []byte) (n int, err error) { @@ -224,11 +224,13 @@ func (s *stdoutProxyWriter) Write(p []byte) (n int, err error) { return s.backend.Write(p) } // Read the pid. See https://github.com/containerssh/agent for details. - // We need 6 bytes because the first 2 bytes will be \r\n from the --wait function injected in the reader above. + // Session-mode PTYs also need the two echoed --wait bytes. Connection + // and persistent execs do not use --wait: their four-byte PID must unblock + // initial resizing before a terminal-dependent application emits output. if n, err := s.buf.Write(p); err != nil { return n, err } - if (s.tty && s.buf.Len() >= 6) || (!s.tty && s.buf.Len() >= 4) { + if (s.waitEcho && s.buf.Len() >= 6) || (!s.waitEcho && s.buf.Len() >= 4) { bufferBytes := s.buf.Bytes() s.pidRead = true s.buf = nil @@ -264,7 +266,7 @@ func (k *kubernetesExecutionImpl) run( } } stdout = &stdoutProxyWriter{ - tty: k.tty, + waitEcho: k.tty && k.pod.config.Pod.Mode == config.KubernetesExecutionModeSession, backend: stdout, lock: &sync.Mutex{}, pidChannel: pidChannel, diff --git a/internal/kubernetes/settings_pid_test.go b/internal/kubernetes/settings_pid_test.go new file mode 100644 index 00000000..d012a6ef --- /dev/null +++ b/internal/kubernetes/settings_pid_test.go @@ -0,0 +1,146 @@ +package kubernetes //nolint:testpackage // Exercises the unexported execution at the Kubernetes stream boundary. + +import ( + "bytes" + "context" + "fmt" + "io" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.containerssh.io/containerssh/config" + "go.containerssh.io/containerssh/internal/metrics" + "go.containerssh.io/containerssh/log" + "k8s.io/client-go/tools/remotecommand" + "k8s.io/client-go/util/exec" +) + +// Follow channelHandler's ordering: run must receive the agent PID before the +// initial resize. The stream emits no application output to rescue PID framing. +func TestSettingsPIDFramingAndResize(t *testing.T) { + for _, tc := range []struct { + mode config.KubernetesExecutionMode + tty bool + }{ + {config.KubernetesExecutionModeConnection, true}, + {config.KubernetesExecutionModePersistent, true}, + {config.KubernetesExecutionModeSession, true}, + {config.KubernetesExecutionModeConnection, false}, + {config.KubernetesExecutionModeSession, false}, + } { + t.Run(fmt.Sprintf("%s/tty=%t", tc.mode, tc.tty), func(t *testing.T) { + waitEcho := tc.mode == config.KubernetesExecutionModeSession && tc.tty + stream := &settingsPIDStream{pidWritten: make(chan struct{}), echo: make(chan bool), + size: make(chan remotecommand.TerminalSize, 1), finish: make(chan struct{}), waitEcho: waitEcho} + collector := metrics.New(nil) + pod := &kubernetesPodImpl{config: config.KubernetesConfig{}, tty: &tc.tty, wg: &sync.WaitGroup{}} + pod.config.Pod.Mode = tc.mode + if tc.mode == config.KubernetesExecutionModeConnection { + pod.wg.Add(1) + } + execution := &kubernetesExecutionImpl{pod: pod, exec: stream, tty: tc.tty, + terminalSizeQueue: &pushSizeQueueImpl{resizeChan: make(chan remotecommand.TerminalSize)}, + logger: log.NewTestLogger(t), doneChan: make(chan struct{}), lock: &sync.Mutex{}, + backendRequestsMetric: collector.MustCreateCounter("requests", "", "test"), + backendFailuresMetric: collector.MustCreateCounter("failures", "", "test")} + var stdout bytes.Buffer + runReturned, exited := make(chan struct{}), make(chan int, 1) + resizeResult := make(chan error, 1) + go func() { + execution.run(bytes.NewReader(nil), &stdout, io.Discard, func() error { return nil }, func(code int) { exited <- code }) + close(runReturned) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + resizeResult <- execution.resize(ctx, 24, 80) + }() + select { + case <-stream.pidWritten: + case <-time.After(time.Second): + t.Fatal("stream did not write PID") + } + stalled := false + if waitEcho { + select { + case <-runReturned: + t.Error("session PTY must retain its six-byte PID/wait-echo framing") + case <-time.After(50 * time.Millisecond): + } + } else { + select { + case <-runReturned: + case <-time.After(500 * time.Millisecond): + t.Error("four-byte PID must unblock run and initial resize without application output") + stalled = true + } + } + // On red, supply the two missing bytes only to drain stock's blocked + // goroutines; they are never part of the successful exec-mode fixture. + stream.echo <- waitEcho || stalled + select { + case size := <-stream.size: + require.Equal(t, remotecommand.TerminalSize{Width: 80, Height: 24}, size) + case <-time.After(2 * time.Second): + t.Fatal("initial resize never reached the stream") + } + require.NoError(t, <-resizeResult) + close(stream.finish) + require.Equal(t, 9, <-exited) + require.Equal(t, 0x1234, execution.pid) + if !stalled { + if waitEcho { + require.Equal(t, "\r\n", stdout.String()) + } else { + require.Empty(t, stdout.String()) + } + } + }) + } +} + +// The remotecommand.Executor boundary supplies agent bytes, then requires a +// terminal size before producing anything else, as a terminal-dependent app does. +type settingsPIDStream struct { + pidWritten chan struct{} + echo chan bool + size chan remotecommand.TerminalSize + finish chan struct{} + waitEcho bool +} + +func (s *settingsPIDStream) Stream(options remotecommand.StreamOptions) error { + return s.StreamWithContext(context.Background(), options) +} + +func (s *settingsPIDStream) StreamWithContext(_ context.Context, options remotecommand.StreamOptions) error { + if s.waitEcho { + var start [1]byte + if _, err := io.ReadFull(options.Stdin, start[:]); err != nil { + return err + } + if start[0] != '\n' { + return fmt.Errorf("missing session --wait start newline") + } + } + // Fragment the four-byte little-endian PID at the transport boundary. + if _, err := options.Stdout.Write([]byte{0x34}); err != nil { + return err + } + if _, err := options.Stdout.Write([]byte{0x12, 0, 0}); err != nil { + return err + } + close(s.pidWritten) + if <-s.echo { + if _, err := options.Stdout.Write([]byte("\r\n")); err != nil { + return err + } + } + size := options.TerminalSizeQueue.Next() + if size == nil { + return fmt.Errorf("size queue closed before initial resize") + } + s.size <- *size + <-s.finish + return exec.CodeExitError{Err: fmt.Errorf("fixture exit"), Code: 9} +} diff --git a/internal/kubernetes/settings_rollback_test.go b/internal/kubernetes/settings_rollback_test.go new file mode 100644 index 00000000..c900af14 --- /dev/null +++ b/internal/kubernetes/settings_rollback_test.go @@ -0,0 +1,114 @@ +package kubernetes //nolint:testpackage // Exercises the real client/readiness lifecycle against an HTTP API fixture. + +import ( + "context" + "encoding/json" + "net" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.containerssh.io/containerssh/config" + "go.containerssh.io/containerssh/internal/metrics" + "go.containerssh.io/containerssh/internal/structutils" + "go.containerssh.io/containerssh/log" + "go.containerssh.io/containerssh/metadata" + core "k8s.io/api/core/v1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// Use the real client, readiness watch and network-handler lifecycle against an +// HTTP API seam. No cluster/scheduler is needed to reproduce a pending pod leak. +func TestSettingsReadinessFailureRollback(t *testing.T) { + for _, tc := range []struct { + name string + mode config.KubernetesExecutionMode + deleteFails bool + wantDelete bool + }{ + {"connection healthy API", config.KubernetesExecutionModeConnection, false, true}, + {"connection unavailable delete", config.KubernetesExecutionModeConnection, true, true}, + {"persistent unchanged", config.KubernetesExecutionModePersistent, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + var lock sync.Mutex + var created, deleted int + var deleteAfterStartTimeout bool + var started time.Time + pod := core.Pod{TypeMeta: meta.TypeMeta{APIVersion: "v1", Kind: "Pod"}, + ObjectMeta: meta.ObjectMeta{Name: "settings-pending", Namespace: "sessions", UID: "created-uid", ResourceVersion: "1"}, + Status: core.PodStatus{Phase: core.PodPending}} + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodPost: + lock.Lock() + created++ + lock.Unlock() + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(pod) + case r.Method == http.MethodDelete: + lock.Lock() + deleted++ + deleteAfterStartTimeout = time.Since(started) >= 300*time.Millisecond + lock.Unlock() + if tc.deleteFails { + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(meta.Status{Status: "Failure", Reason: meta.StatusReasonServiceUnavailable, Code: 503}) + } else { + // Kubernetes can acknowledge deletion while finalizers retain the object. + _ = json.NewEncoder(w).Encode(meta.Status{Status: "Success", Code: 200}) + } + case r.URL.Query().Get("watch") == "true": + _ = json.NewEncoder(w).Encode(map[string]interface{}{"type": "ADDED", "object": pod}) + w.(http.Flusher).Flush() + <-r.Context().Done() + default: + _ = json.NewEncoder(w).Encode(core.PodList{TypeMeta: meta.TypeMeta{APIVersion: "v1", Kind: "PodList"}, + ListMeta: meta.ListMeta{ResourceVersion: "1"}, Items: []core.Pod{pod}}) + } + })) + defer api.Close() + cfg := config.KubernetesConfig{} + structutils.Defaults(&cfg) + cfg.Pod.Mode = tc.mode + cfg.Connection.Host = api.URL + cfg.Timeouts.PodStart = 300 * time.Millisecond + cfg.Timeouts.PodStop = 200 * time.Millisecond + logger := log.NewTestLogger(t) + collector := metrics.New(nil) + factory := &kubernetesClientFactoryImpl{ + backendRequestsMetric: collector.MustCreateCounter("requests", "", "test"), + backendFailuresMetric: collector.MustCreateCounter("failures", "", "test"), + } + cli, err := factory.get(context.Background(), cfg, logger) + require.NoError(t, err) + network := &networkHandler{mutex: &sync.Mutex{}, cli: cli, config: cfg, logger: logger, + client: net.TCPAddr{IP: net.IPv4(127, 0, 0, 1)}, connectionID: "settings", done: make(chan struct{})} + // Persistent mode uses findPod first; exercise creation directly to avoid its stock retry sleeps. + started = time.Now() + if tc.mode == config.KubernetesExecutionModePersistent { + ctx, cancel := context.WithTimeout(context.Background(), cfg.Timeouts.PodStart) + defer cancel() + _, err = cli.createPod(ctx, nil, nil, nil, nil, nil) + } else { + _, _, err = network.OnHandshakeSuccess(metadata.ConnectionAuthenticatedMetadata{AuthenticatedUsername: "owner"}) + } + require.Error(t, err, "pending pod must fail startup") + require.Less(t, time.Since(started), 2*time.Second, "rollback must not reuse a background context without a deadline") + network.OnDisconnect() + lock.Lock() + defer lock.Unlock() + require.Equal(t, 1, created) + if tc.wantDelete { + require.Equal(t, 1, deleted, "failed readiness must not orphan the created connection pod") + require.True(t, deleteAfterStartTimeout, "rollback uses a fresh context after startup expires") + } else { + require.Zero(t, deleted, "persistent pods retain their existing lifecycle") + } + }) + } +} diff --git a/internal/security/handler_session.go b/internal/security/handler_session.go index 33d50240..0abac0e5 100644 --- a/internal/security/handler_session.go +++ b/internal/security/handler_session.go @@ -3,10 +3,10 @@ package security import ( "context" - config2 "go.containerssh.io/containerssh/config" - "go.containerssh.io/containerssh/internal/sshserver" - "go.containerssh.io/containerssh/log" - "go.containerssh.io/containerssh/message" + config2 "go.containerssh.io/containerssh/config" + "go.containerssh.io/containerssh/internal/sshserver" + "go.containerssh.io/containerssh/log" + "go.containerssh.io/containerssh/message" ) type sessionHandler struct { @@ -262,7 +262,7 @@ func (s *sessionHandler) OnSubsystem( } func (s *sessionHandler) OnSignal(requestID uint64, signal string) error { - mode := s.getPolicy(s.config.Shell.Mode) + mode := s.getPolicy(s.config.Signal.Mode) switch mode { case config2.ExecutionPolicyDisable: err := message.UserMessage( diff --git a/internal/security/settings_signal_test.go b/internal/security/settings_signal_test.go new file mode 100644 index 00000000..742388e2 --- /dev/null +++ b/internal/security/settings_signal_test.go @@ -0,0 +1,51 @@ +package security //nolint:testpackage // Exercises internal policy routing, consistent with existing security tests. + +import ( + "testing" + + "github.com/stretchr/testify/require" + "go.containerssh.io/containerssh/config" + "go.containerssh.io/containerssh/log" +) + +func TestSettingsSignalPolicyIndependentOfShell(t *testing.T) { + for _, tc := range []struct { + name string + shell, signal config.SecurityExecutionPolicy + allow, deny []string + wantForward bool + }{ + {"disabled with shell enabled", config.ExecutionPolicyEnable, config.ExecutionPolicyDisable, nil, nil, false}, + {"enabled with shell disabled", config.ExecutionPolicyDisable, config.ExecutionPolicyEnable, nil, nil, true}, + {"filter allowed", config.ExecutionPolicyDisable, config.ExecutionPolicyFilter, []string{"TERM"}, nil, true}, + {"filter unlisted", config.ExecutionPolicyEnable, config.ExecutionPolicyFilter, []string{"INT"}, nil, false}, + {"enabled denied", config.ExecutionPolicyEnable, config.ExecutionPolicyEnable, nil, []string{"TERM"}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + backend := &settingsSignalBackend{} + session := &sessionHandler{backend: backend, logger: log.NewTestLogger(t)} + session.config.Shell.Mode = tc.shell + session.config.Signal.Mode = tc.signal + session.config.Signal.Allow = tc.allow + session.config.Signal.Deny = tc.deny + err := session.OnSignal(7, "TERM") + if tc.wantForward { + require.NoError(t, err) + require.Equal(t, []string{"TERM"}, backend.signals) + } else { + require.Error(t, err) + require.Empty(t, backend.signals) + } + }) + } +} + +type settingsSignalBackend struct { + dummyBackend + signals []string +} + +func (b *settingsSignalBackend) OnSignal(_ uint64, signal string) error { + b.signals = append(b.signals, signal) + return nil +} diff --git a/internal/sshserver/serverImpl.go b/internal/sshserver/serverImpl.go index 17a169d1..43dab5a2 100644 --- a/internal/sshserver/serverImpl.go +++ b/internal/sshserver/serverImpl.go @@ -858,6 +858,7 @@ func (s *serverImpl) handleSessionChannel( channelMetadata.Connection.ConnectionID, channelMetadata.ChannelID, ) + handlerChannel = &startOnceSessionChannelHandler{SessionChannelHandler: handlerChannel} s.shutdownHandlers.Register(shutdownHandlerID, handlerChannel) channel, requests, err := newChannel.Accept() if err != nil { diff --git a/internal/sshserver/settings_start_test.go b/internal/sshserver/settings_start_test.go new file mode 100644 index 00000000..3f564da8 --- /dev/null +++ b/internal/sshserver/settings_start_test.go @@ -0,0 +1,126 @@ +package sshserver_test + +import ( + "fmt" + "net" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.containerssh.io/containerssh/internal/sshserver" + "go.containerssh.io/containerssh/log" + "go.containerssh.io/containerssh/metadata" + "golang.org/x/crypto/ssh" +) + +// Exercise the real request/reply loop, not a backend that already refuses restarts. +func TestSettingsSingleAcceptedStart(t *testing.T) { + logger := log.NewTestLogger(t) + user := sshserver.NewTestUser("settings") + user.SetPassword("test") + handler := &settingsStartHandler{} + srv := sshserver.NewTestServer(t, sshserver.NewTestAuthenticationHandler(handler, user), logger, nil) + srv.Start() + defer srv.Stop(5 * time.Second) + conn, err := net.DialTimeout("tcp", srv.GetListen(), 5*time.Second) + require.NoError(t, err) + defer func() { _ = conn.Close() }() + require.NoError(t, conn.SetDeadline(time.Now().Add(20*time.Second))) + hostKey, err := ssh.ParsePrivateKey([]byte(srv.GetHostKey())) + require.NoError(t, err) + sshConn, channels, requests, err := ssh.NewClientConn(conn, srv.GetListen(), &ssh.ClientConfig{ + User: "settings", Auth: []ssh.AuthMethod{ssh.Password("test")}, + HostKeyCallback: ssh.FixedHostKey(hostKey.PublicKey()), + }) + require.NoError(t, err) + client := ssh.NewClient(sshConn, channels, requests) + defer func() { _ = client.Close() }() + starts := []struct { + name string + payload []byte + }{ + {"shell", nil}, + {"exec", ssh.Marshal(struct{ Command string }{"/app"})}, + {"subsystem", ssh.Marshal(struct{ Name string }{"sftp"})}, + } + for _, first := range starts { + t.Run(first.name, func(t *testing.T) { + channel, replies, err := client.OpenChannel("session", nil) + require.NoError(t, err) + defer func() { _ = channel.Close() }() + go ssh.DiscardRequests(replies) + before := handler.attempts.Load() + // A backend-rejected start must not consume the channel's opportunity. + ok, err := channel.SendRequest(first.name, true, first.payload) + require.NoError(t, err) + require.False(t, ok) + ok, err = channel.SendRequest(first.name, true, first.payload) + require.NoError(t, err) + require.True(t, ok) + for _, next := range starts { + ok, err = channel.SendRequest(next.name, true, next.payload) + require.NoError(t, err) + require.False(t, ok, "second %s after %s must be rejected", next.name, first.name) + } + // Exit status doesn't make the same channel reusable; non-start requests still work. + ok, err = channel.SendRequest("env", true, ssh.Marshal(struct{ Name, Value string }{"exit", "0"})) + require.NoError(t, err) + require.True(t, ok) + ok, err = channel.SendRequest("shell", true, nil) + require.NoError(t, err) + require.False(t, ok) + require.Equal(t, before+2, handler.attempts.Load(), "only failed start and first accepted start reach backend") + }) + } +} + +type settingsStartHandler struct { + sshserver.AbstractHandler + attempts atomic.Int32 +} + +func (h *settingsStartHandler) OnNetworkConnection(meta metadata.ConnectionMetadata) (sshserver.NetworkConnectionHandler, metadata.ConnectionMetadata, error) { + return &settingsStartNetwork{handler: h}, meta, nil +} + +type settingsStartNetwork struct { + sshserver.AbstractNetworkConnectionHandler + handler *settingsStartHandler +} + +func (n *settingsStartNetwork) OnHandshakeSuccess(meta metadata.ConnectionAuthenticatedMetadata) (sshserver.SSHConnectionHandler, metadata.ConnectionAuthenticatedMetadata, error) { + return &settingsStartConnection{handler: n.handler}, meta, nil +} + +type settingsStartConnection struct { + fullSSHConnectionHandler + handler *settingsStartHandler +} + +func (c *settingsStartConnection) OnSessionChannel(_ metadata.ChannelMetadata, _ []byte, channel sshserver.SessionChannel) (sshserver.SessionChannelHandler, sshserver.ChannelRejection) { + return &settingsStartSession{handler: c.handler, channel: channel, rejectFirst: true}, nil +} + +type settingsStartSession struct { + sshserver.AbstractSessionChannelHandler + handler *settingsStartHandler + channel sshserver.SessionChannel + rejectFirst bool +} + +func (s *settingsStartSession) start() error { + s.handler.attempts.Add(1) + if s.rejectFirst { + s.rejectFirst = false + return fmt.Errorf("start failed") + } + return nil +} +func (s *settingsStartSession) OnShell(_ uint64) error { return s.start() } +func (s *settingsStartSession) OnSubsystem(_ uint64, _ string) error { return s.start() } +func (s *settingsStartSession) OnExecRequest(_ uint64, _ string) error { return s.start() } +func (s *settingsStartSession) OnEnvRequest(_ uint64, _, _ string) error { + s.channel.ExitStatus(0) + return nil +} diff --git a/internal/sshserver/startOnceSessionChannelHandler.go b/internal/sshserver/startOnceSessionChannelHandler.go new file mode 100644 index 00000000..3130eec6 --- /dev/null +++ b/internal/sshserver/startOnceSessionChannelHandler.go @@ -0,0 +1,42 @@ +// Package sshserver implements SSH transport and dispatches requests to backend handlers. +package sshserver + +import ( + "fmt" + "sync" +) + +// startOnceSessionChannelHandler permits one successful program start over the +// channel's lifetime. Failed starts remain retryable, as in the stock server. +// The lock is separate from channelWrapper's IO lock: a backend start may send +// exit status or close the channel synchronously. +type startOnceSessionChannelHandler struct { + SessionChannelHandler + startLock sync.Mutex + started bool +} + +func (s *startOnceSessionChannelHandler) start(start func() error) error { + s.startLock.Lock() + defer s.startLock.Unlock() + if s.started { + return fmt.Errorf("a program has already been started on this channel") + } + if err := start(); err != nil { + return err + } + s.started = true + return nil +} + +func (s *startOnceSessionChannelHandler) OnShell(requestID uint64) error { + return s.start(func() error { return s.SessionChannelHandler.OnShell(requestID) }) +} + +func (s *startOnceSessionChannelHandler) OnExecRequest(requestID uint64, command string) error { + return s.start(func() error { return s.SessionChannelHandler.OnExecRequest(requestID, command) }) +} + +func (s *startOnceSessionChannelHandler) OnSubsystem(requestID uint64, subsystem string) error { + return s.start(func() error { return s.SessionChannelHandler.OnSubsystem(requestID, subsystem) }) +} From d0f6cbded67c5930e1189a349050df8c04c967dd Mon Sep 17 00:00:00 2001 From: "Matt, Matthias" Date: Fri, 16 Oct 2026 12:10:00 +0200 Subject: [PATCH 4/6] test: build pinned MinIO source instead of pulling removed images Both PR and push CI failed TestSmallUpload, TestLargeUpload, and TestS3 because the public minio/minio image is no longer available. Official Quay images and binary downloads are also unavailable. Build the official RELEASE.2025-10-15T17-29-55Z source commit as a local Docker fixture and reuse the existing S3 helper for upload tests. Keep integration assertions enabled, use dynamically allocated ports, and clean up temporary storage via testing.T. Validation: all three original failures reproduced locally; pinned source builds; targeted vet and golangci-lint pass; Settings and resize regressions pass with race detection (20 repetitions). Local Docker integration verification is blocked by a full Docker VM disk; validate the complete suite in CI. --- internal/auditlog/README.md | 2 +- internal/auditlog/storage/s3/upload_test.go | 259 ++------------------ internal/test/README.md | 2 + internal/test/minio/Dockerfile | 9 + internal/test/s3.go | 9 +- 5 files changed, 43 insertions(+), 238 deletions(-) create mode 100644 internal/test/minio/Dockerfile diff --git a/internal/auditlog/README.md b/internal/auditlog/README.md index 0c55c1e3..1eccee5e 100644 --- a/internal/auditlog/README.md +++ b/internal/auditlog/README.md @@ -184,7 +184,7 @@ for { ## Development -In order to successfully run the tests for this library you will need a working [Docker](https://www.docker.com/) or [Podman](https://podman.io/) setup to run `minio/minio` for the S3 upload. +In order to successfully run the tests for this library you will need a working [Docker](https://www.docker.com/) or Docker-compatible [Podman](https://podman.io/) API socket. The [S3 test helper](../test/README.md#starting-an-s3-service) builds MinIO from pinned official source for the S3 upload tests; allow several minutes for the first build. ### Manually encoding messages diff --git a/internal/auditlog/storage/s3/upload_test.go b/internal/auditlog/storage/s3/upload_test.go index 927ab767..3862ae2f 100644 --- a/internal/auditlog/storage/s3/upload_test.go +++ b/internal/auditlog/storage/s3/upload_test.go @@ -2,11 +2,7 @@ package s3_test import ( "context" - "fmt" "io" - goLog "log" - "net" - "os" "testing" "time" @@ -14,224 +10,43 @@ import ( "github.com/aws/aws-sdk-go/aws/credentials" "github.com/aws/aws-sdk-go/aws/session" awsS3 "github.com/aws/aws-sdk-go/service/s3" - "github.com/docker/docker/api/types/container" - "github.com/docker/docker/api/types/image" - "github.com/docker/docker/client" - "github.com/docker/go-connections/nat" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.containerssh.io/containerssh/config" auditLogStorage "go.containerssh.io/containerssh/internal/auditlog/storage" "go.containerssh.io/containerssh/internal/auditlog/storage/s3" + "go.containerssh.io/containerssh/internal/test" "go.containerssh.io/containerssh/log" ) -type minio struct { - containerID string - dir string - storage auditLogStorage.ReadWriteStorage -} - -func (m *minio) getClient() (*client.Client, error) { - cli, err := client.NewClientWithOpts() - if err != nil { - return cli, err - } - cli.NegotiateAPIVersion(context.Background()) - return cli, nil -} - -func (m *minio) Start( - t *testing.T, - accessKey string, - secretKey string, - region string, - bucket string, - endpoint string, -) (auditLogStorage.ReadWriteStorage, error) { - if m.containerID == "" { - if err := m.startMinio(t, accessKey, secretKey); err != nil { - return nil, err - } - } - - var err error - for i := 0; i < 6; i++ { - if err = setupS3(accessKey, secretKey, endpoint, region, bucket); err != nil { - time.Sleep(5 * time.Second) - continue - } - break - } - - if err != nil { - return nil, err - } - - m.dir, err = os.MkdirTemp(os.TempDir(), "containerssh-s3-upload-test") - if err != nil { - assert.Fail(t, "failed to create temporary directory (%v)", err) - return nil, err - } - - err = m.setupStorage(t, accessKey, secretKey, region, bucket, endpoint) - if err != nil { - return nil, err - } - - return m.storage, nil -} +func newTestStorage(t *testing.T) auditLogStorage.ReadWriteStorage { + t.Helper() + s3Service := test.S3(t) + bucket := "auditlog" + require.NoError(t, setupS3( + s3Service.AccessKey(), s3Service.SecretKey(), s3Service.URL(), s3Service.Region(), bucket, + )) -func (m *minio) setupStorage( - t *testing.T, - accessKey string, - secretKey string, - region string, - bucket string, - endpoint string, -) error { - logger := log.NewTestLogger(t) - var err error - m.storage, err = s3.NewStorage( + storage, err := s3.NewStorage( config.AuditLogS3Config{ - Local: m.dir, - AccessKey: accessKey, - SecretKey: secretKey, + Local: t.TempDir(), + AccessKey: s3Service.AccessKey(), + SecretKey: s3Service.SecretKey(), Bucket: bucket, - Region: region, - Endpoint: endpoint, - PathStyleAccess: true, + Region: s3Service.Region(), + Endpoint: s3Service.URL(), + PathStyleAccess: s3Service.PathStyle(), UploadPartSize: 5 * 1024 * 1024, ParallelUploads: 20, Metadata: config.AuditLogS3Metadata{}, }, - logger, + log.NewTestLogger(t), ) - if err != nil { - assert.Fail(t, "failed to create storage (%v)", err) - return err - } - return nil -} - -var hostConfig = &container.HostConfig{ - AutoRemove: true, - PortBindings: map[nat.Port][]nat.PortBinding{ - "9000/tcp": { - { - HostIP: "127.0.0.1", - HostPort: "9000", - }, - }, - }, -} - -func (m *minio) startMinio(t *testing.T, accessKey string, secretKey string) error { - ctx := context.Background() - - cli, err := m.getClient() - if err != nil { - assert.Fail(t, "failed to create Docker client (%v)", err) - return err - } - - reader, err := cli.ImagePull(ctx, "docker.io/minio/minio", image.PullOptions{}) - if err != nil { - assert.Fail(t, "failed to pull Minio image (%v)", err) - return err - } - if _, err := io.Copy(os.Stdout, reader); err != nil { - assert.Fail(t, "failed to stream logs from Minio image pull (%v)", err) - return err - } - - env := []string{ - fmt.Sprintf("MINIO_ACCESS_KEY=%s", accessKey), - fmt.Sprintf("MINIO_SECRET_KEY=%s", secretKey), - } - - resp, err := cli.ContainerCreate( - ctx, - &container.Config{ - Image: "minio/minio", - Cmd: []string{"server", "/testdata"}, - Env: env, - }, - hostConfig, - nil, - nil, - "", - ) - if err != nil { - assert.Fail(t, "failed to create Minio container (%v)", err) - return err - } - - m.containerID = resp.ID - - if err := cli.ContainerStart(ctx, resp.ID, container.StartOptions{}); err != nil { - assert.Fail(t, "failed to start minio container (%v)", err) - return err - } - - if err := m.waitForMinio(t); err != nil { - return err - } - - return nil -} - -func (m *minio) waitForMinio(t *testing.T) error { - tries := 0 - for { - if tries > 30 { - m.Stop() - assert.Fail(t, "minio failed to come up within 30 seconds") - return fmt.Errorf("minio failed to come up") - } - sock, err := net.Dial("tcp", "127.0.0.1:9000") - if err != nil { - tries++ - time.Sleep(time.Second) - } else { - _ = sock.Close() - - break - } - } - return nil -} - -func (m *minio) Stop() { - if m.containerID != "" { - ctx := context.Background() - - cli, err := m.getClient() - if err != nil { - goLog.Printf("failed to create Docker client (%v)\n", err) - } - - if err := cli.ContainerRemove(ctx, m.containerID, container.RemoveOptions{ - RemoveVolumes: false, - RemoveLinks: false, - Force: true, - }); err != nil { - goLog.Println("failed to stop Minio container", err) - } - - ok, errChan := cli.ContainerWait(ctx, m.containerID, container.WaitConditionRemoved) - select { - case <-ok: - m.containerID = "" - case <-errChan: - } - } - - if m.dir != "" { - m.dir = "" - if err := os.RemoveAll(m.dir); err != nil { - goLog.Printf("failed to remove temporary directory (%v)", err) - } - } + require.NoError(t, err) + t.Cleanup(func() { + storage.Shutdown(context.Background()) + }) + return storage } func setupS3(accessKey string, secretKey string, endpoint string, region string, bucket string) error { @@ -305,20 +120,7 @@ func TestSmallUpload(t *testing.T) { if testing.Short() { t.Skipf("skipping test in short mode") } - accessKey := "asdfasdfasdf" - secretKey := "asdfasdfasdf" - - region := "us-east-1" - bucket := "auditlog" - endpoint := "http://127.0.0.1:9000" - - m := &minio{} - - storage, err := m.Start(t, accessKey, secretKey, region, bucket, endpoint) - defer m.Stop() - if err != nil { - return - } + storage := newTestStorage(t) writer, err := storage.OpenWriter("test") if err != nil { @@ -358,20 +160,7 @@ func TestLargeUpload(t *testing.T) { if testing.Short() { t.Skipf("skipping test in short mode") } - accessKey := "asdfasdfasdf" - secretKey := "asdfasdfasdf" - - region := "us-east-1" - bucket := "auditlog" - endpoint := "http://127.0.0.1:9000" - - m := &minio{} - - storage, err := m.Start(t, accessKey, secretKey, region, bucket, endpoint) - defer m.Stop() - if err != nil { - return - } + storage := newTestStorage(t) writer, err := storage.OpenWriter("test") if err != nil { diff --git a/internal/test/README.md b/internal/test/README.md index 08366528..696d38b4 100644 --- a/internal/test/README.md +++ b/internal/test/README.md @@ -5,6 +5,8 @@ This library helps with bringing up services for testing, such as S3, oAuth, etc ## Starting an S3 service +Allow several minutes for the first run: the helper builds MinIO from a pinned official source commit using [minio/Dockerfile](minio/Dockerfile). Docker caches the build for subsequent runs. The build requires access to Docker Hub and the Go module proxy. + To start the S3 service and then use it with the AWS client as follows: ```go diff --git a/internal/test/minio/Dockerfile b/internal/test/minio/Dockerfile new file mode 100644 index 00000000..01bfe71c --- /dev/null +++ b/internal/test/minio/Dockerfile @@ -0,0 +1,9 @@ +FROM golang:1.25.3-alpine3.22 AS build + +# Official prebuilt community images are no longer publicly available. +# Pin RELEASE.2025-10-15T17-29-55Z instead of depending on a moving tag. +RUN CGO_ENABLED=0 go install github.com/minio/minio@9e49d5e7a648f00e26f2246f4dc28e6b07f8c84a + +FROM alpine:3.23 +COPY --from=build /go/bin/minio /usr/local/bin/minio +ENTRYPOINT ["/usr/local/bin/minio"] diff --git a/internal/test/s3.go b/internal/test/s3.go index fe1bcaeb..a6626b83 100644 --- a/internal/test/s3.go +++ b/internal/test/s3.go @@ -1,6 +1,7 @@ package test import ( + "embed" "fmt" "testing" "time" @@ -11,6 +12,9 @@ import ( "github.com/aws/aws-sdk-go/service/s3" ) +//go:embed minio +var minioBuildRoot embed.FS + // s3Lock is used to start a maximum of 5 Minio instances for testing. var s3Lock = make(chan struct{}, 5) @@ -34,9 +38,10 @@ func S3(t *testing.T) S3Helper { } t.Log("Starting Minio in a container...") m := &minio{ - cnt: containerFromPull( + cnt: containerFromBuild( t, - "docker.io/minio/minio", + "containerssh-test-minio", + dockerBuildRootFiles(minioBuildRoot, "minio"), []string{"server", "/data", "--console-address", ":9001"}, env, map[string]string{ From bd2383d7de0e1369dfd8a4302219376fd1b613d5 Mon Sep 17 00:00:00 2001 From: "Matt, Matthias" Date: Fri, 16 Oct 2026 12:29:03 +0200 Subject: [PATCH 5/6] test: expose the source-built MinIO fixture's S3 port CI built the pinned source successfully, but Docker returned no port mapping because the replacement image lacked the original image's EXPOSE metadata. Declare port 9000 so the existing test helper can allocate and discover its host port. --- internal/test/minio/Dockerfile | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/test/minio/Dockerfile b/internal/test/minio/Dockerfile index 01bfe71c..89901c2b 100644 --- a/internal/test/minio/Dockerfile +++ b/internal/test/minio/Dockerfile @@ -6,4 +6,5 @@ RUN CGO_ENABLED=0 go install github.com/minio/minio@9e49d5e7a648f00e26f2246f4dc2 FROM alpine:3.23 COPY --from=build /go/bin/minio /usr/local/bin/minio +EXPOSE 9000 ENTRYPOINT ["/usr/local/bin/minio"] From c588dba330ffbf8814b25f32b57da1c8b32ae649 Mon Sep 17 00:00:00 2001 From: "Matt, Matthias" Date: Fri, 16 Oct 2026 13:31:32 +0200 Subject: [PATCH 6/6] test: drop custom MinIO fixture in favor of upstream fix Revert the source-built MinIO fixture and upload-helper refactor. The updated deploy branch already carries upstream's minimal switch to pgsty/minio, so Settings does not need a separate test-infrastructure implementation. --- internal/auditlog/README.md | 2 +- internal/auditlog/storage/s3/upload_test.go | 259 ++++++++++++++++++-- internal/test/README.md | 2 - internal/test/minio/Dockerfile | 10 - internal/test/s3.go | 9 +- 5 files changed, 238 insertions(+), 44 deletions(-) delete mode 100644 internal/test/minio/Dockerfile diff --git a/internal/auditlog/README.md b/internal/auditlog/README.md index 1eccee5e..0c55c1e3 100644 --- a/internal/auditlog/README.md +++ b/internal/auditlog/README.md @@ -184,7 +184,7 @@ for { ## Development -In order to successfully run the tests for this library you will need a working [Docker](https://www.docker.com/) or Docker-compatible [Podman](https://podman.io/) API socket. The [S3 test helper](../test/README.md#starting-an-s3-service) builds MinIO from pinned official source for the S3 upload tests; allow several minutes for the first build. +In order to successfully run the tests for this library you will need a working [Docker](https://www.docker.com/) or [Podman](https://podman.io/) setup to run `minio/minio` for the S3 upload. ### Manually encoding messages diff --git a/internal/auditlog/storage/s3/upload_test.go b/internal/auditlog/storage/s3/upload_test.go index 3862ae2f..927ab767 100644 --- a/internal/auditlog/storage/s3/upload_test.go +++ b/internal/auditlog/storage/s3/upload_test.go @@ -2,7 +2,11 @@ package s3_test import ( "context" + "fmt" "io" + goLog "log" + "net" + "os" "testing" "time" @@ -10,43 +14,224 @@ import ( "github.com/aws/aws-sdk-go/aws/credentials" "github.com/aws/aws-sdk-go/aws/session" awsS3 "github.com/aws/aws-sdk-go/service/s3" + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/image" + "github.com/docker/docker/client" + "github.com/docker/go-connections/nat" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "go.containerssh.io/containerssh/config" auditLogStorage "go.containerssh.io/containerssh/internal/auditlog/storage" "go.containerssh.io/containerssh/internal/auditlog/storage/s3" - "go.containerssh.io/containerssh/internal/test" "go.containerssh.io/containerssh/log" ) -func newTestStorage(t *testing.T) auditLogStorage.ReadWriteStorage { - t.Helper() - s3Service := test.S3(t) - bucket := "auditlog" - require.NoError(t, setupS3( - s3Service.AccessKey(), s3Service.SecretKey(), s3Service.URL(), s3Service.Region(), bucket, - )) +type minio struct { + containerID string + dir string + storage auditLogStorage.ReadWriteStorage +} - storage, err := s3.NewStorage( +func (m *minio) getClient() (*client.Client, error) { + cli, err := client.NewClientWithOpts() + if err != nil { + return cli, err + } + cli.NegotiateAPIVersion(context.Background()) + return cli, nil +} + +func (m *minio) Start( + t *testing.T, + accessKey string, + secretKey string, + region string, + bucket string, + endpoint string, +) (auditLogStorage.ReadWriteStorage, error) { + if m.containerID == "" { + if err := m.startMinio(t, accessKey, secretKey); err != nil { + return nil, err + } + } + + var err error + for i := 0; i < 6; i++ { + if err = setupS3(accessKey, secretKey, endpoint, region, bucket); err != nil { + time.Sleep(5 * time.Second) + continue + } + break + } + + if err != nil { + return nil, err + } + + m.dir, err = os.MkdirTemp(os.TempDir(), "containerssh-s3-upload-test") + if err != nil { + assert.Fail(t, "failed to create temporary directory (%v)", err) + return nil, err + } + + err = m.setupStorage(t, accessKey, secretKey, region, bucket, endpoint) + if err != nil { + return nil, err + } + + return m.storage, nil +} + +func (m *minio) setupStorage( + t *testing.T, + accessKey string, + secretKey string, + region string, + bucket string, + endpoint string, +) error { + logger := log.NewTestLogger(t) + var err error + m.storage, err = s3.NewStorage( config.AuditLogS3Config{ - Local: t.TempDir(), - AccessKey: s3Service.AccessKey(), - SecretKey: s3Service.SecretKey(), + Local: m.dir, + AccessKey: accessKey, + SecretKey: secretKey, Bucket: bucket, - Region: s3Service.Region(), - Endpoint: s3Service.URL(), - PathStyleAccess: s3Service.PathStyle(), + Region: region, + Endpoint: endpoint, + PathStyleAccess: true, UploadPartSize: 5 * 1024 * 1024, ParallelUploads: 20, Metadata: config.AuditLogS3Metadata{}, }, - log.NewTestLogger(t), + logger, ) - require.NoError(t, err) - t.Cleanup(func() { - storage.Shutdown(context.Background()) - }) - return storage + if err != nil { + assert.Fail(t, "failed to create storage (%v)", err) + return err + } + return nil +} + +var hostConfig = &container.HostConfig{ + AutoRemove: true, + PortBindings: map[nat.Port][]nat.PortBinding{ + "9000/tcp": { + { + HostIP: "127.0.0.1", + HostPort: "9000", + }, + }, + }, +} + +func (m *minio) startMinio(t *testing.T, accessKey string, secretKey string) error { + ctx := context.Background() + + cli, err := m.getClient() + if err != nil { + assert.Fail(t, "failed to create Docker client (%v)", err) + return err + } + + reader, err := cli.ImagePull(ctx, "docker.io/minio/minio", image.PullOptions{}) + if err != nil { + assert.Fail(t, "failed to pull Minio image (%v)", err) + return err + } + if _, err := io.Copy(os.Stdout, reader); err != nil { + assert.Fail(t, "failed to stream logs from Minio image pull (%v)", err) + return err + } + + env := []string{ + fmt.Sprintf("MINIO_ACCESS_KEY=%s", accessKey), + fmt.Sprintf("MINIO_SECRET_KEY=%s", secretKey), + } + + resp, err := cli.ContainerCreate( + ctx, + &container.Config{ + Image: "minio/minio", + Cmd: []string{"server", "/testdata"}, + Env: env, + }, + hostConfig, + nil, + nil, + "", + ) + if err != nil { + assert.Fail(t, "failed to create Minio container (%v)", err) + return err + } + + m.containerID = resp.ID + + if err := cli.ContainerStart(ctx, resp.ID, container.StartOptions{}); err != nil { + assert.Fail(t, "failed to start minio container (%v)", err) + return err + } + + if err := m.waitForMinio(t); err != nil { + return err + } + + return nil +} + +func (m *minio) waitForMinio(t *testing.T) error { + tries := 0 + for { + if tries > 30 { + m.Stop() + assert.Fail(t, "minio failed to come up within 30 seconds") + return fmt.Errorf("minio failed to come up") + } + sock, err := net.Dial("tcp", "127.0.0.1:9000") + if err != nil { + tries++ + time.Sleep(time.Second) + } else { + _ = sock.Close() + + break + } + } + return nil +} + +func (m *minio) Stop() { + if m.containerID != "" { + ctx := context.Background() + + cli, err := m.getClient() + if err != nil { + goLog.Printf("failed to create Docker client (%v)\n", err) + } + + if err := cli.ContainerRemove(ctx, m.containerID, container.RemoveOptions{ + RemoveVolumes: false, + RemoveLinks: false, + Force: true, + }); err != nil { + goLog.Println("failed to stop Minio container", err) + } + + ok, errChan := cli.ContainerWait(ctx, m.containerID, container.WaitConditionRemoved) + select { + case <-ok: + m.containerID = "" + case <-errChan: + } + } + + if m.dir != "" { + m.dir = "" + if err := os.RemoveAll(m.dir); err != nil { + goLog.Printf("failed to remove temporary directory (%v)", err) + } + } } func setupS3(accessKey string, secretKey string, endpoint string, region string, bucket string) error { @@ -120,7 +305,20 @@ func TestSmallUpload(t *testing.T) { if testing.Short() { t.Skipf("skipping test in short mode") } - storage := newTestStorage(t) + accessKey := "asdfasdfasdf" + secretKey := "asdfasdfasdf" + + region := "us-east-1" + bucket := "auditlog" + endpoint := "http://127.0.0.1:9000" + + m := &minio{} + + storage, err := m.Start(t, accessKey, secretKey, region, bucket, endpoint) + defer m.Stop() + if err != nil { + return + } writer, err := storage.OpenWriter("test") if err != nil { @@ -160,7 +358,20 @@ func TestLargeUpload(t *testing.T) { if testing.Short() { t.Skipf("skipping test in short mode") } - storage := newTestStorage(t) + accessKey := "asdfasdfasdf" + secretKey := "asdfasdfasdf" + + region := "us-east-1" + bucket := "auditlog" + endpoint := "http://127.0.0.1:9000" + + m := &minio{} + + storage, err := m.Start(t, accessKey, secretKey, region, bucket, endpoint) + defer m.Stop() + if err != nil { + return + } writer, err := storage.OpenWriter("test") if err != nil { diff --git a/internal/test/README.md b/internal/test/README.md index 696d38b4..08366528 100644 --- a/internal/test/README.md +++ b/internal/test/README.md @@ -5,8 +5,6 @@ This library helps with bringing up services for testing, such as S3, oAuth, etc ## Starting an S3 service -Allow several minutes for the first run: the helper builds MinIO from a pinned official source commit using [minio/Dockerfile](minio/Dockerfile). Docker caches the build for subsequent runs. The build requires access to Docker Hub and the Go module proxy. - To start the S3 service and then use it with the AWS client as follows: ```go diff --git a/internal/test/minio/Dockerfile b/internal/test/minio/Dockerfile deleted file mode 100644 index 89901c2b..00000000 --- a/internal/test/minio/Dockerfile +++ /dev/null @@ -1,10 +0,0 @@ -FROM golang:1.25.3-alpine3.22 AS build - -# Official prebuilt community images are no longer publicly available. -# Pin RELEASE.2025-10-15T17-29-55Z instead of depending on a moving tag. -RUN CGO_ENABLED=0 go install github.com/minio/minio@9e49d5e7a648f00e26f2246f4dc28e6b07f8c84a - -FROM alpine:3.23 -COPY --from=build /go/bin/minio /usr/local/bin/minio -EXPOSE 9000 -ENTRYPOINT ["/usr/local/bin/minio"] diff --git a/internal/test/s3.go b/internal/test/s3.go index a6626b83..fe1bcaeb 100644 --- a/internal/test/s3.go +++ b/internal/test/s3.go @@ -1,7 +1,6 @@ package test import ( - "embed" "fmt" "testing" "time" @@ -12,9 +11,6 @@ import ( "github.com/aws/aws-sdk-go/service/s3" ) -//go:embed minio -var minioBuildRoot embed.FS - // s3Lock is used to start a maximum of 5 Minio instances for testing. var s3Lock = make(chan struct{}, 5) @@ -38,10 +34,9 @@ func S3(t *testing.T) S3Helper { } t.Log("Starting Minio in a container...") m := &minio{ - cnt: containerFromBuild( + cnt: containerFromPull( t, - "containerssh-test-minio", - dockerBuildRootFiles(minioBuildRoot, "minio"), + "docker.io/minio/minio", []string{"server", "/data", "--console-address", ":9001"}, env, map[string]string{