Upgrades Envoy 1.11 -> 1.12 and Istio 1.2 -> 1.6 #168

codefromthecrypt · 2021-04-07T11:56:30Z

This upgrades Envoy from 1.11 -> 1.12 and Istio from 1.2 -> 1.6 and fixes the MacOS build.

Since Envoy 1.12 fixes /dev/stdout on OS/x, EnableEnvoyLogCollection can now work on macOS. Upgrading further will be in another pull request.

This also fixes functionality related to lsof and debug. Before, we didn't use timeouts, which could result in hangs. Now, we wait up to 3 seconds to capture this information.

fixes #88

General notes

This also fixes race conditions by reading back the admin port in tests via the "--admin-address-path" flag as implemented in envoy.EnableAdminAddressDetection

This intentionally stops at Istio 1.6 as versions beyond this imply configuration changes to our test data.

This should also fix #88 (macOS support)

On go:embed for the istio envoy bootstrap template

Before, our unit tests would download a 30MB+ targz and copy it to verify that a json file from envoy was still the same contents. This replaces that code with go:embed so we don't need to test it (because there's no risk of code formatters or anything else interfering with it.

This removes a large amount of the unit test runtime and with it a test that isn't good to execute -race against!

$ go test ./pkg/binary/envoy/controlplane -run TestIstioBootStrapTemplateEqualsReleaseJson
ok  	github.com/tetratelabs/getenvoy/pkg/binary/envoy/controlplane	7.127s

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

pkg/binary/envoy/controlplane/istio.go

codefromthecrypt · 2021-04-08T01:22:08Z

tests were failing because docker install failed for rust+osx, then later they failed because I kicked the build too many times (docker.io rate limit) oh well

codefromthecrypt · 2021-04-08T01:28:39Z

added a commit to hopefully help with #149

codefromthecrypt · 2021-04-08T05:03:33Z

I will repurpose this PR to fix all the debug/controlplane tests as the build is so flakey it is annoying to have to keep re-kicking it

lizan · 2021-04-09T06:58:57Z

.github/workflows/commit.yaml

        run: |
-          go run cmd/getenvoy/main.go fetch standard:1.11.0
+          go run cmd/getenvoy/main.go fetch standard:1.12.7


Let's update this to latest, 1.17.1?

There are some issues in doing that related to dropped config v2 support, so want to make a step first. Happy to do that later according to #169 sg?

yeah let's do v3 support and upgrading to 1.17.1 in a separate PR

codefromthecrypt · 2021-04-09T07:03:56Z

pkg/binary/envoy/adminport.go

+// EnableAdminAddressDetection sets the "--admin-address-path" flag and reads it back before attempting ready checks.
+//
+// Notably, this allows ephemeral admin ports via bootstrap configuration admin/port_value=0
+func EnableAdminAddressDetection(r *Runtime) {


I owe unit tests on this, but waiting to see if this passes CI first as there are some files that only build on linux and tests that only execute on linux. Once this PR passes, I'll backfill tests on this..

codefromthecrypt · 2021-04-09T07:04:49Z

pkg/binary/envoy/controlplane/istio_test.go


-func setupMockPilot() (*bootstrap.Server, util.TearDownFunc) {
-	return util.EnsureTestServer(func(args *bootstrap.PilotArgs) {


this file was deleted from istio, so I had to re-do it from scratch...

codefromthecrypt · 2021-04-09T07:05:39Z

pkg/binary/envoy/debug/testdata/log.yaml

@@ -59,4 +59,4 @@ admin:
  address:
    socket_address:
      address: 0.0.0.0
-      port_value: 15000
+      port_value: 0


this was a revelation, the insight that you can set this to zero and use --admin-address-path to avoid constant port conflicts!

codefromthecrypt · 2021-04-09T09:07:29Z

the gnostic case sensitivity issue caused by istio 1.6 is wreaking havoc on the code quality tools, sadly in a way I can't reproduce locally. I'd really like to hear we can move to at least istio 1.7 or delete this function as I don't know if anyone is using it especially as it was using templates from istio 1.2.

codefromthecrypt · 2021-04-09T11:00:28Z

pkg/binary/envoy/runtime.go

-		if r.envoyReady() {
-			return binary.StatusReady
+	case r.cmd.ProcessState == nil: // the process started, but it hasn't completed, yet
+		// TODO: envoyReady() can succeed when there's a port collision even when the process managed by this


this took me a lot of head scratching to eventually understand, hence the comment now.

codefromthecrypt · 2021-04-09T11:00:58Z

pkg/binary/envoy/testdata/sleep.sh

-}
-
-trap terminate SIGINT SIGTERM SIGKILL
+# This script is used to simulate a process that never terminates unless it receives SIGINT or SIGTERM


changes to make this macOS friendly

codefromthecrypt · 2021-04-10T01:54:48Z

pkg/binary/envoy/debug/log.go

-// EnableEnvoyLogCollection is a preset option that registers collection of Envoy Access Logs
-//
-// This is not supported on non-Linux platforms as Envoy is not able to access /dev/stdout unless
-// *exec.Cmd.Stdout == os.Stdout. This makes it impossible to add the MultiWriter and start Envoy.


this is fixed in envoy 1.12

codefromthecrypt · 2021-04-10T01:55:45Z

pkg/binary/envoy/debug/lsof.go

-	f, err := os.Create(filepath.Join(r.DebugStore(), "lsof/lsof.json"))
-	if err != nil {
-		return fmt.Errorf("error in creating a file to write open file statistics to: %v", err)
+	ctx, cancel := context.WithTimeout(context.Background(), processTimeout)


we have to use the XXXWithContext functionality here, else things can hang forever

codefromthecrypt · 2021-04-10T02:07:37Z

pkg/binary/envoy/runtime.go

-
-// WaitWithContext blocks until the child process reaches the state passed or the context is canceled
-// Note: It does not guarantee that it is in the specified state just that it has reached it
-func (r *Runtime) WaitWithContext(ctx context.Context, state int) {


the wait function was only used once EnableEnvoyLogCollection, so moved it there

codefromthecrypt · 2021-04-10T08:52:17Z

fingers crossed this time... it has been a battlefield, but I think macOS finally works in CI now

codefromthecrypt · 2021-04-10T14:38:57Z

removed the docker dep from unit tests as it has been getting rate-limited and was never a good idea anyway.

the flakiness in the debug and istio tests has to do with parallel runs, something we don't do in e2e and these are definitely more like e2e tests than unit tests (they literally download and run envoy). I'll take another look tomorrow to see if I can improve it.

codefromthecrypt · 2021-04-10T23:54:01Z

I rebased over #171 as getting tired of waiting for docker.io rate limits to expire every day

codefromthecrypt · 2021-04-11T00:08:52Z

@mathetake PTAL as I'm pretty done on this, but will help follow-up on istio upgrade if there's a decision in another PR

codefromthecrypt · 2021-04-11T00:09:42Z

.github/workflows/commit.yaml

      matrix:
        runner:
-          # - os: macos-latest  ## revisit once all tests are off ginkgo
+          - os: macos-latest


easy to miss this, but this is now using macos

godownloader.sh

pkg/binary/envoytest/util.go

pkg/binary/envoy/adminport.go

pkg/binary/envoy/runtime.go

pkg/binary/envoytest/util.go

mathetake · 2021-04-12T01:24:45Z

pkg/binary/envoy/debug/log_linux.go

@@ -1,85 +0,0 @@
-// Copyright 2019 Tetrate


just curious why this file is prefixed _linux..? 😄

this makes it only compile in linux, which is a compilation bug trap I ran into! you can drift files and it will compile fine since the build skips based on this pattern

pkg/binary/envoy/debug/node.go

This upgrades Envoy from 1.11 -> 1.12 and Istio from 1.2 -> 1.6. This also fixes race conditions by reading back the admin port in tests via the "--admin-address-path" flag as implemented in `envoy.EnableAdminAddressDetection` This intentionally stops at Istio 1.6 as versions beyond this imply configuration changes to our test data. Signed-off-by: Adrian Cole <[email protected]>

Co-authored-by: Takeshi Yoneda <[email protected]>

Signed-off-by: Adrian Cole <[email protected]>

codefromthecrypt · 2021-04-12T01:52:55Z

Thanks @mathetake once again, it is better after you reviewed it, and also I didn't accidentally break the website!

codefromthecrypt requested a review from mathetake April 7, 2021 11:56

codefromthecrypt commented Apr 7, 2021

View reviewed changes

pkg/binary/envoy/controlplane/istio.tmpl_test.go Outdated Show resolved Hide resolved

codefromthecrypt changed the title ~~Upgrades archiver lib and makes bootstrap template test streaming~~ Upgrades archiver lib and makes bootstrap template test simpler Apr 7, 2021

codefromthecrypt commented Apr 7, 2021

View reviewed changes

pkg/binary/envoy/controlplane/istio.tmpl_test.go Outdated Show resolved Hide resolved

codefromthecrypt commented Apr 7, 2021

View reviewed changes

pkg/binary/envoy/controlplane/istio.go Outdated Show resolved Hide resolved

codefromthecrypt changed the title ~~Upgrades archiver lib and makes bootstrap template test simpler~~ Upgrades archiver lib and replaces istio.tmpl.go with go:embed Apr 7, 2021

codefromthecrypt added the tech debt label Apr 7, 2021

codefromthecrypt force-pushed the upgrade-archiver branch from 0f33c85 to b80c8f6 Compare April 7, 2021 23:16

mathetake approved these changes Apr 8, 2021

View reviewed changes

codefromthecrypt force-pushed the upgrade-archiver branch from 0f308ea to b80c8f6 Compare April 8, 2021 02:07

codefromthecrypt force-pushed the upgrade-archiver branch from b80c8f6 to cb96ef0 Compare April 9, 2021 06:44

codefromthecrypt changed the title ~~Upgrades archiver lib and replaces istio.tmpl.go with go:embed~~ Upgrades Envoy 1.11 -> 1.12 and Istio 1.2 -> 1.6 Apr 9, 2021

codefromthecrypt mentioned this pull request Apr 9, 2021

WIP: updates to latest dependencies #128

Closed

codefromthecrypt requested a review from mathetake April 9, 2021 06:52

codefromthecrypt mentioned this pull request Apr 9, 2021

Clarify version support policy #169

Closed

lizan reviewed Apr 9, 2021

View reviewed changes

codefromthecrypt force-pushed the upgrade-archiver branch from 412adb2 to 80d5584 Compare April 9, 2021 07:01

codefromthecrypt commented Apr 9, 2021

View reviewed changes

codefromthecrypt force-pushed the upgrade-archiver branch from efe4164 to 35c2e6f Compare April 9, 2021 09:21

codefromthecrypt commented Apr 9, 2021

View reviewed changes

codefromthecrypt commented Apr 10, 2021

View reviewed changes

codefromthecrypt force-pushed the upgrade-archiver branch from caaaf08 to 959f71f Compare April 10, 2021 08:51

codefromthecrypt force-pushed the upgrade-archiver branch from 56fd1b1 to 8089525 Compare April 10, 2021 23:53

codefromthecrypt changed the base branch from master to fix-rate-limits April 10, 2021 23:53

codefromthecrypt commented Apr 11, 2021

View reviewed changes

codefromthecrypt force-pushed the upgrade-archiver branch from 8089525 to e9ecbd4 Compare April 11, 2021 00:14