kubernetes / kubernetes/kubectl
kubectl diff fails if diff command requires positional arguments at the end
- 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
Assessment
This issue has not been assessed yet.