kubernetes / kubernetes/kubectl

kubectl diff fails if diff command requires positional arguments at the end

Open
#1,436 11 comments 3 reactions 1 assignee Claimed by @satyampsoni View on GitHub
kind/bug lifecycle/rotten needs-triage
Dominant language
Go
Stars
3.3k
Forks
1k
PR merge metrics
No merged PRs in 30d

Description

**What happened**:

Many CLI programs expect positional arguments to be provided last.
For example with `git diff --no-index` as `KUBECTL_EXTERNAL_DIFF`, `kubectl diff` fails with:

```
git: '/tmp/LIVE-179413626' is not a git command. See 'git --help'.
```

Because the built command is `git /tmp/LIVE-179413626 /tmp/MERGED-179413626 diff --no-index`, instead of `git diff --no-index /tmp/LIVE-179413626 /tmp/MERGED-179413626`.

**What you expected to happen**:

`kubectl diff` should append the `from` and `to` arguments as positional arguments at the end of the command provided by `KUBECTL_EXTERNAL_DIFF`.

The default command does it properly: https://github.com/kubernetes/kubectl/blob/8ea394338436f98ba1744d8f267677054edfd2ed/pkg/cmd/diff/diff.go#L202

A patch that would do it (changes the loop order to avoid reversing the flags, their order could matter too):

```diff
diff --git a/pkg/cmd/diff/diff.go b/pkg/cmd/diff/diff.go
index 72d16b71..808724af 100644
--- a/pkg/cmd/diff/diff.go
+++ b/pkg/cmd/diff/diff.go
@@ -191,9 +191,9 @@ func (d *DiffProgram) getCommand(args ...string) (string, exec.Cmd) {
if len(diffCommand) > 1 {
// Regex accepts: Alphanumeric (case-insensitive), dash and equal
isValidChar := regexp.MustCompile(`^[a-zA-Z0-9-=]+$`).MatchString
- for i := 1; i < len(diffCommand); i++ {
+ for i := len(diffCommand) - 1; i < 1; i-- {
if isValidChar(diffCommand[i]) {
- args = append(args, diffCommand[i])
+ args = append([]string{diffCommand[i]}, args...)
}
}
}
```

Additionally, I am not too sure why the arguments are silently filtered, it seems have been reported in multiple issues and I may be missing some context:

1. What are the concerns? `os/exec` (which is used down the line if I am correct) does not invoke a shell and an external program is invoked anyway, it could still do anything.
2. The user might not know their command is being altered, may be it should error instead? Or at least log a warning?
3. Why not validate the whole content of the environment variable before splitting?

Thus, I am tempted to suggest:

```diff
diff --git a/pkg/cmd/diff/diff.go b/pkg/cmd/diff/diff.go
index 72d16b71..ae7c23d1 100644
--- a/pkg/cmd/diff/diff.go
+++ b/pkg/cmd/diff/diff.go
@@ -21,7 +21,6 @@ import (
"io"
"os"
"path/filepath"
- "regexp"
"strings"

"github.com/jonboulle/clockwork"
@@ -189,13 +188,7 @@ func (d *DiffProgram) getCommand(args ...string) (string, exec.Cmd) {
diff = diffCommand[0]

if len(diffCommand) > 1 {
- // Regex accepts: Alphanumeric (case-insensitive), dash and equal
- isValidChar := regexp.MustCompile(`^[a-zA-Z0-9-=]+$`).MatchString
- for i := 1; i < len(diffCommand); i++ {
- if isValidChar(diffCommand[i]) {
- args = append(args, diffCommand[i])
- }
- }
+ args = append(diffCommand[1:], args...)
}
} else {
diff = "diff"
```

**How to reproduce it (as minimally and precisely as possible)**:

```shell
KUBECTL_EXTERNAL_DIFF='git --no-pager diff --no-index' kubectl diff -f
```

**Anything else we need to know?**:

**Environment**:
- Kubernetes client and server versions (use `kubectl version`):
- Cloud provider or hardware configuration:
- OS (e.g: `cat /etc/os-release`):

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.