diff --git a/pkg/functions/runner.go b/pkg/functions/runner.go index 27c4ce3895..20dac05da0 100644 --- a/pkg/functions/runner.go +++ b/pkg/functions/runner.go @@ -11,11 +11,12 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "time" ) const ( - defaultRunHost = "127.0.0.1" // TODO allow to be altered via a runOpt + defaultRunHost = "127.0.0.1" defaultRunPort = "8080" readinessEndpoint = "/health/readiness" ) @@ -169,13 +170,23 @@ func runGo(ctx context.Context, job *Job) (err error) { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - // cmd.Cancel = stop // TODO: use when we upgrade to go 1.20 + cmd.Cancel = func() error { + if runtime.GOOS == "windows" { + // Interrupt is not implemented on windows apparently + return cmd.Process.Kill() + } + return cmd.Process.Signal(os.Interrupt) + } + // force kill after delay if Interrupt signal did not work to not hang indefinitely + cmd.WaitDelay = 5 * time.Second - // See the 1.19 [release notes](https://tip.golang.org/doc/go1.19) which state: - // A Cmd with a non-empty Dir field and nil Env now implicitly sets the PWD environment variable for the subprocess to match Dir. - // The new method Cmd.Environ reports the environment that would be used to run the command, including the implicitly set PWD variable. - // cmd.Env = append(cmd.Environ(), "PORT="+job.Port) // requires go 1.19 - cmd.Env = append(cmd.Env, "LISTEN_ADDRESS="+net.JoinHostPort(job.Host, job.Port), "PWD="+cmd.Dir) + cmd.Env, err = buildRunnerEnv(job, map[string]string{ + "LISTEN_ADDRESS": net.JoinHostPort(job.Host, job.Port), + "PWD": cmd.Dir, + }) + if err != nil { + return fmt.Errorf("error building runner environment: %w", err) + } // Running asynchronously allows for the client Run method to return // metadata about the running function such as its chosen port. @@ -244,12 +255,22 @@ func runPython(ctx context.Context, job *Job) (err error) { cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - // See 1.19 [release notes](https://tip.golang.org/doc/go1.19) which state: - // A Cmd with a non-empty Dir field and nil Env now implicitly sets the - // PWD environment variable for the subprocess to match Dir. - // The new method Cmd.Environ reports the environment that would be used - // to run the command, including the implicitly set PWD variable. - cmd.Env = append(cmd.Env, "PORT="+job.Port, "LISTEN_ADDRESS="+listenAddress, "PWD="+cmd.Dir) + cmd.Cancel = func() error { + if runtime.GOOS == "windows" { + return cmd.Process.Kill() + } + return cmd.Process.Signal(os.Interrupt) + } + cmd.WaitDelay = 5 * time.Second + + cmd.Env, err = buildRunnerEnv(job, map[string]string{ + "PORT": job.Port, + "LISTEN_ADDRESS": listenAddress, + "PWD": cmd.Dir, + }) + if err != nil { + return fmt.Errorf("error building runner environment: %w", err) + } // Running asynchronously allows for the client Run method to return // metadata about the running function such as its chosen port. @@ -266,6 +287,29 @@ func runPython(ctx context.Context, job *Job) (err error) { return } +// buildRunnerEnv constructs the environment for a host-run subprocess. +// It starts with the parent process environment (os.Environ), layers on the +// provided extras (e.g. PORT, LISTEN_ADDRESS, PWD), and then applies any +// environment variables defined in func.yaml or passed via -e flags. +func buildRunnerEnv(job *Job, extras map[string]string) ([]string, error) { + env := os.Environ() + + for k, v := range extras { + env = append(env, k+"="+v) + } + + // Interpolate and append env vars from func.yaml / -e flags. + funcEnvs, err := Interpolate(job.Function.Run.Envs) + if err != nil { + return nil, fmt.Errorf("error interpolating environment variables: %w", err) + } + for k, v := range funcEnvs { + env = append(env, k+"="+v) + } + + return env, nil +} + func waitFor(ctx context.Context, job *Job, timeout time.Duration) error { var ( uri = fmt.Sprintf("http://%s%s", net.JoinHostPort(job.Host, job.Port), readinessEndpoint) diff --git a/pkg/functions/runner_env_test.go b/pkg/functions/runner_env_test.go new file mode 100644 index 0000000000..48225f4fd8 --- /dev/null +++ b/pkg/functions/runner_env_test.go @@ -0,0 +1,150 @@ +package functions + +import ( + "os" + "slices" + "strings" + "testing" + + "k8s.io/utils/ptr" +) + +// TestBuildRunnerEnv_InheritsParentEnv ensures that the parent process +// environment is inherited by the subprocess. +func TestBuildRunnerEnv_InheritsParentEnv(t *testing.T) { + const testKey = "FUNC_TEST_INHERIT_CHECK" + const testVal = "inherited_value" + t.Setenv(testKey, testVal) + + job := &Job{Function: Function{}} + env, err := buildRunnerEnv(job, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := testKey + "=" + testVal + if !slices.Contains(env, expected) { + t.Errorf("expected parent env var %q in result, but not found", expected) + } +} + +// TestBuildRunnerEnv_ExtrasAreIncluded ensures that extras like PORT, +// LISTEN_ADDRESS, and PWD are present in the environment. +func TestBuildRunnerEnv_ExtrasAreIncluded(t *testing.T) { + job := &Job{Function: Function{}} + extras := map[string]string{ + "PORT": "8080", + "LISTEN_ADDRESS": "127.0.0.1:8080", + "PWD": "/tmp/func", + } + + env, err := buildRunnerEnv(job, extras) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + for k, v := range extras { + expected := k + "=" + v + if !slices.Contains(env, expected) { + t.Errorf("expected extra env var %q in result, but not found", expected) + } + } +} + +// TestBuildRunnerEnv_FuncYamlEnvsIncluded ensures that envs from func.yaml +// (Function.Run.Envs) are interpolated and included. +func TestBuildRunnerEnv_FuncYamlEnvsIncluded(t *testing.T) { + job := &Job{ + Function: Function{ + Run: RunSpec{ + Envs: Envs{ + {Name: ptr.To("MY_VAR"), Value: ptr.To("my_value")}, + {Name: ptr.To("ANOTHER"), Value: ptr.To("another_value")}, + }, + }, + }, + } + + env, err := buildRunnerEnv(job, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !slices.Contains(env, "MY_VAR=my_value") { + t.Error("expected MY_VAR=my_value in result") + } + if !slices.Contains(env, "ANOTHER=another_value") { + t.Error("expected ANOTHER=another_value in result") + } +} + +// TestBuildRunnerEnv_FuncYamlEnvsOverrideParent ensures that func.yaml envs +// take precedence over parent environment variables (last value wins in exec). +func TestBuildRunnerEnv_FuncYamlEnvsOverrideParent(t *testing.T) { + const testKey = "FUNC_TEST_OVERRIDE" + t.Setenv(testKey, "parent_value") + + job := &Job{ + Function: Function{ + Run: RunSpec{ + Envs: Envs{ + {Name: ptr.To(testKey), Value: ptr.To("func_yaml_value")}, + }, + }, + }, + } + + env, err := buildRunnerEnv(job, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // The func.yaml value should appear after the parent value. + // In exec.Cmd, the last duplicate key wins. + lastValue := "" + for _, e := range env { + if v, ok := strings.CutPrefix(e, testKey+"="); ok { + lastValue = v + } + } + if lastValue != "func_yaml_value" { + t.Errorf("expected func.yaml value to override parent, got %q", lastValue) + } +} + +// TestBuildRunnerEnv_PathInherited is a quick check that PATH specifically +// is inherited from parent env. +func TestBuildRunnerEnv_PathInherited(t *testing.T) { + job := &Job{Function: Function{}} + env, err := buildRunnerEnv(job, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + pathPresent := false + for _, e := range env { + if strings.HasPrefix(e, "PATH=") { + pathPresent = true + break + } + } + + // PATH should always be set in a normal Unix environment + if path := os.Getenv("PATH"); path != "" && !pathPresent { + t.Error("expected PATH to be inherited from parent environment") + } +} + +// TestBuildRunnerEnv_EmptyRunEnvs ensures no error when there are no +// func.yaml envs configured. +func TestBuildRunnerEnv_EmptyRunEnvs(t *testing.T) { + job := &Job{Function: Function{}} + env, err := buildRunnerEnv(job, map[string]string{"PORT": "8080"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !slices.Contains(env, "PORT=8080") { + t.Error("expected PORT=8080 in result") + } +}