Refactor UNIX domain socket

api/api.go

@@ -120,8 +120,8 @@ func DefaultConfig() *Config {
 		HttpClient: http.DefaultClient,
 	}
 
-	if len(os.Getenv("CONSUL_HTTP_ADDR")) > 0 {
-		config.Address = os.Getenv("CONSUL_HTTP_ADDR")
+	if addr := os.Getenv("CONSUL_HTTP_ADDR"); addr != "" {
+		config.Address = addr
 	}
 
 	return config
@@ -137,11 +137,7 @@ func NewClient(config *Config) (*Client, error) {
 	// bootstrap the config
 	defConfig := DefaultConfig()
 
-	switch {
-	case len(config.Address) != 0:
-	case len(os.Getenv("CONSUL_HTTP_ADDR")) > 0:
-		config.Address = os.Getenv("CONSUL_HTTP_ADDR")
-	default:
+	if len(config.Address) == 0 {
 		config.Address = defConfig.Address
 	}
 
@@ -153,14 +149,15 @@ func NewClient(config *Config) (*Client, error) {
 		config.HttpClient = defConfig.HttpClient
 	}
 
-	if strings.HasPrefix(config.Address, "unix://") {
-		shortStr := strings.TrimPrefix(config.Address, "unix://")
-		t := &http.Transport{}
-		t.Dial = func(_, _ string) (net.Conn, error) {
-			return net.Dial("unix", shortStr)
+	if parts := strings.SplitN(config.Address, "unix://", 2); len(parts) == 2 {
+		config.HttpClient = &http.Client{
+			Transport: &http.Transport{
+				Dial: func(_, _ string) (net.Conn, error) {
+					return net.Dial("unix", parts[1])
+				},
+			},
 		}
-		config.HttpClient.Transport = t
-		config.Address = shortStr
+		config.Address = parts[1]
 	}
 
 	client := &Client{

api/api_test.go

@@ -8,6 +8,8 @@ import (
 	"net/http"
 	"os"
 	"os/exec"
+	"path/filepath"
+	"runtime"
 	"testing"
 	"time"
 
@@ -42,6 +44,10 @@ type testServerConfig struct {
 	Ports     testPortConfig     `json:"ports,omitempty"`
 }
 
+// Callback functions for modifying config
+type configCallback func(c *Config)
+type serverConfigCallback func(c *testServerConfig)
+
 func defaultConfig() *testServerConfig {
 	return &testServerConfig{
 		Bootstrap: true,
@@ -72,7 +78,7 @@ func newTestServer(t *testing.T) *testServer {
 	return newTestServerWithConfig(t, func(c *testServerConfig) {})
 }
 
-func newTestServerWithConfig(t *testing.T, cb func(c *testServerConfig)) *testServer {
+func newTestServerWithConfig(t *testing.T, cb serverConfigCallback) *testServer {
 	if path, err := exec.LookPath("consul"); err != nil || path == "" {
 		t.Log("consul not found on $PATH, skipping")
 		t.SkipNow()
@@ -131,15 +137,20 @@ func makeClient(t *testing.T) (*Client, *testServer) {
 	}, func(c *testServerConfig) {})
 }
 
-func makeClientWithConfig(t *testing.T, clientConfig func(c *Config), serverConfig func(c *testServerConfig)) (*Client, *testServer) {
-	server := newTestServerWithConfig(t, serverConfig)
+func makeClientWithConfig(t *testing.T, cb1 configCallback, cb2 serverConfigCallback) (*Client, *testServer) {
+	// Make client config
 	conf := DefaultConfig()
-	clientConfig(conf)
+	cb1(conf)
+
+	// Create client
 	client, err := NewClient(conf)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
 
+	// Create server
+	server := newTestServerWithConfig(t, cb2)
+
 	// Allow the server some time to start, and verify we have a leader.
 	testutil.WaitForResult(func() (bool, error) {
 		req := client.newRequest("GET", "/v1/catalog/nodes")
@@ -278,3 +289,35 @@ func TestParseQueryMeta(t *testing.T) {
 		t.Fatalf("Bad: %v", qm)
 	}
 }
+
+func TestAPI_UnixSocket(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.SkipNow()
+	}
+
+	tempDir, err := ioutil.TempDir("", "consul")
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	defer os.RemoveAll(tempDir)
+	socket := filepath.Join(tempDir, "test.sock")
+
+	c, s := makeClientWithConfig(t, func(c *Config) {
+		c.Address = "unix://" + socket
+	}, func(c *testServerConfig) {
+		c.Addresses = &testAddressConfig{
+			HTTP: "unix://" + socket,
+		}
+	})
+	defer s.stop()
+
+	agent := c.Agent()
+
+	info, err := agent.Self()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if info["Config"]["NodeName"] == "" {
+		t.Fatalf("bad: %v", info)
+	}
+}

api/status_test.go

@@ -1,13 +1,10 @@
 package api
 
 import (
-	"io/ioutil"
-	"os/user"
-	"runtime"
 	"testing"
 )
 
-func TestStatusLeaderTCP(t *testing.T) {
+func TestStatusLeader(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.stop()
 
@@ -22,48 +19,6 @@ func TestStatusLeaderTCP(t *testing.T) {
 	}
 }
 
-func TestStatusLeaderUnix(t *testing.T) {
-	if runtime.GOOS == "windows" {
-		t.SkipNow()
-	}
-
-	tempdir, err := ioutil.TempDir("", "consul-test-")
-	if err != nil {
-		t.Fatal("Could not create a working directory")
-	}
-
-	socket := "unix://" + tempdir + "/unix-http-test.sock"
-
-	clientConfig := func(c *Config) {
-		c.Address = socket
-	}
-
-	serverConfig := func(c *testServerConfig) {
-		user, err := user.Current()
-		if err != nil {
-			t.Fatal("Could not get current user")
-		}
-
-		if c.Addresses == nil {
-			c.Addresses = &testAddressConfig{}
-		}
-		c.Addresses.HTTP = socket + ";" + user.Uid + ";" + user.Gid + ";640"
-	}
-
-	c, s := makeClientWithConfig(t, clientConfig, serverConfig)
-	defer s.stop()
-
-	status := c.Status()
-
-	leader, err := status.Leader()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if leader == "" {
-		t.Fatalf("Expected leader")
-	}
-}
-
 func TestStatusPeers(t *testing.T) {
 	c, s := makeClient(t)
 	defer s.stop()

command/agent/agent.go

@@ -22,6 +22,13 @@ const (
 
 	// Path to save local agent checks
 	checksDir = "checks"
+
+	// errSocketFileExists is the human-friendly error message displayed when
+	// trying to bind a socket to an existing file.
+	errSocketFileExists = "A file exists at the requested socket path %q. " +
+		"If Consul was not shut down properly, the socket file may " +
+		"be left behind. If the path looks correct, remove the file " +
+		"and try again."
 )
 
 /*

command/agent/agent_test.go

@@ -7,10 +7,8 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"os/user"
 	"path/filepath"
 	"reflect"
-	"runtime"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -125,7 +123,7 @@ func TestAgentStartStop(t *testing.T) {
 	}
 }
 
-func TestAgent_RPCPingTCP(t *testing.T) {
+func TestAgent_RPCPing(t *testing.T) {
 	dir, agent := makeAgent(t, nextConfig())
 	defer os.RemoveAll(dir)
 	defer agent.Shutdown()
@@ -136,35 +134,6 @@ func TestAgent_RPCPingTCP(t *testing.T) {
 	}
 }
 
-func TestAgent_RPCPingUnix(t *testing.T) {
-	if runtime.GOOS == "windows" {
-		t.SkipNow()
-	}
-
-	nextConf := nextConfig()
-
-	tempdir, err := ioutil.TempDir("", "consul-test-")
-	if err != nil {
-		t.Fatal("Could not create a working directory")
-	}
-
-	user, err := user.Current()
-	if err != nil {
-		t.Fatal("Could not get current user")
-	}
-
-	nextConf.Addresses.RPC = "unix://" + tempdir + "/unix-rpc-test.sock;" + user.Uid + ";" + user.Gid + ";640"
-
-	dir, agent := makeAgent(t, nextConf)
-	defer os.RemoveAll(dir)
-	defer agent.Shutdown()
-
-	var out struct{}
-	if err := agent.RPC("Status.Ping", struct{}{}, &out); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-}
-
 func TestAgent_AddService(t *testing.T) {
 	dir, agent := makeAgent(t, nextConfig())
 	defer os.RemoveAll(dir)

command/agent/command.go

@@ -295,9 +295,12 @@ func (c *Command) setupAgent(config *Config, logOutput io.Writer, logWriter *log
 		return err
 	}
 
-	if _, ok := rpcAddr.(*net.UnixAddr); ok {
-		// Remove the socket if it exists, or we'll get a bind error
-		_ = os.Remove(rpcAddr.String())
+	// Error if we are trying to bind a domain socket to an existing path
+	if path, ok := unixSocketAddr(config.Addresses.RPC); ok {
+		if _, err := os.Stat(path); err == nil || !os.IsNotExist(err) {
+			c.Ui.Output(fmt.Sprintf(errSocketFileExists, path))
+			return fmt.Errorf(errSocketFileExists, path)
+		}
 	}
 
 	rpcListener, err := net.Listen(rpcAddr.Network(), rpcAddr.String())
@@ -307,14 +310,6 @@ func (c *Command) setupAgent(config *Config, logOutput io.Writer, logWriter *log
 		return err
 	}
 
-	if _, ok := rpcAddr.(*net.UnixAddr); ok {
-		if err := adjustUnixSocketPermissions(config.Addresses.RPC); err != nil {
-			agent.Shutdown()
-			c.Ui.Error(fmt.Sprintf("Error adjusting Unix socket permissions: %s", err))
-			return err
-		}
-	}
-
 	// Start the IPC layer
 	c.Ui.Output("Starting Consul agent RPC...")
 	c.rpcServer = NewAgentRPC(agent, rpcListener, logOutput, logWriter)