Fixes broken local macOS build and completes migration to GH actions (#…

…160)

Signed-off-by: Adrian Cole <adrian@tetrate.io>

.github/workflows/commit.yaml

@@ -17,6 +17,55 @@ on:
   workflow_dispatch:
 
 jobs:
+  test:
+    name: "Run unit tests"
+    runs-on: ${{ matrix.runner.os }}
+    timeout-minutes: 90  # instead of 360 by default
+    strategy:
+      matrix:
+        runner:
+          # - os: macos-latest  ## revisit once all tests are off ginkgo
+          - os: ubuntu-latest
+    steps:
+      - name: "Checkout"
+        uses: actions/checkout@v2
+
+      - name: "Install Go"
+        uses: actions/setup-go@v2
+        with:
+          go-version: '1.16.2'
+
+      - name: "Install 'Docker for Mac' (Latest)"
+        uses: docker-practice/actions-setup-docker@v1  # needed while TestGetEnvoyExtensionPush uses a real registry
+        if: runner.os == 'macOS'
+        with:
+          docker_buildx: false  # Install is flakey. When it, we can install it via docker/setup-buildx-action@v1
+        timeout-minutes: 20  # fail fast if MacOS install takes too long
+
+      - name: "Init on first use"
+        run: make init
+
+      - name: "Verify clean check-in"
+        run: make check
+
+      - name: "Run unit tests"
+        # prefetch implicit version needed by pkg/binary/envoy/controlplane/istio_test.go until #136. This avoids:
+        # Unable to start Envoy process: fork/exec /home/circleci/.getenvoy/builds/standard/1.11.0/linux_glibc/bin/envoy: text file busy
+        run: |
+          go run cmd/getenvoy/main.go fetch standard:1.11.0
+          make test
+
+      - name: "Generate test coverage report"
+        if: runner.os == 'Linux'  # no need to do this per operating system
+        run: make coverage
+
+      - name: "Upload test coverage report"
+        uses: actions/upload-artifact@v2
+        if: runner.os == 'Linux'  # no need to do this per operating system
+        with:
+          name: coverage
+          path: build/coverage
+
   bin:
     name: "Build the `getenvoy` binary for use in e2e tests"
     runs-on: ubuntu-latest

Makefile

@@ -47,11 +47,9 @@ GO_TEST_EXTRA_OPTS ?=
 
 # TODO(yskopets): include all packages into test run once blocking issues have been resolved, including
 # * https://github.com/tetratelabs/getenvoy/issues/87 `go test -race` fails
-# * https://github.com/tetratelabs/getenvoy/issues/88 `go test ./...` fails on Mac
-# * https://github.com/tetratelabs/getenvoy/issues/89 `go test github.com/tetratelabs/getenvoy/pkg/binary/envoy/controlplane` removes `/tmp` dir
 COVERAGE_PKG_LIST ?= $(shell go list ./pkg/... | grep -v -e github.com/tetratelabs/getenvoy/pkg/binary/envoy/controlplane -e github.com/tetratelabs/getenvoy/pkg/binary/envoy/debug)
 GO_COVERAGE_OPTS ?= -covermode=atomic -coverpkg=./...
-GO_COVERAGE_EXTRA_OPTS ?=
+GO_COVERAGE_EXTRA_OPTS ?= -p 1
 
 E2E_PKG_LIST ?= ./test/e2e
 # Set the default timeout >10m as particularly rust e2e tests are slow https://golang.org/cmd/go/#hdr-Testing_flags
@@ -100,10 +98,6 @@ test: generate
 	docker-compose up -d
 	go test $(GO_TEST_OPTS) $(GO_TEST_EXTRA_OPTS) $(TEST_PKG_LIST)
 
-.PHONY: test.ci
-test.ci: generate
-	go test $(GO_TEST_OPTS) $(GO_TEST_EXTRA_OPTS) $(TEST_PKG_LIST)
-
 .PHONY: e2e
 e2e: $(call GETENVOY_OUT_PATH,$(GOOS),$(GOARCH))
 	docker-compose up -d

go.mod

@@ -36,7 +36,6 @@ require (
 	github.com/tetratelabs/getenvoy-package v0.0.0-20190730071641-da31aed4333e
 	github.com/tetratelabs/log v0.0.0-20190710134534-eb04d1e84fb8
 	github.com/tetratelabs/multierror v1.1.0
-	gotest.tools v2.2.0+incompatible
 	istio.io/api v0.0.0-20200227213531-891bf31f3c32
 	istio.io/istio v0.0.0-20200304114959-c3c353285578
 	rsc.io/letsencrypt v0.0.3 // indirect

pkg/binary/envoy/controlplane/istio.tmpl_test.go

@@ -24,55 +24,43 @@ import (
 	"testing"
 
 	"github.com/mholt/archiver"
-	"gotest.tools/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func Test_VersionedIstioTemplate(t *testing.T) {
-	t.Run(fmt.Sprintf("checking Istio bootstrap matches version %s", IstioVersion), func(t *testing.T) {
-		got := retrieveIstioBootstrap(t)
-		assert.Equal(t, istioBootStrapTemplate, string(got))
-	})
-}
-
-func retrieveIstioBootstrap(t *testing.T) []byte {
+// TestIstioBootStrapTemplateEqualsReleaseJson ensures istioBootStrapTemplate is exactly the same as what would have been
+// downloaded from the istio release for version IstioVersion.
+func TestIstioBootStrapTemplateEqualsReleaseJson(t *testing.T) {
 	// Retrieve Istio tarball os doesn't matter we only car about the bootstrap JSON
-	url := fmt.Sprintf("https://github.com/istio/istio/releases/download/%v/istio-%v-linux.tar.gz", IstioVersion, IstioVersion)
+	url := fmt.Sprintf("https://github.com/istio/istio/releases/download/%s/istio-%s-linux.tar.gz", IstioVersion, IstioVersion)
 	resp, err := http.Get(url)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err, "error getting tarball for istio version %s", IstioVersion)
+
 	defer resp.Body.Close() //nolint
-	if resp.StatusCode != http.StatusOK {
-		t.Errorf("received %v status code", resp.StatusCode)
-	}
-	dst := os.TempDir()
+	require.Equal(t, http.StatusOK, resp.StatusCode, "unexpected HTTP status from %s", url)
+
+	dst, err := ioutil.TempDir("", "")
+	require.NoError(t, err, `ioutil.TempDir("", "") erred`)
 	defer os.RemoveAll(dst)
+
 	tarball := filepath.Join(dst, "istio.tar.gz")
 	f, err := os.OpenFile(tarball, os.O_CREATE|os.O_WRONLY, 0600)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err, "Couldn't open file %s", tarball)
 	defer f.Close() //nolint
+
 	_, err = io.Copy(f, resp.Body)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err, "Couldn't download %s into %s", url, tarball)
 
 	// Walk the tarball until we find the bootstrap
+	bootstrapName := "envoy_bootstrap_v2.json"
 	var bytes []byte
-	if walkErr := archiver.Walk(tarball, func(f archiver.File) error {
-		if f.Name() == "envoy_bootstrap_v2.json" {
+	err = archiver.Walk(tarball, func(f archiver.File) error {
+		if f.Name() == bootstrapName {
 			bytes, err = ioutil.ReadAll(f)
-			if err != nil {
-				return err
-			}
+			require.NoError(t, err, "error reading %s in %s", bootstrapName, url)
 		}
 		return nil
-	}); walkErr != nil {
-		t.Fatal(err)
-	}
-	if bytes == nil {
-		t.Fatal("no boostrap found")
-	}
-	return bytes
+	})
+
+	require.NotNil(t, bytes, "couldn't find %s in %s", bootstrapName, url)
+	require.Equal(t, istioBootStrapTemplate, string(bytes), "istioBootStrapTemplate isn't the same as the istio %s distribution", IstioVersion)
 }

pkg/binary/envoy/controlplane/istio_test.go

@@ -16,14 +16,13 @@ package controlplane
 
 import (
 	"context"
-	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"istio.io/istio/pilot/pkg/bootstrap"
 	"istio.io/istio/tests/util"
 
@@ -32,42 +31,44 @@ import (
 	"github.com/tetratelabs/getenvoy/pkg/binary/envoytest"
 )
 
-func TestMain(m *testing.M) {
-	if err := envoytest.Fetch(); err != nil {
-		fmt.Println(err)
-		os.Exit(1)
-	}
-	os.Exit(m.Run())
-}
+func TestConnectsToMockPilotAsAGateway(t *testing.T) {
+	err := envoytest.Fetch()
+	require.NoError(t, err, "error running envoytest.Fetch()")
+	_, teardown := setupMockPilot()
+	defer teardown()
 
-// NOTE: This test will fail on macOS due to an issue with Envoy, the same issue as debug logging
-func Test_IstioGateway(t *testing.T) {
-	t.Run("connects to mock Pilot as a gateway", func(t *testing.T) {
-		_, teardown := setupMockPilot()
-		defer teardown()
-		cfg := envoy.NewConfig(
-			func(c *envoy.Config) {
-				c.Mode = envoy.ParseMode("loadbalancer")
-				c.XDSAddress = util.MockPilotGrpcAddr
-				c.IPAddresses = []string{"1.1.1.1"}
-			},
-		)
-		runtime, _ := envoy.NewRuntime(
-			func(r *envoy.Runtime) { r.Config = cfg },
-			debug.EnableEnvoyAdminDataCollection,
-			Istio,
-		)
-		defer os.RemoveAll(runtime.DebugStore() + ".tar.gz")
-		defer os.RemoveAll(runtime.DebugStore())
-		ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
-		defer cancel()
-		assert.NoError(t, envoytest.Run(ctx, runtime, ""))
-		time.Sleep(time.Millisecond * 500) // Pilot config propagation
-		assert.NoError(t, envoytest.Kill(ctx, runtime))
-		gotListeners, _ := ioutil.ReadFile(filepath.Join(runtime.DebugStore(), "listeners.txt"))
-		assert.Contains(t, string(gotListeners), "0.0.0.0_8443::0.0.0.0:8443")
-		assert.Contains(t, string(gotListeners), "0.0.0.0_8080::0.0.0.0:8080")
-	})
+	cfg := envoy.NewConfig(
+		func(c *envoy.Config) {
+			c.Mode = envoy.ParseMode("loadbalancer")
+			c.XDSAddress = util.MockPilotGrpcAddr
+			c.IPAddresses = []string{"1.1.1.1"}
+		},
+	)
+
+	runtime, err := envoy.NewRuntime(
+		func(r *envoy.Runtime) { r.Config = cfg },
+		debug.EnableEnvoyAdminDataCollection,
+		Istio,
+	)
+	require.NoError(t, err, "error creating envoy runtime")
+	defer os.RemoveAll(runtime.DebugStore())
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
+	defer cancel()
+
+	err = envoytest.Run(ctx, runtime, "")
+	require.NoError(t, err, "error running envoy")
+
+	time.Sleep(time.Millisecond * 500) // Pilot config propagation
+
+	err = envoytest.Kill(ctx, runtime)
+	require.NoError(t, err, "error killing envoy")
+
+	gotListeners, err := ioutil.ReadFile(filepath.Join(runtime.DebugStore(), "listeners.txt"))
+	require.NoError(t, err, "error killing envoy listeners")
+
+	require.Contains(t, string(gotListeners), "0.0.0.0_8443::0.0.0.0:8443")
+	require.Contains(t, string(gotListeners), "0.0.0.0_8080::0.0.0.0:8080")
 }
 
 func setupMockPilot() (*bootstrap.Server, util.TearDownFunc) {
@@ -76,6 +77,10 @@ func setupMockPilot() (*bootstrap.Server, util.TearDownFunc) {
 		args.Config.FileDir = "testdata"
 		args.Plugins = bootstrap.DefaultPlugins
 		args.Mesh.MixerAddress = ""
+		// In a normal macOS setup, you cannot write to /dev/stdout, which is the default path here.
+		// While not Docker-specific, there are related notes here https://github.com/moby/moby/issues/31243
+		// Since this test doesn't read access logs anyway, the easier workaround is to disable access logging.
+		args.MeshConfig.AccessLogFile = ""
 		args.Service.Registries = []string{}
 	})
 }

pkg/binary/envoy/debug/lsof.go

@@ -45,7 +45,8 @@ type OpenFileStat struct {
 	Name   string `json:"name"` // name of the mount point and file system on which the file resides
 }
 
-// EnableOpenFilesDataCollection is a preset option that registers collection of statistics of files opened by envoy instance(s)
+// EnableOpenFilesDataCollection is a preset option that registers collection of statistics of files opened by envoy
+// instance(s). This is unsupported on macOS/Darwin because it does not support process.OpenFiles
 func EnableOpenFilesDataCollection(r *envoy.Runtime) {
 	if err := os.Mkdir(filepath.Join(r.DebugStore(), "lsof"), os.ModePerm); err != nil {
 		log.Errorf("error in creating a directory to write open file data of envoy to: %v", err)
@@ -80,7 +81,7 @@ func retrieveOpenFilesData(r binary.Runner) error {
 
 	openFiles, err := envoyProcess.OpenFiles()
 	if err != nil {
-		return fmt.Errorf("error in getting open file statistics: %v", err)
+		return fmt.Errorf("error in getting open file statistics: %w", err)
 	}
 
 	for _, stat := range openFiles {