From 9cea27c66e84f0ec8fba0e1ec8d7faf8db1f038c Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 24 May 2018 15:45:39 +0100 Subject: [PATCH] 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 | 1 + agent/config/runtime.go | 10 +++++++++ agent/config/runtime_test.go | 1 + agent/proxy/daemon.go | 39 +++++++++++++++++++++++++++++++++--- agent/proxy/manager.go | 11 ++++++++++ agent/testagent.go | 8 ++++++++ 6 files changed, 67 insertions(+), 3 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 40a50dcb1..79bbdca60 100644 --- a/agent/agent.go +++ b/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, diff --git a/agent/config/runtime.go b/agent/config/runtime.go index 8baf02ab2..464072069 100644 --- a/agent/config/runtime.go +++ b/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. diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 2e72b4639..f232407b6 100644 --- a/agent/config/runtime_test.go +++ b/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, diff --git a/agent/proxy/daemon.go b/agent/proxy/daemon.go index aac992e16..c51e42181 100644 --- a/agent/proxy/daemon.go +++ b/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 } diff --git a/agent/proxy/manager.go b/agent/proxy/manager.go index 266459968..9d7295d20 100644 --- a/agent/proxy/manager.go +++ b/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: diff --git a/agent/testagent.go b/agent/testagent.go index 007d01322..0ca7cc1c3 100644 --- a/agent/testagent.go +++ b/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 }