From a80559e439cf7b989db47a612fd97f9fff9bd3e2 Mon Sep 17 00:00:00 2001
From: Paul Banks <banks@banksco.de>
Date: Tue, 22 May 2018 15:11:13 +0100
Subject: [PATCH] Make invalid clusterID be fatal

---
 agent/agent.go      | 17 +++++++++++------
 agent/agent_test.go | 37 ++++++++++++++++++++++++++-----------
 agent/testagent.go  | 11 +++++++++++
 3 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/agent/agent.go b/agent/agent.go
index eb9e203dc5..622a105e82 100644
--- a/agent/agent.go
+++ b/agent/agent.go
@@ -946,8 +946,12 @@ func (a *Agent) consulConfig() (*consul.Config, error) {
 				}
 			}
 			if base.CAConfig.ClusterID == "" {
-				a.logger.Println("[WARN] connect CA config cluster_id specified but ",
-					"is not a valid UUID, ignoring")
+				// If the tried to specify an ID but typoed it don't ignore as they will
+				// then bootstrap with a new ID and have to throw away the whole cluster
+				// and start again.
+				a.logger.Println("[ERR] connect CA config cluster_id specified but " +
+					"is not a valid UUID, aborting startup")
+				return nil, fmt.Errorf("cluster_id was supplied but was not a valid UUID")
 			}
 		}
 
@@ -1315,8 +1319,10 @@ func (a *Agent) ShutdownAgent() error {
 	// NOTE(mitchellh): we use Kill for now to kill the processes since
 	// the local state isn't snapshotting meaning the proxy tokens are
 	// regenerated each time forcing the processes to restart anyways.
-	if err := a.proxyManager.Kill(); err != nil {
-		a.logger.Printf("[WARN] agent: error shutting down proxy manager: %s", err)
+	if a.proxyManager != nil {
+		if err := a.proxyManager.Kill(); err != nil {
+			a.logger.Printf("[WARN] agent: error shutting down proxy manager: %s", err)
+		}
 	}
 
 	var err error
@@ -2177,8 +2183,7 @@ func (a *Agent) verifyProxyToken(token, targetService, targetProxy string) (stri
 
 		// Resolve the actual ACL token used to register the proxy/service and
 		// return that for use in RPC calls.
-		aclToken := a.State.ServiceToken(targetService)
-		return aclToken, nil
+		return a.State.ServiceToken(targetService), nil
 	}
 
 	// Retrieve the service specified. This should always exist because
diff --git a/agent/agent_test.go b/agent/agent_test.go
index 911ed63a03..993bf3b25e 100644
--- a/agent/agent_test.go
+++ b/agent/agent_test.go
@@ -60,6 +60,7 @@ func TestAgent_ConnectClusterIDConfig(t *testing.T) {
 		name          string
 		hcl           string
 		wantClusterID string
+		wantPanic     bool
 	}{
 		{
 			name:          "default TestAgent has fixed cluster id",
@@ -72,22 +73,36 @@ func TestAgent_ConnectClusterIDConfig(t *testing.T) {
 			wantClusterID: "",
 		},
 		{
-			name: "non-UUID cluster_id is ignored",
-			hcl: `connect { 
-				enabled = true
-				ca_config {
-					cluster_id = "fake-id"
-				}
-			}`,
+			name: "non-UUID cluster_id is fatal",
+			hcl: `connect {
+	   enabled = true
+	   ca_config {
+	     cluster_id = "fake-id"
+	   }
+	 }`,
 			wantClusterID: "",
+			wantPanic:     true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			a := NewTestAgent("test", tt.hcl)
-			cfg := a.consulConfig()
-			assert.Equal(t, tt.wantClusterID, cfg.CAConfig.ClusterID)
+			// Indirection to support panic recovery cleanly
+			testFn := func() {
+				a := &TestAgent{Name: "test", HCL: tt.hcl}
+				a.ExpectConfigError = tt.wantPanic
+				a.Start()
+				defer a.Shutdown()
+
+				cfg := a.consulConfig()
+				assert.Equal(t, tt.wantClusterID, cfg.CAConfig.ClusterID)
+			}
+
+			if tt.wantPanic {
+				require.Panics(t, testFn)
+			} else {
+				testFn()
+			}
 		})
 	}
 }
@@ -95,7 +110,7 @@ func TestAgent_ConnectClusterIDConfig(t *testing.T) {
 func TestAgent_StartStop(t *testing.T) {
 	t.Parallel()
 	a := NewTestAgent(t.Name(), "")
-	// defer a.Shutdown()
+	defer a.Shutdown()
 
 	if err := a.Leave(); err != nil {
 		t.Fatalf("err: %v", err)
diff --git a/agent/testagent.go b/agent/testagent.go
index 26c81a81dc..007d01322f 100644
--- a/agent/testagent.go
+++ b/agent/testagent.go
@@ -45,6 +45,12 @@ type TestAgent struct {
 
 	HCL string
 
+	// ExpectConfigError can be set to prevent the agent retrying Start on errors
+	// and eventually blowing up with runtime.Goexit. This enables tests to assert
+	// that some specific bit of config actually does prevent startup entirely in
+	// a reasonable way without reproducing a lot of the boilerplate here.
+	ExpectConfigError bool
+
 	// Config is the agent configuration. If Config is nil then
 	// TestConfig() is used. If Config.DataDir is set then it is
 	// the callers responsibility to clean up the data directory.
@@ -159,6 +165,11 @@ func (a *TestAgent) Start() *TestAgent {
 		} else if i == 0 {
 			fmt.Println(id, a.Name, "Error starting agent:", err)
 			runtime.Goexit()
+		} else if a.ExpectConfigError {
+			// Panic the error since this can be caught if needed. Pretty gross way to
+			// detect errors but enough for now and this is a tiny edge case that I'd
+			// otherwise not have a way to test at all...
+			panic(err)
 		} else {
 			agent.ShutdownAgent()
 			agent.ShutdownEndpoints()