From 0080db8047d41829ffe373ed7f4c1e70fc3d4449 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Fri, 14 May 2021 12:26:30 -0700 Subject: [PATCH] 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 --- pkg/envtest/crd.go | 37 ++++++++++++++++++++++++++++++++++++- pkg/envtest/envtest_test.go | 28 ++++++++++++++-------------- pkg/envtest/server.go | 15 +++++++++++---- pkg/envtest/webhook.go | 31 ++++++++++++++----------------- 4 files changed, 75 insertions(+), 36 deletions(-) diff --git a/pkg/envtest/crd.go b/pkg/envtest/crd.go index 56d7ae532f..aeea1198c0 100644 --- a/pkg/envtest/crd.go +++ b/pkg/envtest/crd.go @@ -20,12 +20,15 @@ import ( "bufio" "bytes" "context" + "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 +38,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" ) @@ -66,7 +70,7 @@ const defaultPollInterval = 100 * time.Millisecond const defaultMaxWait = 10 * time.Second // InstallCRDs installs a collection of CRDs into a cluster by reading the crd yaml files from a directory -func InstallCRDs(config *rest.Config, options CRDInstallOptions) ([]client.Object, error) { +func InstallCRDs(config *rest.Config, options CRDInstallOptions, webhookOptions WebhookInstallOptions) ([]client.Object, error) { defaultCRDOptions(&options) // Read the CRD yamls into options.CRDs @@ -74,6 +78,10 @@ func InstallCRDs(config *rest.Config, options CRDInstallOptions) ([]client.Objec return nil, err } + if err := modifyConversionWebhooks(options.CRDs, 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 +348,33 @@ func renderCRDs(options *CRDInstallOptions) ([]client.Object, error) { return res, nil } +func modifyConversionWebhooks(crds []client.Object, webhookOptions WebhookInstallOptions) error { + // 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 diff --git a/pkg/envtest/envtest_test.go b/pkg/envtest/envtest_test.go index 18ed4c22c8..4c34950c9f 100644 --- a/pkg/envtest/envtest_test.go +++ b/pkg/envtest/envtest_test.go @@ -84,7 +84,7 @@ var _ = Describe("Test", func() { It("should install the unserved CRDs into the cluster", func() { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{filepath.Join(".", "testdata", "crds", "examplecrd_unserved.yaml")}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Expect to find the CRDs @@ -122,7 +122,7 @@ var _ = Describe("Test", func() { It("should install the CRDs into the cluster using directory", func(done Done) { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{validDirectory}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Expect to find the CRDs @@ -226,7 +226,7 @@ var _ = Describe("Test", func() { It("should install the CRDs into the cluster using file", func(done Done) { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{filepath.Join(".", "testdata", "crds", "examplecrd3.yaml")}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) crd := &v1beta1.CustomResourceDefinition{} @@ -257,7 +257,7 @@ var _ = Describe("Test", func() { filepath.Join(".", "testdata", "examplecrd.yaml"), filepath.Join(".", "testdata", "examplecrd_v1.yaml"), }, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) Expect(crds).To(HaveLen(2)) @@ -270,7 +270,7 @@ var _ = Describe("Test", func() { filepath.Join(".", "testdata"), filepath.Join(".", "testdata", "examplecrd1.yaml"), }, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) crd := &apiextensionsv1.CustomResourceDefinition{} @@ -307,7 +307,7 @@ var _ = Describe("Test", func() { }, 10) It("should not return an not error if the directory doesn't exist", func(done Done) { - crds, err = InstallCRDs(env.Config, CRDInstallOptions{Paths: []string{invalidDirectory}}) + crds, err = InstallCRDs(env.Config, CRDInstallOptions{Paths: []string{invalidDirectory}}, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) close(done) @@ -316,7 +316,7 @@ var _ = Describe("Test", func() { It("should return an error if the directory doesn't exist", func(done Done) { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{invalidDirectory}, ErrorIfPathMissing: true, - }) + }, WebhookInstallOptions{}) Expect(err).To(HaveOccurred()) close(done) @@ -325,7 +325,7 @@ var _ = Describe("Test", func() { It("should return an error if the file doesn't exist", func(done Done) { crds, err = InstallCRDs(env.Config, CRDInstallOptions{Paths: []string{ filepath.Join(".", "testdata", "fake.yaml")}, ErrorIfPathMissing: true, - }) + }, WebhookInstallOptions{}) Expect(err).To(HaveOccurred()) close(done) @@ -353,7 +353,7 @@ var _ = Describe("Test", func() { It("should return an error if the resource isn't found in the group version", func(done Done) { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{"."}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Wait for a CRD that doesn't exist, but the Group and Version do @@ -385,7 +385,7 @@ var _ = Describe("Test", func() { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{filepath.Join(".", "testdata")}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Expect to find the CRDs @@ -487,7 +487,7 @@ var _ = Describe("Test", func() { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{filepath.Join(".", "testdata")}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Expect to find the CRDs @@ -594,7 +594,7 @@ var _ = Describe("Test", func() { // Install only the CRDv1 multi-version example crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{filepath.Join(".", "testdata")}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Expect to find the CRDs @@ -636,7 +636,7 @@ var _ = Describe("Test", func() { // Add one more version and update _, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{filepath.Join(".", "testdata", "crdv1_updated")}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Expect to find updated CRD @@ -686,7 +686,7 @@ var _ = Describe("Test", func() { crds, err = InstallCRDs(env.Config, CRDInstallOptions{ Paths: []string{validDirectory}, - }) + }, WebhookInstallOptions{}) Expect(err).NotTo(HaveOccurred()) // Expect to find the CRDs diff --git a/pkg/envtest/server.go b/pkg/envtest/server.go index 614d861fde..2e24dc1c26 100644 --- a/pkg/envtest/server.go +++ b/pkg/envtest/server.go @@ -263,20 +263,27 @@ 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) te.CRDInstallOptions.ErrorIfPathMissing = te.ErrorIfCRDPathMissing - crds, err := InstallCRDs(te.Config, te.CRDInstallOptions) + crds, err := InstallCRDs(te.Config, te.CRDInstallOptions, te.WebhookInstallOptions) if err != nil { return te.Config, err } 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 { diff --git a/pkg/envtest/webhook.go b/pkg/envtest/webhook.go index 9f96e87930..5c0b579579 100644 --- a/pkg/envtest/webhook.go +++ b/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,10 +180,6 @@ 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 err := createWebhooks(config, o.MutatingWebhooks, o.ValidatingWebhooks); err != nil { return err } @@ -269,38 +266,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 {