From 2e45ce14a4188eddedd59bf8794b142b7f548c6e Mon Sep 17 00:00:00 2001
From: Zach Koopmans <zkoopmans@google.com>
Date: Fri, 21 Apr 2023 14:22:58 -0700
Subject: [PATCH] Fix flakes in portforward test.

Attempt to fix flaky port forward test using "netstat" in the container
to check the a process is listening on the port before attempting to
connect.

Also, re-enable the test in a seperate job, but w/ a soft fail. This
will be removed later in favor of adding port-forward to integration
tests to remove the high overhead of starting a new job (scheduling,
compiling runsc, downloading images, etc).

PiperOrigin-RevId: 526135859
---
 .buildkite/pipeline.yaml      | 11 +++++--
 Makefile                      |  4 +--
 images/basic/redis/Dockerfile |  2 ++
 test/root/portforward_test.go | 60 +++++++++++++++++++++--------------
 4 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/.buildkite/pipeline.yaml b/.buildkite/pipeline.yaml
index 5cb3c04fd..59f6f7728 100644
--- a/.buildkite/pipeline.yaml
+++ b/.buildkite/pipeline.yaml
@@ -238,7 +238,6 @@ steps:
       <<: *kvm_agents
       cgroup: "v2"
       arch: "amd64"
-
   # All system call tests.
   - <<: *common
     label: ":toolbox: System call tests (AMD64)"
@@ -271,7 +270,7 @@ steps:
     # See above: not truly a source test.
     <<: *source_test
     label: ":docker: Docker tests (cgroupv2)"
-    command: make portforward-tests docker-tests
+    command: make docker-tests
     agents:
       <<: *ubuntu_agents
       arch: "amd64"
@@ -284,6 +283,14 @@ steps:
       <<: *platform_specific_agents
       <<: *ubuntu_agents
       arch: "amd64"
+  - <<: *common
+    <<: *docker
+    label: ":one-does-not-simply: Port-Forward tests"
+    command: make portforward-tests
+    agents:
+      <<: *ubuntu_agents
+      arch: "amd64"
+    soft_fail: true
   - <<: *common
     <<: *docker
     label: ":safety_pin: Host network tests"
diff --git a/Makefile b/Makefile
index 07b928753..c542e867f 100644
--- a/Makefile
+++ b/Makefile
@@ -264,9 +264,9 @@ simple-tests: unit-tests # Compatibility target.
 
 portforward-tests: load-basic_redis load-basic_nginx $(RUNTIME_BIN)
 	@$(call install_runtime,$(RUNTIME),--network=sandbox)
-	@$(call sudo,test/root:portforward_test,--runtime=$(RUNTIME) -test.v)
+	@$(call sudo,test/root:portforward_test,--runtime=$(RUNTIME) -test.v $(ARGS))
 	@$(call install_runtime,$(RUNTIME),--network=host)
-	@$(call sudo,test/root:portforward_test,--runtime=$(RUNTIME) -test.v)
+	@$(call sudo,test/root:portforward_test,--runtime=$(RUNTIME) -test.v $(ARGS))
 .PHONY: portforward-test
 
 # Standard integration targets.
diff --git a/images/basic/redis/Dockerfile b/images/basic/redis/Dockerfile
index 0f17249af..3083193b3 100644
--- a/images/basic/redis/Dockerfile
+++ b/images/basic/redis/Dockerfile
@@ -1 +1,3 @@
 FROM redis:5.0.4
+
+RUN apt-get update && apt-get install -y net-tools
\ No newline at end of file
diff --git a/test/root/portforward_test.go b/test/root/portforward_test.go
index 4af9d56cf..03d1343bc 100644
--- a/test/root/portforward_test.go
+++ b/test/root/portforward_test.go
@@ -54,6 +54,10 @@ func TestPortForwardLocalMode(t *testing.T) {
 		t.Fatalf("failed to create redis server: %v", err)
 	}
 
+	if err := waitUntilServerIsUp(ctx, server, redisPort); err != nil {
+		t.Fatalf("failed to wait for redis server to be up: %v", err)
+	}
+
 	localPort, err := getUnusedPort()
 	if err != nil {
 		t.Fatalf("failed to pick unused port: %v", err)
@@ -120,30 +124,8 @@ func TestPortForwardStreamMode(t *testing.T) {
 		t.Fatalf("failed to create nginx server: %v", err)
 	}
 
-	// This is a bit crude, but we need to make sure the server is up without exposing a port to the
-	// host. When the server container boots, the nginx process should run first. If we run nginx
-	// again, it will fail to bind to port 80. Run exec calls until we get that failure.
-	serverUpChan := make(chan struct{}, 1)
-	var upOut string
-	var upErr error
-	reg := regexp.MustCompile(`0\.0\.0\.0:80[\s]*0\.0\.0\.0:\*[\s]*LISTEN`)
-	go func() {
-		for {
-			time.Sleep(time.Millisecond * 500)
-			upOut, upErr = server.Exec(ctx, dockerutil.ExecOpts{}, []string{"netstat", "-l"}...)
-			if reg.MatchString(upOut) {
-				serverUpChan <- struct{}{}
-				return
-			}
-		}
-	}()
-
-	// If the server isn't up after 10 seconds, there is probably something wrong.
-	select {
-	case <-serverUpChan:
-		break
-	case <-time.After(time.Second * 30):
-		t.Fatalf("could not verify server is up: err: %v out: %s", upErr, upOut)
+	if err := waitUntilServerIsUp(ctx, server, nginxPort); err != nil {
+		t.Fatalf("failed to wait for nginx server to be up: %v", err)
 	}
 
 	socket, err := net.Listen("unix", sockAddr)
@@ -192,6 +174,36 @@ func getUnusedPort() (int, error) {
 	return l.Addr().(*net.TCPAddr).Port, nil
 }
 
+func waitUntilServerIsUp(ctx context.Context, server *dockerutil.Container, port int) error {
+	// This is a bit crude, but we need to make sure the server is up without exposing a port to the
+	// host. When the server container boots, the nginx process should run first. If we run nginx
+	// again, it will fail to bind to port 80. Run exec calls until we get that failure.
+	serverUpChan := make(chan struct{})
+	var upOut string
+	var upErr error
+	reg := regexp.MustCompile(fmt.Sprintf(`0\.0\.0\.0:%d[\s]*0\.0\.0\.0:\*[\s]*LISTEN`, port))
+	go func() {
+		for {
+			time.Sleep(time.Millisecond * 500)
+			upOut, upErr = server.Exec(ctx, dockerutil.ExecOpts{}, []string{"netstat", "-l"}...)
+			if reg.MatchString(upOut) {
+				close(serverUpChan)
+				return
+			}
+		}
+	}()
+
+	// If the server isn't up after 30 seconds, there is probably something wrong.
+	select {
+	case <-serverUpChan:
+		break
+	case <-time.After(time.Second * 30):
+		return fmt.Errorf("could not verify server is up: err: %v out: %s", upErr, upOut)
+	}
+
+	return nil
+}
+
 type portForwardProcess struct {
 	cmd *exec.Cmd
 	buf bytes.Buffer