From a14a68cdaba04ccbc2dd4501f162f8472f11d800 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Thu, 16 Sep 2021 09:14:44 -0700 Subject: [PATCH] :warning: Avoid shallow copies of webhooks and CRDs in testenv Signed-off-by: Vince Prignano --- pkg/builder/builder_suite_test.go | 2 +- pkg/envtest/crd.go | 28 +++++++++++++-------------- pkg/envtest/envtest_suite_test.go | 2 +- pkg/envtest/envtest_test.go | 28 +++++++++++++-------------- pkg/envtest/helper.go | 8 ++++---- pkg/envtest/server.go | 2 +- pkg/envtest/webhook.go | 32 +++++++++++++++---------------- pkg/webhook/webhook_suite_test.go | 2 +- 8 files changed, 52 insertions(+), 52 deletions(-) diff --git a/pkg/builder/builder_suite_test.go b/pkg/builder/builder_suite_test.go index a59d4a5308..5ae6fd8616 100644 --- a/pkg/builder/builder_suite_test.go +++ b/pkg/builder/builder_suite_test.go @@ -78,7 +78,7 @@ var _ = AfterSuite(func() { func addCRDToEnvironment(env *envtest.Environment, gvks ...schema.GroupVersionKind) { for _, gvk := range gvks { plural, singular := meta.UnsafeGuessKindToResource(gvk) - crd := apiextensionsv1.CustomResourceDefinition{ + crd := &apiextensionsv1.CustomResourceDefinition{ TypeMeta: metav1.TypeMeta{ APIVersion: "apiextensions.k8s.io/v1", Kind: "CustomResourceDefinition", diff --git a/pkg/envtest/crd.go b/pkg/envtest/crd.go index 51bf366792..a15c1dacaa 100644 --- a/pkg/envtest/crd.go +++ b/pkg/envtest/crd.go @@ -60,7 +60,7 @@ type CRDInstallOptions struct { Paths []string // CRDs is a list of CRDs to install - CRDs []apiextensionsv1.CustomResourceDefinition + CRDs []*apiextensionsv1.CustomResourceDefinition // ErrorIfPathMissing will cause an error if a Path does not exist ErrorIfPathMissing bool @@ -88,7 +88,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) ([]apiextensionsv1.CustomResourceDefinition, error) { +func InstallCRDs(config *rest.Config, options CRDInstallOptions) ([]*apiextensionsv1.CustomResourceDefinition, error) { defaultCRDOptions(&options) // Read the CRD yamls into options.CRDs @@ -140,7 +140,7 @@ func defaultCRDOptions(o *CRDInstallOptions) { } // WaitForCRDs waits for the CRDs to appear in discovery. -func WaitForCRDs(config *rest.Config, crds []apiextensionsv1.CustomResourceDefinition, options CRDInstallOptions) error { +func WaitForCRDs(config *rest.Config, crds []*apiextensionsv1.CustomResourceDefinition, options CRDInstallOptions) error { // Add each CRD to a map of GroupVersion to Resource waitingFor := map[schema.GroupVersion]*sets.String{} for _, crd := range crds { @@ -229,7 +229,7 @@ func UninstallCRDs(config *rest.Config, options CRDInstallOptions) error { for _, crd := range options.CRDs { crd := crd log.V(1).Info("uninstalling CRD", "crd", crd.GetName()) - if err := cs.Delete(context.TODO(), &crd); err != nil { + if err := cs.Delete(context.TODO(), crd); err != nil { // If CRD is not found, we can consider success if !apierrors.IsNotFound(err) { return err @@ -241,7 +241,7 @@ func UninstallCRDs(config *rest.Config, options CRDInstallOptions) error { } // CreateCRDs creates the CRDs. -func CreateCRDs(config *rest.Config, crds []apiextensionsv1.CustomResourceDefinition) error { +func CreateCRDs(config *rest.Config, crds []*apiextensionsv1.CustomResourceDefinition) error { cs, err := client.New(config, client.Options{}) if err != nil { return fmt.Errorf("unable to create client: %w", err) @@ -255,7 +255,7 @@ func CreateCRDs(config *rest.Config, crds []apiextensionsv1.CustomResourceDefini err := cs.Get(context.TODO(), client.ObjectKey{Name: crd.GetName()}, existingCrd) switch { case apierrors.IsNotFound(err): - if err := cs.Create(context.TODO(), &crd); err != nil { + if err := cs.Create(context.TODO(), crd); err != nil { return fmt.Errorf("unable to create CRD %q: %w", crd.GetName(), err) } case err != nil: @@ -267,7 +267,7 @@ func CreateCRDs(config *rest.Config, crds []apiextensionsv1.CustomResourceDefini return err } crd.SetResourceVersion(existingCrd.GetResourceVersion()) - return cs.Update(context.TODO(), &crd) + return cs.Update(context.TODO(), crd) }); err != nil { return err } @@ -277,7 +277,7 @@ func CreateCRDs(config *rest.Config, crds []apiextensionsv1.CustomResourceDefini } // renderCRDs iterate through options.Paths and extract all CRD files. -func renderCRDs(options *CRDInstallOptions) ([]apiextensionsv1.CustomResourceDefinition, error) { +func renderCRDs(options *CRDInstallOptions) ([]*apiextensionsv1.CustomResourceDefinition, error) { var ( err error info os.FileInfo @@ -289,7 +289,7 @@ func renderCRDs(options *CRDInstallOptions) ([]apiextensionsv1.CustomResourceDef Name string } - crds := map[GVKN]apiextensionsv1.CustomResourceDefinition{} + crds := map[GVKN]*apiextensionsv1.CustomResourceDefinition{} for _, path := range options.Paths { var filePath = path @@ -326,7 +326,7 @@ func renderCRDs(options *CRDInstallOptions) ([]apiextensionsv1.CustomResourceDef } // Converting map to a list to return - res := []apiextensionsv1.CustomResourceDefinition{} + res := []*apiextensionsv1.CustomResourceDefinition{} for _, obj := range crds { res = append(res, obj) } @@ -335,7 +335,7 @@ func renderCRDs(options *CRDInstallOptions) ([]apiextensionsv1.CustomResourceDef // modifyConversionWebhooks takes all the registered CustomResourceDefinitions and applies modifications // to conditionally enable webhooks if the type is registered within the scheme. -func modifyConversionWebhooks(crds []apiextensionsv1.CustomResourceDefinition, scheme *runtime.Scheme, webhookOptions WebhookInstallOptions) error { +func modifyConversionWebhooks(crds []*apiextensionsv1.CustomResourceDefinition, scheme *runtime.Scheme, webhookOptions WebhookInstallOptions) error { if len(webhookOptions.LocalServingCAData) == 0 { return nil } @@ -389,8 +389,8 @@ func modifyConversionWebhooks(crds []apiextensionsv1.CustomResourceDefinition, s } // readCRDs reads the CRDs from files and Unmarshals them into structs. -func readCRDs(basePath string, files []os.FileInfo) ([]apiextensionsv1.CustomResourceDefinition, error) { - var crds []apiextensionsv1.CustomResourceDefinition +func readCRDs(basePath string, files []os.FileInfo) ([]*apiextensionsv1.CustomResourceDefinition, error) { + var crds []*apiextensionsv1.CustomResourceDefinition // White list the file extensions that may contain CRDs crdExts := sets.NewString(".json", ".yaml", ".yml") @@ -416,7 +416,7 @@ func readCRDs(basePath string, files []os.FileInfo) ([]apiextensionsv1.CustomRes if crd.Kind != "CustomResourceDefinition" || crd.Spec.Names.Kind == "" || crd.Spec.Group == "" { continue } - crds = append(crds, *crd) + crds = append(crds, crd) } log.V(1).Info("read CRDs from file", "file", file.Name()) diff --git a/pkg/envtest/envtest_suite_test.go b/pkg/envtest/envtest_suite_test.go index 38332b7989..0d5bb0eae2 100644 --- a/pkg/envtest/envtest_suite_test.go +++ b/pkg/envtest/envtest_suite_test.go @@ -54,7 +54,7 @@ func initializeWebhookInEnvironment() { webhookPathV1 := "/failing" env.WebhookInstallOptions = WebhookInstallOptions{ - ValidatingWebhooks: []admissionv1.ValidatingWebhookConfiguration{ + ValidatingWebhooks: []*admissionv1.ValidatingWebhookConfiguration{ { ObjectMeta: metav1.ObjectMeta{ Name: "deployment-validation-webhook-config", diff --git a/pkg/envtest/envtest_test.go b/pkg/envtest/envtest_test.go index 745afeeb25..11eedca0ac 100644 --- a/pkg/envtest/envtest_test.go +++ b/pkg/envtest/envtest_test.go @@ -33,7 +33,7 @@ import ( ) var _ = Describe("Test", func() { - var crds []apiextensionsv1.CustomResourceDefinition + var crds []*apiextensionsv1.CustomResourceDefinition var err error var s *runtime.Scheme var c client.Client @@ -45,7 +45,7 @@ var _ = Describe("Test", func() { // Initialize the client BeforeEach(func() { - crds = []apiextensionsv1.CustomResourceDefinition{} + crds = []*apiextensionsv1.CustomResourceDefinition{} s = scheme.Scheme err = apiextensionsv1.AddToScheme(s) Expect(err).NotTo(HaveOccurred()) @@ -69,7 +69,7 @@ var _ = Describe("Test", func() { continue } Expect(err).NotTo(HaveOccurred()) - Expect(c.Delete(context.TODO(), &crd)).To(Succeed()) + Expect(c.Delete(context.TODO(), crd)).To(Succeed()) Eventually(func() bool { err := c.Get(context.TODO(), crdObjectKey, &placeholder) return apierrors.IsNotFound(err) @@ -91,7 +91,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) Expect(crd.Spec.Names.Kind).To(Equal("Frigate")) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "ship.example.com", @@ -149,7 +149,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) Expect(crd.Spec.Names.Kind).To(Equal("Driver")) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "bar.example.com", @@ -256,7 +256,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) Expect(crd.Spec.Names.Kind).To(Equal("Config")) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "foo.example.com", @@ -305,7 +305,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) Expect(crd.Spec.Names.Kind).To(Equal("Foo")) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "bar.example.com", @@ -353,7 +353,7 @@ var _ = Describe("Test", func() { It("should return an error if the resource group version isn't found", func() { // Wait for a CRD where the Group and Version don't exist err := WaitForCRDs(env.Config, - []apiextensionsv1.CustomResourceDefinition{ + []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ @@ -383,7 +383,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) // Wait for a CRD that doesn't exist, but the Group and Version do - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "qux.example.com", @@ -457,7 +457,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) Expect(crd.Spec.Names.Kind).To(Equal("Driver")) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "bar.example.com", @@ -586,7 +586,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) Expect(crd.Spec.Names.Kind).To(Equal("Driver")) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "bar.example.com", @@ -702,7 +702,7 @@ var _ = Describe("Test", func() { // Store resource version for comparison later on firstRV := crd.ResourceVersion - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "crew.example.com", @@ -742,7 +742,7 @@ var _ = Describe("Test", func() { Expect(len(crd.Spec.Versions)).To(BeEquivalentTo(3)) Expect(crd.ResourceVersion).NotTo(BeEquivalentTo(firstRV)) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "crew.example.com", @@ -808,7 +808,7 @@ var _ = Describe("Test", func() { Expect(err).NotTo(HaveOccurred()) Expect(crd.Spec.Names.Kind).To(Equal("Driver")) - err = WaitForCRDs(env.Config, []apiextensionsv1.CustomResourceDefinition{ + err = WaitForCRDs(env.Config, []*apiextensionsv1.CustomResourceDefinition{ { Spec: apiextensionsv1.CustomResourceDefinitionSpec{ Group: "bar.example.com", diff --git a/pkg/envtest/helper.go b/pkg/envtest/helper.go index 70906db6d6..d3b52017d2 100644 --- a/pkg/envtest/helper.go +++ b/pkg/envtest/helper.go @@ -51,18 +51,18 @@ func mergePaths(s1, s2 []string) []string { // mergeCRDs merges two CRD slices using their names. // This function makes no guarantees about order of the merged slice. -func mergeCRDs(s1, s2 []apiextensionsv1.CustomResourceDefinition) []apiextensionsv1.CustomResourceDefinition { - m := make(map[string]apiextensionsv1.CustomResourceDefinition) +func mergeCRDs(s1, s2 []*apiextensionsv1.CustomResourceDefinition) []*apiextensionsv1.CustomResourceDefinition { + m := make(map[string]*apiextensionsv1.CustomResourceDefinition) for _, obj := range s1 { m[obj.GetName()] = obj } for _, obj := range s2 { m[obj.GetName()] = obj } - merged := make([]apiextensionsv1.CustomResourceDefinition, len(m)) + merged := make([]*apiextensionsv1.CustomResourceDefinition, len(m)) i := 0 for _, obj := range m { - merged[i] = obj + merged[i] = obj.DeepCopy() i++ } return merged diff --git a/pkg/envtest/server.go b/pkg/envtest/server.go index 571f04bf60..5347f074de 100644 --- a/pkg/envtest/server.go +++ b/pkg/envtest/server.go @@ -136,7 +136,7 @@ type Environment struct { // CRDs is a list of CRDs to install. // If both this field and CRDs field in CRDInstallOptions are specified, the // values are merged. - CRDs []apiextensionsv1.CustomResourceDefinition + CRDs []*apiextensionsv1.CustomResourceDefinition // CRDDirectoryPaths is a list of paths containing CRD yaml or json configs. // If both this field and Paths field in CRDInstallOptions are specified, the diff --git a/pkg/envtest/webhook.go b/pkg/envtest/webhook.go index bdd160bb2d..8552d3ba61 100644 --- a/pkg/envtest/webhook.go +++ b/pkg/envtest/webhook.go @@ -45,10 +45,10 @@ type WebhookInstallOptions struct { Paths []string // MutatingWebhooks is a list of MutatingWebhookConfigurations to install - MutatingWebhooks []admissionv1.MutatingWebhookConfiguration + MutatingWebhooks []*admissionv1.MutatingWebhookConfiguration // ValidatingWebhooks is a list of ValidatingWebhookConfigurations to install - ValidatingWebhooks []admissionv1.ValidatingWebhookConfiguration + ValidatingWebhooks []*admissionv1.ValidatingWebhookConfiguration // IgnoreErrorIfPathMissing will ignore an error if a DirectoryPath does not exist when set to true IgnoreErrorIfPathMissing bool @@ -171,14 +171,14 @@ func (o *WebhookInstallOptions) Cleanup() error { // WaitForWebhooks waits for the Webhooks to be available through API server. func WaitForWebhooks(config *rest.Config, - mutatingWebhooks []admissionv1.MutatingWebhookConfiguration, - validatingWebhooks []admissionv1.ValidatingWebhookConfiguration, + mutatingWebhooks []*admissionv1.MutatingWebhookConfiguration, + validatingWebhooks []*admissionv1.ValidatingWebhookConfiguration, options WebhookInstallOptions) error { waitingFor := map[schema.GroupVersionKind]*sets.String{} for _, hook := range mutatingWebhooks { h := hook - gvk, err := apiutil.GVKForObject(&h, scheme.Scheme) + gvk, err := apiutil.GVKForObject(h, scheme.Scheme) if err != nil { return fmt.Errorf("unable to get gvk for MutatingWebhookConfiguration %s: %v", hook.GetName(), err) } @@ -191,7 +191,7 @@ func WaitForWebhooks(config *rest.Config, for _, hook := range validatingWebhooks { h := hook - gvk, err := apiutil.GVKForObject(&h, scheme.Scheme) + gvk, err := apiutil.GVKForObject(h, scheme.Scheme) if err != nil { return fmt.Errorf("unable to get gvk for ValidatingWebhookConfiguration %s: %v", hook.GetName(), err) } @@ -288,7 +288,7 @@ func (o *WebhookInstallOptions) setupCA() error { return err } -func createWebhooks(config *rest.Config, mutHooks []admissionv1.MutatingWebhookConfiguration, valHooks []admissionv1.ValidatingWebhookConfiguration) error { +func createWebhooks(config *rest.Config, mutHooks []*admissionv1.MutatingWebhookConfiguration, valHooks []*admissionv1.ValidatingWebhookConfiguration) error { cs, err := client.New(config, client.Options{}) if err != nil { return err @@ -298,14 +298,14 @@ func createWebhooks(config *rest.Config, mutHooks []admissionv1.MutatingWebhookC for _, hook := range mutHooks { hook := hook log.V(1).Info("installing mutating webhook", "webhook", hook.GetName()) - if err := ensureCreated(cs, &hook); err != nil { + if err := ensureCreated(cs, hook); err != nil { return err } } for _, hook := range valHooks { hook := hook log.V(1).Info("installing validating webhook", "webhook", hook.GetName()) - if err := ensureCreated(cs, &hook); err != nil { + if err := ensureCreated(cs, hook); err != nil { return err } } @@ -357,7 +357,7 @@ func parseWebhook(options *WebhookInstallOptions) error { // readWebhooks reads the Webhooks from files and Unmarshals them into structs // returns slice of mutating and validating webhook configurations. -func readWebhooks(path string) ([]admissionv1.MutatingWebhookConfiguration, []admissionv1.ValidatingWebhookConfiguration, error) { +func readWebhooks(path string) ([]*admissionv1.MutatingWebhookConfiguration, []*admissionv1.ValidatingWebhookConfiguration, error) { // Get the webhook files var files []os.FileInfo var err error @@ -375,8 +375,8 @@ func readWebhooks(path string) ([]admissionv1.MutatingWebhookConfiguration, []ad // file extensions that may contain Webhooks resourceExtensions := sets.NewString(".json", ".yaml", ".yml") - var mutHooks []admissionv1.MutatingWebhookConfiguration - var valHooks []admissionv1.ValidatingWebhookConfiguration + var mutHooks []*admissionv1.MutatingWebhookConfiguration + var valHooks []*admissionv1.ValidatingWebhookConfiguration for _, file := range files { // Only parse allowlisted file types if !resourceExtensions.Has(filepath.Ext(file.Name())) { @@ -403,8 +403,8 @@ func readWebhooks(path string) ([]admissionv1.MutatingWebhookConfiguration, []ad if generic.APIVersion != admissionregv1 { return nil, nil, fmt.Errorf("only v1 is supported right now for MutatingWebhookConfiguration (name: %s)", generic.Name) } - hook := admissionv1.MutatingWebhookConfiguration{} - if err := yaml.Unmarshal(doc, &hook); err != nil { + hook := &admissionv1.MutatingWebhookConfiguration{} + if err := yaml.Unmarshal(doc, hook); err != nil { return nil, nil, err } mutHooks = append(mutHooks, hook) @@ -412,8 +412,8 @@ func readWebhooks(path string) ([]admissionv1.MutatingWebhookConfiguration, []ad if generic.APIVersion != admissionregv1 { return nil, nil, fmt.Errorf("only v1 is supported right now for ValidatingWebhookConfiguration (name: %s)", generic.Name) } - hook := admissionv1.ValidatingWebhookConfiguration{} - if err := yaml.Unmarshal(doc, &hook); err != nil { + hook := &admissionv1.ValidatingWebhookConfiguration{} + if err := yaml.Unmarshal(doc, hook); err != nil { return nil, nil, err } valHooks = append(valHooks, hook) diff --git a/pkg/webhook/webhook_suite_test.go b/pkg/webhook/webhook_suite_test.go index dbc22543af..b8ee879d36 100644 --- a/pkg/webhook/webhook_suite_test.go +++ b/pkg/webhook/webhook_suite_test.go @@ -65,7 +65,7 @@ func initializeWebhookInEnvironment() { webhookPathV1 := "/failing" testenv.WebhookInstallOptions = envtest.WebhookInstallOptions{ - ValidatingWebhooks: []admissionv1.ValidatingWebhookConfiguration{ + ValidatingWebhooks: []*admissionv1.ValidatingWebhookConfiguration{ { ObjectMeta: metav1.ObjectMeta{ Name: "deployment-validation-webhook-config",