Add fakebinary for acommodating testing the child process management (#…

…371)

This adds the `internal/test/fakebinary` package allowing us to test running a fake `func-e` process that spawns a fake `envoy`. This allows us to isolate the process control angle and can lead to quicker diagnosis of platform-specific behaviors, as well as show potential for future process abstraction. For example: to check the behavior of `func-e` when we send `SIGKILL` to it (in Linux, we need to make sure the child dies and receive whatever signal we have configured in `SysProcAttr.Pdeathsig`).

We changed the setting for `SysProcAttr.Pdeathsig` to `SIGKILL` since when anyone sends `SIGKILL` on Linux to `func-e`, we want the spawned envoy to receive `SIGKILL` as well. Reference: #371 (comment).

This also abstracts away the process for building any fake binary that optionally uses some of `func-e` internal packages. For example, in this change, we can isolate the build for fake `func-e` to use `internal/moreos` package (to check on `moreos.ProcessGroupAttr()`) only and no more than that what it requires.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Co-authored-by: Adrian Cole <adrian@tetrate.io>
Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>

internal/envoy/run.go

@@ -89,7 +89,7 @@ func (r *Runtime) Run(ctx context.Context, args []string) (err error) { //nolint
 	// Block until it exits to ensure file descriptors are closed prior to archival.
 	// Allow up to 5 seconds for a clean stop, killing if it can't for any reason.
 	select {
-	case <-waitCtx.Done():
+	case <-waitCtx.Done(): // cmd.Wait goroutine finished
 	case <-time.After(5 * time.Second):
 		_ = moreos.EnsureProcessDone(r.cmd.Process)
 	}

internal/envoy/run_test.go

@@ -31,6 +31,7 @@ import (
 	"github.com/tetratelabs/func-e/internal/globals"
 	"github.com/tetratelabs/func-e/internal/moreos"
 	"github.com/tetratelabs/func-e/internal/test"
+	"github.com/tetratelabs/func-e/internal/test/fakebinary"
 )
 
 func TestRuntime_Run(t *testing.T) {
@@ -41,7 +42,7 @@ func TestRuntime_Run(t *testing.T) {
 	adminFlag := fmt.Sprint("--admin-address-path ", filepath.Join(runDir, "admin-address.txt"))
 
 	fakeEnvoy := filepath.Join(tempDir, "envoy"+moreos.Exe)
-	test.RequireFakeEnvoy(t, fakeEnvoy)
+	fakebinary.RequireFakeEnvoy(t, fakeEnvoy)
 
 	tests := []struct {
 		name                           string

internal/envoy/shutdown/admin_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/tetratelabs/func-e/internal/globals"
 	"github.com/tetratelabs/func-e/internal/moreos"
 	"github.com/tetratelabs/func-e/internal/test"
+	"github.com/tetratelabs/func-e/internal/test/fakebinary"
 )
 
 func TestEnableEnvoyAdminDataCollection(t *testing.T) {
@@ -45,7 +46,7 @@ func TestEnableEnvoyAdminDataCollection(t *testing.T) {
 // runWithShutdownHook is like RequireRun, except invokes the hook on shutdown
 func runWithShutdownHook(t *testing.T, runDir string, hook func(r *envoy.Runtime) error) error {
 	fakeEnvoy := filepath.Join(runDir, "envoy"+moreos.Exe)
-	test.RequireFakeEnvoy(t, fakeEnvoy)
+	fakebinary.RequireFakeEnvoy(t, fakeEnvoy)
 
 	o := &globals.RunOpts{EnvoyPath: fakeEnvoy, RunDir: runDir, DontArchiveRunDir: true}
 

internal/moreos/moreos_func-e_test.go

@@ -0,0 +1,138 @@
+// Copyright 2021 Tetrate
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package moreos
+
+import (
+	"bufio"
+	"bytes"
+	"embed"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/shirou/gopsutil/v3/process"
+	"github.com/stretchr/testify/require"
+
+	"github.com/tetratelabs/func-e/internal/test/fakebinary"
+	"github.com/tetratelabs/func-e/internal/version"
+)
+
+var (
+	// fakeFuncESrc is a test source file used to simulate how func-e manages its child process
+	//go:embed testdata/fake_func-e.go
+	fakeFuncESrc []byte // Embedding the fakeFuncESrc is easier than file I/O and ensures it doesn't skew coverage
+
+	// Include the source imported by fakeFuncESrc directly and indirectly
+	// TODO: replace wildcard with {{goos}} after https://github.com/golang/go/issues/48348.
+	//go:embed moreos.go
+	//go:embed proc_*.go
+	moreosSrcDir embed.FS
+)
+
+// TestProcessGroupAttr_Kill tests sending SIGKILL to fake func-e.
+// On linux, we propagate SIGKILL to the child process as the configured SysProcAttr.Pdeathsig
+// in proc_linux.go.
+func TestProcessGroupAttr_Kill(t *testing.T) {
+	// This works only for linux, sending kill -9 on darwin will not kill the process, we need to kill
+	// via pgid or kill the child first.
+	if runtime.GOOS != "linux" {
+		t.Skip()
+	}
+	tempDir := t.TempDir()
+
+	// Build a fake envoy and pass the ENV hint so that fake func-e uses it
+	fakeEnvoy := filepath.Join(tempDir, "envoy"+Exe)
+	fakebinary.RequireFakeEnvoy(t, fakeEnvoy)
+	t.Setenv("ENVOY_PATH", fakeEnvoy)
+
+	fakeFuncE := filepath.Join(tempDir, "func-e"+Exe)
+	requireFakeFuncE(t, fakeFuncE)
+
+	stderr := new(bytes.Buffer)
+	stdout := new(bytes.Buffer)
+
+	// With an arg so fakeFuncE runs fakeEnvoy as its child and doesn't exit.
+	arg := string(version.LastKnownEnvoy)
+	cmd := exec.Command(fakeFuncE, "run", arg, "-c")
+	cmd.Stdout = stdout
+	cmd.Stderr = stderr
+
+	require.NoError(t, cmd.Start())
+
+	// Block until we reach an expected line or timeout.
+	reader := bufio.NewReader(stderr)
+	waitFor := "initializing epoch 0"
+	require.Eventually(t, func() bool {
+		b, _ := reader.Peek(512) // the error value is always EOF.
+		return strings.HasPrefix(string(b), waitFor)
+	}, 5*time.Second, 100*time.Millisecond, "timeout waiting for stderr to contain %q", waitFor)
+
+	require.Equal(t, Sprintf("starting: %s %s -c\n", fakeEnvoy, arg), stdout.String())
+
+	fakeFuncEProcess, err := process.NewProcess(int32(cmd.Process.Pid))
+	require.NoError(t, err)
+
+	// Get all fake func-e children processes.
+	children, err := fakeFuncEProcess.Children()
+	require.NoError(t, err)
+	require.Equal(t, len(children), 1) // Should have only one child process i.e. the fake envoy process.
+	fakeEnvoyProcess := &os.Process{Pid: int(children[0].Pid)}
+
+	// Kill the fake func-e process.
+	require.NoError(t, cmd.Process.Kill())
+
+	// Wait for the process to die; this could error due to the kill signal.
+	cmd.Wait() //nolint
+
+	// Wait and check if fake func-e and envoy processes are killed.
+	requireFindProcessError(t, cmd.Process, process.ErrorProcessNotRunning)
+	requireFindProcessError(t, fakeEnvoyProcess, process.ErrorProcessNotRunning)
+
+	// Ensure both processes are killed.
+	require.NoError(t, EnsureProcessDone(cmd.Process))
+	require.NoError(t, EnsureProcessDone(fakeEnvoyProcess))
+}
+
+// requireFakeFuncE builds a func-e binary only depends on fakeFuncESrc and the sources in this package.
+// This is used to test integrated use of tools like ProcessGroupAttr without mixing unrelated concerns or dependencies.
+func requireFakeFuncE(t *testing.T, path string) {
+	workDir := t.TempDir()
+
+	// Copy the sources needed for fake func-e, but nothing else
+	moreosDir := filepath.Join(workDir, "internal", "moreos")
+	require.NoError(t, os.MkdirAll(moreosDir, 0700))
+	moreosSrcs, err := moreosSrcDir.ReadDir(".")
+	require.NoError(t, err)
+	for _, src := range moreosSrcs {
+		data, err := moreosSrcDir.ReadFile(src.Name())
+		require.NoError(t, err)
+		require.NoError(t, os.WriteFile(filepath.Join(moreosDir, src.Name()), data, 0600))
+	}
+
+	fakeFuncEBin := fakebinary.RequireBuildFakeBinary(t, workDir, "func-e", fakeFuncESrc)
+	require.NoError(t, os.WriteFile(path, fakeFuncEBin, 0700)) //nolint:gosec
+}
+
+func requireFindProcessError(t *testing.T, proc *os.Process, expectedErr error) {
+	// Wait until the operating system removes or adds the scheduled process.
+	require.Eventually(t, func() bool {
+		_, err := process.NewProcess(int32(proc.Pid)) // because os.FindProcess is no-op in Linux!
+		return err == expectedErr
+	}, 100*time.Millisecond, 5*time.Millisecond, "timeout waiting for expected error %v", expectedErr)
+}

internal/moreos/proc_linux.go

@@ -22,8 +22,9 @@ import (
 const exe = ""
 
 func processGroupAttr() *syscall.SysProcAttr {
-	// Pdeathsig aims to ensure the process group is cleaned up even if this process dies
-	return &syscall.SysProcAttr{Setpgid: true, Pdeathsig: syscall.SIGTERM}
+	// Pdeathsig aims to ensure the process group is cleaned up even if this process dies. When func-e
+	// dies, the process (envoy) will get SIGKILL.
+	return &syscall.SysProcAttr{Setpgid: true, Pdeathsig: syscall.SIGKILL}
 }
 
 func interrupt(p *os.Process) error {

internal/moreos/testdata/fake_func-e.go

@@ -0,0 +1,73 @@
+package main
+
+// only import moreos, as that's what we are testing
+import (
+	"context"
+	"os"
+	"os/exec"
+	"os/signal"
+	"strings"
+	"syscall"
+	"time"
+
+	"github.com/tetratelabs/func-e/internal/moreos"
+)
+
+// main simulates ../../.../main.go, but only focuses on sub process control style used by envoy.Run.
+// This allows us to write unit tests and identify failures more directly than e2e tests.
+//
+// Notably, this uses a variable ENVOY_PATH instead of envoy.GetHomeVersion, in order to reduce logic.
+//
+// In the future, some of this process control structure might move to moreos in order to reduce copy/paste between here
+// and internal/envoy/run.go (envoy.Run).
+func main() {
+	if len(os.Args) < 2 {
+		moreos.Fprintf(os.Stderr, "not enough args\n")
+		os.Exit(1)
+	}
+
+	if os.Args[1] != "run" {
+		moreos.Fprintf(os.Stderr, "%s not supported\n", os.Args[1])
+		os.Exit(1)
+	}
+
+	// Like envoy.GetHomeVersion, $FUNC_E_HOME/versions/$(cat $FUNC_E_HOME/version)/bin/envoy$GOEXE.
+	cmd := exec.Command(os.Getenv("ENVOY_PATH"), os.Args[2:]...)
+	cmd.SysProcAttr = moreos.ProcessGroupAttr()
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+
+	// Like envoy.Run.
+	waitCtx, waitCancel := context.WithCancel(context.Background())
+	sigCtx, _ := signal.NotifyContext(waitCtx, syscall.SIGINT, syscall.SIGTERM)
+	defer waitCancel()
+
+	moreos.Fprintf(os.Stdout, "starting: %s\n", strings.Join(cmd.Args, " ")) //nolint
+	if err := cmd.Start(); err != nil {
+		moreos.Fprintf(os.Stderr, "error: unable to start Envoy process: %s\n", err)
+		os.Exit(1)
+	}
+
+	// Wait in a goroutine. We may need to kill the process if a signal occurs first.
+	go func() {
+		defer waitCancel()
+		_ = cmd.Wait() // Envoy logs like "caught SIGINT" or "caught ENVOY_SIGTERM", so we don't repeat logging here.
+	}()
+
+	// Block until we receive SIGINT or are canceled because Envoy has died.
+	<-sigCtx.Done()
+
+	// Block until it exits to ensure file descriptors are closed prior to archival.
+	// Allow up to 5 seconds for a clean stop, killing if it can't for any reason.
+	select {
+	case <-waitCtx.Done(): // cmd.Wait goroutine finished
+	case <-time.After(5 * time.Second):
+		_ = moreos.EnsureProcessDone(cmd.Process)
+	}
+
+	if cmd.ProcessState != nil && cmd.ProcessState.ExitCode() > 0 {
+		moreos.Fprintf(os.Stderr, "envoy exited with status: %d\n", cmd.ProcessState.ExitCode())
+		os.Exit(1)
+	}
+	os.Exit(0)
+}

internal/test/envoy.go

@@ -20,19 +20,11 @@ import (
 	_ "embed" // Embedding the fakeEnvoySrc is easier than file I/O and ensures it doesn't skew coverage
 	"fmt"
 	"io"
-	"os"
-	"os/exec"
-	"path/filepath"
-	"runtime"
 	"strings"
-	"sync"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-
-	"github.com/tetratelabs/func-e/internal/moreos"
 )
 
 // Runner allows us to not introduce dependency cycles on envoy.Runtime
@@ -77,50 +69,3 @@ func RequireRun(t *testing.T, shutdown func(), r Runner, stderr io.Reader, args
 	<-ran // block until the runner finished
 	return err
 }
-
-var (
-	// fakeEnvoySrc is a test source file used to simulate Envoy console output and signal processing.
-	//go:embed testdata/fake_envoy.go
-	fakeEnvoySrc []byte
-	// fakeEnvoyBin is the compiled code of fakeEnvoySrc which will be runtime.GOOS dependent.
-	fakeEnvoyBin   []byte
-	builtFakeEnvoy sync.Once
-)
-
-// RequireFakeEnvoy writes fakeEnvoyBin to the given path
-func RequireFakeEnvoy(t *testing.T, path string) {
-	builtFakeEnvoy.Do(func() {
-		fakeEnvoyBin = requireBuildFakeEnvoy(t)
-	})
-	require.NoError(t, os.WriteFile(path, fakeEnvoyBin, 0700)) //nolint:gosec
-}
-
-// requireBuildFakeEnvoy builds a fake envoy binary and returns its contents.
-func requireBuildFakeEnvoy(t *testing.T) []byte {
-	goBin := requireGoBin(t)
-	tempDir := t.TempDir()
-
-	name := "envoy"
-	bin := name + moreos.Exe
-	src := name + ".go"
-	require.NoError(t, os.WriteFile(filepath.Join(tempDir, src), fakeEnvoySrc, 0600))
-	cmd := exec.Command(goBin, "build", "-o", bin, src) //nolint:gosec
-	cmd.Dir = tempDir
-	out, err := cmd.CombinedOutput()
-	require.NoError(t, err, "couldn't compile %s: %s", src, string(out))
-	bytes, err := os.ReadFile(filepath.Join(tempDir, bin))
-	require.NoError(t, err)
-	return bytes
-}
-
-func requireGoBin(t *testing.T) string {
-	binName := "go" + moreos.Exe
-	goBin := filepath.Join(runtime.GOROOT(), "bin", binName)
-	if _, err := os.Stat(goBin); err == nil {
-		return goBin
-	}
-	// Now, search the path
-	goBin, err := exec.LookPath(binName)
-	require.NoError(t, err, "couldn't find %s in the PATH", goBin)
-	return goBin
-}