Benchmarks Will Now Be Tested During CI (#44486)

* Adding flags for benchmarks in unit test CI

* Upping timeout for unit tests to accomodate benchmarks

* Fixing a typo

* Run benchmarks once

* Adding a makefile target for ci bench test

* Removing a stray quotation

* Compacting workflow

* Benchmarks now with nicer output

* Benchmarks back to their own step

* With error messages

* Adding pipefail to preserve exit code

* Using bash

* Simpler command and including e

* Separating benchmarks into its own workflow

* Fixing benchmarks workflow name

* Ignoring benchmarks that require root

* Updating to account for root and nonroot bench

* Cleaning up the benchmark workflows

* Bench requires test log dir to be created

* Excluding all BenchmarkRoot benchmarks from running

* Bumping benchGetNodes timeout duration to account for some jitter

* Fixing multiplexer benchmark by mirroring unit tests

* Remembered that skip exists

* Skipping BenchmarkGetMaxNodes due to flake

* Fixing formatting of summary

* Actually fixing summary

* Adding another newline

* Simplifying setup for mux listener

* Excluding BenchmarkRootExecCommand

.github/workflows/benchmark-code-nonroot.yaml

@@ -0,0 +1,71 @@
+name: Benchmarks (Go)
+run-name: Benchmarks (Go) - ${{ github.run_id }} - @${{ github.actor }}
+
+on:
+  pull_request:
+
+  merge_group:
+
+jobs:
+  changes:
+    name: Check for relevant changes
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: read
+    outputs:
+      changed: ${{ steps.changes.outputs.changed }}
+    steps:
+      - name: Checkout
+        if: ${{ github.event_name == 'merge_group' }}
+        uses: actions/checkout@v4
+      - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
+        id: changes
+        with:
+          base: ${{ github.event.pull_request.base.ref || github.event.merge_group.base_ref }}
+          ref: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }}
+          filters: |
+            changed:
+              - '.github/workflows/benchmark-code-nonroot.yaml'
+              - '**.go'
+              - 'go.mod'
+              - 'go.sum'
+              - 'build.assets/Makefile'
+              - 'build.assets/Dockerfile*'
+              - 'Makefile'
+
+  test:
+    name: Benchmarks (Go)
+    needs: changes
+    if: ${{ !startsWith(github.head_ref, 'dependabot/') && needs.changes.outputs.changed == 'true' }}
+    runs-on: ubuntu-22.04-32core
+
+    permissions:
+      contents: read
+
+    container:
+      image: ghcr.io/gravitational/teleport-buildbox:teleport17
+      env:
+        TELEPORT_XAUTH_TEST: yes
+        WEBASSETS_SKIP_BUILD: 1
+
+    steps:
+      - name: Checkout Teleport
+        uses: actions/checkout@v4
+
+      - name: Prepare workspace
+        id: prepare
+        uses: ./.github/actions/prepare-workspace
+
+      # Run benchmarks once to make sure they don't break
+      # Must be run separate since gotestsum is not compatible with benchmark output
+      - name: Run Benchmarks Once
+        timeout-minutes: 5
+        shell: bash # Overriding default shell which is `sh -e`
+        run: make test-go-bench | sed -u -E "s/^(FAIL\s+github)/::error title=Benchmark Failed::\1/"
+
+      - name: Construct Summary
+        shell: bash
+        run: |
+          echo '```' >> $GITHUB_STEP_SUMMARY
+          go run golang.org/x/perf/cmd/benchstat@latest test-logs/bench.txt \
+          | sed -E -e 's/^pkg:\s+(github.*)/\n```\n## \1\n\n```/'  >> "$GITHUB_STEP_SUMMARY"

.github/workflows/benchmark-code-root.yaml

@@ -0,0 +1,71 @@
+name: Benchmarks (Root) (Go)
+run-name: Benchmarks (Root) (Go) - ${{ github.run_id }} - @${{ github.actor }}
+
+on:
+  pull_request:
+
+  merge_group:
+
+jobs:
+  changes:
+    name: Check for relevant changes
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: read
+    outputs:
+      changed: ${{ steps.changes.outputs.changed }}
+    steps:
+      - name: Checkout
+        if: ${{ github.event_name == 'merge_group' }}
+        uses: actions/checkout@v4
+      - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
+        id: changes
+        with:
+          base: ${{ github.event.pull_request.base.ref || github.event.merge_group.base_ref }}
+          ref: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }}
+          filters: |
+            changed:
+              - '.github/workflows/benchmark-code-root.yaml'
+              - '**.go'
+              - 'go.mod'
+              - 'go.sum'
+              - 'build.assets/Makefile'
+              - 'build.assets/Dockerfile*'
+              - 'Makefile'
+
+  test:
+    name: Benchmarks (Root)
+    needs: changes
+    if: ${{ !startsWith(github.head_ref, 'dependabot/') && needs.changes.outputs.changed == 'true' }}
+    runs-on: ubuntu-22.04-32core
+
+    permissions:
+      contents: read
+
+    container:
+      image: ghcr.io/gravitational/teleport-buildbox:teleport17
+      options: --cap-add=SYS_ADMIN --privileged
+      env:
+        WEBASSETS_SKIP_BUILD: 1
+        TELEPORT_XAUTH_TEST: yes
+
+    steps:
+      - name: Checkout Teleport
+        uses: actions/checkout@v4
+
+      - name: Prepare workspace
+        uses: ./.github/actions/prepare-workspace
+
+      # Run benchmarks once to make sure they don't break
+      # Must be run separate since gotestsum is not compatible with benchmark output
+      - name: Run Benchmarks Once
+        timeout-minutes: 5
+        shell: bash # Overriding default shell which is `sh -e`
+        run: make test-go-bench-root | sed -u -E "s/^(FAIL\s+github)/::error title=Benchmark Failed::\1/"
+
+      - name: Construct Summary
+        shell: bash
+        run: |
+          echo '```' >> $GITHUB_STEP_SUMMARY
+          go run golang.org/x/perf/cmd/benchstat@latest test-logs/bench.txt \
+          | sed -E -e 's/^pkg:\s+(github.*)/\n```\n## \1\n\n```/'  >> "$GITHUB_STEP_SUMMARY"

.github/workflows/unit-tests-code.yaml

@@ -63,6 +63,7 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Prepare workspace
+        id: prepare
         uses: ./.github/actions/prepare-workspace
 
       - name: Mount debugfs

Makefile

@@ -878,6 +878,25 @@ ifneq ("$(TOUCHID_TAG)", "")
 		| gotestsum --raw-command -- cat
 endif
 
+# Runs benchmarks once to make sure they pass.
+# This is intended to run in CI during unit testing to make sure benchmarks don't break.
+# To limit noise and improve speed this will only run on packages that have benchmarks.
+# Race detection is not enabled because it significantly slows down benchmarks.
+# todo: Use gotestsum when it is compatible with benchmark output. Currently will consider all benchmarks failed.
+.PHONY: test-go-bench
+test-go-bench: PACKAGES = $(shell grep --exclude-dir api --include "*_test.go" -lr testing.B .  | xargs dirname | xargs go list | sort -u)
+test-go-bench: BENCHMARK_SKIP_PATTERN = "^BenchmarkRoot|^BenchmarkGetMaxNodes$$"
+test-go-bench: | $(TEST_LOG_DIR)
+	go test -run ^$$ -bench . -skip $(BENCHMARK_SKIP_PATTERN) -benchtime 1x $(PACKAGES) \
+		| tee $(TEST_LOG_DIR)/bench.txt
+
+test-go-bench-root: PACKAGES = $(shell grep --exclude-dir api --include "*_test.go" -lr BenchmarkRoot .  | xargs dirname | xargs go list | sort -u)
+test-go-bench-root: BENCHMARK_PATTERN = "^BenchmarkRoot"
+test-go-bench-root: BENCHMARK_SKIP_PATTERN = "^BenchmarkRootExecCommand$$"
+test-go-bench-root: | $(TEST_LOG_DIR)
+	go test -run ^$$ -bench $(BENCHMARK_PATTERN) -skip $(BENCHMARK_SKIP_PATTERN) -benchtime 1x $(PACKAGES) \
+		| tee $(TEST_LOG_DIR)/bench.txt
+
 # Make sure untagged vnetdaemon code build/tests.
 .PHONY: test-go-vnet-daemon
 test-go-vnet-daemon: FLAGS ?= -race -shuffle on

lib/multiplexer/multiplexer_test.go

@@ -832,42 +832,20 @@ func TestMux(t *testing.T) {
 		// If listener for IPv6 will fail to be created we'll skip IPv6 portion of test.
 		listener6, _ := net.Listen("tcp6", "[::1]:0")
 
-		startServing := func(muxListener net.Listener, cluster string) (*Mux, *httptest.Server) {
-			mux, err := New(Config{
-				Listener:            muxListener,
-				PROXYProtocolMode:   PROXYProtocolUnspecified,
-				CertAuthorityGetter: casGetter,
-				Clock:               clockwork.NewFakeClockAt(time.Now()),
-				LocalClusterName:    cluster,
-			})
-			require.NoError(t, err)
-
-			muxTLSListener := mux.TLS()
-
-			go mux.Serve()
-
-			backend := &httptest.Server{
-				Listener: muxTLSListener,
-
-				Config: &http.Server{
-					Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-						fmt.Fprint(w, r.RemoteAddr)
-					}),
-				},
-			}
-			backend.StartTLS()
-
-			return mux, backend
+		server := muxServer{
+			certAuthorityGetter: casGetter,
 		}
 
-		mux4, backend4 := startServing(listener4, clusterName)
+		mux4, backend4, err := server.startServing(listener4, clusterName)
+		require.NoError(t, err)
 		defer mux4.Close()
 		defer backend4.Close()
 
 		var backend6 *httptest.Server
 		var mux6 *Mux
 		if listener6 != nil {
-			mux6, backend6 = startServing(listener6, clusterName)
+			mux6, backend6, err = server.startServing(listener6, clusterName)
+			require.NoError(t, err)
 			defer mux6.Close()
 			defer backend6.Close()
 		}
@@ -1066,7 +1044,8 @@ func TestMux(t *testing.T) {
 			require.NoError(t, err)
 
 			// start multiplexer with wrong cluster name specified
-			mux, backend := startServing(listener, "different-cluster")
+			mux, backend, err := server.startServing(listener, "different-cluster")
+			require.NoError(t, err)
 			t.Cleanup(func() {
 				require.NoError(t, mux.Close())
 				backend.Close()
@@ -1273,74 +1252,37 @@ func startSSHServer(t *testing.T, listener net.Listener) {
 
 func BenchmarkMux_ProxyV2Signature(b *testing.B) {
 	const clusterName = "test-teleport"
-
-	clock := clockwork.NewFakeClockAt(time.Now())
 	tlsProxyCert, caGetter, jwtSigner := getTestCertCAsGetterAndSigner(b, clusterName)
-
-	ca, err := caGetter(context.Background(), types.CertAuthID{
-		Type:       types.HostCA,
-		DomainName: clusterName,
-	}, false)
-	require.NoError(b, err)
-
-	roots := x509.NewCertPool()
-	ok := roots.AppendCertsFromPEM(ca.GetTrustedTLSKeyPairs()[0].Cert)
-	require.True(b, ok)
-
 	ip := "1.2.3.4"
 	sAddr := net.TCPAddr{IP: net.ParseIP(ip), Port: 444}
 	dAddr := net.TCPAddr{IP: net.ParseIP(ip), Port: 555}
+	listener4, err := net.Listen("tcp", "127.0.0.1:")
+	require.NoError(b, err)
+
+	server := muxServer{
+		disableTLS:          true,
+		certAuthorityGetter: caGetter,
+	}
+	mux4, backend4, err := server.startServing(listener4, clusterName)
+	require.NoError(b, err)
+	defer mux4.Close()
+	defer backend4.Close()
 
 	b.Run("simulation of signing and verifying PROXY header", func(b *testing.B) {
 		for n := 0; n < b.N; n++ {
-			token, err := jwtSigner.SignPROXYJWT(jwt.PROXYSignParams{
-				ClusterName:        clusterName,
-				SourceAddress:      sAddr.String(),
-				DestinationAddress: dAddr.String(),
-			})
-			require.NoError(b, err)
-
-			pl := ProxyLine{
-				Protocol:    TCP4,
-				Source:      sAddr,
-				Destination: dAddr,
-			}
-			err = pl.AddSignature([]byte(token), tlsProxyCert)
+			conn, err := net.Dial("tcp", listener4.Addr().String())
 			require.NoError(b, err)
+			defer conn.Close()
 
-			_, err = pl.Bytes()
+			signedHeader, err := signPROXYHeader(&sAddr, &dAddr, clusterName, tlsProxyCert, jwtSigner)
 			require.NoError(b, err)
 
-			cert, err := tlsca.ParseCertificatePEM(tlsProxyCert)
-			require.NoError(b, err)
-			chains, err := cert.Verify(x509.VerifyOptions{Roots: roots})
+			_, err = conn.Write(signedHeader)
 			require.NoError(b, err)
-			require.NotNil(b, chains)
 
-			identity, err := tlsca.FromSubject(cert.Subject, cert.NotAfter)
+			out, err := utils.RoundtripWithConn(conn)
 			require.NoError(b, err)
-
-			foundRole := checkForSystemRole(identity, types.RoleProxy)
-			require.True(b, foundRole, "Missing 'Proxy' role on the signing certificate")
-
-			// Check JWT using proxy cert's public key
-			jwtVerifier, err := jwt.New(&jwt.Config{
-				Clock:       clock,
-				PublicKey:   cert.PublicKey,
-				ClusterName: clusterName,
-			})
-			require.NoError(b, err, "Could not create JWT verifier")
-
-			claims, err := jwtVerifier.VerifyPROXY(jwt.PROXYVerifyParams{
-				ClusterName:        clusterName,
-				SourceAddress:      sAddr.String(),
-				DestinationAddress: dAddr.String(),
-				RawToken:           token,
-			})
-			require.NoError(b, err, "Got an error while verifying PROXY JWT")
-			require.NotNil(b, claims)
-			require.Equal(b, fmt.Sprintf("%s/%s", sAddr.String(), dAddr.String()), claims.Subject,
-				"IP addresses in PROXY header don't match JWT")
+			require.Equal(b, sAddr.String(), out)
 		}
 	})
 }
@@ -1439,3 +1381,47 @@ func TestIsDifferentTCPVersion(t *testing.T) {
 			fmt.Sprintf("Unexpected result for %q, %q", tt.addr1, tt.addr2))
 	}
 }
+
+type muxServer struct {
+	certAuthorityGetter func(ctx context.Context, id types.CertAuthID, loadKeys bool) (types.CertAuthority, error)
+	disableTLS          bool
+}
+
+func (m *muxServer) startServing(muxListener net.Listener, cluster string) (*Mux, *httptest.Server, error) {
+	mux, err := New(Config{
+		Listener:            muxListener,
+		PROXYProtocolMode:   PROXYProtocolUnspecified,
+		CertAuthorityGetter: m.certAuthorityGetter,
+		Clock:               clockwork.NewFakeClockAt(time.Now()),
+		LocalClusterName:    cluster,
+	})
+	if err != nil {
+		return mux, &httptest.Server{}, err
+	}
+
+	if m.disableTLS {
+		muxListener = mux.HTTP()
+	} else {
+		muxListener = mux.TLS()
+	}
+
+	go mux.Serve()
+
+	backend := &httptest.Server{
+		Listener: muxListener,
+
+		Config: &http.Server{
+			Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				fmt.Fprint(w, r.RemoteAddr)
+			}),
+		},
+	}
+
+	if m.disableTLS {
+		backend.Start()
+	} else {
+		backend.StartTLS()
+	}
+
+	return mux, backend, nil
+}