diff --git a/agent/proxy/daemon.go b/agent/proxy/daemon.go index 231f25cb8f..c43eb48a12 100644 --- a/agent/proxy/daemon.go +++ b/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) +} diff --git a/agent/proxy/daemon_test.go b/agent/proxy/daemon_test.go index 22948bdaf0..a1638b2665 100644 --- a/agent/proxy/daemon_test.go +++ b/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) + }) + } +} diff --git a/agent/proxy/manager.go b/agent/proxy/manager.go index 765e9f0220..d2ab8a1061 100644 --- a/agent/proxy/manager.go +++ b/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 { - // Proxy is deregistered. Remove it from our map and stop it - delete(m.proxies, id) - if err := proxy.Stop(); err != nil { - m.Logger.Printf("[ERROR] agent/proxy: failed to stop deregistered proxy for %q: %s", id, err) + if ok { + // Remove the proxy from the state so we don't start it new. + delete(state, id) + + // 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 - // Proxy is in the state. Always delete it so that the remainder - // are NEW proxies that we start after this loop. - delete(state, id) + // Continue out of `if` as if proxy didn't exist so we stop it + } - // TODO: diff and restart if necessary - println("DIFF", id, stateProxy) + // Proxy is deregistered. Remove it from our map and stop it + delete(m.proxies, id) + if err := proxy.Stop(); err != nil { + m.Logger.Printf("[ERROR] agent/proxy: failed to stop deregistered proxy for %q: %s", id, err) + } } // Remaining entries in state are new proxies. Start them! diff --git a/agent/proxy/manager_test.go b/agent/proxy/manager_test.go index eba7a674a8..97086d4911 100644 --- a/agent/proxy/manager_test.go +++ b/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. diff --git a/agent/proxy/noop.go b/agent/proxy/noop.go index 9b35a24272..9ce0135546 100644 --- a/agent/proxy/noop.go +++ b/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) Stop() error { return nil } +func (p *Noop) Start() error { return nil } +func (p *Noop) Stop() error { return nil } +func (p *Noop) Equal(Proxy) bool { return true } diff --git a/agent/proxy/proxy.go b/agent/proxy/proxy.go index a07bb5681d..549a6ee26d 100644 --- a/agent/proxy/proxy.go +++ b/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 }