Sanity check that we are never trying to self-exec a test binary. Add daemonize bypass for TestAgent so that we don't have to jump through ridiculous self-execution hooks for every package that might possibly invoke a managed proxy

agent/agent.go

@@ -366,6 +366,7 @@ func (a *Agent) Start() error {
 	a.proxyManager = proxy.NewManager()
 	a.proxyManager.State = a.State
 	a.proxyManager.Logger = a.logger
+	a.proxyManager.DisableDetach = a.config.ConnectDisableDetachedDaemons
 	if a.config.DataDir != "" {
 		// DataDir is required for all non-dev mode agents, but we want
 		// to allow setting the data dir for demos and so on for the agent,

agent/config/runtime.go

@@ -621,6 +621,16 @@ type RuntimeConfig struct {
 	// that.
 	ConnectEnabled bool
 
+	// ConnectDisableDetachedDaemons is not exposed publically and is meant for
+	// testing where having processes outlive the test is inconvenient. It also
+	// allows tests outside of the `agent/proxy` package to ignore the unpleasant
+	// details of self-executing the test binary in order to correctly detach a
+	// process. It's set to true by default in TestAgent and setting it to false
+	// in any test requires several hoops to be jumped through to allow the test
+	// binary to behave as a daemonizer and for the agent to be configured to use
+	// the right invocation of the binary for it.
+	ConnectDisableDetachedDaemons bool
+
 	// ConnectProxyBindMinPort is the inclusive start of the range of ports
 	// allocated to the agent for starting proxy listeners on where no explicit
 	// port is specified.

agent/config/runtime_test.go

@@ -4199,6 +4199,7 @@ func TestSanitize(t *testing.T) {
     "ClientAddrs": [],
     "ConnectCAConfig": {},
     "ConnectCAProvider": "",
+    "ConnectDisableDetachedDaemons": false,
     "ConnectEnabled": false,
     "ConnectProxyBindMaxPort": 0,
     "ConnectProxyBindMinPort": 0,

agent/proxy/daemon.go

@@ -62,6 +62,14 @@ type Daemon struct {
 	// should be written to.
 	StdoutPath, StderrPath string
 
+	// DisableDetach is used by tests that don't actually care about detached
+	// child behaviour (i.e. outside proxy package) to bypass detaching and
+	// daemonizing Daemons. This makes tests much simpler as they don't need to
+	// implement a test-binary mode to enable self-exec daemonizing etc. and there
+	// are fewer risks of detached processes being spawned and then not killed in
+	// face of missed teardown/panic/interrupt of test runs etc.
+	DisableDetach bool
+
 	// gracefulWait can be set for tests and controls how long Stop() will wait
 	// for process to terminate before killing. If not set defaults to 5 seconds.
 	// If this is lowered for tests, it must remain higher than pollInterval
@@ -268,6 +276,16 @@ func (p *Daemon) start() (*os.Process, error) {
 		p.Args = []string{p.Path}
 	}
 
+	// If we are running in a test mode that disabled detaching daemon processes
+	// for simplicity, just exec the thing directly. This should never be the case
+	// in real life since this config is not publically exposed but makes testing
+	// way cleaner outside of this package.
+	if p.DisableDetach {
+		cmd := exec.Command(p.Path, p.Args[1:]...)
+		err := cmd.Start()
+		return cmd.Process, err
+	}
+
 	// Watch closely, we now swap out the exec.Cmd args for ones to run the same
 	// command via the connect daemonize command which takes care of correctly
 	// "double forking" to ensure the child is fully detached and adopted by the
@@ -326,16 +344,31 @@ func (p *Daemon) start() (*os.Process, error) {
 
 // daemonizeCommand returns the daemonize command.
 func (p *Daemon) daemonizeCommand() ([]string, error) {
-	// test override
+	// 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.
+	// 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
 	}
+	// Sanity check to prevent runaway test invocations because test didn't setup
+	// daemonizeCmd correctly. This is kinda jank but go doesn't have a way to
+	// detect and alter behaviour in test binaries by design. In this case though
+	// we really need to never allow tests to self-execute which can cause
+	// recursive explosion of test runs. This check seems safe for current go
+	// tooling based on https://github.com/golang/go/issues/12120. If you hit
+	// this, you need to find a way to configure your test
+	// agent/proxyManager/Daemon to use agent/proxy/TestHelperProcess to run
+	// daemonize in a safe way. TestAgent should do this automatically by default.
+	if strings.HasSuffix(execPath, ".test") ||
+		strings.HasSuffix(execPath, ".test.exe") {
+		panic("test did not setup daemonizeCmd override and will dangerously" +
+			" self-execute the test binary.")
+	}
+
 	return []string{execPath, "connect", "daemonize"}, nil
 }
 

agent/proxy/manager.go

@@ -84,6 +84,16 @@ type Manager struct {
 	CoalescePeriod  time.Duration
 	QuiescentPeriod time.Duration
 
+	// DisableDetach is used by tests that don't actually care about detached
+	// child behaviour (i.e. outside proxy package) to bypass detaching and
+	// daemonizing Daemons. This makes tests much simpler as they don't need to
+	// implement a test-binary mode to enable self-exec daemonizing etc. and there
+	// are fewer risks of detached processes being spawned and then not killed in
+	// face of missed teardown/panic/interrupt of test runs etc. It's public since
+	// it needs to be configurable from the agent package when setting up the
+	// proxyManager instance.
+	DisableDetach bool
+
 	// lock is held while reading/writing any internal state of the manager.
 	// cond is a condition variable on lock that is broadcasted for runState
 	// changes.
@@ -422,6 +432,7 @@ func (m *Manager) newProxy(mp *local.ManagedProxy) (Proxy, error) {
 		proxy.ProxyId = id
 		proxy.ProxyToken = mp.ProxyToken
 		proxy.daemonizeCmd = m.daemonizeCmd
+		proxy.DisableDetach = m.DisableDetach
 		return proxy, nil
 
 	default:

agent/testagent.go

@@ -376,6 +376,14 @@ func TestConfig(sources ...config.Source) *config.RuntimeConfig {
 		fmt.Println("WARNING:", w)
 	}
 
+	// Set internal flag to simplify connect daemon execution. We test the full
+	// daemonization behaviour explicitly in `proxy` package, everywhere else it's
+	// just super painful to setup self-executable daemonize commands etc. For now
+	// this is not overridable because it's simpler not to expose this config
+	// publically at all but we could revisit that later if there is a legitimate
+	// reason to want to test full detached daemon behaviour with a TestAgent.
+	cfg.ConnectDisableDetachedDaemons = true
+
 	return &cfg
 }