Python script-only packages: follow-on QA fixes (#50143)
**Related issues:** Resolves #50068, Resolves #50106, Resolves #50107, Resolves #50108, Resolves #50110, Resolves #50114 Follow-on fixes from QA of #41470 (Python script-only packages): - Software-installer validation errors are action-neutral, so the Add and Edit flows each show the correct single verb, and the unsupported-file error names a content/format mismatch instead of blaming the extension (#50068, #50107). - `.py` packages accept `setup_experience_platform` (`darwin`/`linux`), matching `.sh` (#50106). - A failed-to-run install script (exit code `-1`) now renders a diagnostic instead of empty output, and orbit surfaces the underlying execve error (#50108). - The install-rejection message for `.sh`/`.py` packages says "macOS and Linux hosts" instead of "linux" (#50110). - Orbit writes each script's temp file with an extension matching its shebang (`.py`/`.sh`/`.ps1`), so tracebacks reference the right file type (#50114). # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## fleetd/orbit/Fleet Desktop - [x] If the change applies to only one platform, confirmed that `runtime.GOOS` is used as needed to isolate changes. - [x] Verified compatibility with the latest released version of Fleet (orbit-only change; the server↔agent `SoftwareInstallDetails` contract is unchanged). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved installer validation and rejection messaging for unsupported/invalid package contents (including correcting “add” vs “edit” wording and avoiding duplicated phrasing). * Added clearer diagnostics when install scripts fail to start (including empty output cases). * Corrected handling of script-only packages so Python scripts use the proper script type/extension, reducing misleading tracebacks. * Updated platform availability messaging so `.sh`/`.py` packages display macOS+Linux support. * **New Features** * Python script-only packages can now specify macOS and Linux setup experience platforms. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -500,13 +500,8 @@ func (r *Runner) attemptInstall(ctx context.Context, installer *fleet.SoftwareIn
|
||||
installerPath = extractDestination
|
||||
}
|
||||
|
||||
scriptExtension := ".sh"
|
||||
if runtime.GOOS == "windows" {
|
||||
scriptExtension = ".ps1"
|
||||
}
|
||||
|
||||
logger.Info().Msg("about to run install script")
|
||||
installOutput, installExitCode, err := r.runInstallerScript(ctx, installer.InstallScript, installerPath, "install-script"+scriptExtension)
|
||||
installOutput, installExitCode, err := r.runInstallerScript(ctx, installer.InstallScript, installerPath, "install-script"+scriptFileExtension(installer.InstallScript, runtime.GOOS))
|
||||
payload.InstallScriptOutput = &installOutput
|
||||
payload.InstallScriptExitCode = &installExitCode
|
||||
if err != nil {
|
||||
@@ -517,7 +512,7 @@ func (r *Runner) attemptInstall(ctx context.Context, installer *fleet.SoftwareIn
|
||||
|
||||
if installer.PostInstallScript != "" {
|
||||
logger.Info().Str("installerPath", installerPath).Msg("about to run post-install script")
|
||||
postOutput, postExitCode, postErr := r.runInstallerScript(ctx, installer.PostInstallScript, installerPath, "post-install-script"+scriptExtension)
|
||||
postOutput, postExitCode, postErr := r.runInstallerScript(ctx, installer.PostInstallScript, installerPath, "post-install-script"+scriptFileExtension(installer.PostInstallScript, runtime.GOOS))
|
||||
payload.PostInstallScriptOutput = &postOutput
|
||||
payload.PostInstallScriptExitCode = &postExitCode
|
||||
|
||||
@@ -539,7 +534,7 @@ func (r *Runner) attemptInstall(ctx context.Context, installer *fleet.SoftwareIn
|
||||
uninstallScript = file.GetRemoveScript(ext)
|
||||
}
|
||||
uninstallOutput, uninstallExitCode, uninstallErr := r.runInstallerScript(ctx, uninstallScript, installerPath,
|
||||
"rollback-script"+scriptExtension)
|
||||
"rollback-script"+scriptFileExtension(uninstallScript, runtime.GOOS))
|
||||
logger.Info().Msgf(
|
||||
"rollback status: exit code: %d, error: %s, output: %s",
|
||||
uninstallExitCode, uninstallErr, uninstallOutput,
|
||||
@@ -641,6 +636,20 @@ func isNetworkOrTransientError(err error) bool {
|
||||
return false
|
||||
}
|
||||
|
||||
// scriptFileExtension picks the temp script filename extension so interpreter
|
||||
// error output (e.g. Python tracebacks) references a correctly-typed file.
|
||||
// Windows scripts are always PowerShell; on unix we honor the script's shebang.
|
||||
// goos is passed in (rather than read from runtime) so it stays a pure function.
|
||||
func scriptFileExtension(contents, goos string) string {
|
||||
if goos == "windows" {
|
||||
return ".ps1"
|
||||
}
|
||||
if kind, _, err := fleet.ShebangInfo(contents); err == nil && kind == fleet.ShebangPython {
|
||||
return ".py"
|
||||
}
|
||||
return ".sh"
|
||||
}
|
||||
|
||||
func (r *Runner) runInstallerScript(ctx context.Context, scriptContents string, installerPath string, fileName string) (string, int, error) {
|
||||
// run script in installer directory
|
||||
installerDir := filepath.Dir(installerPath)
|
||||
@@ -664,7 +673,14 @@ func (r *Runner) runInstallerScript(ctx context.Context, scriptContents string,
|
||||
|
||||
output, exitCode, err := execFn(ctx, scriptPath, env)
|
||||
if err != nil {
|
||||
return string(output), exitCode, err
|
||||
out := string(output)
|
||||
// An execve failure (e.g. a missing shebang interpreter) surfaces as exit
|
||||
// code -1 with no output; the only diagnostic lives in err, so fold it into
|
||||
// the output so the server reports something actionable instead of blank.
|
||||
if exitCode == -1 && out == "" {
|
||||
out = err.Error()
|
||||
}
|
||||
return out, exitCode, err
|
||||
}
|
||||
|
||||
return string(output), exitCode, nil
|
||||
|
||||
@@ -1412,3 +1412,103 @@ func TestInstallSoftwareNotFoundRetryWindow(t *testing.T) {
|
||||
require.False(t, stillTracked, "non-404 error path must clear tracker")
|
||||
})
|
||||
}
|
||||
|
||||
// attemptInstallExtTestSetup wires a Runner whose exec fn records the base name
|
||||
// of every script it runs, so tests can assert the temp file extension picked
|
||||
// for each script's contents.
|
||||
func attemptInstallExtTestSetup(t *testing.T, execFn func(context.Context, string, []string) ([]byte, int, error)) (*Runner, *[]string) {
|
||||
t.Helper()
|
||||
var executed []string
|
||||
oc := &TestOrbitClient{
|
||||
downloadInstallerFn: func(installerID uint, downloadDir string) (string, error) {
|
||||
return filepath.Join(downloadDir, fmt.Sprint(installerID)+".pkg"), nil
|
||||
},
|
||||
}
|
||||
r := &Runner{
|
||||
OrbitClient: oc,
|
||||
scriptsEnabled: func() bool { return true },
|
||||
installerExecutionTimeout: time.Minute,
|
||||
tempDirFn: func(string, string) (string, error) { return t.TempDir(), nil },
|
||||
removeAllFn: func(string) error { return nil },
|
||||
execCmdFn: func(ctx context.Context, scriptPath string, env []string) ([]byte, int, error) {
|
||||
executed = append(executed, filepath.Base(scriptPath))
|
||||
return execFn(ctx, scriptPath, env)
|
||||
},
|
||||
}
|
||||
return r, &executed
|
||||
}
|
||||
|
||||
func TestScriptFileExtension(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
require.Equal(t, ".sh", scriptFileExtension("", "darwin"), "no shebang defaults to shell")
|
||||
require.Equal(t, ".sh", scriptFileExtension("#!/bin/bash\necho hi", "darwin"), "shell shebang")
|
||||
require.Equal(t, ".py", scriptFileExtension("#!/usr/bin/env python3\nprint('hi')", "darwin"), "python shebang")
|
||||
require.Equal(t, ".py", scriptFileExtension("#!/usr/bin/env python3\nprint('hi')", "linux"), "python shebang on linux")
|
||||
|
||||
require.Equal(t, ".ps1", scriptFileExtension("#!/usr/bin/env python3\nprint('hi')", "windows"), "windows is always powershell")
|
||||
require.Equal(t, ".ps1", scriptFileExtension("", "windows"))
|
||||
}
|
||||
|
||||
func TestAttemptInstallScriptExtension(t *testing.T) {
|
||||
success := func(context.Context, string, []string) ([]byte, int, error) { return []byte("ok"), 0, nil }
|
||||
|
||||
t.Run("python install script", func(t *testing.T) {
|
||||
r, executed := attemptInstallExtTestSetup(t, success)
|
||||
_, err := r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{
|
||||
InstallerID: 1,
|
||||
InstallScript: "#!/usr/bin/env python3\nprint('install')",
|
||||
}, &fleet.HostSoftwareInstallResultPayload{}, log.With().Logger())
|
||||
require.NoError(t, err)
|
||||
require.Contains(t, *executed, "install-script.py")
|
||||
})
|
||||
|
||||
t.Run("no-shebang install script", func(t *testing.T) {
|
||||
r, executed := attemptInstallExtTestSetup(t, success)
|
||||
_, err := r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{
|
||||
InstallerID: 1,
|
||||
InstallScript: "echo install",
|
||||
}, &fleet.HostSoftwareInstallResultPayload{}, log.With().Logger())
|
||||
require.NoError(t, err)
|
||||
require.Contains(t, *executed, "install-script.sh")
|
||||
})
|
||||
|
||||
t.Run("python install with shell post-install and uninstall", func(t *testing.T) {
|
||||
// A .py package can carry a Python install script but shell post-install
|
||||
// and uninstall scripts; each temp file must reflect its own shebang.
|
||||
exitPost := func(_ context.Context, scriptPath string, _ []string) ([]byte, int, error) {
|
||||
if strings.Contains(scriptPath, "post-install-script") {
|
||||
return []byte("boom"), 1, &exec.ExitError{}
|
||||
}
|
||||
return []byte("ok"), 0, nil
|
||||
}
|
||||
r, executed := attemptInstallExtTestSetup(t, exitPost)
|
||||
_, _ = r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{
|
||||
InstallerID: 1,
|
||||
InstallScript: "#!/usr/bin/env python3\nprint('install')",
|
||||
PostInstallScript: "#!/bin/sh\necho post",
|
||||
UninstallScript: "#!/bin/sh\necho uninstall",
|
||||
}, &fleet.HostSoftwareInstallResultPayload{}, log.With().Logger())
|
||||
require.Contains(t, *executed, "install-script.py")
|
||||
require.Contains(t, *executed, "post-install-script.sh")
|
||||
require.Contains(t, *executed, "rollback-script.sh")
|
||||
})
|
||||
}
|
||||
|
||||
// An execve failure (exit code -1, empty output) must surface the underlying
|
||||
// error to the server rather than reporting a blank result.
|
||||
func TestRunInstallerScriptSurfacesExecveError(t *testing.T) {
|
||||
const execveErr = "fork/exec /usr/local/bin/python3: no such file or directory"
|
||||
r, _ := attemptInstallExtTestSetup(t, func(context.Context, string, []string) ([]byte, int, error) {
|
||||
return nil, -1, errors.New(execveErr)
|
||||
})
|
||||
|
||||
payload := &fleet.HostSoftwareInstallResultPayload{}
|
||||
_, err := r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{
|
||||
InstallerID: 1,
|
||||
InstallScript: "#!/usr/local/bin/python3\nprint('install')",
|
||||
}, payload, log.With().Logger())
|
||||
require.Error(t, err)
|
||||
require.NotNil(t, payload.InstallScriptOutput)
|
||||
require.Contains(t, *payload.InstallScriptOutput, execveErr)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user