fix: Rootless error concerning /var/run/docker.sock (#2181)

* Use same socket defaulting strategy every time

* Always default to DOCKER_HOST

* Add more debug logs

* Commenting, and massively simplified socket logic

* Rever to upstream run_context.go

* Fix EACCESS error regarding /opt/hostedtoolcache

* Revert "Fix EACCESS error regarding /opt/hostedtoolcache"

This reverts commit b2a8394d3358e1b5aab9dabe555d4a3f2bf0b2f9.

* Revert CLI debug logs

* Move socket and host handling to own function, and simplify logic

* Move to container package

* Make return be a struct

* Write tests to verify functionality

* Fix DOCKER_HOST being set to the string "DOCKER_HOST"

* Always use struct

* Use socketLocation, for DOCKER_HOST and more defaults

* Fixup arguments to GetSocketAndHost in test and root.go

* Un-struct hasDockerHost

* Fixup logic and set hasDockerHost

* Minor scoping & variable name change

* Move functionality to a new file

* Rename corresponding test

* Reviewfix

* Fix DOCKER_HOST expected

* Fix test assertions and add comments

* Swap comparison actual, expected

* Fixed no-DOCKER_HOST env test

* Fixed default socket test

* Add test to verify review comments

* Add more test for greater test coverage

* Consistent comment references

* Fix bug found while writing tests

* Passing tests

* NoMountNoHost testfix

* Rename test appropriately

* NoMount testfix

* Fixed OnlySocket

* Swap expected <-> actual in tests

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

cmd/root.go

@@ -32,7 +32,7 @@ import (
 // Execute is the entry point to running the CLI
 func Execute(ctx context.Context, version string) {
 	input := new(Input)
-	var rootCmd = &cobra.Command{
+	rootCmd := &cobra.Command{
 		Use:               "act [event name to run] [flags]\n\nIf no event name passed, will default to \"on: push\"\nIf actions handles only one event it will be used as default instead of \"on: push\"",
 		Short:             "Run GitHub actions locally by specifying the event name (e.g. `push`) or an action name directly.",
 		Args:              cobra.MaximumNArgs(1),
@@ -125,34 +125,6 @@ func configLocations() []string {
 	return []string{specPath, homePath, invocationPath}
 }
 
-var commonSocketPaths = []string{
-	"/var/run/docker.sock",
-	"/run/podman/podman.sock",
-	"$HOME/.colima/docker.sock",
-	"$XDG_RUNTIME_DIR/docker.sock",
-	"$XDG_RUNTIME_DIR/podman/podman.sock",
-	`\\.\pipe\docker_engine`,
-	"$HOME/.docker/run/docker.sock",
-}
-
-// returns socket path or false if not found any
-func socketLocation() (string, bool) {
-	if dockerHost, exists := os.LookupEnv("DOCKER_HOST"); exists {
-		return dockerHost, true
-	}
-
-	for _, p := range commonSocketPaths {
-		if _, err := os.Lstat(os.ExpandEnv(p)); err == nil {
-			if strings.HasPrefix(p, `\\.\`) {
-				return "npipe://" + filepath.ToSlash(os.ExpandEnv(p)), true
-			}
-			return "unix://" + filepath.ToSlash(os.ExpandEnv(p)), true
-		}
-	}
-
-	return "", false
-}
-
 func args() []string {
 	actrc := configLocations()
 
@@ -185,7 +157,7 @@ func bugReport(ctx context.Context, version string) error {
 
 	report += sprintf("Docker host:", dockerHost)
 	report += fmt.Sprintln("Sockets found:")
-	for _, p := range commonSocketPaths {
+	for _, p := range container.CommonSocketLocations {
 		if _, err := os.Lstat(os.ExpandEnv(p)); err != nil {
 			continue
 		} else if _, err := os.Stat(os.ExpandEnv(p)); err != nil {
@@ -356,18 +328,6 @@ func parseMatrix(matrix []string) map[string]map[string]bool {
 	return matrixes
 }
 
-func isDockerHostURI(daemonPath string) bool {
-	if protoIndex := strings.Index(daemonPath, "://"); protoIndex != -1 {
-		scheme := daemonPath[:protoIndex]
-		if strings.IndexFunc(scheme, func(r rune) bool {
-			return (r < 'a' || r > 'z') && (r < 'A' || r > 'Z')
-		}) == -1 {
-			return true
-		}
-	}
-	return false
-}
-
 //nolint:gocyclo
 func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []string) error {
 	return func(cmd *cobra.Command, args []string) error {
@@ -378,27 +338,12 @@ func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []str
 		if ok, _ := cmd.Flags().GetBool("bug-report"); ok {
 			return bugReport(ctx, cmd.Version)
 		}
-
+		if ret, err := container.GetSocketAndHost(input.containerDaemonSocket); err != nil {
-		// Prefer DOCKER_HOST, don't override it
+			log.Warnf("Couldn't get a valid docker connection: %+v", err)
-		socketPath, hasDockerHost := os.LookupEnv("DOCKER_HOST")
-		if !hasDockerHost {
-			// a - in containerDaemonSocket means don't mount, preserve this value
-			// otherwise if input.containerDaemonSocket is a filepath don't use it as socketPath
-			skipMount := input.containerDaemonSocket == "-" || !isDockerHostURI(input.containerDaemonSocket)
-			if input.containerDaemonSocket != "" && !skipMount {
-				socketPath = input.containerDaemonSocket
 		} else {
-				socket, found := socketLocation()
+			os.Setenv("DOCKER_HOST", ret.Host)
-				if !found {
+			input.containerDaemonSocket = ret.Socket
-					log.Errorln("daemon Docker Engine socket not found and containerDaemonSocket option was not set")
+			log.Infof("Using docker host '%s', and daemon socket '%s'", ret.Host, ret.Socket)
-				} else {
-					socketPath = socket
-				}
-				if !skipMount {
-					input.containerDaemonSocket = socketPath
-				}
-			}
-			os.Setenv("DOCKER_HOST", socketPath)
 		}
 
 		if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" && input.containerArchitecture == "" {

pkg/container/docker_socket.go

@@ -0,0 +1,134 @@
+package container
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+
+	log "github.com/sirupsen/logrus"
+)
+
+var CommonSocketLocations = []string{
+	"/var/run/docker.sock",
+	"/run/podman/podman.sock",
+	"$HOME/.colima/docker.sock",
+	"$XDG_RUNTIME_DIR/docker.sock",
+	"$XDG_RUNTIME_DIR/podman/podman.sock",
+	`\\.\pipe\docker_engine`,
+	"$HOME/.docker/run/docker.sock",
+}
+
+// returns socket URI or false if not found any
+func socketLocation() (string, bool) {
+	if dockerHost, exists := os.LookupEnv("DOCKER_HOST"); exists {
+		return dockerHost, true
+	}
+
+	for _, p := range CommonSocketLocations {
+		if _, err := os.Lstat(os.ExpandEnv(p)); err == nil {
+			if strings.HasPrefix(p, `\\.\`) {
+				return "npipe://" + filepath.ToSlash(os.ExpandEnv(p)), true
+			}
+			return "unix://" + filepath.ToSlash(os.ExpandEnv(p)), true
+		}
+	}
+
+	return "", false
+}
+
+// This function, `isDockerHostURI`, takes a string argument `daemonPath`. It checks if the
+// `daemonPath` is a valid Docker host URI. It does this by checking if the scheme of the URI (the
+// part before "://") contains only alphabetic characters. If it does, the function returns true,
+// indicating that the `daemonPath` is a Docker host URI. If it doesn't, or if the "://" delimiter
+// is not found in the `daemonPath`, the function returns false.
+func isDockerHostURI(daemonPath string) bool {
+	if protoIndex := strings.Index(daemonPath, "://"); protoIndex != -1 {
+		scheme := daemonPath[:protoIndex]
+		if strings.IndexFunc(scheme, func(r rune) bool {
+			return (r < 'a' || r > 'z') && (r < 'A' || r > 'Z')
+		}) == -1 {
+			return true
+		}
+	}
+	return false
+}
+
+type SocketAndHost struct {
+	Socket string
+	Host   string
+}
+
+func GetSocketAndHost(containerSocket string) (SocketAndHost, error) {
+	log.Debugf("Handling container host and socket")
+
+	// Prefer DOCKER_HOST, don't override it
+	dockerHost, hasDockerHost := socketLocation()
+	socketHost := SocketAndHost{Socket: containerSocket, Host: dockerHost}
+
+	// ** socketHost.Socket cases **
+	// Case 1: User does _not_ want to mount a daemon socket (passes a dash)
+	// Case 2: User passes a filepath to the socket; is that even valid?
+	// Case 3: User passes a valid socket; do nothing
+	// Case 4: User omitted the flag; set a sane default
+
+	// ** DOCKER_HOST cases **
+	// Case A: DOCKER_HOST is set; use it, i.e. do nothing
+	// Case B: DOCKER_HOST is empty; use sane defaults
+
+	// Set host for sanity's sake, when the socket isn't useful
+	if !hasDockerHost && (socketHost.Socket == "-" || !isDockerHostURI(socketHost.Socket) || socketHost.Socket == "") {
+		// Cases: 1B, 2B, 4B
+		socket, found := socketLocation()
+		socketHost.Host = socket
+		hasDockerHost = found
+	}
+
+	// A - (dash) in socketHost.Socket means don't mount, preserve this value
+	// otherwise if socketHost.Socket is a filepath don't use it as socket
+	// Exit early if we're in an invalid state (e.g. when no DOCKER_HOST and user supplied "-", a dash or omitted)
+	if !hasDockerHost && socketHost.Socket != "" && !isDockerHostURI(socketHost.Socket) {
+		// Cases: 1B, 2B
+		// Should we early-exit here, since there is no host nor socket to talk to?
+		return SocketAndHost{}, fmt.Errorf("DOCKER_HOST was not set, couldn't be found in the usual locations, and the container daemon socket ('%s') is invalid", socketHost.Socket)
+	}
+
+	// Default to DOCKER_HOST if set
+	if socketHost.Socket == "" && hasDockerHost {
+		// Cases: 4A
+		log.Debugf("Defaulting container socket to DOCKER_HOST")
+		socketHost.Socket = socketHost.Host
+	}
+	// Set sane default socket location if user omitted it
+	if socketHost.Socket == "" {
+		// Cases: 4B
+		socket, _ := socketLocation()
+		// socket is empty if it isn't found, so assignment here is at worst a no-op
+		log.Debugf("Defaulting container socket to default '%s'", socket)
+		socketHost.Socket = socket
+	}
+
+	// Exit if both the DOCKER_HOST and socket are fulfilled
+	if hasDockerHost {
+		// Cases: 1A, 2A, 3A, 4A
+		if !isDockerHostURI(socketHost.Socket) {
+			// Cases: 1A, 2A
+			log.Debugf("DOCKER_HOST is set, but socket is invalid '%s'", socketHost.Socket)
+		}
+		return socketHost, nil
+	}
+
+	// Set a sane DOCKER_HOST default if we can
+	if isDockerHostURI(socketHost.Socket) {
+		// Cases: 3B
+		log.Debugf("Setting DOCKER_HOST to container socket '%s'", socketHost.Socket)
+		socketHost.Host = socketHost.Socket
+		// Both DOCKER_HOST and container socket are valid; short-circuit exit
+		return socketHost, nil
+	}
+
+	// Here there is no DOCKER_HOST _and_ the supplied container socket is not a valid URI (either invalid or a file path)
+	// Cases: 2B <- but is already handled at the top
+	// I.e. this path should never be taken
+	return SocketAndHost{}, fmt.Errorf("no DOCKER_HOST and an invalid container socket '%s'", socketHost.Socket)
+}

pkg/container/docker_socket_test.go

@@ -0,0 +1,150 @@
+package container
+
+import (
+	"os"
+	"testing"
+
+	log "github.com/sirupsen/logrus"
+	assert "github.com/stretchr/testify/assert"
+)
+
+func init() {
+	log.SetLevel(log.DebugLevel)
+}
+
+var originalCommonSocketLocations = CommonSocketLocations
+
+func TestGetSocketAndHostWithSocket(t *testing.T) {
+	// Arrange
+	CommonSocketLocations = originalCommonSocketLocations
+	dockerHost := "unix:///my/docker/host.sock"
+	socketURI := "/path/to/my.socket"
+	os.Setenv("DOCKER_HOST", dockerHost)
+
+	// Act
+	ret, err := GetSocketAndHost(socketURI)
+
+	// Assert
+	assert.Nil(t, err)
+	assert.Equal(t, SocketAndHost{socketURI, dockerHost}, ret)
+}
+
+func TestGetSocketAndHostNoSocket(t *testing.T) {
+	// Arrange
+	dockerHost := "unix:///my/docker/host.sock"
+	os.Setenv("DOCKER_HOST", dockerHost)
+
+	// Act
+	ret, err := GetSocketAndHost("")
+
+	// Assert
+	assert.Nil(t, err)
+	assert.Equal(t, SocketAndHost{dockerHost, dockerHost}, ret)
+}
+
+func TestGetSocketAndHostOnlySocket(t *testing.T) {
+	// Arrange
+	socketURI := "/path/to/my.socket"
+	os.Unsetenv("DOCKER_HOST")
+	CommonSocketLocations = originalCommonSocketLocations
+	defaultSocket, defaultSocketFound := socketLocation()
+
+	// Act
+	ret, err := GetSocketAndHost(socketURI)
+
+	// Assert
+	assert.NoError(t, err, "Expected no error from GetSocketAndHost")
+	assert.Equal(t, true, defaultSocketFound, "Expected to find default socket")
+	assert.Equal(t, socketURI, ret.Socket, "Expected socket to match common location")
+	assert.Equal(t, defaultSocket, ret.Host, "Expected ret.Host to match default socket location")
+}
+
+func TestGetSocketAndHostDontMount(t *testing.T) {
+	// Arrange
+	CommonSocketLocations = originalCommonSocketLocations
+	dockerHost := "unix:///my/docker/host.sock"
+	os.Setenv("DOCKER_HOST", dockerHost)
+
+	// Act
+	ret, err := GetSocketAndHost("-")
+
+	// Assert
+	assert.Nil(t, err)
+	assert.Equal(t, SocketAndHost{"-", dockerHost}, ret)
+}
+
+func TestGetSocketAndHostNoHostNoSocket(t *testing.T) {
+	// Arrange
+	CommonSocketLocations = originalCommonSocketLocations
+	os.Unsetenv("DOCKER_HOST")
+	defaultSocket, found := socketLocation()
+
+	// Act
+	ret, err := GetSocketAndHost("")
+
+	// Assert
+	assert.Equal(t, true, found, "Expected a default socket to be found")
+	assert.Nil(t, err, "Expected no error from GetSocketAndHost")
+	assert.Equal(t, SocketAndHost{defaultSocket, defaultSocket}, ret, "Expected to match default socket location")
+}
+
+// Catch
+// > Your code breaks setting DOCKER_HOST if shouldMount is false.
+// > This happens if neither DOCKER_HOST nor --container-daemon-socket has a value, but socketLocation() returns a URI
+func TestGetSocketAndHostNoHostNoSocketDefaultLocation(t *testing.T) {
+	// Arrange
+	mySocketFile, tmpErr := os.CreateTemp("", "act-*.sock")
+	mySocket := mySocketFile.Name()
+	unixSocket := "unix://" + mySocket
+	defer os.RemoveAll(mySocket)
+	assert.NoError(t, tmpErr)
+	os.Unsetenv("DOCKER_HOST")
+
+	CommonSocketLocations = []string{mySocket}
+	defaultSocket, found := socketLocation()
+
+	// Act
+	ret, err := GetSocketAndHost("")
+
+	// Assert
+	assert.Equal(t, unixSocket, defaultSocket, "Expected default socket to match common socket location")
+	assert.Equal(t, true, found, "Expected default socket to be found")
+	assert.Nil(t, err, "Expected no error from GetSocketAndHost")
+	assert.Equal(t, SocketAndHost{unixSocket, unixSocket}, ret, "Expected to match default socket location")
+}
+
+func TestGetSocketAndHostNoHostInvalidSocket(t *testing.T) {
+	// Arrange
+	os.Unsetenv("DOCKER_HOST")
+	mySocket := "/my/socket/path.sock"
+	CommonSocketLocations = []string{"/unusual", "/socket", "/location"}
+	defaultSocket, found := socketLocation()
+
+	// Act
+	ret, err := GetSocketAndHost(mySocket)
+
+	// Assert
+	assert.Equal(t, false, found, "Expected no default socket to be found")
+	assert.Equal(t, "", defaultSocket, "Expected no default socket to be found")
+	assert.Equal(t, SocketAndHost{}, ret, "Expected to match default socket location")
+	assert.Error(t, err, "Expected an error in invalid state")
+}
+
+func TestGetSocketAndHostOnlySocketValidButUnusualLocation(t *testing.T) {
+	// Arrange
+	socketURI := "unix:///path/to/my.socket"
+	CommonSocketLocations = []string{"/unusual", "/location"}
+	os.Unsetenv("DOCKER_HOST")
+	defaultSocket, found := socketLocation()
+
+	// Act
+	ret, err := GetSocketAndHost(socketURI)
+
+	// Assert
+	// Default socket locations
+	assert.Equal(t, "", defaultSocket, "Expect default socket location to be empty")
+	assert.Equal(t, false, found, "Expected no default socket to be found")
+	// Sane default
+	assert.Nil(t, err, "Expect no error from GetSocketAndHost")
+	assert.Equal(t, socketURI, ret.Host, "Expect host to default to unusual socket")
+}