Envtest should modify CRDs appropriately when using webhooks

Before this change, the webhook code was patching mutation or admission
webhooks to point to the temporary URL and CA bundle generated when the
webhook server would start.

Conversion webhooks are handled at the CRD level in Kubernetes, and we
need to make sure to patch those client configurations as well.

This patch also unexports a lot of methods that should have been private
from the very beginning, so it's marked as a breaking change.

Signed-off-by: Vince Prignano <[email protected]>

Author: vincepri

pkg/envtest/crd.go

@@ -20,12 +20,16 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"time"
 
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -35,6 +39,7 @@ import (
 	k8syaml "k8s.io/apimachinery/pkg/util/yaml"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/util/retry"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/yaml"
 )
@@ -60,6 +65,13 @@ type CRDInstallOptions struct {
 	// uninstalled when terminating the test environment.
 	// Defaults to false.
 	CleanUpAfterUse bool
+
+	// WebhookOptions contains the conversion webhook information to install
+	// on the CRDs. This field is usually inherited by the EnvTest options.
+	//
+	// If you're passing this field manually, you need to make sure that
+	// the CA information and host port is filled in properly.
+	WebhookOptions WebhookInstallOptions
 }
 
 const defaultPollInterval = 100 * time.Millisecond
@@ -74,6 +86,10 @@ func InstallCRDs(config *rest.Config, options CRDInstallOptions) ([]client.Objec
 		return nil, err
 	}
 
+	if err := modifyConversionWebhooks(options.CRDs, options.WebhookOptions); err != nil {
+		return nil, err
+	}
+
 	// Create the CRDs in the apiserver
 	if err := CreateCRDs(config, options.CRDs); err != nil {
 		return options.CRDs, err
@@ -340,6 +356,37 @@ func renderCRDs(options *CRDInstallOptions) ([]client.Object, error) {
 	return res, nil
 }
 
+func modifyConversionWebhooks(crds []client.Object, webhookOptions WebhookInstallOptions) error {
+	if len(webhookOptions.LocalServingCAData) == 0 {
+		return errors.New("cannot modify conversion webhooks, LocalServingCAData missing in WebhookOptions")
+	}
+
+	// generate host port.
+	hostPort, err := webhookOptions.generateHostPort()
+	if err != nil {
+		return err
+	}
+	url := pointer.StringPtr(fmt.Sprintf("https://%s/convert", hostPort))
+
+	for _, crd := range crds {
+		switch c := crd.(type) {
+		case *apiextensionsv1beta1.CustomResourceDefinition:
+			c.Spec.Conversion.WebhookClientConfig.Service = nil
+			c.Spec.Conversion.WebhookClientConfig = &apiextensionsv1beta1.WebhookClientConfig{
+				CABundle: webhookOptions.LocalServingCAData,
+			}
+		case *apiextensionsv1.CustomResourceDefinition:
+			c.Spec.Conversion.Webhook.ClientConfig.Service = nil
+			c.Spec.Conversion.Webhook.ClientConfig = &apiextensionsv1.WebhookClientConfig{
+				URL:      url,
+				CABundle: webhookOptions.LocalServingCAData,
+			}
+		}
+	}
+
+	return nil
+}
+
 // readCRDs reads the CRDs from files and Unmarshals them into structs
 func readCRDs(basePath string, files []os.FileInfo) ([]*unstructured.Unstructured, error) {
 	var crds []*unstructured.Unstructured

pkg/envtest/server.go

@@ -263,6 +263,12 @@ func (te *Environment) Start() (*rest.Config, error) {
 		}
 	}
 
+	// Call PrepWithoutInstalling to setup certificates first
+	// and have them available to patch CRD conversion webhook as well.
+	if err := te.WebhookInstallOptions.PrepWithoutInstalling(); err != nil {
+		return nil, err
+	}
+
 	log.V(1).Info("installing CRDs")
 	te.CRDInstallOptions.CRDs = mergeCRDs(te.CRDInstallOptions.CRDs, te.CRDs)
 	te.CRDInstallOptions.Paths = mergePaths(te.CRDInstallOptions.Paths, te.CRDDirectoryPaths)
@@ -274,9 +280,10 @@ func (te *Environment) Start() (*rest.Config, error) {
 	te.CRDs = crds
 
 	log.V(1).Info("installing webhooks")
-	err = te.WebhookInstallOptions.Install(te.Config)
-
-	return te.Config, err
+	if err := te.WebhookInstallOptions.Install(te.Config); err != nil {
+		return nil, err
+	}
+	return te.Config, nil
 }
 
 func (te *Environment) startControlPlane() error {

pkg/envtest/webhook.go

@@ -80,7 +80,10 @@ type WebhookInstallOptions struct {
 // ModifyWebhookDefinitions modifies webhook definitions by:
 // - applying CABundle based on the provided tinyca
 // - if webhook client config uses service spec, it's removed and replaced with direct url
-func (o *WebhookInstallOptions) ModifyWebhookDefinitions(caData []byte) error {
+func (o *WebhookInstallOptions) ModifyWebhookDefinitions() error {
+	caData := o.LocalServingCAData
+
+	// generate host port.
 	hostPort, err := o.generateHostPort()
 	if err != nil {
 		return err
@@ -161,16 +164,14 @@ func (o *WebhookInstallOptions) generateHostPort() (string, error) {
 // controller-runtime, where we need a random host-port & caData for webhook
 // tests, but may be useful in similar scenarios.
 func (o *WebhookInstallOptions) PrepWithoutInstalling() error {
-	hookCA, err := o.setupCA()
-	if err != nil {
+	if err := o.setupCA(); err != nil {
 		return err
 	}
 	if err := parseWebhook(o); err != nil {
 		return err
 	}
 
-	err = o.ModifyWebhookDefinitions(hookCA)
-	if err != nil {
+	if err := o.ModifyWebhookDefinitions(); err != nil {
 		return err
 	}
 
@@ -179,8 +180,10 @@ func (o *WebhookInstallOptions) PrepWithoutInstalling() error {
 
 // Install installs specified webhooks to the API server
 func (o *WebhookInstallOptions) Install(config *rest.Config) error {
-	if err := o.PrepWithoutInstalling(); err != nil {
-		return err
+	if len(o.LocalServingCAData) == 0 {
+		if err := o.PrepWithoutInstalling(); err != nil {
+			return err
+		}
 	}
 
 	if err := createWebhooks(config, o.MutatingWebhooks, o.ValidatingWebhooks); err != nil {
@@ -269,38 +272,38 @@ func (p *webhookPoller) poll() (done bool, err error) {
 }
 
 // setupCA creates CA for testing and writes them to disk
-func (o *WebhookInstallOptions) setupCA() ([]byte, error) {
+func (o *WebhookInstallOptions) setupCA() error {
 	hookCA, err := integration.NewTinyCA()
 	if err != nil {
-		return nil, fmt.Errorf("unable to set up webhook CA: %v", err)
+		return fmt.Errorf("unable to set up webhook CA: %v", err)
 	}
 
 	names := []string{"localhost", o.LocalServingHost, o.LocalServingHostExternalName}
 	hookCert, err := hookCA.NewServingCert(names...)
 	if err != nil {
-		return nil, fmt.Errorf("unable to set up webhook serving certs: %v", err)
+		return fmt.Errorf("unable to set up webhook serving certs: %v", err)
 	}
 
 	localServingCertsDir, err := ioutil.TempDir("", "envtest-serving-certs-")
 	o.LocalServingCertDir = localServingCertsDir
 	if err != nil {
-		return nil, fmt.Errorf("unable to create directory for webhook serving certs: %v", err)
+		return fmt.Errorf("unable to create directory for webhook serving certs: %v", err)
 	}
 
 	certData, keyData, err := hookCert.AsBytes()
 	if err != nil {
-		return nil, fmt.Errorf("unable to marshal webhook serving certs: %v", err)
+		return fmt.Errorf("unable to marshal webhook serving certs: %v", err)
 	}
 
 	if err := ioutil.WriteFile(filepath.Join(localServingCertsDir, "tls.crt"), certData, 0640); err != nil {
-		return nil, fmt.Errorf("unable to write webhook serving cert to disk: %v", err)
+		return fmt.Errorf("unable to write webhook serving cert to disk: %v", err)
 	}
 	if err := ioutil.WriteFile(filepath.Join(localServingCertsDir, "tls.key"), keyData, 0640); err != nil {
-		return nil, fmt.Errorf("unable to write webhook serving key to disk: %v", err)
+		return fmt.Errorf("unable to write webhook serving key to disk: %v", err)
 	}
 
 	o.LocalServingCAData = certData
-	return certData, nil
+	return err
 }
 
 func createWebhooks(config *rest.Config, mutHooks []client.Object, valHooks []client.Object) error {