Repository: knative/pkg
Pull request #2750 (2023-05-24 14:02:30)

Drop the unused injection.Dynamic interface
Fixes: https://github.com/knative/pkg/issues/2739

dprotaso: /hold 

Eventing hasn't updated their k8s deps - will wait for that

Patch:
diff --git a/injection/clients.go b/injection/clients.go
index 92e991218..b71ef1d94 100644
--- a/injection/clients.go
+++ b/injection/clients.go
@@ -62,22 +62,3 @@ func (i *impl) FetchAllClients(ctx context.Context) []interface{} {
 	}
 	return clients
 }
-
-// DynamicClientInjector holds the type of a callback that attaches a particular
-// client type to a context.
-type DynamicClientInjector func(context.Context) context.Context
-
-func (i *impl) RegisterDynamicClient(ci DynamicClientInjector) {
-	i.m.Lock()
-	defer i.m.Unlock()
-
-	i.dynamicClients = append(i.dynamicClients, ci)
-}
-
-func (i *impl) GetDynamicClients() []DynamicClientInjector {
-	i.m.RLock()
-	defer i.m.RUnlock()
-
-	// Copy the slice before returning.
-	return append(i.dynamicClients[:0:0], i.dynamicClients...)
-}
diff --git a/injection/informers.go b/injection/informers.go
index ce5d481e8..9356f8d7f 100644
--- a/injection/informers.go
+++ b/injection/informers.go
@@ -28,10 +28,6 @@ import (
 // informer type to a context.
 type InformerInjector func(context.Context) (context.Context, controller.Informer)
 
-// DynamicInformerInjector holds the type of a callback that attaches a particular
-// informer type (backed by a Dynamic) to a context.
-type DynamicInformerInjector func(context.Context) context.Context
-
 // FilteredInformersInjector holds the type of a callback that attaches a set of particular
 // filtered informers type to a context.
 type FilteredInformersInjector func(context.Context) (context.Context, []controller.Informer)
@@ -43,13 +39,6 @@ func (i *impl) RegisterInformer(ii InformerInjector) {
 	i.informers = append(i.informers, ii)
 }
 
-func (i *impl) RegisterDynamicInformer(ii DynamicInformerInjector) {
-	i.m.Lock()
-	defer i.m.Unlock()
-
-	i.dynamicInformers = append(i.dynamicInformers, ii)
-}
-
 func (i *impl) RegisterFilteredInformers(fii FilteredInformersInjector) {
 	i.m.Lock()
 	defer i.m.Unlock()
@@ -65,14 +54,6 @@ func (i *impl) GetInformers() []InformerInjector {
 	return append(i.informers[:0:0], i.informers...)
 }
 
-func (i *impl) GetDynamicInformers() []DynamicInformerInjector {
-	i.m.RLock()
-	defer i.m.RUnlock()
-
-	// Copy the slice before returning.
-	return append(i.dynamicInformers[:0:0], i.dynamicInformers...)
-}
-
 func (i *impl) GetFilteredInformers() []FilteredInformersInjector {
 	i.m.RLock()
 	defer i.m.RUnlock()
@@ -81,22 +62,6 @@ func (i *impl) GetFilteredInformers() []FilteredInformersInjector {
 	return append(i.filteredInformers[:0:0], i.filteredInformers...)
 }
 
-func (i *impl) SetupDynamic(ctx context.Context) context.Context {
-	// Based on the reconcilers we have linked, build up a set of clients and inject
-	// them onto the context.
-	for _, ci := range i.GetDynamicClients() {
-		ctx = ci(ctx)
-	}
-
-	// Based on the reconcilers we have linked, build up a set of informers
-	// and inject them onto the context.
-	for _, ii := range i.GetDynamicInformers() {
-		ctx = ii(ctx)
-	}
-
-	return ctx
-}
-
 func (i *impl) SetupInformers(ctx context.Context, cfg *rest.Config) (context.Context, []controller.Informer) {
 	// Based on the reconcilers we have linked, build up a set of clients and inject
 	// them onto the context.
diff --git a/injection/interface.go b/injection/interface.go
index 158864015..c6d5715ad 100644
--- a/injection/interface.go
+++ b/injection/interface.go
@@ -78,29 +78,6 @@ type Interface interface {
 	SetupInformers(context.Context, *rest.Config) (context.Context, []controller.Informer)
 }
 
-// DynamicInterface is the interface for interacting with dynamicclient-based injection
-// implementations, such as Dynamic below.
-type DynamicInterface interface {
-	// RegisterDynamicClient registers a new injector callback for associating
-	// a new dynamicclient-based client with a context.
-	RegisterDynamicClient(DynamicClientInjector)
-
-	// GetDynamicClients fetches all of the registered dynamicclient-based client injectors.
-	GetDynamicClients() []DynamicClientInjector
-
-	// RegisterDynamicInformer registers a new injector callback for associating
-	// a new dynamicclient-based informer with a context.
-	RegisterDynamicInformer(DynamicInformerInjector)
-
-	// GetDynamicInformers fetches all of the registered dynamicclient-based informer injectors.
-	GetDynamicInformers() []DynamicInformerInjector
-
-	// SetupDynamic runs all of the injectors against a context, starting with
-	// the clients and the given stream.  A context infused with the various elements
-	// is returned.
-	SetupDynamic(context.Context) context.Context
-}
-
 type ControllerConstructor func(context.Context, configmap.Watcher) *controller.Impl
 
 // NamedControllerConstructor is a ControllerConstructor with an associated name.
@@ -120,10 +97,6 @@ var (
 	// are being run for real.
 	Default Interface = &impl{}
 
-	// Dynamic is the injection interface to use when bootstrapping a version
-	// of things based on the prototype dynamicclient-based reconciler framework.
-	Dynamic DynamicInterface = &impl{}
-
 	// Fake is the injection interface with which informers should register
 	// to make themselves available to the controller process when it is being
 	// unit tested.
@@ -134,11 +107,9 @@ type impl struct {
 	m sync.RWMutex
 
 	clients           []ClientInjector
-	dynamicClients    []DynamicClientInjector
 	clientFetchers    []ClientFetcher
 	factories         []InformerFactoryInjector
 	informers         []InformerInjector
-	dynamicInformers  []DynamicInformerInjector
 	filteredInformers []FilteredInformersInjector
 	ducks             []DuckFactoryInjector
 }
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)
 	}
 }