crossplane-contrib / crossplane-contrib/function-shell
Shell command failures report an empty reason: exiterr.Stderr is always nil
- Dominant language
- Go
- Stars
- 15
- Forks
- 12
- Avg merge
- 15d 15h
- Merged PRs (30d)
- 1
Description
### What happened?
When a `shellCommand` exits non-zero, the composition fails with an error message whose reason is
empty, and the actual stderr never reaches the XR. The only place the real cause is visible is the
function pod's log.
Example: a command that hits a missing binary produces
```
shellCmd "git clone https://..." for "XShell" failed with : exit status 127
```
Note the empty string after `failed with`. `git: not found` appears nowhere in the user's XR.
### Cause
Two separate things combine.
**1. `exiterr.Stderr` is always nil here.** `fn.go:147` assigns `cmd.Stderr` and `fn.go:149` calls
`cmd.Run()`:
```go
cmd.Stderr = &stderr
cmderr := cmd.Run()
```
`exec.ExitError.Stderr` is only populated by `Output()`/`CombinedOutput()`, and only when
`cmd.Stderr` was nil. See https://pkg.go.dev/os/exec#ExitError ("Stderr holds a subset of the
standard error output from the Cmd.Output method if the Cmd.Stderr field was not set"). So at
`fn.go:172`:
```go
msg := fmt.Sprintf("shellCmd %q for %q failed with %s", shellCmd, oxr.Resource.GetKind(), exiterr.Stderr)
```
`exiterr.Stderr` is nil, and `%s` of a nil `[]byte` prints the empty string. Confirmed with a minimal
program: `Run()` with `cmd.Stderr` set yields `ExitError.Stderr == nil`, while `Output()` yields
`"boom\n"`.
The captured stderr is already sitting in `serr` (`fn.go:151`). It just isn't used in the message.
**2. `response.Fatal` discards the desired XR, so `stderrField` never lands.** `fn.go:161` does set
the field:
```go
err = dxr.Resource.SetValue(stderrField, serr)
```
but `response.Fatal` (`function-sdk-go/response/result.go`) appends a fatal result, and Crossplane
returns a `PipelineFatalError` from `composition_functions.go` before the desired resources are
loaded and applied. So `status.atFunction.shell.stderr` is never written for a failing command,
which is exactly the case where a user most wants it.
### How can we reproduce it?
Any `shellCommand` that exits non-zero, e.g. `shellCommand: "exit 1"` or a reference to a binary
that isn't in the image. Observe the composition error on the XR versus the function pod's log.
### Suggested fix
The message fix is one line. Use the stderr that was actually captured:
```go
msg := fmt.Sprintf("shellCmd %q for %q failed with %s", shellCmd, oxr.Resource.GetKind(), serr)
```
The second part is a design question worth a maintainer opinion: should a non-zero exit be `Fatal`
at all? If the intent is for users to read `stderrField` off the XR, a failing command needs
`response.Warning` (or a fatal result emitted only after the desired XR is preserved), otherwise the
field is silently unreachable on the failure path.
### Why it matters now
This is independent of #141, but it's what makes a missing-tool regression in the runtime image very
hard to diagnose: the user sees a fatal composition error with no reason, and has to reach the
function pod's logs to find `git: not found`.
Happy to open a PR for the one-line message fix if that's useful.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.