From aa092466df336014c8007ad18a7442dbb8da67df Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Tue, 15 Sep 2026 22:21:48 +0200 Subject: [PATCH 01/12] fix(eks): show eksctl progress and the failure cause during cluster lifecycle eksctl logs CloudFormation progress and the cause of a failure on stdout, and prints only a generic line on stderr. KSail discarded stdout for create and kept only the first stderr line, so a failed EKS create reported no cause and ran silently for its whole duration. Long-running mutating commands (create cluster, create nodegroup, delete, scale, upgrade) now stream both streams line by line to a progress writer, redacted, and the factory wires it to stderr. Commands whose stdout is parsed never stream. A failed command's error now carries a bounded, redacted tail of stdout and stderr. Progress is best-effort: a failing writer never fails the command. Fixes #7078 Part of #6369 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/client.go | 138 ++++++++++-- pkg/client/eksctl/commands.go | 20 +- .../eksctl/exec_runner_progress_test.go | 99 +++++++++ pkg/client/eksctl/progress.go | 115 ++++++++++ pkg/client/eksctl/progress_test.go | 198 ++++++++++++++++++ pkg/svc/provisioner/cluster/factory_eks.go | 4 + 6 files changed, 543 insertions(+), 31 deletions(-) create mode 100644 pkg/client/eksctl/exec_runner_progress_test.go create mode 100644 pkg/client/eksctl/progress.go create mode 100644 pkg/client/eksctl/progress_test.go diff --git a/pkg/client/eksctl/client.go b/pkg/client/eksctl/client.go index 5f75ba3b28..7e2bb68f83 100644 --- a/pkg/client/eksctl/client.go +++ b/pkg/client/eksctl/client.go @@ -39,6 +39,20 @@ type EnvironmentRunner interface { ) (stdout, stderr []byte, err error) } +// ProgressRunner is the optional extension implemented by runners that can write +// a command's output to progress while it runs, in addition to returning it +// buffered. A nil environment preserves os/exec's default inheritance. +type ProgressRunner interface { + RunWithProgress( + ctx context.Context, + name string, + args []string, + stdin io.Reader, + environment []string, + progress io.Writer, + ) (stdout, stderr []byte, err error) +} + // ExecRunner is the default Runner that shells out via os/exec. type ExecRunner struct{} @@ -61,6 +75,31 @@ func (ExecRunner) RunWithEnvironment( args []string, stdin io.Reader, environment []string, +) ([]byte, []byte, error) { + return runCommand(ctx, name, args, stdin, environment, nil) +} + +// RunWithProgress executes the command with environment, writing each complete +// output line to progress as it arrives while still returning both buffers. +func (ExecRunner) RunWithProgress( + ctx context.Context, + name string, + args []string, + stdin io.Reader, + environment []string, + progress io.Writer, +) ([]byte, []byte, error) { + return runCommand(ctx, name, args, stdin, environment, progress) +} + +// runCommand executes one eksctl process, optionally mirroring its output to progress. +func runCommand( + ctx context.Context, + name string, + args []string, + stdin io.Reader, + environment []string, + progress io.Writer, ) ([]byte, []byte, error) { // #nosec G204 -- This uses os/exec directly with a program name and argv // slice; it does not invoke a shell, so user-influenced values in args @@ -74,6 +113,18 @@ func (ExecRunner) RunWithEnvironment( cmd.Stdout = &stdout cmd.Stderr = &stderr + if progress != nil { + // One line buffer per stream, so stdout and stderr never interleave mid-line. + stdoutLines := newLineWriter(progress, nil) + stderrLines := newLineWriter(progress, nil) + + defer stdoutLines.Flush() + defer stderrLines.Flush() + + cmd.Stdout = io.MultiWriter(&stdout, stdoutLines) + cmd.Stderr = io.MultiWriter(&stderr, stderrLines) + } + if stdin != nil { cmd.Stdin = stdin } @@ -91,10 +142,21 @@ type Client struct { binary string runner Runner environment []string + progress io.Writer requireCredentialValues bool } +// WithProgressWriter streams the output of long-running eksctl commands (create, +// delete, scale, upgrade) to w while they run, with credential values redacted. +// Read-only listings are never streamed, because their stdout is parsed. A nil +// writer disables streaming. +func WithProgressWriter(w io.Writer) Option { + return func(c *Client) { + c.progress = w + } +} + // Option configures a Client at construction time. type Option func(*Client) @@ -179,14 +241,7 @@ func (c *Client) CheckAvailable() error { // escape hatch used by all higher-level methods on this client and can be // used directly when a helper has not been written yet. func (c *Client) Exec(ctx context.Context, args ...string) ([]byte, []byte, error) { - stdout, stderr, err := c.run(ctx, args, nil) - - stderr = c.redactCredentialValues(stderr) - if err != nil { - return stdout, stderr, wrapExecErr(args, stderr, err) - } - - return stdout, stderr, nil + return c.exec(ctx, nil, nil, args) } // ExecWithStdin runs eksctl with the given arguments and feeds stdin from the @@ -196,11 +251,30 @@ func (c *Client) ExecWithStdin( stdin io.Reader, args ...string, ) ([]byte, []byte, error) { - stdout, stderr, err := c.run(ctx, args, stdin) + return c.exec(ctx, stdin, nil, args) +} + +// execWithProgress runs a long-running mutating command, streaming its output to the +// configured progress writer. Commands whose stdout is parsed must use Exec instead. +func (c *Client) execWithProgress(ctx context.Context, args ...string) error { + _, _, err := c.exec(ctx, nil, c.progress, args) + + return err +} + +// exec runs eksctl, redacts credential values from stderr, and wraps a failure with the +// trailing output of both streams. +func (c *Client) exec( + ctx context.Context, + stdin io.Reader, + progress io.Writer, + args []string, +) ([]byte, []byte, error) { + stdout, stderr, err := c.run(ctx, args, stdin, progress) stderr = c.redactCredentialValues(stderr) if err != nil { - return stdout, stderr, wrapExecErr(args, stderr, err) + return stdout, stderr, wrapExecErr(args, c.redactCredentialValues(stdout), stderr, err) } return stdout, stderr, nil @@ -211,12 +285,32 @@ func (c *Client) run( ctx context.Context, args []string, stdin io.Reader, + progress io.Writer, ) ([]byte, []byte, error) { err := c.validateCredentialValues() if err != nil { return nil, nil, err } + if progressRunner, ok := c.runner.(ProgressRunner); ok && progress != nil { + lines := newLineWriter(progress, c.redactCredentialValues) + defer lines.Flush() + + stdout, stderr, err := progressRunner.RunWithProgress( + ctx, + c.binary, + args, + stdin, + cloneStrings(c.environment), + lines, + ) + if err != nil { + return stdout, stderr, fmt.Errorf("run eksctl with progress: %w", err) + } + + return stdout, stderr, nil + } + if c.environment == nil { stdout, stderr, err := c.runner.Run(ctx, c.binary, args, stdin) if err != nil { @@ -329,18 +423,30 @@ func environmentValues(environment []string) map[string]string { return values } -// wrapExecErr annotates an exec failure with the invoked arguments and the -// first line of stderr (if any) to produce actionable error messages without -// leaking the full eksctl output into Go error strings. -func wrapExecErr(args []string, stderr []byte, err error) error { +// wrapExecErr annotates an exec failure with the invoked arguments, the first +// line of stderr (if any), and a bounded tail of stdout and stderr. eksctl logs +// the cause of a failure on stdout and prints only a generic line on stderr, so +// the tail is what makes the error actionable. Both streams must already be +// redacted; the tail is capped so the full eksctl output never enters an error. +func wrapExecErr(args []string, stdout, stderr []byte, err error) error { const ( firstLineParts = 2 ) firstStderrLine := strings.SplitN(strings.TrimSpace(string(stderr)), "\n", firstLineParts)[0] if firstStderrLine == "" { - return fmt.Errorf("eksctl %s: %w", strings.Join(args, " "), err) + return withOutputTail( + fmt.Errorf("eksctl %s: %w", strings.Join(args, " "), err), + stdout, + stderr, + firstStderrLine, + ) } - return fmt.Errorf("eksctl %s: %w: %s", strings.Join(args, " "), err, firstStderrLine) + return withOutputTail( + fmt.Errorf("eksctl %s: %w: %s", strings.Join(args, " "), err, firstStderrLine), + stdout, + stderr, + firstStderrLine, + ) } diff --git a/pkg/client/eksctl/commands.go b/pkg/client/eksctl/commands.go index 077f9e279a..edd4feda1c 100644 --- a/pkg/client/eksctl/commands.go +++ b/pkg/client/eksctl/commands.go @@ -82,9 +82,7 @@ func (c *Client) createCluster( args = append(args, "--kubeconfig", path) } - _, _, err := c.Exec(ctx, args...) - - return err + return c.execWithProgress(ctx, args...) } // CreateNodegroup creates the node groups in a config file. The caller supplies @@ -94,9 +92,7 @@ func (c *Client) CreateNodegroup(ctx context.Context, configPath string) error { return ErrEmptyConfigPath } - _, _, err := c.Exec(ctx, "create", "nodegroup", "--config-file", configPath) - - return err + return c.execWithProgress(ctx, "create", "nodegroup", "--config-file", configPath) } // DeleteCluster invokes `eksctl delete cluster --name [--region ]`. @@ -125,9 +121,7 @@ func (c *Client) DeleteCluster( args = append(args, "--wait") } - _, _, err := c.Exec(ctx, args...) - - return err + return c.execWithProgress(ctx, args...) } // GetCluster returns the summary for a named cluster. Returns @@ -267,9 +261,7 @@ func (c *Client) ScaleNodegroup( args = append(args, "--region", region) } - _, _, err := c.Exec(ctx, args...) - - return err + return c.execWithProgress(ctx, args...) } // UpgradeCluster invokes `eksctl upgrade cluster -f `. @@ -289,9 +281,7 @@ func (c *Client) UpgradeCluster( args = append(args, "--approve") } - _, _, err := c.Exec(ctx, args...) - - return err + return c.execWithProgress(ctx, args...) } // parseClusterSummaries unmarshals the JSON output of `eksctl get cluster`. diff --git a/pkg/client/eksctl/exec_runner_progress_test.go b/pkg/client/eksctl/exec_runner_progress_test.go new file mode 100644 index 0000000000..af3f902cb0 --- /dev/null +++ b/pkg/client/eksctl/exec_runner_progress_test.go @@ -0,0 +1,99 @@ +package eksctl_test + +import ( + "bytes" + "errors" + "fmt" + "runtime" + "sync" + "testing" + + "github.com/devantler-tech/ksail/v7/pkg/client/eksctl" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var errProgressClosed = errors.New("progress writer closed") + +// lockedBuffer is a goroutine-safe buffer: os/exec copies stdout and stderr on separate goroutines. +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + + n, err := b.buf.Write(p) + if err != nil { + return n, fmt.Errorf("write locked buffer: %w", err) + } + + return n, nil +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + + return b.buf.String() +} + +// failingWriter rejects every write, standing in for a closed terminal. +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { + return 0, errProgressClosed +} + +// TestExecRunner_RunWithProgress_StreamsBothStreamsFromARealProcess exercises the real +// os/exec path: both streams reach progress, including a final line without a newline, +// and the buffered output is still returned intact alongside the wrapped exit error. +func TestExecRunner_RunWithProgress_StreamsBothStreamsFromARealProcess(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("uses a POSIX shell") + } + + var progress lockedBuffer + + stdout, stderr, err := eksctl.ExecRunner{}.RunWithProgress( + t.Context(), + "sh", + []string{"-c", "printf 'out-1\\nout-2'; printf 'err-1\\n' >&2; exit 3"}, + nil, + nil, + &progress, + ) + + require.ErrorIs(t, err, eksctl.ErrExecFailed) + assert.Equal(t, "out-1\nout-2", string(stdout)) + assert.Equal(t, "err-1\n", string(stderr)) + assert.Contains(t, progress.String(), "out-1\n") + assert.Contains(t, progress.String(), "out-2") + assert.Contains(t, progress.String(), "err-1\n") +} + +// TestExecRunner_RunWithProgress_FailingProgressDoesNotFailTheCommand verifies progress is +// best-effort: a writer that rejects output never turns a successful command into a failure. +func TestExecRunner_RunWithProgress_FailingProgressDoesNotFailTheCommand(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("uses a POSIX shell") + } + + stdout, _, err := eksctl.ExecRunner{}.RunWithProgress( + t.Context(), + "sh", + []string{"-c", "printf 'created\\n'"}, + nil, + nil, + failingWriter{}, + ) + + require.NoError(t, err) + assert.Equal(t, "created\n", string(stdout)) +} diff --git a/pkg/client/eksctl/progress.go b/pkg/client/eksctl/progress.go new file mode 100644 index 0000000000..4286d2775e --- /dev/null +++ b/pkg/client/eksctl/progress.go @@ -0,0 +1,115 @@ +package eksctl + +import ( + "bytes" + "fmt" + "io" + "strings" + "sync" +) + +// errorOutputTailLines bounds how many trailing lines of eksctl output an error carries. +// eksctl logs the cause of a failure on stdout just before it exits, so the end of the +// output is where the diagnosis is; the rest stays out of Go error strings. +const errorOutputTailLines = 20 + +// maxPendingLineBytes caps how much of an unterminated line a lineWriter holds before it +// forwards the partial line anyway, so a stream without newlines cannot grow memory unbounded. +const maxPendingLineBytes = 64 * 1024 + +// lineWriter forwards only complete lines to its target, optionally transforming each one. +// Whole-line writes keep two streams that share one target from interleaving mid-line, and +// let redaction see a credential value in one piece. Progress is best-effort: a failing +// target never fails the command whose output is being streamed. +type lineWriter struct { + mu sync.Mutex + target io.Writer + transform func([]byte) []byte + pending []byte +} + +// newLineWriter returns a lineWriter forwarding to target; transform may be nil. +func newLineWriter(target io.Writer, transform func([]byte) []byte) *lineWriter { + return &lineWriter{ + mu: sync.Mutex{}, + target: target, + transform: transform, + pending: nil, + } +} + +// Write buffers p and forwards every complete line it now holds. +func (w *lineWriter) Write(p []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + + w.pending = append(w.pending, p...) + + for { + index := bytes.IndexByte(w.pending, '\n') + if index < 0 { + break + } + + w.forward(w.pending[:index+1]) + w.pending = w.pending[index+1:] + } + + if len(w.pending) >= maxPendingLineBytes { + w.forward(w.pending) + w.pending = nil + } + + return len(p), nil +} + +// Flush forwards a final unterminated line, if any. +func (w *lineWriter) Flush() { + w.mu.Lock() + defer w.mu.Unlock() + + if len(w.pending) > 0 { + w.forward(w.pending) + w.pending = nil + } +} + +// forward writes one chunk to the target, ignoring target errors because progress is best-effort. +func (w *lineWriter) forward(chunk []byte) { + out := append([]byte(nil), chunk...) + if w.transform != nil { + out = w.transform(out) + } + + _, _ = w.target.Write(out) +} + +// outputTail returns the last errorOutputTailLines non-empty lines of stdout followed by stderr. +func outputTail(stdout, stderr []byte) string { + lines := make([]string, 0, errorOutputTailLines) + + for _, stream := range [][]byte{stdout, stderr} { + for line := range strings.SplitSeq(string(stream), "\n") { + if trimmed := strings.TrimRight(line, "\r "); strings.TrimSpace(trimmed) != "" { + lines = append(lines, trimmed) + } + } + } + + if len(lines) > errorOutputTailLines { + lines = lines[len(lines)-errorOutputTailLines:] + } + + return strings.Join(lines, "\n") +} + +// withOutputTail appends the trailing eksctl output to err when it adds anything beyond the +// first stderr line the error already names. +func withOutputTail(err error, stdout, stderr []byte, firstStderrLine string) error { + tail := outputTail(stdout, stderr) + if tail == "" || tail == firstStderrLine { + return err + } + + return fmt.Errorf("%w\neksctl output (last %d lines):\n%s", err, errorOutputTailLines, tail) +} diff --git a/pkg/client/eksctl/progress_test.go b/pkg/client/eksctl/progress_test.go new file mode 100644 index 0000000000..e05d3e94bf --- /dev/null +++ b/pkg/client/eksctl/progress_test.go @@ -0,0 +1,198 @@ +package eksctl_test + +import ( + "bytes" + "context" + "fmt" + "io" + "strings" + "testing" + + "github.com/devantler-tech/ksail/v7/pkg/client/eksctl" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fixtureSecret is a credential value that must never reach progress output or errors. +const fixtureSecret = "fixture-secret-access-key-value" + +// streamingRunner is a fake runner that, like the real ExecRunner, writes the command's +// output to the progress writer while it runs and also returns it buffered. +type streamingRunner struct { + stdout []byte + stderr []byte + err error + + progressCalls int + plainCalls int +} + +// Run records a non-streaming invocation. +func (s *streamingRunner) Run( + context.Context, + string, + []string, + io.Reader, +) ([]byte, []byte, error) { + s.plainCalls++ + + return s.stdout, s.stderr, s.err +} + +// RunWithEnvironment records a non-streaming invocation with an explicit environment. +func (s *streamingRunner) RunWithEnvironment( + context.Context, + string, + []string, + io.Reader, + []string, +) ([]byte, []byte, error) { + s.plainCalls++ + + return s.stdout, s.stderr, s.err +} + +// RunWithProgress writes stdout and stderr to progress, the way a streaming runner would. +func (s *streamingRunner) RunWithProgress( + _ context.Context, + _ string, + _ []string, + _ io.Reader, + _ []string, + progress io.Writer, +) ([]byte, []byte, error) { + s.progressCalls++ + + _, _ = progress.Write(s.stdout) + _, _ = progress.Write(s.stderr) + + return s.stdout, s.stderr, s.err +} + +func newStreamingClient(runner *streamingRunner, progress io.Writer) *eksctl.Client { + return eksctl.NewClient( + eksctl.WithBinary("eksctl-under-test"), + eksctl.WithRunner(runner), + eksctl.WithEnvironment([]string{ + "AWS_ACCESS_KEY_ID=fixture-access-key-id", + "AWS_SECRET_ACCESS_KEY=" + fixtureSecret, + }), + eksctl.WithProgressWriter(progress), + ) +} + +// TestCreateCluster_StreamsRedactedProgress verifies eksctl's output reaches the progress +// writer while create runs, with credential values redacted. +func TestCreateCluster_StreamsRedactedProgress(t *testing.T) { + t.Parallel() + + runner := &streamingRunner{ + stdout: []byte("[ℹ] waiting for CloudFormation stack \"eksctl-demo-cluster\"\n" + + "[ℹ] using " + fixtureSecret + "\n"), + } + + var progress bytes.Buffer + + err := newStreamingClient(runner, &progress).CreateCluster(t.Context(), "eks.yaml", "") + require.NoError(t, err) + + assert.Equal(t, 1, runner.progressCalls) + assert.Contains(t, progress.String(), "waiting for CloudFormation stack") + assert.Contains(t, progress.String(), "[REDACTED]") + assert.NotContains(t, progress.String(), fixtureSecret) +} + +// TestMutatingCommands_Stream verifies every long-running mutating command streams progress. +func TestMutatingCommands_Stream(t *testing.T) { + t.Parallel() + + commands := map[string]func(*eksctl.Client) error{ + "create cluster": func(c *eksctl.Client) error { + return c.CreateClusterWithKubeconfig(t.Context(), "eks.yaml", "", "/tmp/kubeconfig") + }, + "create nodegroup": func(c *eksctl.Client) error { + return c.CreateNodegroup(t.Context(), "eks.yaml") + }, + "delete cluster": func(c *eksctl.Client) error { + return c.DeleteCluster(t.Context(), "demo", "eu-west-1", "", true) + }, + "scale nodegroup": func(c *eksctl.Client) error { + return c.ScaleNodegroup(t.Context(), "demo", "ng-1", "eu-west-1", 2, -1, -1) + }, + "upgrade cluster": func(c *eksctl.Client) error { + return c.UpgradeCluster(t.Context(), "eks.yaml", true) + }, + } + + for name, run := range commands { + t.Run(name, func(t *testing.T) { + t.Parallel() + + runner := &streamingRunner{stdout: []byte("progress line\n")} + + var progress bytes.Buffer + + require.NoError(t, run(newStreamingClient(runner, &progress))) + assert.Equal(t, 1, runner.progressCalls) + assert.Equal(t, 0, runner.plainCalls) + assert.Contains(t, progress.String(), "progress line") + }) + } +} + +// TestGetCommands_DoNotStream verifies machine-readable listings never reach the progress writer. +func TestGetCommands_DoNotStream(t *testing.T) { + t.Parallel() + + runner := &streamingRunner{stdout: []byte(`[{"Name":"demo","Region":"eu-west-1"}]`)} + + var progress bytes.Buffer + + clusters, err := newStreamingClient(runner, &progress).ListClusters(t.Context(), "eu-west-1") + require.NoError(t, err) + require.Len(t, clusters, 1) + + assert.Equal(t, 0, runner.progressCalls) + assert.Empty(t, progress.String()) +} + +// TestCreateCluster_ErrorCarriesStdoutCause reproduces ksail#7078: eksctl logs the real +// cause to stdout and prints only a generic line on stderr, so the error must carry both. +func TestCreateCluster_ErrorCarriesStdoutCause(t *testing.T) { + t.Parallel() + + runner := &fakeRunner{ + stdout: []byte("[ℹ] waiting for CloudFormation stack \"eksctl-demo-nodegroup-ng-1\"\n" + + "[✖] waiter state transitioned to Failure: exceeded max wait time\n"), + stderr: []byte("Error: failed to create cluster \"demo\"\n"), + err: errExitStatus1, + } + + err := newTestClient(runner).CreateCluster(t.Context(), "eks.yaml", "") + require.Error(t, err) + + assert.Contains(t, err.Error(), "failed to create cluster \"demo\"") + assert.Contains(t, err.Error(), "exceeded max wait time") +} + +// TestExec_ErrorOutputTailIsBoundedAndRedacted verifies only the end of a long output is kept +// and credential values never reach the error. +func TestExec_ErrorOutputTailIsBoundedAndRedacted(t *testing.T) { + t.Parallel() + + var stdout strings.Builder + for line := range 200 { + fmt.Fprintf(&stdout, "line-%03d\n", line) + } + + stdout.WriteString("final cause using " + fixtureSecret + "\n") + + runner := &streamingRunner{stdout: []byte(stdout.String()), err: errExitStatus1} + + _, _, err := newStreamingClient(runner, io.Discard).Exec(t.Context(), "get", "cluster") + require.Error(t, err) + + assert.Contains(t, err.Error(), "final cause using [REDACTED]") + assert.NotContains(t, err.Error(), fixtureSecret) + assert.NotContains(t, err.Error(), "line-000") +} diff --git a/pkg/svc/provisioner/cluster/factory_eks.go b/pkg/svc/provisioner/cluster/factory_eks.go index fa38fec3ce..863ab475df 100644 --- a/pkg/svc/provisioner/cluster/factory_eks.go +++ b/pkg/svc/provisioner/cluster/factory_eks.go @@ -156,5 +156,9 @@ func (f DefaultFactory) resolveEKSCredentialOptions( eksprovisioner.RequireCredentialValues, ) + // eksctl reports CloudFormation progress and the cause of a failure on its own + // output, so stream it to stderr rather than leaving a long create silent. + eksctlOptions = append(eksctlOptions, eksctlclient.WithProgressWriter(os.Stderr)) + return eksctlclient.NewClient(eksctlOptions...), providerOptions, provisionerOptions, nil } From 5ce5991a48a1be3f20491c4849fe7b98222200e9 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Tue, 15 Sep 2026 22:26:17 +0200 Subject: [PATCH 02/12] refactor(eks): wrap eksctl exec errors once and document the progress cap Simplify wrapExecErr to build the base error and append the output tail in one place, keep WithProgressWriter beside the other client options, and note that a line longer than the pending cap is redacted chunk by chunk. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/client.go | 38 ++++++++++++++--------------------- pkg/client/eksctl/progress.go | 2 ++ 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/pkg/client/eksctl/client.go b/pkg/client/eksctl/client.go index 7e2bb68f83..b7c29c5bb8 100644 --- a/pkg/client/eksctl/client.go +++ b/pkg/client/eksctl/client.go @@ -147,16 +147,6 @@ type Client struct { requireCredentialValues bool } -// WithProgressWriter streams the output of long-running eksctl commands (create, -// delete, scale, upgrade) to w while they run, with credential values redacted. -// Read-only listings are never streamed, because their stdout is parsed. A nil -// writer disables streaming. -func WithProgressWriter(w io.Writer) Option { - return func(c *Client) { - c.progress = w - } -} - // Option configures a Client at construction time. type Option func(*Client) @@ -189,6 +179,16 @@ func WithEnvironment(environment []string) Option { } } +// WithProgressWriter streams the output of long-running eksctl commands (create, +// delete, scale, upgrade) to w while they run, with credential values redacted. +// Read-only listings are never streamed, because their stdout is parsed. A nil +// writer disables streaming. +func WithProgressWriter(w io.Writer) Option { + return func(c *Client) { + c.progress = w + } +} + // RequireCredentialValues makes execution fail closed unless the explicit // child environment contains either a profile or a complete static credential // pair. Use it when custom source names were configured so an unset alias @@ -434,19 +434,11 @@ func wrapExecErr(args []string, stdout, stderr []byte, err error) error { ) firstStderrLine := strings.SplitN(strings.TrimSpace(string(stderr)), "\n", firstLineParts)[0] - if firstStderrLine == "" { - return withOutputTail( - fmt.Errorf("eksctl %s: %w", strings.Join(args, " "), err), - stdout, - stderr, - firstStderrLine, - ) + + wrapped := fmt.Errorf("eksctl %s: %w", strings.Join(args, " "), err) + if firstStderrLine != "" { + wrapped = fmt.Errorf("eksctl %s: %w: %s", strings.Join(args, " "), err, firstStderrLine) } - return withOutputTail( - fmt.Errorf("eksctl %s: %w: %s", strings.Join(args, " "), err, firstStderrLine), - stdout, - stderr, - firstStderrLine, - ) + return withOutputTail(wrapped, stdout, stderr, firstStderrLine) } diff --git a/pkg/client/eksctl/progress.go b/pkg/client/eksctl/progress.go index 4286d2775e..2c6a9ba9b3 100644 --- a/pkg/client/eksctl/progress.go +++ b/pkg/client/eksctl/progress.go @@ -15,6 +15,8 @@ const errorOutputTailLines = 20 // maxPendingLineBytes caps how much of an unterminated line a lineWriter holds before it // forwards the partial line anyway, so a stream without newlines cannot grow memory unbounded. +// A line longer than this is redacted chunk by chunk, so a credential value that straddles +// the boundary would not be matched; eksctl's log lines are far shorter than the cap. const maxPendingLineBytes = 64 * 1024 // lineWriter forwards only complete lines to its target, optionally transforming each one. From 7a577c82ecbda19a9d8e136734a00478eb4a04b3 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Tue, 15 Sep 2026 23:21:58 +0200 Subject: [PATCH 03/12] style(eks): name the line writer's buffer parameter for the linter golangci-lint's varnamelen flagged the one-letter parameter in lineWriter.Write as too short for its scope. Rename it to data; behaviour is unchanged. Part of #7078 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/progress.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/client/eksctl/progress.go b/pkg/client/eksctl/progress.go index 2c6a9ba9b3..cfa2250806 100644 --- a/pkg/client/eksctl/progress.go +++ b/pkg/client/eksctl/progress.go @@ -40,12 +40,12 @@ func newLineWriter(target io.Writer, transform func([]byte) []byte) *lineWriter } } -// Write buffers p and forwards every complete line it now holds. -func (w *lineWriter) Write(p []byte) (int, error) { +// Write buffers data and forwards every complete line it now holds. +func (w *lineWriter) Write(data []byte) (int, error) { w.mu.Lock() defer w.mu.Unlock() - w.pending = append(w.pending, p...) + w.pending = append(w.pending, data...) for { index := bytes.IndexByte(w.pending, '\n') @@ -62,7 +62,7 @@ func (w *lineWriter) Write(p []byte) (int, error) { w.pending = nil } - return len(p), nil + return len(data), nil } // Flush forwards a final unterminated line, if any. From 0dc195cc4c3d35c1a1a2a5a74ef04ec819218b0f Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 00:39:52 +0200 Subject: [PATCH 04/12] fix(eks): serialize progress writes, never split a redacted line, keep the stdout cause Three review findings, all valid: - runCommand gave stdout and stderr separate line buffers writing straight to the caller's writer, which os/exec drives from two goroutines. A caller passing a writer that is not safe for concurrent use could race. Both buffers now write through one shared lock. - A line longer than the pending cap was forwarded in pieces, and redaction only sees one piece at a time, so a credential split across the boundary reached progress output. Such a line is now dropped from the stream and replaced by a placeholder; the returned buffers and the redacted error tail are unaffected. - The error tail took the last 20 lines of stdout and stderr combined, so 20 stderr lines evicted the eksctl cause. Each stream is bounded on its own: 15 stdout lines, then 5 stderr lines. A fourth finding asked for an explicit eksctl --timeout. Declined with reasoning on the thread: that flag bounds each wait, not the command. Part of #7078 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/client.go | 9 +- .../eksctl/exec_runner_progress_test.go | 26 +++ pkg/client/eksctl/progress.go | 154 +++++++++++++----- pkg/client/eksctl/progress_test.go | 96 +++++++++++ 4 files changed, 242 insertions(+), 43 deletions(-) diff --git a/pkg/client/eksctl/client.go b/pkg/client/eksctl/client.go index b7c29c5bb8..ad6af1c2d3 100644 --- a/pkg/client/eksctl/client.go +++ b/pkg/client/eksctl/client.go @@ -114,9 +114,12 @@ func runCommand( cmd.Stderr = &stderr if progress != nil { - // One line buffer per stream, so stdout and stderr never interleave mid-line. - stdoutLines := newLineWriter(progress, nil) - stderrLines := newLineWriter(progress, nil) + // One line buffer per stream, so stdout and stderr never interleave mid-line. os/exec copies + // the two streams on separate goroutines, and a caller's writer need not be safe for + // concurrent use, so both buffers write through one shared lock. + shared := newLockedWriter(progress) + stdoutLines := newLineWriter(shared, nil) + stderrLines := newLineWriter(shared, nil) defer stdoutLines.Flush() defer stderrLines.Flush() diff --git a/pkg/client/eksctl/exec_runner_progress_test.go b/pkg/client/eksctl/exec_runner_progress_test.go index af3f902cb0..0b3d273719 100644 --- a/pkg/client/eksctl/exec_runner_progress_test.go +++ b/pkg/client/eksctl/exec_runner_progress_test.go @@ -97,3 +97,29 @@ func TestExecRunner_RunWithProgress_FailingProgressDoesNotFailTheCommand(t *test require.NoError(t, err) assert.Equal(t, "created\n", string(stdout)) } + +// TestExecRunner_RunWithProgress_SerializesStreamsForAPlainWriter verifies a caller may pass a +// writer that is not safe for concurrent use: stdout and stderr lines must not race or be lost. +// Run with -race to catch an unserialized write. +func TestExecRunner_RunWithProgress_SerializesStreamsForAPlainWriter(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("uses a POSIX shell") + } + + var progress bytes.Buffer + + _, _, err := eksctl.ExecRunner{}.RunWithProgress( + t.Context(), + "sh", + []string{"-c", "i=0; while [ $i -lt 200 ]; do echo out-$i; echo err-$i >&2; i=$((i+1)); done"}, + nil, + nil, + &progress, + ) + + require.NoError(t, err) + assert.Equal(t, 200, bytes.Count(progress.Bytes(), []byte("out-"))) + assert.Equal(t, 200, bytes.Count(progress.Bytes(), []byte("err-"))) +} diff --git a/pkg/client/eksctl/progress.go b/pkg/client/eksctl/progress.go index cfa2250806..0f525e5541 100644 --- a/pkg/client/eksctl/progress.go +++ b/pkg/client/eksctl/progress.go @@ -8,26 +8,32 @@ import ( "sync" ) -// errorOutputTailLines bounds how many trailing lines of eksctl output an error carries. -// eksctl logs the cause of a failure on stdout just before it exits, so the end of the -// output is where the diagnosis is; the rest stays out of Go error strings. -const errorOutputTailLines = 20 - -// maxPendingLineBytes caps how much of an unterminated line a lineWriter holds before it -// forwards the partial line anyway, so a stream without newlines cannot grow memory unbounded. -// A line longer than this is redacted chunk by chunk, so a credential value that straddles -// the boundary would not be matched; eksctl's log lines are far shorter than the cap. +// errorOutputStdoutLines bounds how many trailing stdout lines an error carries. eksctl logs the +// cause of a failure on stdout just before it exits, so the end of stdout is where the diagnosis is. +const errorOutputStdoutLines = 15 + +// errorOutputStderrLines bounds how many trailing stderr lines an error carries. It is capped +// separately from stdout so a long stderr can never push the stdout cause out of the error. +const errorOutputStderrLines = 5 + +// maxPendingLineBytes caps how much of an unterminated line a lineWriter holds. A longer line is +// dropped from the stream and replaced by a placeholder rather than forwarded in pieces: redaction +// sees one piece at a time, so a credential split across two pieces would otherwise leak. const maxPendingLineBytes = 64 * 1024 +// omittedLinePlaceholder replaces a line longer than maxPendingLineBytes in the forwarded stream. +const omittedLinePlaceholder = "[line longer than 64 KiB omitted]\n" + // lineWriter forwards only complete lines to its target, optionally transforming each one. -// Whole-line writes keep two streams that share one target from interleaving mid-line, and -// let redaction see a credential value in one piece. Progress is best-effort: a failing -// target never fails the command whose output is being streamed. +// Whole-line writes keep two streams that share one target from interleaving mid-line, and let +// redaction see a credential value in one piece. Progress is best-effort: a failing target never +// fails the command whose output is being streamed. type lineWriter struct { mu sync.Mutex target io.Writer transform func([]byte) []byte pending []byte + overlong bool } // newLineWriter returns a lineWriter forwarding to target; transform may be nil. @@ -37,48 +43,75 @@ func newLineWriter(target io.Writer, transform func([]byte) []byte) *lineWriter target: target, transform: transform, pending: nil, + overlong: false, } } -// Write buffers data and forwards every complete line it now holds. +// Write buffers data and forwards every line it completes. func (w *lineWriter) Write(data []byte) (int, error) { w.mu.Lock() defer w.mu.Unlock() - w.pending = append(w.pending, data...) + written := len(data) - for { - index := bytes.IndexByte(w.pending, '\n') + for len(data) > 0 { + index := bytes.IndexByte(data, '\n') if index < 0 { + w.hold(data) + break } - w.forward(w.pending[:index+1]) - w.pending = w.pending[index+1:] - } + w.hold(data[:index+1]) + w.finishLine() - if len(w.pending) >= maxPendingLineBytes { - w.forward(w.pending) - w.pending = nil + data = data[index+1:] } - return len(data), nil + return written, nil } -// Flush forwards a final unterminated line, if any. +// Flush forwards a final unterminated line, or its placeholder if it outgrew the cap. func (w *lineWriter) Flush() { w.mu.Lock() defer w.mu.Unlock() - if len(w.pending) > 0 { - w.forward(w.pending) + if w.overlong || len(w.pending) > 0 { + w.finishLine() + } +} + +// hold appends part of the current line, or drops the line once it outgrows the cap. +func (w *lineWriter) hold(part []byte) { + if w.overlong { + return + } + + if len(w.pending)+len(part) > maxPendingLineBytes { w.pending = nil + w.overlong = true + + return } + + w.pending = append(w.pending, part...) } -// forward writes one chunk to the target, ignoring target errors because progress is best-effort. -func (w *lineWriter) forward(chunk []byte) { - out := append([]byte(nil), chunk...) +// finishLine forwards the held line, or a placeholder for a line dropped as overlong. +func (w *lineWriter) finishLine() { + if w.overlong { + w.forward([]byte(omittedLinePlaceholder)) + } else { + w.forward(w.pending) + } + + w.pending = nil + w.overlong = false +} + +// forward writes one line to the target, ignoring target errors because progress is best-effort. +func (w *lineWriter) forward(line []byte) { + out := append([]byte(nil), line...) if w.transform != nil { out = w.transform(out) } @@ -86,23 +119,58 @@ func (w *lineWriter) forward(chunk []byte) { _, _ = w.target.Write(out) } -// outputTail returns the last errorOutputTailLines non-empty lines of stdout followed by stderr. +// lockedWriter serializes writes to a target that may not be safe for concurrent use. +type lockedWriter struct { + mu sync.Mutex + target io.Writer +} + +// newLockedWriter returns a writer that serializes every write to target. +func newLockedWriter(target io.Writer) *lockedWriter { + return &lockedWriter{mu: sync.Mutex{}, target: target} +} + +// Write writes data to the target while holding the lock. +func (w *lockedWriter) Write(data []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + + written, err := w.target.Write(data) + if err != nil { + return written, fmt.Errorf("write eksctl progress: %w", err) + } + + return written, nil +} + +// outputTail returns the last non-empty stdout lines followed by the last non-empty stderr lines, +// each stream bounded on its own. func outputTail(stdout, stderr []byte) string { - lines := make([]string, 0, errorOutputTailLines) + stdoutLines := tailLines(stdout, errorOutputStdoutLines) + stderrLines := tailLines(stderr, errorOutputStderrLines) - for _, stream := range [][]byte{stdout, stderr} { - for line := range strings.SplitSeq(string(stream), "\n") { - if trimmed := strings.TrimRight(line, "\r "); strings.TrimSpace(trimmed) != "" { - lines = append(lines, trimmed) - } + lines := make([]string, 0, len(stdoutLines)+len(stderrLines)) + lines = append(lines, stdoutLines...) + lines = append(lines, stderrLines...) + + return strings.Join(lines, "\n") +} + +// tailLines returns the last limit non-empty lines of stream, with trailing spaces trimmed. +func tailLines(stream []byte, limit int) []string { + lines := make([]string, 0, limit) + + for line := range strings.SplitSeq(string(stream), "\n") { + if trimmed := strings.TrimRight(line, "\r "); strings.TrimSpace(trimmed) != "" { + lines = append(lines, trimmed) } } - if len(lines) > errorOutputTailLines { - lines = lines[len(lines)-errorOutputTailLines:] + if len(lines) > limit { + lines = lines[len(lines)-limit:] } - return strings.Join(lines, "\n") + return lines } // withOutputTail appends the trailing eksctl output to err when it adds anything beyond the @@ -113,5 +181,11 @@ func withOutputTail(err error, stdout, stderr []byte, firstStderrLine string) er return err } - return fmt.Errorf("%w\neksctl output (last %d lines):\n%s", err, errorOutputTailLines, tail) + return fmt.Errorf( + "%w\neksctl output (last %d stdout and %d stderr lines):\n%s", + err, + errorOutputStdoutLines, + errorOutputStderrLines, + tail, + ) } diff --git a/pkg/client/eksctl/progress_test.go b/pkg/client/eksctl/progress_test.go index e05d3e94bf..6daf6da1a6 100644 --- a/pkg/client/eksctl/progress_test.go +++ b/pkg/client/eksctl/progress_test.go @@ -196,3 +196,99 @@ func TestExec_ErrorOutputTailIsBoundedAndRedacted(t *testing.T) { assert.NotContains(t, err.Error(), fixtureSecret) assert.NotContains(t, err.Error(), "line-000") } + +// chunkedRunner writes its output to progress in the given chunks, the way a pipe can split a long +// line across several writes. +type chunkedRunner struct { + chunks []string +} + +// Run is never used by the streaming path; it satisfies the Runner interface. +func (r *chunkedRunner) Run(context.Context, string, []string, io.Reader) ([]byte, []byte, error) { + return nil, nil, nil +} + +// RunWithEnvironment is never used by the streaming path; it satisfies EnvironmentRunner. +func (r *chunkedRunner) RunWithEnvironment( + context.Context, + string, + []string, + io.Reader, + []string, +) ([]byte, []byte, error) { + return nil, nil, nil +} + +// RunWithProgress writes each chunk as a separate write, then returns the joined output. +func (r *chunkedRunner) RunWithProgress( + _ context.Context, + _ string, + _ []string, + _ io.Reader, + _ []string, + progress io.Writer, +) ([]byte, []byte, error) { + for _, chunk := range r.chunks { + _, _ = progress.Write([]byte(chunk)) + } + + return []byte(strings.Join(r.chunks, "")), nil, nil +} + +// TestCreateCluster_OverlongLineNeverLeaksASplitCredential reproduces a credential split across the +// pending-line cap: neither half of it may reach progress output. +func TestCreateCluster_OverlongLineNeverLeaksASplitCredential(t *testing.T) { + t.Parallel() + + const pendingCap = 64 * 1024 + + head := fixtureSecret[:15] + tail := fixtureSecret[15:] + runner := &chunkedRunner{chunks: []string{ + strings.Repeat("a", pendingCap-len(head)) + head, + tail + "\n", + "[ℹ] next line\n", + }} + + var progress bytes.Buffer + + client := eksctl.NewClient( + eksctl.WithBinary("eksctl-under-test"), + eksctl.WithRunner(runner), + eksctl.WithEnvironment([]string{ + "AWS_ACCESS_KEY_ID=fixture-access-key-id", + "AWS_SECRET_ACCESS_KEY=" + fixtureSecret, + }), + eksctl.WithProgressWriter(&progress), + ) + + require.NoError(t, client.CreateCluster(t.Context(), "eks.yaml", "")) + + assert.NotContains(t, progress.String(), head) + assert.NotContains(t, progress.String(), tail) + assert.Contains(t, progress.String(), "omitted") + assert.Contains(t, progress.String(), "next line") +} + +// TestExec_ErrorTailKeepsStdoutCauseDespiteLongStderr verifies a long stderr cannot push eksctl's +// stdout cause out of the bounded error tail. +func TestExec_ErrorTailKeepsStdoutCauseDespiteLongStderr(t *testing.T) { + t.Parallel() + + var stderr strings.Builder + for line := range 30 { + fmt.Fprintf(&stderr, "stderr-%02d\n", line) + } + + runner := &fakeRunner{ + stdout: []byte("[✖] exceeded max wait time for StackCreateComplete waiter\n"), + stderr: []byte(stderr.String()), + err: errExitStatus1, + } + + err := newTestClient(runner).CreateCluster(t.Context(), "eks.yaml", "") + require.Error(t, err) + + assert.Contains(t, err.Error(), "exceeded max wait time") + assert.Contains(t, err.Error(), "stderr-29") +} From e793c77efb8ea3eeff4cae9af1ff20b36428736e Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 00:56:13 +0200 Subject: [PATCH 05/12] style(eks): name the skipped GOOS in the progress runner tests The third POSIX-shell skip pushed the "windows" literal to three occurrences, which goconst reports. Name it once as a file-local constant; behaviour is unchanged. Part of #7078 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/exec_runner_progress_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/client/eksctl/exec_runner_progress_test.go b/pkg/client/eksctl/exec_runner_progress_test.go index 0b3d273719..897bb6e71b 100644 --- a/pkg/client/eksctl/exec_runner_progress_test.go +++ b/pkg/client/eksctl/exec_runner_progress_test.go @@ -15,6 +15,9 @@ import ( var errProgressClosed = errors.New("progress writer closed") +// windowsGOOS is the runtime.GOOS value whose shell these tests do not target. +const windowsGOOS = "windows" + // lockedBuffer is a goroutine-safe buffer: os/exec copies stdout and stderr on separate goroutines. type lockedBuffer struct { mu sync.Mutex @@ -53,7 +56,7 @@ func (failingWriter) Write([]byte) (int, error) { func TestExecRunner_RunWithProgress_StreamsBothStreamsFromARealProcess(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { + if runtime.GOOS == windowsGOOS { t.Skip("uses a POSIX shell") } @@ -81,7 +84,7 @@ func TestExecRunner_RunWithProgress_StreamsBothStreamsFromARealProcess(t *testin func TestExecRunner_RunWithProgress_FailingProgressDoesNotFailTheCommand(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { + if runtime.GOOS == windowsGOOS { t.Skip("uses a POSIX shell") } @@ -104,7 +107,7 @@ func TestExecRunner_RunWithProgress_FailingProgressDoesNotFailTheCommand(t *test func TestExecRunner_RunWithProgress_SerializesStreamsForAPlainWriter(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { + if runtime.GOOS == windowsGOOS { t.Skip("uses a POSIX shell") } From 67b92a10689cda354817be2f39bfc539d35e7e1c Mon Sep 17 00:00:00 2001 From: "ksail-bot[bot]" <262010955+ksail-bot[bot]@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:06:06 +0000 Subject: [PATCH 06/12] chore: apply golangci-lint fixes --- pkg/client/eksctl/exec_runner_progress_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/client/eksctl/exec_runner_progress_test.go b/pkg/client/eksctl/exec_runner_progress_test.go index 897bb6e71b..a5dfc96fce 100644 --- a/pkg/client/eksctl/exec_runner_progress_test.go +++ b/pkg/client/eksctl/exec_runner_progress_test.go @@ -116,7 +116,10 @@ func TestExecRunner_RunWithProgress_SerializesStreamsForAPlainWriter(t *testing. _, _, err := eksctl.ExecRunner{}.RunWithProgress( t.Context(), "sh", - []string{"-c", "i=0; while [ $i -lt 200 ]; do echo out-$i; echo err-$i >&2; i=$((i+1)); done"}, + []string{ + "-c", + "i=0; while [ $i -lt 200 ]; do echo out-$i; echo err-$i >&2; i=$((i+1)); done", + }, nil, nil, &progress, From 7b95f94483a4da1b5663f6ad4d66804be26e0ee7 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 03:30:16 +0200 Subject: [PATCH 07/12] fix(eks): give eksctl create an explicit wait budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #7078 names two reasons an EKS create failed with no visible cause. This branch already fixed the first — eksctl's output was buffered and all but the first stderr line discarded. The second was still open: no --timeout was passed, so eksctl fell back to its own default wait. That default is the problem rather than its length. It is not stated anywhere in KSail and cannot be relied on to sit below the workflow's create-step budget, so the step can expire first and kill eksctl mid-wait — which is how a run records that create failed but not what it was waiting on. 45m is bounded on both sides: above a real provision, since a live create was still building its node group at 36m22s (run 34999766125), and below the 60-minute step budget #7008 gives that step. #7008 therefore lands first; until it does, the current 30-minute step budget still expires first and this wait is an upper bound that is never reached. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/client_test.go | 29 +++++++++++++++++++++++++++-- pkg/client/eksctl/commands.go | 22 ++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/pkg/client/eksctl/client_test.go b/pkg/client/eksctl/client_test.go index 54f0b8a531..e59618bceb 100644 --- a/pkg/client/eksctl/client_test.go +++ b/pkg/client/eksctl/client_test.go @@ -328,7 +328,7 @@ func TestCreateCluster_ConfigRegionIsNotDuplicatedAsAFlag(t *testing.T) { assert.Equal(t, "eksctl-under-test", runner.lastName) assert.Equal(t, - []string{"create", "cluster", "--config-file", "eks.yaml"}, + []string{"create", "cluster", "--config-file", "eks.yaml", "--timeout", "45m"}, runner.lastArgs, ) } @@ -343,7 +343,7 @@ func TestCreateCluster_NoRegionOmitsFlag(t *testing.T) { require.NoError(t, err) assert.Equal(t, - []string{"create", "cluster", "--config-file", "eks.yaml"}, + []string{"create", "cluster", "--config-file", "eks.yaml", "--timeout", "45m"}, runner.lastArgs, ) } @@ -367,6 +367,31 @@ func TestCreateCluster_WithKubeconfigPinsOutputPath(t *testing.T) { "create", "cluster", "--config-file", "eks.yaml", "--kubeconfig", "/tmp/ksail-kubeconfig", + "--timeout", "45m", + }, + runner.lastArgs, + ) +} + +func TestCreateCluster_PassesExplicitWaitTimeout(t *testing.T) { + t.Parallel() + + runner := &fakeRunner{} + client := newTestClient(runner) + + err := client.CreateCluster(t.Context(), "eks.yaml", "us-east-1") + require.NoError(t, err) + + // Without this flag eksctl falls back to its own default wait. That is what let a + // slow provision outlive the smoke workflow's create-step budget and be killed + // with no cause in the log (#7078): the step timeout fires first and discards the + // diagnosis eksctl was about to print. The value is asserted literally so that + // changing the budget has to be a deliberate edit here too. + assert.Equal(t, + []string{ + "create", "cluster", + "--config-file", "eks.yaml", + "--timeout", "45m", }, runner.lastArgs, ) diff --git a/pkg/client/eksctl/commands.go b/pkg/client/eksctl/commands.go index edd4feda1c..5e9ffe9f74 100644 --- a/pkg/client/eksctl/commands.go +++ b/pkg/client/eksctl/commands.go @@ -21,6 +21,26 @@ const subcommandCluster = "cluster" // outputFormatJSON is the eksctl --output value requesting JSON, shared by every listing call. const outputFormatJSON = "json" +// flagTimeout is the eksctl flag bounding how long a command waits for its +// CloudFormation stacks to settle. +const flagTimeout = "--timeout" + +// createClusterWaitTimeout bounds how long `eksctl create cluster` waits before it +// gives up and says why. +// +// Without it eksctl uses its own default, which is not stated in KSail and cannot be +// relied on to sit below the CI budget. When the workflow's create step expires first +// it kills eksctl mid-wait, so the run records that create failed but not what it was +// waiting on — the missing-cause failure in #7078. +// +// The value sits above a realistic provision — a live EKS create was still building +// its node group at 36m22s (run 34999766125) — and below the 60-minute create-step +// budget the smoke workflow gets in #7008. That ordering is what makes eksctl the one +// that gives up first, with a message, so #7008 lands before this is fully effective; +// until it does, the step's current 30-minute budget still expires first and this wait +// is an upper bound that is never reached. +const createClusterWaitTimeout = "45m" + // ClusterSummary represents a single cluster entry returned by // `eksctl get cluster -o json`. Field tags preserve eksctl's PascalCase // JSON keys (eksctl emits these names verbatim). @@ -82,6 +102,8 @@ func (c *Client) createCluster( args = append(args, "--kubeconfig", path) } + args = append(args, flagTimeout, createClusterWaitTimeout) + return c.execWithProgress(ctx, args...) } From 8c4aeadeacc284ed37e851278099e59886afe800 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 04:19:29 +0200 Subject: [PATCH 08/12] test(eks): assert the eksctl create wait budget in the remaining arg checks `createCluster` now appends `--timeout 45m`, but two argument assertions still expected the old six-element command and failed at head 7b95f944: pkg/svc/provisioner/cluster/factory_eks_test.go:96 pkg/svc/provisioner/cluster/eks/provisioner_test.go:137 Both now expect the flag alongside the three assertions already updated in pkg/client/eksctl/client_test.go, so every `create cluster` assertion site describes the same command. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/svc/provisioner/cluster/eks/provisioner_test.go | 3 ++- pkg/svc/provisioner/cluster/factory_eks_test.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/svc/provisioner/cluster/eks/provisioner_test.go b/pkg/svc/provisioner/cluster/eks/provisioner_test.go index dfc248eab9..97e123e190 100644 --- a/pkg/svc/provisioner/cluster/eks/provisioner_test.go +++ b/pkg/svc/provisioner/cluster/eks/provisioner_test.go @@ -134,13 +134,14 @@ func TestCreate_ShellsOutWithConfig(t *testing.T) { require.NoError(t, prov.Create(context.Background(), "")) require.Len(t, runner.calls, 1) - require.Len(t, runner.calls[0], 6) + require.Len(t, runner.calls[0], 8) assert.Equal( t, []string{ "create", "cluster", "--config-file", runner.calls[0][3], "--kubeconfig", "/tmp/kubeconfig", + "--timeout", "45m", }, runner.calls[0], ) diff --git a/pkg/svc/provisioner/cluster/factory_eks_test.go b/pkg/svc/provisioner/cluster/factory_eks_test.go index cd326749e4..ee5d218b3e 100644 --- a/pkg/svc/provisioner/cluster/factory_eks_test.go +++ b/pkg/svc/provisioner/cluster/factory_eks_test.go @@ -93,11 +93,12 @@ func TestCreateEKSProvisionerPinsKubeconfigPathWithoutOverridingConfigRegion(t * require.NoError(t, err) argFields := strings.Fields(string(args)) - require.Len(t, argFields, 6) + require.Len(t, argFields, 8) assert.Equal(t, []string{ "create", "cluster", "--config-file", argFields[3], "--kubeconfig", "/tmp/ksail-kubeconfig", + "--timeout", "45m", }, argFields) assert.NotEqual(t, sourceConfigPath, argFields[3]) From 4085c30e2ac0dd46657e4437eec40c7d381391e3 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 05:44:40 +0200 Subject: [PATCH 09/12] fix(eks): bound the failure tail in bytes, not only in lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wrapExecErr documents that the tail is capped so the full eksctl output never enters an error, but tailLines bounded only the line COUNT — 15 stdout and 5 stderr. The 64 KiB pending-line cap guards the progress writer, not the buffered streams the error is built from, so one very long line (a CloudFormation reason, a JSON payload) reached the error whole. Measured on a single 35 KB stdout line: the returned error was 32,739 characters. Cap each retained line at 512 bytes, cutting on a rune boundary so a multi-byte character is never split, and mark a shortened line. The tail is now bounded by construction at (15 + 5) * 512. The streams are already redacted before they reach here, so truncating cannot expose part of a credential value. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/client.go | 3 ++- pkg/client/eksctl/progress.go | 34 +++++++++++++++++++++-- pkg/client/eksctl/progress_test.go | 43 ++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/pkg/client/eksctl/client.go b/pkg/client/eksctl/client.go index ad6af1c2d3..a9af93612f 100644 --- a/pkg/client/eksctl/client.go +++ b/pkg/client/eksctl/client.go @@ -430,7 +430,8 @@ func environmentValues(environment []string) map[string]string { // line of stderr (if any), and a bounded tail of stdout and stderr. eksctl logs // the cause of a failure on stdout and prints only a generic line on stderr, so // the tail is what makes the error actionable. Both streams must already be -// redacted; the tail is capped so the full eksctl output never enters an error. +// redacted; the tail is capped in both lines and bytes, so neither a long output nor a single +// very long line can carry the whole of it into an error. func wrapExecErr(args []string, stdout, stderr []byte, err error) error { const ( firstLineParts = 2 diff --git a/pkg/client/eksctl/progress.go b/pkg/client/eksctl/progress.go index 0f525e5541..ffa1b7ac9f 100644 --- a/pkg/client/eksctl/progress.go +++ b/pkg/client/eksctl/progress.go @@ -6,6 +6,7 @@ import ( "io" "strings" "sync" + "unicode/utf8" ) // errorOutputStdoutLines bounds how many trailing stdout lines an error carries. eksctl logs the @@ -16,6 +17,17 @@ const errorOutputStdoutLines = 15 // separately from stdout so a long stderr can never push the stdout cause out of the error. const errorOutputStderrLines = 5 +// maxErrorTailLineBytes caps how many bytes of any single line the failure tail keeps. tailLines +// bounds the tail by line COUNT, which is not a size bound on its own: eksctl can emit one very +// long line (a CloudFormation reason, or a JSON payload), so without this the tail could carry +// the whole of it. With it, the tail is bounded by construction at +// (errorOutputStdoutLines + errorOutputStderrLines) * maxErrorTailLineBytes. +const maxErrorTailLineBytes = 512 + +// errorTailLineTruncationMarker ends a line the failure tail shortened, so a truncated cause is +// never mistaken for the whole of it. +const errorTailLineTruncationMarker = " …[truncated]" + // maxPendingLineBytes caps how much of an unterminated line a lineWriter holds. A longer line is // dropped from the stream and replaced by a placeholder rather than forwarded in pieces: redaction // sees one piece at a time, so a credential split across two pieces would otherwise leak. @@ -156,13 +168,14 @@ func outputTail(stdout, stderr []byte) string { return strings.Join(lines, "\n") } -// tailLines returns the last limit non-empty lines of stream, with trailing spaces trimmed. +// tailLines returns the last limit non-empty lines of stream, with trailing spaces trimmed and +// each line capped at maxErrorTailLineBytes, so the tail is bounded in bytes and not only in lines. func tailLines(stream []byte, limit int) []string { lines := make([]string, 0, limit) for line := range strings.SplitSeq(string(stream), "\n") { if trimmed := strings.TrimRight(line, "\r "); strings.TrimSpace(trimmed) != "" { - lines = append(lines, trimmed) + lines = append(lines, truncateTailLine(trimmed)) } } @@ -173,6 +186,23 @@ func tailLines(stream []byte, limit int) []string { return lines } +// truncateTailLine caps one tail line at maxErrorTailLineBytes. It cuts on a rune boundary so a +// multi-byte character is never split, and marks the line so a shortened cause cannot be read as +// the whole of it. The streams reaching here are already redacted, so truncating cannot expose +// part of a credential value. +func truncateTailLine(line string) string { + if len(line) <= maxErrorTailLineBytes { + return line + } + + cut := maxErrorTailLineBytes + for cut > 0 && !utf8.RuneStart(line[cut]) { + cut-- + } + + return line[:cut] + errorTailLineTruncationMarker +} + // withOutputTail appends the trailing eksctl output to err when it adds anything beyond the // first stderr line the error already names. func withOutputTail(err error, stdout, stderr []byte, firstStderrLine string) error { diff --git a/pkg/client/eksctl/progress_test.go b/pkg/client/eksctl/progress_test.go index 6daf6da1a6..24395f5b3a 100644 --- a/pkg/client/eksctl/progress_test.go +++ b/pkg/client/eksctl/progress_test.go @@ -7,6 +7,7 @@ import ( "io" "strings" "testing" + "unicode/utf8" "github.com/devantler-tech/ksail/v7/pkg/client/eksctl" "github.com/stretchr/testify/assert" @@ -292,3 +293,45 @@ func TestExec_ErrorTailKeepsStdoutCauseDespiteLongStderr(t *testing.T) { assert.Contains(t, err.Error(), "exceeded max wait time") assert.Contains(t, err.Error(), "stderr-29") } + +// TestExec_ErrorTailBoundsASingleOverlongLine pins the byte bound on the failure tail. The line +// limits alone are not a size bound: eksctl can emit one very long line, and without a per-line +// cap that whole line reaches the error even though only 15 stdout lines are kept. +func TestExec_ErrorTailBoundsASingleOverlongLine(t *testing.T) { + t.Parallel() + + const cause = "[✖] AWS::EKS::Nodegroup CREATE_FAILED: " + + runner := &fakeRunner{ + stdout: []byte(cause + strings.Repeat("detail ", 5000) + "\n"), + err: errExitStatus1, + } + + err := newTestClient(runner).CreateCluster(t.Context(), "eks.yaml", "") + require.Error(t, err) + + // The head of the line survives, so the cause is still diagnosable. + assert.Contains(t, err.Error(), cause) + // The line is marked as shortened rather than silently cut. + assert.Contains(t, err.Error(), "…[truncated]") + // And the whole error stays small: one line can no longer dominate it. + assert.Less(t, len(err.Error()), 2000, + "a single overlong stdout line must not carry its full length into the error") +} + +// TestExec_ErrorTailTruncatesOnARuneBoundary verifies a multi-byte character is never split by the +// byte cap, which would otherwise put invalid UTF-8 into an error string. +func TestExec_ErrorTailTruncatesOnARuneBoundary(t *testing.T) { + t.Parallel() + + // Every rune is 3 bytes, so a naive byte cut at 512 lands mid-rune. + runner := &fakeRunner{ + stdout: []byte(strings.Repeat("日", 1000) + "\n"), + err: errExitStatus1, + } + + err := newTestClient(runner).CreateCluster(t.Context(), "eks.yaml", "") + require.Error(t, err) + + assert.True(t, utf8.ValidString(err.Error()), "the error must remain valid UTF-8") +} From 838ad11eea3983a2e143c72e910b74147e756753 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 06:11:53 +0200 Subject: [PATCH 10/12] test(eks): use a non-CJK rune in the rune-boundary truncation test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rune-boundary test proved the tail cap never splits a multi-byte character by repeating a 3-byte CJK rune. gosmopolitan rejects Han-script string literals, so the check failed lint while testing the right thing. Any 3-byte rune exercises the same boundary — 512 = 3*170 + 2 still lands mid-rune — so the script was never what the test needed. Swap in a non-CJK 3-byte rune and say so, to stop a later edit reaching for CJK again. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/progress_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/client/eksctl/progress_test.go b/pkg/client/eksctl/progress_test.go index 24395f5b3a..05452523b4 100644 --- a/pkg/client/eksctl/progress_test.go +++ b/pkg/client/eksctl/progress_test.go @@ -324,9 +324,11 @@ func TestExec_ErrorTailBoundsASingleOverlongLine(t *testing.T) { func TestExec_ErrorTailTruncatesOnARuneBoundary(t *testing.T) { t.Parallel() - // Every rune is 3 bytes, so a naive byte cut at 512 lands mid-rune. + // Every rune is 3 bytes, so a naive byte cut at 512 lands mid-rune (512 = 3*170 + 2). + // The rune is deliberately not CJK: gosmopolitan rejects Han-script literals, and + // what this test needs is the byte width, not the script. runner := &fakeRunner{ - stdout: []byte(strings.Repeat("日", 1000) + "\n"), + stdout: []byte(strings.Repeat("€", 1000) + "\n"), err: errExitStatus1, } From 2f63dab6dfe54d1e7bc0d4368c440b946dfd63c4 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 07:28:48 +0200 Subject: [PATCH 11/12] fix(eks): reserve the truncation marker inside the tail byte cap truncateTailLine cut at maxErrorTailLineBytes and then appended the 15-byte marker, so a retained truncated line was 527 bytes against the 512-byte cap the constant documents. The whole-error bound could not see it: 527 is far below the 2000-byte assertion that test makes. Reserve the marker's bytes before the rune-boundary walk, and pin the per-line invariant with a regression test that fails at 527 without the fix. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/eksctl/progress.go | 4 +++- pkg/client/eksctl/progress_test.go | 36 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/pkg/client/eksctl/progress.go b/pkg/client/eksctl/progress.go index ffa1b7ac9f..21a3f22351 100644 --- a/pkg/client/eksctl/progress.go +++ b/pkg/client/eksctl/progress.go @@ -195,7 +195,9 @@ func truncateTailLine(line string) string { return line } - cut := maxErrorTailLineBytes + // The marker is part of the retained line, so its bytes come out of the cap rather than being + // added to it: reserving them here is what keeps a truncated line within maxErrorTailLineBytes. + cut := maxErrorTailLineBytes - len(errorTailLineTruncationMarker) for cut > 0 && !utf8.RuneStart(line[cut]) { cut-- } diff --git a/pkg/client/eksctl/progress_test.go b/pkg/client/eksctl/progress_test.go index 05452523b4..e850e6c3d1 100644 --- a/pkg/client/eksctl/progress_test.go +++ b/pkg/client/eksctl/progress_test.go @@ -337,3 +337,39 @@ func TestExec_ErrorTailTruncatesOnARuneBoundary(t *testing.T) { assert.True(t, utf8.ValidString(err.Error()), "the error must remain valid UTF-8") } + +// TestExec_ErrorTailRetainedLineStaysWithinTheByteCap pins the per-line bound the tail's byte cap +// is documented to give: (stdout lines + stderr lines) * 512. The marker the truncation appends is +// part of the retained line, so reserving no space for it puts the line over the cap it is capped +// by — 512 bytes of content plus a 15-byte marker is 527. The whole-error bound in +// TestExec_ErrorTailBoundsASingleOverlongLine cannot see this: 527 is still far below 2000. +func TestExec_ErrorTailRetainedLineStaysWithinTheByteCap(t *testing.T) { + t.Parallel() + + // maxErrorTailLineBytes, restated: the constant is unexported and this file is an external + // test package, so the bound is pinned by its value rather than by the symbol. + const maxRetainedLineBytes = 512 + + runner := &fakeRunner{ + stdout: []byte(strings.Repeat("detail ", 5000) + "\n"), + err: errExitStatus1, + } + + err := newTestClient(runner).CreateCluster(t.Context(), "eks.yaml", "") + require.Error(t, err) + + var truncated []string + + for _, line := range strings.Split(err.Error(), "\n") { + if strings.Contains(line, "…[truncated]") { + truncated = append(truncated, line) + } + } + + require.NotEmpty(t, truncated, "the overlong line must be truncated, or this proves nothing") + + for _, line := range truncated { + assert.LessOrEqual(t, len(line), maxRetainedLineBytes, + "a retained truncated line must fit the byte cap INCLUDING its truncation marker") + } +} From 45b0a7c34a75ee6ecc13d5d056c6de4f365386e8 Mon Sep 17 00:00:00 2001 From: "ksail-bot[bot]" <262010955+ksail-bot[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 05:36:58 +0000 Subject: [PATCH 12/12] chore: apply golangci-lint fixes --- pkg/client/eksctl/progress_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/client/eksctl/progress_test.go b/pkg/client/eksctl/progress_test.go index e850e6c3d1..040b30ea14 100644 --- a/pkg/client/eksctl/progress_test.go +++ b/pkg/client/eksctl/progress_test.go @@ -360,7 +360,7 @@ func TestExec_ErrorTailRetainedLineStaysWithinTheByteCap(t *testing.T) { var truncated []string - for _, line := range strings.Split(err.Error(), "\n") { + for line := range strings.SplitSeq(err.Error(), "\n") { if strings.Contains(line, "…[truncated]") { truncated = append(truncated, line) }