From 49035e2e8ed74f1054a2c77969b744e136a428e1 Mon Sep 17 00:00:00 2001 From: Benson Wong Date: Wed, 18 Jun 2025 11:09:13 -0700 Subject: [PATCH] Append custom env vars instead of replace in Process (#171) Append custom env vars instead of replace in Process (#168, #169) PR #162 refactored the default configuration code. This introduced a subtle bug where `env` became `[]string{}` instead of the default of `nil`. In golang, `exec.Cmd.Env == nil` means to use the "current process's environment". By setting it to `[]string{}` as a default the Process's environment was emptied out which caused an array of strange and difficult to troubleshoot behaviour. See issues #168 and #169 This commit changes the behaviour to append model configured environment variables to the default list rather than replace them. --- proxy/process.go | 7 ++++--- proxy/process_test.go | 28 +++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/proxy/process.go b/proxy/process.go index fcbeaab..2fd1bf8 100644 --- a/proxy/process.go +++ b/proxy/process.go @@ -189,18 +189,19 @@ func (p *Process) start() error { p.waitStarting.Add(1) defer p.waitStarting.Done() cmdContext, ctxCancelUpstream := context.WithCancel(context.Background()) - p.proxyLogger.Debugf("<%s> Executing start command: %s", p.ID, strings.Join(args, " ")) + p.cmd = exec.CommandContext(cmdContext, args[0], args[1:]...) p.cmd.Stdout = p.processLogger p.cmd.Stderr = p.processLogger - p.cmd.Env = p.config.Env - + p.cmd.Env = append(p.cmd.Environ(), p.config.Env...) p.cmd.Cancel = p.cmdStopUpstreamProcess p.cmd.WaitDelay = p.gracefulStopTimeout p.cancelUpstream = ctxCancelUpstream p.cmdWaitChan = make(chan struct{}) p.failedStartCount++ // this will be reset to zero when the process has successfully started + + p.proxyLogger.Debugf("<%s> Executing start command: %s, env: %s", p.ID, strings.Join(args, " "), strings.Join(p.config.Env, ", ")) err = p.cmd.Start() // Set process state to failed diff --git a/proxy/process_test.go b/proxy/process_test.go index a87b4db..50a8c15 100644 --- a/proxy/process_test.go +++ b/proxy/process_test.go @@ -394,6 +394,9 @@ func TestProcess_StopImmediately(t *testing.T) { // Test that SIGKILL is sent when gracefulStopTimeout is reached and properly terminates // the upstream command func TestProcess_ForceStopWithKill(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping SIGTERM test on Windows ") + } expectedMessage := "test_sigkill" binaryPath := getSimpleResponderPath() @@ -405,7 +408,6 @@ func TestProcess_ForceStopWithKill(t *testing.T) { Cmd: fmt.Sprintf("%s --port %d --respond %s --silent --ignore-sig-term", binaryPath, port, expectedMessage), Proxy: fmt.Sprintf("http://127.0.0.1:%d", port), CheckEndpoint: "/health", - CmdStop: "taskkill /f /t /pid ${PID}", } process := NewProcess("stop_immediate", 2, config, debugLogger, debugLogger) @@ -465,3 +467,27 @@ func TestProcess_StopCmd(t *testing.T) { process.StopImmediately() assert.Equal(t, process.CurrentState(), StateStopped) } + +func TestProcess_EnvironmentSetCorrectly(t *testing.T) { + expectedMessage := "test_env_not_emptied" + config := getTestSimpleResponderConfig(expectedMessage) + + // ensure that the the default config does not blank out the inherited environment + configWEnv := config + + // ensure the additiona variables are appended to the process' environment + configWEnv.Env = append(configWEnv.Env, "TEST_ENV1=1", "TEST_ENV2=2") + + process1 := NewProcess("env_test", 2, config, debugLogger, debugLogger) + process2 := NewProcess("env_test", 2, configWEnv, debugLogger, debugLogger) + + process1.start() + defer process1.Stop() + process2.start() + defer process2.Stop() + + assert.NotZero(t, len(process1.cmd.Environ())) + assert.NotZero(t, len(process2.cmd.Environ())) + assert.Equal(t, len(process1.cmd.Environ())+2, len(process2.cmd.Environ()), "process2 should have 2 more environment variables than process1") + +}