Skip to content

Commit 882fc0c

Browse files
authored
fix: host builder passes environment variables to spawned subprocesses (#3577)
The runPython() and runGo() functions in runner.go were setting cmd.Env by appending to a nil slice, which discarded the parent environment and any func.yaml/CLI environment variables. Now both functions inherit the parent process environment via os.Environ() and inject Run.Envs using Interpolate(), matching the behavior of container-based builders.
1 parent b48840e commit 882fc0c

File tree

2 files changed

+207
-13
lines changed

2 files changed

+207
-13
lines changed

pkg/functions/runner.go

Lines changed: 57 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,12 @@ import (
1111
"os"
1212
"os/exec"
1313
"path/filepath"
14+
"runtime"
1415
"time"
1516
)
1617

1718
const (
18-
defaultRunHost = "127.0.0.1" // TODO allow to be altered via a runOpt
19+
defaultRunHost = "127.0.0.1"
1920
defaultRunPort = "8080"
2021
readinessEndpoint = "/health/readiness"
2122
)
@@ -169,13 +170,23 @@ func runGo(ctx context.Context, job *Job) (err error) {
169170
cmd.Stdout = os.Stdout
170171
cmd.Stderr = os.Stderr
171172

172-
// cmd.Cancel = stop // TODO: use when we upgrade to go 1.20
173+
cmd.Cancel = func() error {
174+
if runtime.GOOS == "windows" {
175+
// Interrupt is not implemented on windows apparently
176+
return cmd.Process.Kill()
177+
}
178+
return cmd.Process.Signal(os.Interrupt)
179+
}
180+
// force kill after delay if Interrupt signal did not work to not hang indefinitely
181+
cmd.WaitDelay = 5 * time.Second
173182

174-
// See the 1.19 [release notes](https://tip.golang.org/doc/go1.19) which state:
175-
// A Cmd with a non-empty Dir field and nil Env now implicitly sets the PWD environment variable for the subprocess to match Dir.
176-
// The new method Cmd.Environ reports the environment that would be used to run the command, including the implicitly set PWD variable.
177-
// cmd.Env = append(cmd.Environ(), "PORT="+job.Port) // requires go 1.19
178-
cmd.Env = append(cmd.Env, "LISTEN_ADDRESS="+net.JoinHostPort(job.Host, job.Port), "PWD="+cmd.Dir)
183+
cmd.Env, err = buildRunnerEnv(job, map[string]string{
184+
"LISTEN_ADDRESS": net.JoinHostPort(job.Host, job.Port),
185+
"PWD": cmd.Dir,
186+
})
187+
if err != nil {
188+
return fmt.Errorf("error building runner environment: %w", err)
189+
}
179190

180191
// Running asynchronously allows for the client Run method to return
181192
// metadata about the running function such as its chosen port.
@@ -244,12 +255,22 @@ func runPython(ctx context.Context, job *Job) (err error) {
244255
cmd.Stdout = os.Stdout
245256
cmd.Stderr = os.Stderr
246257

247-
// See 1.19 [release notes](https://tip.golang.org/doc/go1.19) which state:
248-
// A Cmd with a non-empty Dir field and nil Env now implicitly sets the
249-
// PWD environment variable for the subprocess to match Dir.
250-
// The new method Cmd.Environ reports the environment that would be used
251-
// to run the command, including the implicitly set PWD variable.
252-
cmd.Env = append(cmd.Env, "PORT="+job.Port, "LISTEN_ADDRESS="+listenAddress, "PWD="+cmd.Dir)
258+
cmd.Cancel = func() error {
259+
if runtime.GOOS == "windows" {
260+
return cmd.Process.Kill()
261+
}
262+
return cmd.Process.Signal(os.Interrupt)
263+
}
264+
cmd.WaitDelay = 5 * time.Second
265+
266+
cmd.Env, err = buildRunnerEnv(job, map[string]string{
267+
"PORT": job.Port,
268+
"LISTEN_ADDRESS": listenAddress,
269+
"PWD": cmd.Dir,
270+
})
271+
if err != nil {
272+
return fmt.Errorf("error building runner environment: %w", err)
273+
}
253274

254275
// Running asynchronously allows for the client Run method to return
255276
// metadata about the running function such as its chosen port.
@@ -266,6 +287,29 @@ func runPython(ctx context.Context, job *Job) (err error) {
266287
return
267288
}
268289

290+
// buildRunnerEnv constructs the environment for a host-run subprocess.
291+
// It starts with the parent process environment (os.Environ), layers on the
292+
// provided extras (e.g. PORT, LISTEN_ADDRESS, PWD), and then applies any
293+
// environment variables defined in func.yaml or passed via -e flags.
294+
func buildRunnerEnv(job *Job, extras map[string]string) ([]string, error) {
295+
env := os.Environ()
296+
297+
for k, v := range extras {
298+
env = append(env, k+"="+v)
299+
}
300+
301+
// Interpolate and append env vars from func.yaml / -e flags.
302+
funcEnvs, err := Interpolate(job.Function.Run.Envs)
303+
if err != nil {
304+
return nil, fmt.Errorf("error interpolating environment variables: %w", err)
305+
}
306+
for k, v := range funcEnvs {
307+
env = append(env, k+"="+v)
308+
}
309+
310+
return env, nil
311+
}
312+
269313
func waitFor(ctx context.Context, job *Job, timeout time.Duration) error {
270314
var (
271315
uri = fmt.Sprintf("http://%s%s", net.JoinHostPort(job.Host, job.Port), readinessEndpoint)

pkg/functions/runner_env_test.go

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
package functions
2+
3+
import (
4+
"os"
5+
"slices"
6+
"strings"
7+
"testing"
8+
9+
"k8s.io/utils/ptr"
10+
)
11+
12+
// TestBuildRunnerEnv_InheritsParentEnv ensures that the parent process
13+
// environment is inherited by the subprocess.
14+
func TestBuildRunnerEnv_InheritsParentEnv(t *testing.T) {
15+
const testKey = "FUNC_TEST_INHERIT_CHECK"
16+
const testVal = "inherited_value"
17+
t.Setenv(testKey, testVal)
18+
19+
job := &Job{Function: Function{}}
20+
env, err := buildRunnerEnv(job, nil)
21+
if err != nil {
22+
t.Fatalf("unexpected error: %v", err)
23+
}
24+
25+
expected := testKey + "=" + testVal
26+
if !slices.Contains(env, expected) {
27+
t.Errorf("expected parent env var %q in result, but not found", expected)
28+
}
29+
}
30+
31+
// TestBuildRunnerEnv_ExtrasAreIncluded ensures that extras like PORT,
32+
// LISTEN_ADDRESS, and PWD are present in the environment.
33+
func TestBuildRunnerEnv_ExtrasAreIncluded(t *testing.T) {
34+
job := &Job{Function: Function{}}
35+
extras := map[string]string{
36+
"PORT": "8080",
37+
"LISTEN_ADDRESS": "127.0.0.1:8080",
38+
"PWD": "/tmp/func",
39+
}
40+
41+
env, err := buildRunnerEnv(job, extras)
42+
if err != nil {
43+
t.Fatalf("unexpected error: %v", err)
44+
}
45+
46+
for k, v := range extras {
47+
expected := k + "=" + v
48+
if !slices.Contains(env, expected) {
49+
t.Errorf("expected extra env var %q in result, but not found", expected)
50+
}
51+
}
52+
}
53+
54+
// TestBuildRunnerEnv_FuncYamlEnvsIncluded ensures that envs from func.yaml
55+
// (Function.Run.Envs) are interpolated and included.
56+
func TestBuildRunnerEnv_FuncYamlEnvsIncluded(t *testing.T) {
57+
job := &Job{
58+
Function: Function{
59+
Run: RunSpec{
60+
Envs: Envs{
61+
{Name: ptr.To("MY_VAR"), Value: ptr.To("my_value")},
62+
{Name: ptr.To("ANOTHER"), Value: ptr.To("another_value")},
63+
},
64+
},
65+
},
66+
}
67+
68+
env, err := buildRunnerEnv(job, nil)
69+
if err != nil {
70+
t.Fatalf("unexpected error: %v", err)
71+
}
72+
73+
if !slices.Contains(env, "MY_VAR=my_value") {
74+
t.Error("expected MY_VAR=my_value in result")
75+
}
76+
if !slices.Contains(env, "ANOTHER=another_value") {
77+
t.Error("expected ANOTHER=another_value in result")
78+
}
79+
}
80+
81+
// TestBuildRunnerEnv_FuncYamlEnvsOverrideParent ensures that func.yaml envs
82+
// take precedence over parent environment variables (last value wins in exec).
83+
func TestBuildRunnerEnv_FuncYamlEnvsOverrideParent(t *testing.T) {
84+
const testKey = "FUNC_TEST_OVERRIDE"
85+
t.Setenv(testKey, "parent_value")
86+
87+
job := &Job{
88+
Function: Function{
89+
Run: RunSpec{
90+
Envs: Envs{
91+
{Name: ptr.To(testKey), Value: ptr.To("func_yaml_value")},
92+
},
93+
},
94+
},
95+
}
96+
97+
env, err := buildRunnerEnv(job, nil)
98+
if err != nil {
99+
t.Fatalf("unexpected error: %v", err)
100+
}
101+
102+
// The func.yaml value should appear after the parent value.
103+
// In exec.Cmd, the last duplicate key wins.
104+
lastValue := ""
105+
for _, e := range env {
106+
if v, ok := strings.CutPrefix(e, testKey+"="); ok {
107+
lastValue = v
108+
}
109+
}
110+
if lastValue != "func_yaml_value" {
111+
t.Errorf("expected func.yaml value to override parent, got %q", lastValue)
112+
}
113+
}
114+
115+
// TestBuildRunnerEnv_PathInherited is a quick check that PATH specifically
116+
// is inherited from parent env.
117+
func TestBuildRunnerEnv_PathInherited(t *testing.T) {
118+
job := &Job{Function: Function{}}
119+
env, err := buildRunnerEnv(job, nil)
120+
if err != nil {
121+
t.Fatalf("unexpected error: %v", err)
122+
}
123+
124+
pathPresent := false
125+
for _, e := range env {
126+
if strings.HasPrefix(e, "PATH=") {
127+
pathPresent = true
128+
break
129+
}
130+
}
131+
132+
// PATH should always be set in a normal Unix environment
133+
if path := os.Getenv("PATH"); path != "" && !pathPresent {
134+
t.Error("expected PATH to be inherited from parent environment")
135+
}
136+
}
137+
138+
// TestBuildRunnerEnv_EmptyRunEnvs ensures no error when there are no
139+
// func.yaml envs configured.
140+
func TestBuildRunnerEnv_EmptyRunEnvs(t *testing.T) {
141+
job := &Job{Function: Function{}}
142+
env, err := buildRunnerEnv(job, map[string]string{"PORT": "8080"})
143+
if err != nil {
144+
t.Fatalf("unexpected error: %v", err)
145+
}
146+
147+
if !slices.Contains(env, "PORT=8080") {
148+
t.Error("expected PORT=8080 in result")
149+
}
150+
}

0 commit comments

Comments
 (0)