Make daemoinze an option on test binary without hacks. Misc fixes for racey or broken tests. Still failing on several though.

agent/local/state_test.go

@@ -1691,7 +1691,7 @@ func TestStateProxyManagement(t *testing.T) {
 	require := require.New(t)
 	assert := assert.New(t)
 
-	_, err := state.AddProxy(&p1, "fake-token")
+	_, err := state.AddProxy(&p1, "fake-token", "")
 	require.Error(err, "should fail as the target service isn't registered")
 
 	// Sanity check done, lets add a couple of target services to the state
@@ -1710,7 +1710,7 @@ func TestStateProxyManagement(t *testing.T) {
 	require.NoError(err)
 
 	// Should work now
-	pstate, err := state.AddProxy(&p1, "fake-token")
+	pstate, err := state.AddProxy(&p1, "fake-token", "")
 	require.NoError(err)
 
 	svc := pstate.Proxy.ProxyService
@@ -1724,8 +1724,9 @@ func TestStateProxyManagement(t *testing.T) {
 
 	{
 		// Re-registering same proxy again should not pick a random port but re-use
-		// the assigned one.
-		pstateDup, err := state.AddProxy(&p1, "fake-token")
+		// the assigned one. It should also keep the same proxy token since we don't
+		// want to force restart for config change.
+		pstateDup, err := state.AddProxy(&p1, "fake-token", "")
 		require.NoError(err)
 		svcDup := pstateDup.Proxy.ProxyService
 
@@ -1736,6 +1737,8 @@ func TestStateProxyManagement(t *testing.T) {
 		assert.Equal("", svcDup.Address, "should have empty address by default")
 		// Port must be same as before
 		assert.Equal(svc.Port, svcDup.Port)
+		// Same ProxyToken
+		assert.Equal(pstate.ProxyToken, pstateDup.ProxyToken)
 	}
 
 	// Let's register a notifier now
@@ -1748,7 +1751,7 @@ func TestStateProxyManagement(t *testing.T) {
 	// Second proxy should claim other port
 	p2 := p1
 	p2.TargetServiceID = "cache"
-	pstate2, err := state.AddProxy(&p2, "fake-token")
+	pstate2, err := state.AddProxy(&p2, "fake-token", "")
 	require.NoError(err)
 	svc2 := pstate2.Proxy.ProxyService
 	assert.Contains([]int{20000, 20001}, svc2.Port)
@@ -1764,7 +1767,7 @@ func TestStateProxyManagement(t *testing.T) {
 	// Third proxy should fail as all ports are used
 	p3 := p1
 	p3.TargetServiceID = "db"
-	_, err = state.AddProxy(&p3, "fake-token")
+	_, err = state.AddProxy(&p3, "fake-token", "")
 	require.Error(err)
 
 	// Should have a notification but we'll do nothing so that the next
@@ -1775,7 +1778,7 @@ func TestStateProxyManagement(t *testing.T) {
 		"bind_port":    1234,
 		"bind_address": "0.0.0.0",
 	}
-	pstate3, err := state.AddProxy(&p3, "fake-token")
+	pstate3, err := state.AddProxy(&p3, "fake-token", "")
 	require.NoError(err)
 	svc3 := pstate3.Proxy.ProxyService
 	require.Equal("0.0.0.0", svc3.Address)
@@ -1793,7 +1796,7 @@ func TestStateProxyManagement(t *testing.T) {
 	require.NotNil(gotP3)
 	var ws memdb.WatchSet
 	ws.Add(gotP3.WatchCh)
-	pstate3, err = state.AddProxy(&p3updated, "fake-token")
+	pstate3, err = state.AddProxy(&p3updated, "fake-token", "")
 	require.NoError(err)
 	svc3 = pstate3.Proxy.ProxyService
 	require.Equal("0.0.0.0", svc3.Address)
@@ -1817,7 +1820,7 @@ func TestStateProxyManagement(t *testing.T) {
 	// Should be able to create a new proxy for that service with the port (it
 	// should have been "freed").
 	p4 := p2
-	pstate4, err := state.AddProxy(&p4, "fake-token")
+	pstate4, err := state.AddProxy(&p4, "fake-token", "")
 	require.NoError(err)
 	svc4 := pstate4.Proxy.ProxyService
 	assert.Contains([]int{20000, 20001}, svc2.Port)
@@ -1865,3 +1868,65 @@ func drainCh(ch chan struct{}) {
 		}
 	}
 }
+
+// Tests the logic for retaining tokens and ports through restore (i.e.
+// proxy-service already restored and token passed in externally)
+func TestStateProxyRestore(t *testing.T) {
+	t.Parallel()
+
+	state := local.NewState(local.Config{
+		// Wide random range to make it very unlikely to pass by chance
+		ProxyBindMinPort: 10000,
+		ProxyBindMaxPort: 20000,
+	}, log.New(os.Stderr, "", log.LstdFlags), &token.Store{})
+
+	// Stub state syncing
+	state.TriggerSyncChanges = func() {}
+
+	webSvc := structs.NodeService{
+		Service: "web",
+	}
+
+	p1 := structs.ConnectManagedProxy{
+		ExecMode:        structs.ProxyExecModeDaemon,
+		Command:         []string{"consul", "connect", "proxy"},
+		TargetServiceID: "web",
+	}
+
+	p2 := p1
+
+	require := require.New(t)
+	assert := assert.New(t)
+
+	// Add a target service
+	require.NoError(state.AddService(&webSvc, "fake-token-web"))
+
+	// Add the proxy for first time to get the proper service definition to
+	// register
+	pstate, err := state.AddProxy(&p1, "fake-token", "")
+	require.NoError(err)
+
+	// Now start again with a brand new state
+	state2 := local.NewState(local.Config{
+		// Wide random range to make it very unlikely to pass by chance
+		ProxyBindMinPort: 10000,
+		ProxyBindMaxPort: 20000,
+	}, log.New(os.Stderr, "", log.LstdFlags), &token.Store{})
+
+	// Stub state syncing
+	state2.TriggerSyncChanges = func() {}
+
+	// Register the target service
+	require.NoError(state2.AddService(&webSvc, "fake-token-web"))
+
+	// "Restore" the proxy service
+	require.NoError(state.AddService(p1.ProxyService, "fake-token-web"))
+
+	// Now we can AddProxy with the "restored" token
+	pstate2, err := state.AddProxy(&p2, "fake-token", pstate.ProxyToken)
+	require.NoError(err)
+
+	// Check it still has the same port and token as before
+	assert.Equal(pstate.ProxyToken, pstate2.ProxyToken)
+	assert.Equal(p1.ProxyService.Port, p2.ProxyService.Port)
+}

agent/proxy/daemon.go

@@ -6,6 +6,7 @@ import (
 	"log"
 	"os"
 	"os/exec"
+	"path"
 	"reflect"
 	"strconv"
 	"strings"
@@ -75,11 +76,9 @@ type Daemon struct {
 	// second.
 	pollInterval time.Duration
 
-	// daemonizePath is set only in tests to control the path to the daemonize
-	// command. The test executable itself will not respond to the consul command
-	// arguments we need to make this work so we rely on the current source being
-	// built and installed here to run the tests.
-	daemonizePath string
+	// daemonizeCmd is set only in tests to control the path and args to the
+	// daemonize command.
+	daemonizeCmd []string
 
 	// process is the started process
 	lock     sync.Mutex
@@ -112,6 +111,18 @@ func (p *Daemon) Start() error {
 	p.stopCh = stopCh
 	p.exitedCh = exitedCh
 
+	// Ensure log dirs exist
+	if p.StdoutPath != "" {
+		if err := os.MkdirAll(path.Dir(p.StdoutPath), 0755); err != nil {
+			return err
+		}
+	}
+	if p.StderrPath != "" {
+		if err := os.MkdirAll(path.Dir(p.StderrPath), 0755); err != nil {
+			return err
+		}
+	}
+
 	// Start the loop.
 	go p.keepAlive(stopCh, exitedCh)
 
@@ -270,19 +281,21 @@ func (p *Daemon) start() (*os.Process, error) {
 	daemonCmd.Path = dCmd[0]
 	// First arguments are for the stdout, stderr
 	daemonCmd.Args = append(dCmd, p.StdoutPath)
-	daemonCmd.Args = append(daemonCmd.Args, p.StdoutPath)
+	daemonCmd.Args = append(daemonCmd.Args, p.StderrPath)
 	daemonCmd.Args = append(daemonCmd.Args, p.Args...)
 	daemonCmd.Env = env
 
 	// setup stdout so we can read the PID
 	var out bytes.Buffer
 	daemonCmd.Stdout = &out
+	daemonCmd.Stderr = &out
 
 	// Run it to completion - it should exit immediately (this calls wait to
 	// ensure we don't leave the daemonize command as a zombie)
 	p.Logger.Printf("[DEBUG] agent/proxy: starting proxy: %q %#v", daemonCmd.Path,
 		daemonCmd.Args[1:])
 	if err := daemonCmd.Run(); err != nil {
+		p.Logger.Printf("[DEBUG] agent/proxy: daemonize output: %s", out.String())
 		return nil, err
 	}
 
@@ -313,15 +326,16 @@ func (p *Daemon) start() (*os.Process, error) {
 
 // daemonizeCommand returns the daemonize command.
 func (p *Daemon) daemonizeCommand() ([]string, error) {
+	// test override
+	if p.daemonizeCmd != nil {
+		return p.daemonizeCmd, nil
+	}
 	// Get the path to the current executable. This is cached once by the
 	// library so this is effectively just a variable read.
 	execPath, err := os.Executable()
 	if err != nil {
 		return nil, err
 	}
-	if p.daemonizePath != "" {
-		execPath = p.daemonizePath
-	}
 	return []string{execPath, "connect", "daemonize"}, nil
 }
 

agent/proxy/manager.go

@@ -102,6 +102,10 @@ type Manager struct {
 	// proxies (unlikely scenario).
 	lastSnapshot *snapshot
 
+	// daemonizeCmd is set only in tests to control the path and args to the
+	// daemonize command.
+	daemonizeCmd []string
+
 	proxies map[string]Proxy
 }
 
@@ -405,6 +409,7 @@ func (m *Manager) newProxy(mp *local.ManagedProxy) (Proxy, error) {
 		proxy.Args = command // idx 0 is path but preserved since it should be
 		proxy.ProxyId = id
 		proxy.ProxyToken = mp.ProxyToken
+		proxy.daemonizeCmd = m.daemonizeCmd
 		return proxy, nil
 
 	default:

agent/proxy/manager_test.go

@@ -361,6 +361,10 @@ func testManager(t *testing.T) (*Manager, func()) {
 	td, closer := testTempDir(t)
 	m.DataDir = td
 
+	// Override daemonize command to use the built-in test binary. Note that Args
+	// includes the binary path as first arg.
+	m.daemonizeCmd = helperProcess("daemonize").Args
+
 	return m, func() { closer() }
 }
 
@@ -368,8 +372,9 @@ func testManager(t *testing.T) (*Manager, func()) {
 // (expected to be from the helperProcess function call). It returns the
 // ID for deregistration.
 func testStateProxy(t *testing.T, state *local.State, service string, cmd *exec.Cmd) string {
-	command := []string{cmd.Path}
-	command = append(command, cmd.Args...)
+	// Note that exec.Command already ensures the command name is the first
+	// argument in the list so no need to append again
+	command := cmd.Args
 
 	require.NoError(t, state.AddService(&structs.NodeService{
 		Service: service,

agent/proxy/process.go

@@ -1,15 +1,15 @@
 package proxy
 
 import (
-	"log"
 	"strings"
 	"time"
 )
 
 // isProcessAlreadyFinishedErr does a janky comparison with an error string
-// defined in os/exec_unix.go and os/exec_windows.go which we encounter due to races.
-// These case tests to fail since Stop returns an error sometimes so we should
-// notice if this string stops matching the error in a future go version.
+// defined in os/exec_unix.go and os/exec_windows.go which we encounter due to
+// races with polling the external process. These case tests to fail since Stop
+// returns an error sometimes so we should notice if this string stops matching
+// the error in a future go version.
 func isProcessAlreadyFinishedErr(err error) bool {
 	return strings.Contains(err.Error(), "os: process already finished")
 }
@@ -31,7 +31,6 @@ func externalWait(pid int, pollInterval time.Duration) (<-chan struct{}, func())
 				return
 			default:
 			}
-			log.Printf("checking pid %d", pid)
 			if _, err := findProcess(pid); err != nil {
 				close(ch)
 				return

agent/proxy/proxy_test.go

@@ -10,6 +10,9 @@ import (
 	"strconv"
 	"testing"
 	"time"
+
+	"github.com/hashicorp/consul/command/connect/daemonize"
+	"github.com/mitchellh/cli"
 )
 
 // testLogger is a logger that can be used by tests that require a
@@ -55,29 +58,17 @@ func helperProcess(s ...string) *exec.Cmd {
 // interactions. The Daemon has it's Path, Args and stdio paths populated but
 // other fields might need to be set depending on test requirements.
 //
-// NOTE: this relies on a sufficiently recent version on consul being installed
-// in your path so that the daemonize command can be used. That's gross but hard
-// to see how we can do better given that tests are separate binaries and we
-// need consul's daemonize mode to work correctly. I considered hacks around
-// building the local tree and getting the absolute path to the resulting binary
-// but that seems gross in a different way. This is the same or weaker
-// assumption our `api` test suit makes already...
+// This relies on the TestMainInterceptDaemonize hack being active.
 func helperProcessDaemon(s ...string) *Daemon {
 	cs := []string{os.Args[0], "-test.run=TestHelperProcess", "--", helperProcessSentinel}
 	cs = append(cs, s...)
 
-	path, err := exec.LookPath("consul")
-	if err != nil || path == "" {
-		panic("consul not found on $PATH - download and install " +
-			"consul or skip this test")
-	}
-
 	return &Daemon{
 		Path:         os.Args[0],
 		Args:         cs,
 		StdoutPath:   "_", // dev null them for now
 		StderrPath:   "_",
-		daemonizePath: path,
+		daemonizeCmd: helperProcess("daemonize").Args,
 	}
 }
 
@@ -214,6 +205,24 @@ func TestHelperProcess(t *testing.T) {
 			time.Sleep(time.Hour)
 		}
 
+	case "daemonize":
+		// Run daemonize!
+		ui := &cli.BasicUi{Writer: os.Stdout, ErrorWriter: os.Stderr}
+		cli := &cli.CLI{
+			Args: append([]string{"daemonize"}, args...),
+			Commands: map[string]cli.CommandFactory{
+				"daemonize": func() (cli.Command, error) {
+					return daemonize.New(ui), nil
+				},
+			},
+		}
+
+		exitCode, err := cli.Run()
+		if err != nil {
+			log.Printf("[ERR] running hijacked daemonize command: %s", err)
+		}
+		os.Exit(exitCode)
+
 	default:
 		fmt.Fprintf(os.Stderr, "Unknown command: %q\n", cmd)
 		os.Exit(2)

api/agent_test.go

@@ -1140,7 +1140,7 @@ func TestAPI_AgentConnectProxyConfig(t *testing.T) {
 		Port: 8000,
 		Connect: &AgentServiceConnect{
 			Proxy: &AgentServiceConnectProxy{
-				Command: []string{"consul connect proxy"},
+				Command: []string{"consul", "connect", "proxy"},
 				Config: map[string]interface{}{
 					"foo": "bar",
 				},
@@ -1157,7 +1157,7 @@ func TestAPI_AgentConnectProxyConfig(t *testing.T) {
 		ProxyServiceID:    "foo-proxy",
 		TargetServiceID:   "foo",
 		TargetServiceName: "foo",
-		ContentHash:       "93baee1d838888ae",
+		ContentHash:       "2a29f8237db69d0e",
 		ExecMode:          "daemon",
 		Command:           []string{"consul", "connect", "proxy"},
 		Config: map[string]interface{}{

command/connect/daemonize/daemonize.go

@@ -23,17 +23,15 @@ type cmd struct {
 }
 
 func (c *cmd) Run(args []string) int {
-	// Ignore initial `consul connect daemonize`
-	offset := 3
-	numArgs := len(os.Args) - offset
-	if numArgs < 4 {
+	numArgs := len(args)
+	if numArgs < 3 {
 		c.UI.Error("Need at least 3 arguments; stdoutPath, stdinPath, " +
 			"executablePath [arguments...]")
 		os.Exit(1)
 	}
 
-	c.stdoutPath, c.stderrPath = os.Args[offset], os.Args[offset+1]
-	c.cmdArgs = os.Args[offset+2:] // includes the executable as arg 0 as expected
+	c.stdoutPath, c.stderrPath = args[0], args[1]
+	c.cmdArgs = args[2:] // includes the executable as arg 0 as expected
 
 	// Open log files if specified
 	var stdoutF, stderrF *os.File