Skip to content

fix(runner): support chaining kubectl commands with && || ; in K8s CLI action - #506

Merged
RamanKharchee merged 3 commits into
mainfrom
fix/k8s-cli-chained-commands
Jul 4, 2026
Merged

fix(runner): support chaining kubectl commands with && || ; in K8s CLI action#506
RamanKharchee merged 3 commits into
mainfrom
fix/k8s-cli-chained-commands

Conversation

@RamanKharchee

Copy link
Copy Markdown
Contributor

Description

The K8s CLI action (kubectl_command_executor) fails when two valid kubectl commands are joined with &&. Reported in nudgebee/nudgebee-enterprise#33447 (runbook template "Clean Up Evicted and Failed Pods"):

kubectl get pods -n <ns> --field-selector status.phase=Failed -o wide && kubectl get pods -n <ns> --field-selector status.phase=Succeeded -o wide
error: name cannot be provided when a selector is specified

Root cause: KubectlExecutor.Run tokenized the command with shlex.Split and exec'd a single kubectl with no shell (exec.CommandContext(ctx, "kubectl", args...)). shlex treats && as an ordinary word, so every token from && onward (&&, kubectl, get, pods, …) became positional NAME arguments of the first kubectl get pods. With --field-selector also set, kubectl emits exactly name cannot be provided when a selector is specified. Only the first verb was ever allowlist-checked.

What changed

runner/pkg/kube/exec.go:

  • Split the command into segments on the sequential operators &&, ||, ; and run each as its own kubectl process, with shell short-circuit semantics (&& runs on success, || on failure, ; always). &&/|| are equal-precedence, left-associative, so evaluating left-to-right against the carried exit code reproduces bash for mixed chains. stdout/stderr are aggregated; exit_code is the last executed command's status.
  • Every segment's verb is validated against the read-only allowlist before any segment runs — so get pods && delete pod foo is rejected atomically, not after executing the read half. The per-command allowlist guarantee is preserved for each command, not just the first.
  • Pipes and redirects are rejected with a clear error. Without a shell we can't pipe anyway, and piping to a non-kubectl binary (grep, awk) would escape the verb allowlist — out of scope by design.
  • Single (unchained) commands are unchanged: one segment, same behavior as before.

runner/pkg/kube/exec_test.go: the exact reported command, per-segment allowlist enforcement (0 executions on rejection), all five short-circuit cases, AllowWrite chained mutations, pipe/redirect rejection, empty-segment rejection.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • go test ./pkg/kube/ — all pass (existing + new)
  • go vet ./pkg/kube/ and gofmt — clean
  • golangci-lint — blocked locally by a go1.25-vs-module-go1.26.3 toolchain mismatch; relies on CI to lint

Review Notes → Risks & Counterarguments

  • Security is the crux. The read-only allowlist previously only checked the first verb. This change validates every chained segment up front and refuses to execute any if one fails validation. If you review one thing, review validateSegment being called for all segments before runSegments, and TestKubectl_Chained_ValidatesEverySegment.
  • Pipes/redirects unsupported. Rejected rather than shelled out, to keep the "kubectl-only, verb-allowlisted" model. If users need | grep, that's a separate, larger decision (would need a shell + full-string verb parsing).
  • Glued operators (a&&b with no surrounding spaces) are not split — shlex yields them as one token. Standard usage has spaces around operators (as in the report). Noted as a known minor limitation rather than adding a raw-string pre-parser that would fight shlex quoting.
  • Cross-repo issue: the fix lives here (k8s-agent) but the issue is on nudgebee-enterprise, so GitHub won't auto-close it — please close nudgebee/nudgebee-enterprise#33447 manually on merge.

🤖 Generated with Claude Code

…I action

The K8s CLI action (kubectl_command_executor) tokenized the command with
shlex.Split and exec'd a single kubectl with no shell, so `kubectl get ... &&
kubectl get ...` collapsed into one invocation — the second command's tokens
became positional NAME args of the first, producing
`error: name cannot be provided when a selector is specified` (issue #33447).

Split the command into segments on the sequential operators && || ; and run
each as its own kubectl process with shell short-circuit semantics (&& on
success, || on failure, ; always), aggregating stdout/stderr and returning the
last-executed exit code. Every segment's verb is validated against the
read-only allowlist BEFORE any segment runs, so a chain like `get && delete`
is rejected atomically rather than executing the read half first — the
per-command allowlist guarantee is preserved for each command, not just the
first. Pipes and redirects are rejected with a clear error (no shell; piping to
a non-kubectl binary would escape the verb allowlist).

Tests cover the exact reported command, per-segment allowlist enforcement, all
five short-circuit cases, AllowWrite chained mutations, pipe/redirect rejection,
and empty-segment rejection.

Fixes #33447

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@RamanKharchee
RamanKharchee requested a review from a team as a code owner July 4, 2026 08:29

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for sequentially chaining multiple kubectl commands using &&, ||, and ; operators, while explicitly rejecting unsupported shell operators like pipes and redirects. It also ensures that all command segments are validated against the read-only allowlist before any execution begins. The review feedback suggests improving context cancellation handling by checking ctx.Err() at the start of the execution loop and immediately after running a command segment. Additionally, it is recommended to gracefully handle trailing semicolons, which are currently incorrectly rejected as empty segments.

Comment thread runner/pkg/kube/exec.go
Comment thread runner/pkg/kube/exec.go
Comment thread runner/pkg/kube/exec.go
… ';'

- Check ctx.Err() at the top of the segment loop (don't start another kubectl
  after cancel/deadline) and again right after cmd.Run() (a context-killed
  process returns an ExitError that must not be read as a normal non-zero exit,
  which would otherwise look like success on the last segment). Propagate the
  context error and stop the chain.
- Accept a trailing ';' ("kubectl get pods ;") as the valid shell no-op it is,
  and restore the clearer "empty command after stripping prefix" message for a
  bare/empty command. Leading/trailing/doubled &&/|| stay rejected.
- Extract execResult() to avoid repeating the result map literal.

Adds tests for a cancelled context (no segment runs, error propagates) and a
trailing semicolon.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

📦 Image Tags Updated

I've automatically updated the image tags in `charts/nudgebee-agent/values.yaml` to the latest versions from GHCR for the `main` branch.

The image tags are now synchronized with the latest builds and ready for release.

@RamanKharchee

Copy link
Copy Markdown
Contributor Author

Addressed all three @gemini-code-assist comments in 2fe5d3c:

  1. Context cancellation (HIGH, ×2)runSegments now checks ctx.Err() at the top of the loop (won't start another kubectl after a cancel/deadline) and immediately after cmd.Run() (a context-killed process returns an ExitError that must not be read as a normal non-zero kubectl exit — which would otherwise look like success on the last segment). Both paths return the aggregated output plus the context error and stop the chain.

  2. Trailing ; (MEDIUM)kubectl get pods ; is now accepted as the valid shell no-op it is (the trailing empty segment is dropped). A bare/empty command restores the clearer empty command after stripping prefix message. Leading, trailing, and doubled &&/|| stay rejected.

  3. Extracted execResult() to avoid repeating the result-map literal across the new early returns.

Added tests: TestKubectl_Chained_RespectsCancelledContext (no segment runs, error propagates) and TestKubectl_Chained_AcceptsTrailingSemicolon.

go test ./pkg/kube/, go vet, gofmt all clean.

(Note: the branch also picked up an automated chore: update image tags commit from release automation — unrelated to this change; my review fix is rebased on top of it.)

@gemini-code-assist

Copy link
Copy Markdown

Thanks for the detailed update, @RamanKharchee. The implementation of segment-based execution, context-aware cancellation, and the robust test suite for short-circuiting and validation logic looks excellent. The approach to handle ExitError specifically to avoid false positives on context cancellation is exactly the right way to handle that edge case. Everything looks solid and ready to go.

@RamanKharchee
RamanKharchee merged commit e173039 into main Jul 4, 2026
7 checks passed
@RamanKharchee
RamanKharchee deleted the fix/k8s-cli-chained-commands branch July 4, 2026 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants