Repository: knative/pkg
Pull request #2751 (2023-05-24 20:59:10)

Fix some webhook testing tech debt
Contributing to the webhook package is a bit annoying

- Some tests couldn't be run in parallel
- Verbose log messages about tls handshake EOF
- Races occur with ephemeral ports since we temporary allocate them to discover which ones are free but then release them in order for the webhook to bind to them



knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2751#" title="Author self-approved">dprotaso</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/main/webhook/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2751#" title="Author self-approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2751#pullrequestreview-1443127648" title="LGTM">ReToCode</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/main/webhook/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
ReToCode: Sorry, somehow I missed the WIP, but looks good so far 👍 
dprotaso: /retest

just wanted to run the unit tests in github actions a bunch of times

Patch:
diff --git a/webhook/webhook.go b/webhook/webhook.go
index 779d388d2..80bd8c2be 100644
--- a/webhook/webhook.go
+++ b/webhook/webhook.go
@@ -22,6 +22,8 @@ import (
 	"errors"
 	"fmt"
 	"html"
+	"log"
+	"net"
 	"net/http"
 	"time"
 
@@ -94,6 +96,9 @@ type Webhook struct {
 
 	// The TLS configuration to use for serving (or nil for non-TLS)
 	tlsConfig *tls.Config
+
+	// testListener is only used in testing so we don't get port conflicts
+	testListener net.Listener
 }
 
 // New constructs a Webhook
@@ -196,7 +201,6 @@ func New(
 		default:
 			return nil, fmt.Errorf("unknown webhook controller type:  %T", controller)
 		}
-
 	}
 
 	return
@@ -209,6 +213,15 @@ func (wh *Webhook) InformersHaveSynced() {
 	wh.Logger.Info("Informers have been synced, unblocking admission webhooks.")
 }
 
+type zapWrapper struct {
+	logger *zap.SugaredLogger
+}
+
+func (z *zapWrapper) Write(p []byte) (n int, err error) {
+	z.logger.Errorw(string(p))
+	return len(p), nil
+}
+
 // Run implements the admission controller run loop.
 func (wh *Webhook) Run(stop <-chan struct{}) error {
 	logger := wh.Logger
@@ -220,24 +233,34 @@ func (wh *Webhook) Run(stop <-chan struct{}) error {
 	}
 
 	server := &http.Server{
+		ErrorLog:          log.New(&zapWrapper{logger}, "", 0),
 		Handler:           drainer,
 		Addr:              fmt.Sprint(":", wh.Options.Port),
 		TLSConfig:         wh.tlsConfig,
 		ReadHeaderTimeout: time.Minute, //https://medium.com/a-journey-with-go/go-understand-and-mitigate-slowloris-attack-711c1b1403f6
 	}
 
+	var serve = server.ListenAndServe
+
+	if server.TLSConfig != nil && wh.testListener != nil {
+		serve = func() error {
+			return server.ServeTLS(wh.testListener, "", "")
+		}
+	} else if server.TLSConfig != nil {
+		serve = func() error {
+			return server.ListenAndServeTLS("", "")
+		}
+	} else if wh.testListener != nil {
+		serve = func() error {
+			return server.Serve(wh.testListener)
+		}
+	}
+
 	eg, ctx := errgroup.WithContext(ctx)
 	eg.Go(func() error {
-		if server.TLSConfig != nil {
-			if err := server.ListenAndServeTLS("", ""); err != nil && !errors.Is(err, http.ErrServerClosed) {
-				logger.Errorw("ListenAndServeTLS for admission webhook returned error", zap.Error(err))
-				return err
-			}
-		} else {
-			if err := server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
-				logger.Errorw("ListenAndServe for admission webhook returned error", zap.Error(err))
-				return err
-			}
+		if err := serve(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			logger.Errorw("ListenAndServe for admission webhook returned error", zap.Error(err))
+			return err
 		}
 		return nil
 	})


Test patch:
diff --git a/webhook/admission_integration_test.go b/webhook/admission_integration_test.go
index d491d5aac..f66d7b155 100644
--- a/webhook/admission_integration_test.go
+++ b/webhook/admission_integration_test.go
@@ -115,8 +115,7 @@ func TestAdmissionValidResponseForResourceTLS(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 	tlsClient, err := createSecureTLSClient(t, kubeclient.Get(ctx), &wh.Options)
@@ -243,8 +242,7 @@ func TestAdmissionValidResponseForResource(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForNonTLSServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 	client := createNonTLSClient()
@@ -370,8 +368,7 @@ func TestAdmissionInvalidResponseForResource(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 	tlsClient, err := createSecureTLSClient(t, kubeclient.Get(ctx), &wh.Options)
@@ -488,8 +485,7 @@ func TestAdmissionWarningResponseForResource(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 	tlsClient, err := createSecureTLSClient(t, kubeclient.Get(ctx), &wh.Options)
@@ -594,8 +590,7 @@ func TestAdmissionValidResponseForRequestBody(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForNonTLSServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 	client := createNonTLSClient()
diff --git a/webhook/conversion_integration_test.go b/webhook/conversion_integration_test.go
index 3b99d0091..15aecb2a8 100644
--- a/webhook/conversion_integration_test.go
+++ b/webhook/conversion_integration_test.go
@@ -79,8 +79,7 @@ func TestConversionValidResponse(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 	tlsClient, err := createSecureTLSClient(t, kubeclient.Get(ctx), &wh.Options)
@@ -168,8 +167,7 @@ func TestConversionInvalidResponse(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 	tlsClient, err := createSecureTLSClient(t, kubeclient.Get(ctx), &wh.Options)
diff --git a/webhook/helper_test.go b/webhook/helper_test.go
index b5eb612f2..9c165fe7e 100644
--- a/webhook/helper_test.go
+++ b/webhook/helper_test.go
@@ -22,7 +22,6 @@ import (
 	"crypto/x509"
 	"net"
 	"net/http"
-	"strconv"
 	"testing"
 	"time"
 
@@ -128,8 +127,7 @@ func TestEnsureLabelSelectorExpressions(t *testing.T) {
 		})
 	}
 }
-
-func waitForServerAvailable(t *testing.T, serverURL string, timeout time.Duration) error {
+func waitForNonTLSServerAvailable(t *testing.T, serverURL string, timeout time.Duration) error {
 	t.Helper()
 	var interval = 100 * time.Millisecond
 
@@ -146,20 +144,30 @@ func waitForServerAvailable(t *testing.T, serverURL string, timeout time.Duratio
 	return wait.PollImmediate(interval, timeout, conditionFunc)
 }
 
-func newTestPort() (port int, err error) {
-	server, err := net.Listen("tcp", ":0")
-	if err != nil {
-		return 0, err
-	}
+func waitForServerAvailable(t *testing.T, serverURL string, timeout time.Duration) error {
+	t.Helper()
 
-	defer server.Close()
+	var (
+		// if this is too low you'll see TLS handshake EOF warnings
+		interval = 500 * time.Millisecond
+		tlsConf  = &tls.Config{InsecureSkipVerify: true}
+		dialer   = &net.Dialer{
+			Timeout:   interval, // Initial duration.
+			KeepAlive: 5 * time.Second,
+			DualStack: true,
+		}
+	)
 
-	_, portString, err := net.SplitHostPort(server.Addr().String())
-	if err != nil {
-		return 0, err
+	conditionFunc := func() (done bool, err error) {
+		conn, _ := tls.DialWithDialer(dialer, "tcp", serverURL, tlsConf)
+		if conn != nil {
+			conn.Close()
+			return true, nil
+		}
+		return false, nil
 	}
 
-	return strconv.Atoi(portString)
+	return wait.PollImmediate(interval, timeout, conditionFunc)
 }
 
 func createNamespace(t *testing.T, kubeClient kubernetes.Interface, name string) error {
@@ -194,13 +202,11 @@ func createTestConfigMap(t *testing.T, kubeClient kubernetes.Interface) error {
 func createSecureTLSClient(t *testing.T, kubeClient kubernetes.Interface, acOpts *Options) (*http.Client, error) {
 	t.Helper()
 	ctx := TestContextWithLogger(t)
-	secret, err := certresources.MakeSecret(ctx, acOpts.SecretName, system.Namespace(), acOpts.ServiceName)
+
+	secret, err := kubeClient.CoreV1().Secrets(system.Namespace()).Get(ctx, acOpts.SecretName, metav1.GetOptions{})
 	if err != nil {
 		return nil, err
 	}
-	if _, err := kubeClient.CoreV1().Secrets(secret.Namespace).Create(context.Background(), secret, metav1.CreateOptions{}); err != nil {
-		return nil, err
-	}
 
 	serverKey := secret.Data[certresources.ServerKey]
 	serverCert := secret.Data[certresources.ServerCert]
diff --git a/webhook/webhook_integration_test.go b/webhook/webhook_integration_test.go
index b81b2f4eb..247249905 100644
--- a/webhook/webhook_integration_test.go
+++ b/webhook/webhook_integration_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"strings"
 	"testing"
@@ -33,6 +34,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/metrics/metricstest"
 	pkgtest "knative.dev/pkg/testing"
+	certresources "knative.dev/pkg/webhook/certificates/resources"
 )
 
 // createResource creates a testing.Resource with the given name in the system namespace.
@@ -66,8 +68,7 @@ func TestMissingContentType(t *testing.T) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 
@@ -120,8 +121,7 @@ func testEmptyRequestBody(t *testing.T, controller interface{}) {
 		}
 	}()
 
-	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
-	if pollErr != nil {
+	if err = waitForServerAvailable(t, serverURL, testTimeout); err != nil {
 		t.Fatal("waitForServerAvailable() =", err)
 	}
 
@@ -193,31 +193,47 @@ func TestSetupWebhookHTTPServerError(t *testing.T) {
 
 func testSetup(t *testing.T, acs ...interface{}) (*Webhook, string, context.Context, context.CancelFunc, error) {
 	t.Helper()
-	port, err := newTestPort()
+
+	// ephemeral port
+	l, err := net.Listen("tcp", ":0")
 	if err != nil {
-		return nil, "", nil, nil, err
+		t.Fatal("unable to get ephemeral port: ", err)
 	}
 
 	defaultOpts := newDefaultOptions()
-	defaultOpts.Port = port
+
 	ctx, wh, cancel := newNonRunningTestWebhook(t, defaultOpts, acs...)
+	wh.testListener = l
+
+	// Create certificate
+	secret, err := certresources.MakeSecret(ctx, defaultOpts.SecretName, system.Namespace(), defaultOpts.ServiceName)
+	if err != nil {
+		t.Fatalf("failed to create certificate")
+	}
+	kubeClient := kubeclient.Get(ctx)
+
+	if _, err := kubeClient.CoreV1().Secrets(secret.Namespace).Create(context.Background(), secret, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("failed to create secret")
+	}
 
 	resetMetrics()
-	return wh, fmt.Sprintf("0.0.0.0:%d", port), ctx, cancel, nil
+	return wh, l.Addr().String(), ctx, cancel, nil
 }
 
 func testSetupNoTLS(t *testing.T, acs ...interface{}) (*Webhook, string, context.Context, context.CancelFunc, error) {
 	t.Helper()
-	port, err := newTestPort()
+
+	// ephemeral port
+	l, err := net.Listen("tcp", ":0")
 	if err != nil {
 		return nil, "", nil, nil, err
 	}
 
 	defaultOpts := newDefaultOptions()
 	defaultOpts.SecretName = ""
-	defaultOpts.Port = port
 	ctx, wh, cancel := newNonRunningTestWebhook(t, defaultOpts, acs...)
+	wh.testListener = l
 
 	resetMetrics()
-	return wh, fmt.Sprintf("0.0.0.0:%d", port), ctx, cancel, nil
+	return wh, l.Addr().String(), ctx, cancel, nil
 }
diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go
index 34c95d3fe..e1e07697a 100644
--- a/webhook/webhook_test.go
+++ b/webhook/webhook_test.go
@@ -19,7 +19,6 @@ package webhook
 import (
 	"context"
 	"crypto/tls"
-	"fmt"
 	"net"
 	"testing"
 	"time"
@@ -73,25 +72,27 @@ func newNonRunningTestWebhook(t *testing.T, options Options, acs ...interface{})
 }
 
 func TestRegistrationStopChanFire(t *testing.T) {
-	opts := newDefaultOptions()
-	_, ac, cancel := newNonRunningTestWebhook(t, opts)
+	wh, serverURL, _, cancel, err := testSetupNoTLS(t)
+	if err != nil {
+		t.Fatal("testSetup() =", err)
+	}
 	defer cancel()
 
 	stopCh := make(chan struct{})
 
 	var g errgroup.Group
 	g.Go(func() error {
-		return ac.Run(stopCh)
+		return wh.Run(stopCh)
 	})
 	close(stopCh)
 
 	if err := g.Wait(); err != nil {
 		t.Fatal("Error during run: ", err)
 	}
-	conn, err := net.Dial("tcp", fmt.Sprintf(":%d", opts.Port))
+	conn, err := net.Dial("tcp", serverURL)
 	if err == nil {
 		conn.Close()
-		t.Error("Unexpected success to dial to port", opts.Port)
+		t.Error("Unexpected success to dial to ", serverURL)
 	}
 }