From 263614d596d8be352eae16fa8a5158796271cc21 Mon Sep 17 00:00:00 2001
From: Ryan Uber <ru@ryanuber.com>
Date: Fri, 16 Jan 2015 12:39:15 -0800
Subject: [PATCH] agent: error if binding to existing socket file

---
 command/agent/agent.go     |  7 +++++++
 command/agent/command.go   |  9 ++++-----
 command/agent/http.go      | 13 +++----------
 command/agent/http_test.go | 27 ++++++++++++---------------
 4 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/command/agent/agent.go b/command/agent/agent.go
index 778d07135..d5ef05af4 100644
--- a/command/agent/agent.go
+++ b/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."
 )
 
 /*
diff --git a/command/agent/command.go b/command/agent/command.go
index 3afafca5e..17eb5c577 100644
--- a/command/agent/command.go
+++ b/command/agent/command.go
@@ -295,12 +295,11 @@ func (c *Command) setupAgent(config *Config, logOutput io.Writer, logWriter *log
 		return err
 	}
 
+	// Error if we are trying to bind a domain socket to an existing path
 	if path, ok := unixSocketAddr(config.Addresses.RPC); ok {
-		// Remove the socket if it exists, or we'll get a bind error. This
-		// is necessary to avoid situations where Consul cannot start if the
-		// socket file exists in case of unexpected termination.
-		if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
-			return err
+		if _, err := os.Stat(path); err == nil || !os.IsNotExist(err) {
+			c.Ui.Output(fmt.Sprintf(errSocketFileExists, path))
+			return fmt.Errorf(errSocketFileExists, path)
 		}
 	}
 
diff --git a/command/agent/http.go b/command/agent/http.go
index 19a3dc0bf..708fe5b17 100644
--- a/command/agent/http.go
+++ b/command/agent/http.go
@@ -59,13 +59,6 @@ func NewHTTPServers(agent *Agent, config *Config, logOutput io.Writer) ([]*HTTPS
 			return nil, err
 		}
 
-		if path, ok := unixSocketAddr(config.Addresses.HTTPS); ok {
-			// See command/agent/config.go
-			if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
-				return nil, err
-			}
-		}
-
 		ln, err := net.Listen(httpAddr.Network(), httpAddr.String())
 		if err != nil {
 			return nil, fmt.Errorf("Failed to get Listen on %s: %v", httpAddr.String(), err)
@@ -102,10 +95,10 @@ func NewHTTPServers(agent *Agent, config *Config, logOutput io.Writer) ([]*HTTPS
 			return nil, fmt.Errorf("Failed to get ClientListener address:port: %v", err)
 		}
 
+		// Error if we are trying to bind a domain socket to an existing path
 		if path, ok := unixSocketAddr(config.Addresses.HTTP); ok {
-			// See command/agent/config.go
-			if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
-				return nil, err
+			if _, err := os.Stat(path); err == nil || !os.IsNotExist(err) {
+				return nil, fmt.Errorf(errSocketFileExists, path)
 			}
 		}
 
diff --git a/command/agent/http_test.go b/command/agent/http_test.go
index 93be5b5c4..4677d5ba5 100644
--- a/command/agent/http_test.go
+++ b/command/agent/http_test.go
@@ -101,7 +101,7 @@ func TestHTTPServer_UnixSocket(t *testing.T) {
 	}
 }
 
-func TestHTTPServer_UnixSocket_OverwriteFile(t *testing.T) {
+func TestHTTPServer_UnixSocket_FileExists(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.SkipNow()
 	}
@@ -125,21 +125,18 @@ func TestHTTPServer_UnixSocket_OverwriteFile(t *testing.T) {
 		t.Fatalf("not a regular file: %s", socket)
 	}
 
-	// Try to start the server with the same path anyways.
-	dir, srv := makeHTTPServerWithConfig(t, func(c *Config) {
-		c.Addresses.HTTP = "unix://" + socket
-	})
-	defer os.RemoveAll(dir)
-	defer srv.Shutdown()
-	defer srv.agent.Shutdown()
+	conf := nextConfig()
+	conf.Addresses.HTTP = "unix://" + socket
 
-	// Check if the socket overwrote the file
-	fi, err = os.Stat(socket)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if fi.Mode().IsRegular() {
-		t.Fatalf("should have socket file: %s", socket)
+	dir, agent := makeAgent(t, conf)
+	defer os.RemoveAll(dir)
+
+	// Try to start the server with the same path anyways.
+	if servers, err := NewHTTPServers(agent, conf, agent.logOutput); err == nil {
+		for _, server := range servers {
+			server.Shutdown()
+		}
+		t.Fatalf("expected socket binding error")
 	}
 }