update: use DOCKER_HOST as the default

[till]

Summary

Make --docker-host=inherit the default behavior.

Before

The default was to use a variant of /var/run/docker.sock (depending on OS/environment).

After

The default is to check DOCKER_HOST and fall back to the old default before.

Documentation

• Should this change be documented?
  • Yes, see Better defaults for --docker-host. #1338
  • No

Related

Resolves #1338

[matejvasek]

Could we default to DOCKER_HOST conditionally?

• With ssh never default.
• With tcp default only when host in not localhost (localhost, 127.0.0.1, ::1) and DOCKER_CERT_PATH,DOCKER_TLS_VERIFY are not set.
• With unix default only on Linux, do not default on macOS or Windows.

This would be more backward compatible.

[till]

Could we default to DOCKER_HOST conditionally?

• With ssh never default.
• With tcp default only when host in not localhost (localhost, 127.0.0.1, ::1) and DOCKER_CERT_PATH,DOCKER_TLS_VERIFY are not set.

Do you mean, it's not used when the DOCKER_HOST contains ssh://, same for tcp:// (along with the vars)?

[matejvasek]

I mean for ssh the DOCKER_HOST should not be used ever. For TCP it should be used as long as the hostname != localhost and TLS is not employed. For unix socket it should be used only on Linux.

[matejvasek]

Otherwise this will break things for people using ssh (and maybe tcp too).

[till]

I mean for ssh the DOCKER_HOST should not be used ever. For TCP it should be used as long as the hostname != localhost and TLS is not employed. For unix socket it should be used only on Linux.

Oh, you mean — employ a validator for what's "in" DOCKER_HOST?

[till]

So as a summary, these are the cases you would like to address — where a value from DOCKER_HOST is ignored:

• OS != "linux" and DOCKER_HOST=unix://...
• DOCKER_HOST=tcp://localhost (any variant of it, such as 127.0.0.1 or ::1)
• DOCKER_HOST=tcp://...and the vars DOCKER_CERT_PATH and DOCKER_TLS_VERIFY not empty

What happens then? Empty it in code and let the current logic pick up the OS-specific binds for the socket? Add a log line?

[matejvasek]

@till sorry for late reply. I think that we perhaps should keep default to "" at flag level and not use inherit.
Then we could do some sane defaulting in internal/build/phase_config_provider.go.

Draft of what it could look like (note I did not test this code at all):

diff --git a/internal/build/phase_config_provider.go b/internal/build/phase_config_provider.go
index 93861633..042087c3 100644
--- a/internal/build/phase_config_provider.go
+++ b/internal/build/phase_config_provider.go
@@ -3,9 +3,15 @@ package build
 import (
 	"fmt"
 	"io"
+	"net"
+	"net/url"
 	"os"
+	"path/filepath"
+	"runtime"
+	"strconv"
 	"strings"
 
+	"github.com/docker/cli/cli/config"
 	"github.com/docker/docker/api/types/container"
 
 	pcontainer "github.com/buildpacks/pack/internal/container"
@@ -152,17 +158,62 @@ func WithBinds(binds ...string) PhaseConfigProviderOperation {
 	}
 }
 
+func mightUseClientCertTLS() bool {
+	tlsVerifyStr, tlsVerifyChanged := os.LookupEnv("DOCKER_TLS_VERIFY")
+	if !tlsVerifyChanged {
+		return false
+	}
+	tlsVerify := true
+	if b, err := strconv.ParseBool(tlsVerifyStr); err == nil {
+		tlsVerify = b
+	}
+	if !tlsVerify {
+		return false
+	}
+	dockerCertPath := os.Getenv("DOCKER_CERT_PATH")
+	if dockerCertPath == "" {
+		dockerCertPath = config.Dir()
+	}
+	certPath := filepath.Join(dockerCertPath, "cert.pem")
+	keyPath := filepath.Join(dockerCertPath, "key.pem")
+	_, err := os.Stat(certPath)
+	if err != nil {
+		return false
+	}
+	_, err = os.Stat(keyPath)
+	if err != nil {
+		return false
+	}
+	return true
+}
+
 func WithDaemonAccess(dockerHost string) PhaseConfigProviderOperation {
 	return func(provider *PhaseConfigProvider) {
 		WithRoot()(provider)
+		localDockerHost := strings.TrimSpace(os.Getenv("DOCKER_HOST"))
 		if dockerHost == "inherit" {
-			dockerHost = os.Getenv("DOCKER_HOST")
+			dockerHost = localDockerHost
 		}
 		var bind string
 		if dockerHost == "" {
-			bind = "/var/run/docker.sock:/var/run/docker.sock"
-			if provider.os == "windows" {
-				bind = `\\.\pipe\docker_engine:\\.\pipe\docker_engine`
+			switch {
+			case strings.HasPrefix(localDockerHost, "unix://"):
+				if runtime.GOOS == "linux" {
+					dockerHost = localDockerHost
+				}
+			case strings.HasPrefix(localDockerHost, "tcp://"):
+				if u, err := url.Parse(localDockerHost); err == nil {
+					var addr *net.IPAddr
+					addr, err = net.ResolveIPAddr("tcp", u.Host)
+					if err == nil && !addr.IP.IsLoopback() && !mightUseClientCertTLS() {
+						dockerHost = localDockerHost
+					}
+				}
+			default:
+				bind = "/var/run/docker.sock:/var/run/docker.sock"
+				if provider.os == "windows" {
+					bind = `\\.\pipe\docker_engine:\\.\pipe\docker_engine`
+				}
 			}
 		} else {
 			switch {

[natalieparellano]

Any further updates here?

[till]

Not yet, I can add a commit some time to address the tls settings. Is there anything else that needs to be addressed?

[natalieparellano]

@till we discussed this in Slack here: https://cloud-native.slack.com/archives/C0331B61A1Y/p1725391083581559?thread_ts=1724932960.525119&cid=C0331B61A1Y

I'm not sure if we have a clear way forward with this. Do you have any thoughts about it?