From 20554ef8be736c4dd201b7328e7a8c29015948b6 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 13 Sep 2021 15:44:21 +0700 Subject: [PATCH] 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: https://github.com/tetratelabs/func-e/pull/371#discussion_r707015768. 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 Co-authored-by: Adrian Cole Co-authored-by: Takeshi Yoneda --- internal/envoy/run.go | 2 +- internal/envoy/run_test.go | 3 +- internal/envoy/shutdown/admin_test.go | 3 +- internal/moreos/moreos_func-e_test.go | 138 ++++++++++++++++++ internal/moreos/proc_linux.go | 5 +- internal/moreos/testdata/fake_func-e.go | 73 +++++++++ internal/test/envoy.go | 55 ------- internal/test/fakebinary/fake_binary.go | 87 +++++++++++ .../{ => fakebinary}/testdata/fake_envoy.go | 1 + internal/test/server.go | 3 +- 10 files changed, 309 insertions(+), 61 deletions(-) create mode 100644 internal/moreos/moreos_func-e_test.go create mode 100644 internal/moreos/testdata/fake_func-e.go create mode 100644 internal/test/fakebinary/fake_binary.go rename internal/test/{ => fakebinary}/testdata/fake_envoy.go (96%) diff --git a/internal/envoy/run.go b/internal/envoy/run.go index 1a847560..52032879 100644 --- a/internal/envoy/run.go +++ b/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) } diff --git a/internal/envoy/run_test.go b/internal/envoy/run_test.go index b4a67460..f0642704 100644 --- a/internal/envoy/run_test.go +++ b/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 diff --git a/internal/envoy/shutdown/admin_test.go b/internal/envoy/shutdown/admin_test.go index 5b3aa4be..b2a329e9 100644 --- a/internal/envoy/shutdown/admin_test.go +++ b/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} diff --git a/internal/moreos/moreos_func-e_test.go b/internal/moreos/moreos_func-e_test.go new file mode 100644 index 00000000..8604efef --- /dev/null +++ b/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) +} diff --git a/internal/moreos/proc_linux.go b/internal/moreos/proc_linux.go index ef0b9c83..7d14d780 100644 --- a/internal/moreos/proc_linux.go +++ b/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 { diff --git a/internal/moreos/testdata/fake_func-e.go b/internal/moreos/testdata/fake_func-e.go new file mode 100644 index 00000000..ed54d354 --- /dev/null +++ b/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) +} diff --git a/internal/test/envoy.go b/internal/test/envoy.go index 6141886b..cd13cfa0 100644 --- a/internal/test/envoy.go +++ b/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 -} diff --git a/internal/test/fakebinary/fake_binary.go b/internal/test/fakebinary/fake_binary.go new file mode 100644 index 00000000..92ae269d --- /dev/null +++ b/internal/test/fakebinary/fake_binary.go @@ -0,0 +1,87 @@ +// 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 fakebinary as using morerequire introduces build cycles +package fakebinary + +import ( + _ "embed" // Embedding the fakeEnvoySrc is easier than file I/O and ensures it doesn't skew coverage + "os" + "os/exec" + "path/filepath" + "runtime" + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +// exe is like moreos.Exe, except if we used that it would make a build cycle. +var exe = func() string { + if runtime.GOOS == "windows" { + return ".exe" + } + return "" +}() + +var ( + // fakeEnvoySrc is a test source file used to simulate Envoy console output and signal processing. + // This has no other source dependencies. + //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. This is embedded here because it is reused in many places. +func RequireFakeEnvoy(t *testing.T, path string) { + builtFakeEnvoy.Do(func() { + fakeEnvoyBin = RequireBuildFakeBinary(t, t.TempDir(), "envoy", fakeEnvoySrc) + }) + require.NoError(t, os.WriteFile(path, fakeEnvoyBin, 0700)) //nolint:gosec +} + +// RequireBuildFakeBinary builds a fake binary and returns its contents. +func RequireBuildFakeBinary(t *testing.T, workDir, name string, mainSrc []byte) []byte { + goBin := requireGoBin(t) + + bin := name + exe + goArgs := []string{"build", "-o", bin, "main.go"} + require.NoError(t, os.WriteFile(filepath.Join(workDir, "main.go"), mainSrc, 0600)) + + // Don't allow any third party dependencies for now. + require.NoError(t, os.WriteFile(filepath.Join(workDir, "go.mod"), + []byte("module github.com/tetratelabs/func-e\n\ngo 1.17\n"), 0600)) + + cmd := exec.Command(goBin, goArgs...) //nolint:gosec + cmd.Dir = workDir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "couldn't compile %s: %s", bin, string(out)) + bytes, err := os.ReadFile(filepath.Join(workDir, bin)) + require.NoError(t, err) + return bytes +} + +func requireGoBin(t *testing.T) string { + binName := "go" + 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 +} diff --git a/internal/test/testdata/fake_envoy.go b/internal/test/fakebinary/testdata/fake_envoy.go similarity index 96% rename from internal/test/testdata/fake_envoy.go rename to internal/test/fakebinary/testdata/fake_envoy.go index fc7b24dc..c0eb2408 100644 --- a/internal/test/testdata/fake_envoy.go +++ b/internal/test/fakebinary/testdata/fake_envoy.go @@ -1,5 +1,6 @@ package main +// It is essential that no func-e imports are used here. For example, internal/moreos is not used. import ( "fmt" "net/http" diff --git a/internal/test/server.go b/internal/test/server.go index 08710a9f..9a3d2d47 100644 --- a/internal/test/server.go +++ b/internal/test/server.go @@ -33,6 +33,7 @@ import ( "github.com/tetratelabs/func-e/internal/globals" "github.com/tetratelabs/func-e/internal/moreos" "github.com/tetratelabs/func-e/internal/tar" + "github.com/tetratelabs/func-e/internal/test/fakebinary" "github.com/tetratelabs/func-e/internal/version" ) @@ -117,7 +118,7 @@ func RequireFakeEnvoyTarGz(t *testing.T, v version.Version) ([]byte, version.SHA // construct the platform directory based on the input version installDir := filepath.Join(tempDir, string(v)) require.NoError(t, os.MkdirAll(filepath.Join(installDir, "bin"), 0700)) //nolint:gosec - RequireFakeEnvoy(t, filepath.Join(installDir, "bin", "envoy"+moreos.Exe)) + fakebinary.RequireFakeEnvoy(t, filepath.Join(installDir, "bin", "envoy"+moreos.Exe)) // tar.gz the platform dir tempGz := filepath.Join(tempDir, "envoy.tar.gz")