Bugfix: don't stop proxies when using hostnames to specify listen addresses (#631)

* Bugfix: don't stop proxies when using hostnames to specify listen addresses

Before this change we'd stop existing proxies when populating a set of proxies
when the listen addresses were specified using hostnames instead of IP
addresses.

This is very surprising as we encourage users to populate proxies at the begin
of an application start:
> A /populate call can be included for example at application start to ensure
> all required proxies exist. It is safe to make this call several times, since
> proxies will be untouched as long as their fields are consistent with the new
> data.

This is (currently) only partially true as it doesn't work when using hostnames.

For upstream services this isn't a problem because we never store the resolved address.

The change introduces a "Differs" function on a proxy to determine if two
proxies are different in terms of listening address and upstream service. This
is now used both when changing collections as well as updating individual
proxies.

Closes #131.

* Update changelog

Author: robinbrandt

CHANGELOG.md

@@ -1,6 +1,8 @@
 # [Unreleased]
 
 - Update go version to 1.23.0 (#628)
+- Do not restart proxies when using hostnames to specify listen address when updating a proxy
+  and populating a collection (#631, @robinbrandt)
 
 # [2.11.0] - 2024-10-16
 

api_test.go

@@ -256,6 +256,7 @@ func TestPopulateExistingProxy(t *testing.T) {
 		if err != nil {
 			t.Fatal("Unable to create proxy:", err)
 		}
+
 		_, err = client.CreateProxy("two", "localhost:7373", "localhost:7474")
 		if err != nil {
 			t.Fatal("Unable to create proxy:", err)
@@ -270,7 +271,7 @@ func TestPopulateExistingProxy(t *testing.T) {
 		testProxies, err := client.Populate([]tclient.Proxy{
 			{
 				Name:     "one",
-				Listen:   "127.0.0.1:7070",
+				Listen:   "localhost:7070", // intentional: this should be resolved to 127.0.0.1:7070
 				Upstream: "localhost:7171",
 				Enabled:  true,
 			},

proxy.go

@@ -81,7 +81,12 @@ func (proxy *Proxy) Update(input *Proxy) error {
 	proxy.Lock()
 	defer proxy.Unlock()
 
-	if input.Listen != proxy.Listen || input.Upstream != proxy.Upstream {
+	differs, err := proxy.Differs(input)
+	if err != nil {
+		return err
+	}
+
+	if differs {
 		stop(proxy)
 		proxy.Listen = input.Listen
 		proxy.Upstream = input.Upstream
@@ -131,6 +136,19 @@ func (proxy *Proxy) close() {
 	}
 }
 
+func (proxy *Proxy) Differs(other *Proxy) (bool, error) {
+	newResolvedListen, err := net.ResolveTCPAddr("tcp", other.Listen)
+	if err != nil {
+		return false, err
+	}
+
+	if proxy.Listen != newResolvedListen.String() || proxy.Upstream != other.Upstream {
+		return true, nil
+	}
+
+	return false, nil
+}
+
 // This channel is to kill the blocking Accept() call below by closing the
 // net.Listener.
 func (proxy *Proxy) freeBlocker(acceptTomb *tomb.Tomb) {

proxy_collection.go

@@ -43,27 +43,32 @@ func (collection *ProxyCollection) Add(proxy *Proxy, start bool) error {
 	return nil
 }
 
-func (collection *ProxyCollection) AddOrReplace(proxy *Proxy, start bool) error {
+func (collection *ProxyCollection) AddOrReplace(proxy *Proxy, start bool) (*Proxy, error) {
 	collection.Lock()
 	defer collection.Unlock()
 
 	if existing, exists := collection.proxies[proxy.Name]; exists {
-		if existing.Listen == proxy.Listen && existing.Upstream == proxy.Upstream {
-			return nil
+		differs, err := existing.Differs(proxy)
+		if err != nil {
+			return nil, err
+		}
+
+		if !differs {
+			return existing, nil
 		}
 		existing.Stop()
 	}
 
 	if start {
 		err := proxy.Start()
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
 
 	collection.proxies[proxy.Name] = proxy
 
-	return nil
+	return proxy, nil
 }
 
 func (collection *ProxyCollection) PopulateJson(
@@ -98,12 +103,12 @@ func (collection *ProxyCollection) PopulateJson(
 
 	for i := range input {
 		proxy := NewProxy(server, input[i].Name, input[i].Listen, input[i].Upstream)
-		err = collection.AddOrReplace(proxy, *input[i].Enabled)
+		addedOrReplaced, err := collection.AddOrReplace(proxy, *input[i].Enabled)
 		if err != nil {
 			return proxies, err
 		}
 
-		proxies = append(proxies, proxy)
+		proxies = append(proxies, addedOrReplaced)
 	}
 	return proxies, err
 }

proxy_test.go

@@ -3,8 +3,10 @@ package toxiproxy_test
 import (
 	"bytes"
 	"encoding/hex"
+	"errors"
 	"io"
 	"net"
+	"os"
 	"testing"
 	"time"
 
@@ -177,6 +179,62 @@ func TestProxyUpdate(t *testing.T) {
 	})
 }
 
+func TestProxyUpdateWithHostname(t *testing.T) {
+	testhelper.WithTCPServer(t, func(upstream string, response chan []byte) {
+		proxy := NewTestProxy("test", upstream)
+		err := proxy.Start()
+		if err != nil {
+			t.Error("Proxy failed to start", err)
+		}
+		AssertProxyUp(t, proxy.Listen, true)
+
+		connectionLost := make(chan bool)
+
+		// Start a goroutine to check if connection is maintained
+		go func() {
+			conn, err := net.Dial("tcp", proxy.Listen)
+			if err != nil {
+				t.Error("Failed to connect to proxy", err)
+			}
+			defer conn.Close()
+
+			// Try to read from the connection
+			buf := make([]byte, 1024)
+			conn.SetReadDeadline(time.Now().Add(500 * time.Millisecond))
+			_, err = conn.Read(buf)
+			if err != nil && !errors.Is(err, os.ErrDeadlineExceeded) {
+				connectionLost <- true
+				return
+			}
+
+			connectionLost <- false
+		}()
+
+		_, port, err := net.SplitHostPort(proxy.Listen)
+		if err != nil {
+			t.Error("Failed to split host and port", err)
+		}
+
+		input := &toxiproxy.Proxy{
+			Listen:   net.JoinHostPort("localhost", port),
+			Upstream: proxy.Upstream,
+			Enabled:  true,
+		}
+		err = proxy.Update(input)
+		if err != nil {
+			t.Error("Failed to update proxy", err)
+		}
+
+		// Check if the connection was lost during the update
+		if lost := <-connectionLost; lost {
+			t.Error("Connection was lost during proxy update")
+		}
+
+		// Verify proxy is still up after the update
+		AssertProxyUp(t, proxy.Listen, true)
+	})
+}
+
 func TestRestartFailedToStartProxy(t *testing.T) {
 	testhelper.WithTCPServer(t, func(upstream string, response chan []byte) {
 		proxy := NewTestProxy("test", upstream)
@@ -207,3 +265,28 @@ func TestRestartFailedToStartProxy(t *testing.T) {
 		AssertProxyUp(t, proxy.Listen, false)
 	})
 }
+
+func TestProxyDiffers(t *testing.T) {
+	testhelper.WithTCPServer(t, func(upstream string, response chan []byte) {
+		proxy := NewTestProxy("test", upstream)
+		proxy.Start()
+		_, port, err := net.SplitHostPort(proxy.Listen)
+		if err != nil {
+			t.Error("Failed to split host and port", err)
+		}
+		otherProxy := &toxiproxy.Proxy{
+			Name:     "other",
+			Listen:   net.JoinHostPort("localhost", port),
+			Upstream: upstream,
+			Enabled:  true,
+		}
+
+		differs, err := proxy.Differs(otherProxy)
+		if err != nil {
+			t.Error("Failed to check if proxy differs", err)
+		}
+		if differs {
+			t.Error("Proxy should not differ ")
+		}
+	})
+}