Merge pull request #5631 from jandubois/ssh-master-fd-limit

hostagent: lower ssh's soft fd limit to 1024 on macOS

pkg/hostagent/hostagent.go

@@ -640,7 +640,7 @@ func (a *HostAgent) startHostAgentRoutines(ctx context.Context) error {
 set -eux -o pipefail
 ln -sf "${SSH_AUTH_SOCK}" /run/host-services/ssh-auth.sock`
 			faDesc := "linking ssh auth socket to static location /run/host-services/ssh-auth.sock"
-			stdout, stderr, err := ssh.ExecuteScript(a.instSSHAddress, a.sshLocalPort, a.sshConfig, faScript, faDesc)
+			stdout, stderr, err := a.executeUnixScript(a.sshConfig, faScript, faDesc)
 			logrus.Debugf("stdout=%#q, stderr=%#q, err=%v", stdout, stderr, err)
 			if err != nil {
 				errs = append(errs, fmt.Errorf("stdout=%#q, stderr=%#q: %w", stdout, stderr, err))
@@ -1036,7 +1036,7 @@ func executeSSH(ctx context.Context, sshConfig *ssh.SSHConfig, sshAddress string
 		"--",
 	)
 	args = append(args, command...)
-	cmd := exec.CommandContext(ctx, sshConfig.Binary(), args...)
+	cmd := sshCommand(ctx, sshConfig.Binary(), args...)
 	if out, err := cmd.Output(); err != nil {
 		return fmt.Errorf("failed to run %v: %#q: %w", cmd.Args, string(out), err)
 	}
@@ -1145,7 +1145,7 @@ var forwardSSH = func(ctx context.Context, sshConfig *ssh.SSHConfig, sshAddress
 			panic(fmt.Errorf("invalid verb %#q", verb))
 		}
 	}
-	cmd := exec.CommandContext(ctx, sshConfig.Binary(), args...)
+	cmd := sshCommand(ctx, sshConfig.Binary(), args...)
 	logrus.Debugf("Running %#q", cmd)
 	if out, err := cmd.Output(); err != nil {
 		if verb == verbForward && strings.HasPrefix(local, "/") {
@@ -1203,7 +1203,7 @@ func (a *HostAgent) watchCloudInitProgress(ctx context.Context) {
 		fi"`,
 	)
 
-	cmd = exec.CommandContext(ctx, a.sshConfig.Binary(), args...)
+	cmd = sshCommand(ctx, a.sshConfig.Binary(), args...)
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
 		logrus.WithError(err).Warn("Failed to create stdout pipe for cloud-init monitoring")
@@ -1279,7 +1279,7 @@ func (a *HostAgent) watchCloudInitProgress(ctx context.Context) {
 			"sudo", "tail", "-n", "20", "/var/log/cloud-init-output.log",
 		)
 
-		finalCmd := exec.CommandContext(ctx, a.sshConfig.Binary(), finalArgs...)
+		finalCmd := sshCommand(ctx, a.sshConfig.Binary(), finalArgs...)
 		if finalOutput, err := finalCmd.Output(); err == nil {
 			for line := range strings.SplitSeq(string(finalOutput), "\n") {
 				if strings.TrimSpace(line) != "" {
@@ -1327,7 +1327,7 @@ func copyToHost(ctx context.Context, sshConfig *ssh.SSHConfig, sshAddress string
 	if err := os.MkdirAll(filepath.Dir(local), 0o700); err != nil {
 		return fmt.Errorf("can't create directory for local file %#q: %w", local, err)
 	}
-	cmd := exec.CommandContext(ctx, sshConfig.Binary(), args...)
+	cmd := sshCommand(ctx, sshConfig.Binary(), args...)
 	out, err := cmd.Output()
 	if err != nil {
 		var exitErr *exec.ExitError
@@ -1341,7 +1341,7 @@ func copyToHost(ctx context.Context, sshConfig *ssh.SSHConfig, sshAddress string
 				"sudo", "cat",
 				remote,
 			)
-			cmd = exec.CommandContext(ctx, sshConfig.Binary(), sudoArgs...)
+			cmd = sshCommand(ctx, sshConfig.Binary(), sudoArgs...)
 			out, err = cmd.Output()
 		}
 		if errors.As(err, &exitErr) && strings.Contains(string(exitErr.Stderr), "No such file or directory") {

pkg/hostagent/hostagent_unix.go

@@ -14,7 +14,9 @@ import (
 // Default nofile limit is too low on some system.
 // For example in the macOS standard terminal is 256.
 // It means that there are only ~240 connections available from the host to the vm.
-// That function increases the nofile limit for child processes, especially the ssh process
+// That function increases the nofile limit for child processes, especially the ssh process.
+// On macOS, sshCommand then lowers the soft limit of each ssh it starts to 1024
+// when the limit is higher.
 //
 // More about limits in go: https://go.dev/issue/46279
 func adjustNofileRlimit() {

pkg/hostagent/requirements.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os/exec"
 	"runtime"
 	"strconv"
 	"strings"
@@ -103,7 +102,7 @@ func (a *HostAgent) waitForRequirements(ctx context.Context, label string, requi
 	return errors.Join(errs...)
 }
 
-// prefixExportParam will modify a script to be executed by ssh.ExecuteScript so that it exports
+// prefixExportParam will modify a script to be executed by waitForRequirement so that it exports
 // all the variables from /mnt/lima-cidata/param.env before invoking the actual interpreter.
 //
 //   - The script is executed in user mode, so needs to read the file using `sudo`.
@@ -123,7 +122,7 @@ func (a *HostAgent) waitForRequirements(ctx context.Context, label string, requi
 //	EOF
 //	/usr/bin/env ruby
 //
-// ssh.ExecuteScript will strip the `#!` prefix from the first line and invoke the
+// executeUnixScript will strip the `#!` prefix from the first line and invoke the
 // rest of the line as the command. The full script is then passed via STDIN. We use
 // "$(printf '…')" to be able to use \n as newline escapes, to fit everything on a
 // single line:
@@ -194,7 +193,7 @@ func (a *HostAgent) waitForRequirement(r requirement) error {
 			AdditionalArgs: sshutil.DisableControlMasterOptsFromSSHArgs(sshConfig.AdditionalArgs),
 		}
 	}
-	stdout, stderr, err := ssh.ExecuteScript(a.instSSHAddress, a.sshLocalPort, sshConfig, script, r.description)
+	stdout, stderr, err := a.executeUnixScript(sshConfig, script, r.description)
 	logrus.Debugf("stdout=%#q, stderr=%#q, err=%v", stdout, stderr, err)
 	if err != nil {
 		return fmt.Errorf("stdout=%#q, stderr=%#q: %w", stdout, stderr, err)
@@ -202,9 +201,6 @@ func (a *HostAgent) waitForRequirement(r requirement) error {
 	return nil
 }
 
-// This function is copied from sshocker's ExecuteScript, because
-// the function doesn't support Windows commands.
-// TODO: Support Windows on sshocker, and replace this function.
 func (a *HostAgent) waitForWinRequirement(r requirement) error {
 	logrus.Debugf("executing script for windows %q", r.description)
 	script := r.script
@@ -227,25 +223,42 @@ func (a *HostAgent) waitForWinRequirement(r requirement) error {
 			AdditionalArgs: sshutil.DisableControlMasterOptsFromSSHArgs(sshConfig.AdditionalArgs),
 		}
 	}
+	_, _, err := a.executeScript(sshConfig, script, script, r.description)
+	return err
+}
+
+// executeUnixScript runs script on the guest with the interpreter named on its #! line.
+func (a *HostAgent) executeUnixScript(sshConfig *ssh.SSHConfig, script, scriptName string) (stdout, stderr string, err error) {
+	interpreter, err := ssh.ParseScriptInterpreter(script)
+	if err != nil {
+		return "", "", err
+	}
+	return a.executeScript(sshConfig, interpreter, script, scriptName)
+}
+
+// executeScript runs remoteCommand on the guest with script as its stdin,
+// and returns stdout and stderr.
+//
+// scriptName is used only for readability of error strings.
+func (a *HostAgent) executeScript(sshConfig *ssh.SSHConfig, remoteCommand, script, scriptName string) (stdout, stderr string, err error) {
 	if sshConfig == nil {
-		return errors.New("got nil SSHConfig")
+		return "", "", errors.New("got nil SSHConfig")
 	}
-	sshBinary := sshConfig.Binary()
 	sshArgs := sshConfig.Args()
 	if a.sshLocalPort != 0 {
 		sshArgs = append(sshArgs, "-p", strconv.Itoa(a.sshLocalPort))
 	}
-	sshArgs = append(sshArgs, a.instSSHAddress, "--", script)
-	sshCmd := exec.CommandContext(context.Background(), sshBinary, sshArgs...)
+	sshArgs = append(sshArgs, a.instSSHAddress, "--", remoteCommand)
+	sshCmd := sshCommand(context.Background(), sshConfig.Binary(), sshArgs...)
 	sshCmd.Stdin = strings.NewReader(script)
-	var stderr bytes.Buffer
-	sshCmd.Stderr = &stderr
-	logrus.Debugf("executing ssh for script :%s %v", sshCmd.Path, sshCmd.Args)
+	var stderrBuf bytes.Buffer
+	sshCmd.Stderr = &stderrBuf
+	logrus.Debugf("executing ssh for script %q: %s %v", scriptName, sshCmd.Path, sshCmd.Args)
 	out, err := sshCmd.Output()
 	if err != nil {
-		return fmt.Errorf("failed to execute script: stdout=%q, stderr=%q: %w", string(out), stderr.String(), err)
+		return string(out), stderrBuf.String(), fmt.Errorf("failed to execute script %q: stdout=%q, stderr=%q: %w", scriptName, string(out), stderrBuf.String(), err)
 	}
-	return nil
+	return string(out), stderrBuf.String(), nil
 }
 
 type requirement struct {

pkg/hostagent/sshcommand_darwin.go

@@ -0,0 +1,24 @@
+// SPDX-FileCopyrightText: Copyright The Lima Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package hostagent
+
+import (
+	"context"
+	"os/exec"
+)
+
+// sshCommand returns a command that runs ssh with its soft fd limit set to at most 1024.
+// macOS ssh polls with select(), which rejects fds >= FD_SETSIZE (1024), so an ssh
+// that is allowed more fds quits once it holds fd 1024.
+func sshCommand(ctx context.Context, sshBinary string, args ...string) *exec.Cmd {
+	// Resolve ssh with os/exec. When PATH has a relative entry such as ".", the
+	// shell would run an ssh from the working directory, but os/exec refuses.
+	sshPath, err := exec.LookPath(sshBinary)
+	cmd := exec.CommandContext(ctx, "/bin/sh", append([]string{"-c", "ulimit -S -n 1024 2>/dev/null; exec \"$0\" \"$@\"", sshPath}, args...)...)
+	// When the lookup fails, Start returns err without starting sh or ssh.
+	if err != nil {
+		cmd.Err = err
+	}
+	return cmd
+}

pkg/hostagent/sshcommand_darwin_test.go

@@ -0,0 +1,60 @@
+// SPDX-FileCopyrightText: Copyright The Lima Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package hostagent
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+
+	"github.com/lima-vm/sshocker/pkg/ssh"
+	"gotest.tools/v3/assert"
+
+	"github.com/lima-vm/lima/v2/pkg/limatype"
+)
+
+func TestSSHCommandCapsNofile(t *testing.T) {
+	adjustNofileRlimit()
+	var limit syscall.Rlimit
+	assert.NilError(t, syscall.Getrlimit(syscall.RLIMIT_NOFILE, &limit))
+	out, err := sshCommand(t.Context(), "/bin/sh", "-c", "ulimit -S -n").Output()
+	assert.NilError(t, err)
+	assert.Equal(t, strings.TrimSpace(string(out)), strconv.FormatUint(min(limit.Max, 1024), 10))
+}
+
+func TestSSHCommandIgnoresSSHInWorkingDirectory(t *testing.T) {
+	dir := t.TempDir()
+	assert.NilError(t, os.WriteFile(filepath.Join(dir, "ssh"), []byte("#!/bin/sh\n"), 0o755))
+	t.Chdir(dir)
+	t.Setenv("PATH", ".")
+	assert.ErrorIs(t, sshCommand(t.Context(), "ssh").Run(), exec.ErrDot)
+}
+
+// fakeSSHCheckingNofile raises the fd limit as the hostagent does, then puts an ssh
+// on PATH that fails, printing its soft fd limit, unless that limit is at most 1024.
+func fakeSSHCheckingNofile(t *testing.T) {
+	adjustNofileRlimit()
+	dir := t.TempDir()
+	script := `#!/bin/sh
+n=$(ulimit -S -n)
+[ "$n" != unlimited ] && [ "$n" -le 1024 ] || { echo "soft fd limit $n"; exit 1; }
+`
+	assert.NilError(t, os.WriteFile(filepath.Join(dir, "ssh"), []byte(script), 0o755))
+	t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH"))
+}
+
+func TestWaitForRequirementCapsNofile(t *testing.T) {
+	fakeSSHCheckingNofile(t)
+	a := &HostAgent{instConfig: &limatype.LimaYAML{OS: new(limatype.LINUX)}, sshConfig: &ssh.SSHConfig{}}
+	assert.NilError(t, a.waitForRequirement(requirement{description: "fd limit", script: "#!/bin/sh\n"}))
+}
+
+func TestForwardSSHCapsNofile(t *testing.T) {
+	fakeSSHCheckingNofile(t)
+	assert.NilError(t, forwardSSH(t.Context(), &ssh.SSHConfig{}, "127.0.0.1", 22, "127.0.0.1:1", "127.0.0.1:2", verbForward, false))
+}

pkg/hostagent/sshcommand_others.go

@@ -0,0 +1,15 @@
+//go:build !darwin
+
+// SPDX-FileCopyrightText: Copyright The Lima Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package hostagent
+
+import (
+	"context"
+	"os/exec"
+)
+
+func sshCommand(ctx context.Context, sshBinary string, args ...string) *exec.Cmd {
+	return exec.CommandContext(ctx, sshBinary, args...)
+}