agent/proxy: detect config change to stop/start proxies

agent/proxy/daemon.go

@@ -5,6 +5,7 @@ import (
 	"log"
 	"os"
 	"os/exec"
+	"reflect"
 	"sync"
 	"time"
 )
@@ -198,3 +199,18 @@ func (p *Daemon) Stop() error {
 	return err
 	//return p.Command.Process.Kill()
 }
+
+// Equal implements Proxy to check for equality.
+func (p *Daemon) Equal(raw Proxy) bool {
+	p2, ok := raw.(*Daemon)
+	if !ok {
+		return false
+	}
+
+	// We compare equality on a subset of the command configuration
+	return p.ProxyToken == p2.ProxyToken &&
+		p.Command.Path == p2.Command.Path &&
+		p.Command.Dir == p2.Command.Dir &&
+		reflect.DeepEqual(p.Command.Args, p2.Command.Args) &&
+		reflect.DeepEqual(p.Command.Env, p2.Command.Env)
+}

agent/proxy/daemon_test.go

@@ -3,6 +3,7 @@ package proxy
 import (
 	"io/ioutil"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"testing"
 
@@ -97,3 +98,93 @@ func TestDaemonRestart(t *testing.T) {
 	// File should re-appear because the process is restart
 	waitFile()
 }
+
+func TestDaemonEqual(t *testing.T) {
+	cases := []struct {
+		Name     string
+		D1, D2   Proxy
+		Expected bool
+	}{
+		{
+			"Different type",
+			&Daemon{
+				Command: &exec.Cmd{},
+			},
+			&Noop{},
+			false,
+		},
+
+		{
+			"Nil",
+			&Daemon{
+				Command: &exec.Cmd{},
+			},
+			nil,
+			false,
+		},
+
+		{
+			"Equal",
+			&Daemon{
+				Command: &exec.Cmd{},
+			},
+			&Daemon{
+				Command: &exec.Cmd{},
+			},
+			true,
+		},
+
+		{
+			"Different path",
+			&Daemon{
+				Command: &exec.Cmd{Path: "/foo"},
+			},
+			&Daemon{
+				Command: &exec.Cmd{Path: "/bar"},
+			},
+			false,
+		},
+
+		{
+			"Different dir",
+			&Daemon{
+				Command: &exec.Cmd{Dir: "/foo"},
+			},
+			&Daemon{
+				Command: &exec.Cmd{Dir: "/bar"},
+			},
+			false,
+		},
+
+		{
+			"Different args",
+			&Daemon{
+				Command: &exec.Cmd{Args: []string{"foo"}},
+			},
+			&Daemon{
+				Command: &exec.Cmd{Args: []string{"bar"}},
+			},
+			false,
+		},
+
+		{
+			"Different token",
+			&Daemon{
+				Command:    &exec.Cmd{},
+				ProxyToken: "one",
+			},
+			&Daemon{
+				Command:    &exec.Cmd{},
+				ProxyToken: "two",
+			},
+			false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			actual := tc.D1.Equal(tc.D2)
+			require.Equal(t, tc.Expected, actual)
+		})
+	}
+}

agent/proxy/manager.go

@@ -225,22 +225,35 @@ func (m *Manager) sync() {
 	for id, proxy := range m.proxies {
 		// Get the proxy.
 		stateProxy, ok := state[id]
-		if !ok {
+		if ok {
-			// Proxy is deregistered. Remove it from our map and stop it
+			// Remove the proxy from the state so we don't start it new.
-			delete(m.proxies, id)
+			delete(state, id)
-			if err := proxy.Stop(); err != nil {
+
-				m.Logger.Printf("[ERROR] agent/proxy: failed to stop deregistered proxy for %q: %s", id, err)
+			// Make the proxy so we can compare. This does not start it.
+			proxy2, err := m.newProxy(stateProxy)
+			if err != nil {
+				m.Logger.Printf("[ERROR] agent/proxy: failed to initialize proxy for %q: %s", id, err)
+				continue
 			}
 
-			continue
+			// If the proxies are equal, then do nothing
+			if proxy.Equal(proxy2) {
+				continue
+			}
+
+			// Proxies are not equal, so we should stop it. We add it
+			// back to the state here (unlikely case) so the loop below starts
+			// the new one.
+			state[id] = stateProxy
+
+			// Continue out of `if` as if proxy didn't exist so we stop it
 		}
 
-		// Proxy is in the state. Always delete it so that the remainder
+		// Proxy is deregistered. Remove it from our map and stop it
-		// are NEW proxies that we start after this loop.
+		delete(m.proxies, id)
-		delete(state, id)
+		if err := proxy.Stop(); err != nil {
-
+			m.Logger.Printf("[ERROR] agent/proxy: failed to stop deregistered proxy for %q: %s", id, err)
-		// TODO: diff and restart if necessary
+		}
-		println("DIFF", id, stateProxy)
 	}
 
 	// Remaining entries in state are new proxies. Start them!

agent/proxy/manager_test.go

@@ -134,6 +134,53 @@ func TestManagerRun_syncDelete(t *testing.T) {
 	})
 }
 
+func TestManagerRun_syncUpdate(t *testing.T) {
+	t.Parallel()
+
+	state := local.TestState(t)
+	m := NewManager()
+	m.State = state
+	defer m.Kill()
+
+	// Start the manager
+	go m.Run()
+
+	// Add the first proxy
+	td, closer := testTempDir(t)
+	defer closer()
+	path := filepath.Join(td, "file")
+	testStateProxy(t, state, "web", helperProcess("restart", path))
+
+	// We should see the path appear shortly
+	retry.Run(t, func(r *retry.R) {
+		_, err := os.Stat(path)
+		if err == nil {
+			return
+		}
+		r.Fatalf("error waiting for path: %s", err)
+	})
+
+	// Update the proxy with a new path
+	oldPath := path
+	path = path + "2"
+	testStateProxy(t, state, "web", helperProcess("restart", path))
+	retry.Run(t, func(r *retry.R) {
+		_, err := os.Stat(path)
+		if err == nil {
+			return
+		}
+		r.Fatalf("error waiting for path: %s", err)
+	})
+
+	// Old path should be gone
+	retry.Run(t, func(r *retry.R) {
+		_, err := os.Stat(oldPath)
+		if err == nil {
+			r.Fatalf("old path exists")
+		}
+	})
+}
+
 // testStateProxy registers a proxy with the given local state and the command
 // (expected to be from the helperProcess function call). It returns the
 // ID for deregistration.

agent/proxy/noop.go

@@ -3,5 +3,6 @@ package proxy
 // Noop implements Proxy and does nothing.
 type Noop struct{}
 
-func (p *Noop) Start() error { return nil }
+func (p *Noop) Start() error     { return nil }
-func (p *Noop) Stop() error  { return nil }
+func (p *Noop) Stop() error      { return nil }
+func (p *Noop) Equal(Proxy) bool { return true }

agent/proxy/proxy.go

@@ -31,4 +31,12 @@ type Proxy interface {
 	// it should disallow Start from working again. If the proxy is already
 	// stopped, this should not return an error.
 	Stop() error
+
+	// Equal returns true if the argument is equal to the proxy being called.
+	// This is called by the manager to determine if a change in configuration
+	// results in a proxy that needs to be restarted or not. If Equal returns
+	// false, then the manager will stop the old proxy and start the new one.
+	// If Equal returns true, the old proxy will remain running and the new
+	// one will be ignored.
+	Equal(Proxy) bool
 }