diff --git a/pkg/client/eksctl/client.go b/pkg/client/eksctl/client.go index 5f75ba3b28..a9af93612f 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,21 @@ 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. 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() + + cmd.Stdout = io.MultiWriter(&stdout, stdoutLines) + cmd.Stderr = io.MultiWriter(&stderr, stderrLines) + } + if stdin != nil { cmd.Stdin = stdin } @@ -91,6 +145,7 @@ type Client struct { binary string runner Runner environment []string + progress io.Writer requireCredentialValues bool } @@ -127,6 +182,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 @@ -179,14 +244,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 +254,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 +288,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 +426,23 @@ 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 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 ) firstStderrLine := strings.SplitN(strings.TrimSpace(string(stderr)), "\n", firstLineParts)[0] - if firstStderrLine == "" { - return fmt.Errorf("eksctl %s: %w", strings.Join(args, " "), err) + + 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 fmt.Errorf("eksctl %s: %w: %s", strings.Join(args, " "), err, firstStderrLine) + return withOutputTail(wrapped, stdout, stderr, firstStderrLine) } 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 077f9e279a..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,9 +102,9 @@ func (c *Client) createCluster( args = append(args, "--kubeconfig", path) } - _, _, err := c.Exec(ctx, args...) + args = append(args, flagTimeout, createClusterWaitTimeout) - return err + return c.execWithProgress(ctx, args...) } // CreateNodegroup creates the node groups in a config file. The caller supplies @@ -94,9 +114,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 +143,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 +283,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 +303,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..a5dfc96fce --- /dev/null +++ b/pkg/client/eksctl/exec_runner_progress_test.go @@ -0,0 +1,131 @@ +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") + +// 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 + 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 == windowsGOOS { + 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 == windowsGOOS { + 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)) +} + +// 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 == windowsGOOS { + 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 new file mode 100644 index 0000000000..21a3f22351 --- /dev/null +++ b/pkg/client/eksctl/progress.go @@ -0,0 +1,223 @@ +package eksctl + +import ( + "bytes" + "fmt" + "io" + "strings" + "sync" + "unicode/utf8" +) + +// 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 + +// 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. +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. +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. +func newLineWriter(target io.Writer, transform func([]byte) []byte) *lineWriter { + return &lineWriter{ + mu: sync.Mutex{}, + target: target, + transform: transform, + pending: nil, + overlong: false, + } +} + +// Write buffers data and forwards every line it completes. +func (w *lineWriter) Write(data []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + + written := len(data) + + for len(data) > 0 { + index := bytes.IndexByte(data, '\n') + if index < 0 { + w.hold(data) + + break + } + + w.hold(data[:index+1]) + w.finishLine() + + data = data[index+1:] + } + + return written, nil +} + +// 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 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...) +} + +// 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) + } + + _, _ = w.target.Write(out) +} + +// 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 { + stdoutLines := tailLines(stdout, errorOutputStdoutLines) + stderrLines := tailLines(stderr, errorOutputStderrLines) + + 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 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, truncateTailLine(trimmed)) + } + } + + if len(lines) > limit { + lines = lines[len(lines)-limit:] + } + + 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 + } + + // 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-- + } + + 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 { + tail := outputTail(stdout, stderr) + if tail == "" || tail == firstStderrLine { + return err + } + + 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 new file mode 100644 index 0000000000..040b30ea14 --- /dev/null +++ b/pkg/client/eksctl/progress_test.go @@ -0,0 +1,375 @@ +package eksctl_test + +import ( + "bytes" + "context" + "fmt" + "io" + "strings" + "testing" + "unicode/utf8" + + "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") +} + +// 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") +} + +// 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 (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"), + 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") +} + +// 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.SplitSeq(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") + } +} 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.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 } 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])