Merge pull request #69 from Shopify/fix_name_conflict_leak

Fix proxy name conflict leaking a port

CHANGELOG.md

@@ -1,3 +1,7 @@
+# Unreleased
+
+* Fix proxy name conflicts leaking an open port #69
+
 # 1.2.0
 
 * Add a Toxic and Toxics type for the Go client

api.go

@@ -117,15 +117,8 @@ func (server *server) ProxyCreate(response http.ResponseWriter, request *http.Re
 	proxy.Name = input.Name
 	proxy.Listen = input.Listen
 	proxy.Upstream = input.Upstream
-	if input.Enabled {
-		err = proxy.Start()
-		if err != nil {
-			http.Error(response, server.apiError(err, http.StatusConflict), http.StatusConflict)
-			return
-		}
-	}
 
-	err = server.collection.Add(proxy)
+	err = server.collection.Add(proxy, input.Enabled)
 	if err != nil {
 		http.Error(response, server.apiError(err, http.StatusConflict), http.StatusConflict)
 		return

api_test.go

@@ -231,16 +231,56 @@ func TestDeleteProxy(t *testing.T) {
 	})
 }
 
-func TestCreateProxyTwice(t *testing.T) {
+func TestCreateProxyPortConflict(t *testing.T) {
 	WithServer(t, func(addr string) {
 		err := testProxy.Create()
 		if err != nil {
 			t.Fatal("Unable to create proxy")
 		}
 
-		err = testProxy.Create()
+		testProxy2 := *testProxy
+		testProxy2.Name = "test"
+		err = testProxy2.Create()
 		if err == nil {
-			t.Fatal("Expected error when creating same proxy twice")
+			t.Fatal("Proxy did not result in conflict.")
+		} else if err.Error() != "Create: HTTP 409: listen tcp 127.0.0.1:3310: bind: address already in use" {
+			t.Fatal("Incorrect error adding proxy:", err)
+		}
+
+		err = testProxy.Delete()
+		if err != nil {
+			t.Fatal("Unable to delete proxy: ", err)
+		}
+		err = testProxy2.Create()
+		if err != nil {
+			t.Fatal("Unable to create proxy: ", err)
+		}
+	})
+}
+
+func TestCreateProxyNameConflict(t *testing.T) {
+	WithServer(t, func(addr string) {
+		err := testProxy.Create()
+		if err != nil {
+			t.Fatal("Unable to create proxy: ", err)
+		}
+
+		testProxy2 := *testProxy
+		testProxy2.Listen = "localhost:3311"
+		err = testProxy2.Create()
+		if err == nil {
+			t.Fatal("Proxy did not result in conflict.")
+		} else if err.Error() != "Create: HTTP 409: Proxy with name mysql_master already exists" {
+			t.Fatal("Incorrect error adding proxy:", err)
+		}
+
+		err = testProxy.Delete()
+		if err != nil {
+			t.Fatal("Unable to delete proxy: ", err)
+		}
+		err = testProxy2.Create()
+		if err != nil {
+			t.Fatal("Unable to create proxy: ", err)
 		}
 	})
 }

proxy_collection.go

@@ -21,14 +21,21 @@ func NewProxyCollection() *ProxyCollection {
 	}
 }
 
-func (collection *ProxyCollection) Add(proxy *Proxy) error {
+func (collection *ProxyCollection) Add(proxy *Proxy, start bool) error {
 	collection.Lock()
 	defer collection.Unlock()
 
 	if _, exists := collection.proxies[proxy.Name]; exists {
 		return fmt.Errorf("Proxy with name %s already exists", proxy.Name)
 	}
 
+	if start {
+		err := proxy.Start()
+		if err != nil {
+			return err
+		}
+	}
+
 	collection.proxies[proxy.Name] = proxy
 
 	return nil

proxy_collection_test.go

@@ -14,7 +14,7 @@ func TestAddProxyToCollection(t *testing.T) {
 		t.Error("Expected proxies to be empty")
 	}
 
-	err := collection.Add(proxy)
+	err := collection.Add(proxy, false)
 	if err != nil {
 		t.Error("Expected to be able to add first proxy to collection")
 	}
@@ -28,12 +28,12 @@ func TestAddTwoProxiesToCollection(t *testing.T) {
 	collection := NewProxyCollection()
 	proxy := NewTestProxy("test", "localhost:20000")
 
-	err := collection.Add(proxy)
+	err := collection.Add(proxy, false)
 	if err != nil {
 		t.Error("Expected to be able to add first proxy to collection")
 	}
 
-	err = collection.Add(proxy)
+	err = collection.Add(proxy, false)
 	if err == nil {
 		t.Error("Expected to not be able to add proxy with same name")
 	}
@@ -43,14 +43,35 @@ func TestListProxies(t *testing.T) {
 	collection := NewProxyCollection()
 	proxy := NewTestProxy("test", "localhost:20000")
 
-	err := collection.Add(proxy)
+	err := collection.Add(proxy, false)
 	if err != nil {
 		t.Error("Expected to be able to add first proxy to collection")
 	}
 
 	proxies := collection.Proxies()
-	if _, ok := proxies[proxy.Name]; !ok {
+	proxy, ok := proxies[proxy.Name]
+	if !ok {
 		t.Error("Expected to be able to see existing proxy")
+	} else if proxy.Enabled {
+		t.Error("Expected proxy not to be running")
+	}
+}
+
+func TestAddProxyAndStart(t *testing.T) {
+	collection := NewProxyCollection()
+	proxy := NewTestProxy("test", "localhost:20000")
+
+	err := collection.Add(proxy, true)
+	if err != nil {
+		t.Error("Expected to be able to add proxy to collection:", err)
+	}
+
+	proxies := collection.Proxies()
+	proxy, ok := proxies[proxy.Name]
+	if !ok {
+		t.Error("Expected to be able to see existing proxy")
+	} else if !proxy.Enabled {
+		t.Error("Expected proxy to be running")
 	}
 }
 
@@ -62,7 +83,7 @@ func TestAddAndRemoveProxyFromCollection(t *testing.T) {
 			t.Error("Expected proxies to be empty")
 		}
 
-		err := collection.Add(proxy)
+		err := collection.Add(proxy, false)
 		if err != nil {
 			t.Error("Expected to be able to add first proxy to collection")
 		}