From 3c9b982b805d9c71265d19c19ea72d3f1dedfcc1 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Sun, 23 Jul 2023 11:53:59 +0200 Subject: [PATCH 1/2] Add integration test to avoid manager.Start deadlocks --- pkg/envtest/server.go | 3 + pkg/manager/integration/manager_suite_test.go | 29 ++ pkg/manager/integration/manager_test.go | 286 ++++++++++++++++++ pkg/manager/integration/v1/driver_types.go | 93 ++++++ .../integration/v1/groupversion_info.go | 34 +++ pkg/manager/integration/v2/driver_types.go | 111 +++++++ .../integration/v2/groupversion_info.go | 34 +++ 7 files changed, 590 insertions(+) create mode 100644 pkg/manager/integration/manager_suite_test.go create mode 100644 pkg/manager/integration/manager_test.go create mode 100644 pkg/manager/integration/v1/driver_types.go create mode 100644 pkg/manager/integration/v1/groupversion_info.go create mode 100644 pkg/manager/integration/v2/driver_types.go create mode 100644 pkg/manager/integration/v2/groupversion_info.go diff --git a/pkg/envtest/server.go b/pkg/envtest/server.go index 4ee440df9c..fc715345d2 100644 --- a/pkg/envtest/server.go +++ b/pkg/envtest/server.go @@ -289,6 +289,9 @@ func (te *Environment) Start() (*rest.Config, error) { } log.V(1).Info("installing CRDs") + if te.CRDInstallOptions.Scheme == nil { + te.CRDInstallOptions.Scheme = te.Scheme + } te.CRDInstallOptions.CRDs = mergeCRDs(te.CRDInstallOptions.CRDs, te.CRDs) te.CRDInstallOptions.Paths = mergePaths(te.CRDInstallOptions.Paths, te.CRDDirectoryPaths) te.CRDInstallOptions.ErrorIfPathMissing = te.ErrorIfCRDPathMissing diff --git a/pkg/manager/integration/manager_suite_test.go b/pkg/manager/integration/manager_suite_test.go new file mode 100644 index 0000000000..cc8a728794 --- /dev/null +++ b/pkg/manager/integration/manager_suite_test.go @@ -0,0 +1,29 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package integration + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestSource(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Manager Integration Suite") +} diff --git a/pkg/manager/integration/manager_test.go b/pkg/manager/integration/manager_test.go new file mode 100644 index 0000000000..938f786dc8 --- /dev/null +++ b/pkg/manager/integration/manager_test.go @@ -0,0 +1,286 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package integration + +import ( + "context" + "fmt" + "net" + "net/http" + "reflect" + "sync/atomic" + "time" + "unsafe" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/manager" + crewv1 "sigs.k8s.io/controller-runtime/pkg/manager/integration/v1" + crewv2 "sigs.k8s.io/controller-runtime/pkg/manager/integration/v2" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/conversion" +) + +var ( + scheme = runtime.NewScheme() + + driverCRD = &apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "drivers.crew.example.com", + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: crewv1.GroupVersion.Group, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "drivers", + Singular: "driver", + Kind: "Driver", + }, + Scope: apiextensionsv1.NamespaceScoped, + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: crewv1.GroupVersion.Version, + Served: true, + // At creation v1 will be the storage version. + // During the test v2 will become the storage version. + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + }, + }, + }, + { + Name: crewv2.GroupVersion.Version, + Served: true, + Storage: false, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + }, + }, + }, + }, + }, + } +) + +var _ = Describe("manger.Manager", func() { + // This test ensure the Manager starts without running into any deadlocks as it can be very tricky + // to start health probes, webhooks, caches (including informers) and reconcilers in the right order. + // + // To verify this we set up a test environment in the following state: + // * Ensure Informer sync requires a functioning conversion webhook (and thus readiness probe) + // * Driver CRD is deployed with v1 as storage version + // * A Driver CR is created and stored in the v1 version + // * The CRD is updated to make v2 the storage version + // * This ensures every Driver list call goes through conversion. + // * Setup manager: + // * Set up health probes + // * Set up a Driver reconciler to verify reconciliation works + // * Set up a conversion webhook which only works if readiness probe succeeds (just like a Kubernetes service) + // * Add an index on v2 Driver to ensure we start and wait for an informer during cache.Start (as part of manager.Start) + // * Note: cache.Start would fail if the conversion webhook doesn't work (which in turn depends on the readiness probe) + Describe("Start should start all components without deadlock", func() { + ctx := ctrl.SetupSignalHandler() + + // Set up schema. + Expect(clientgoscheme.AddToScheme(scheme)).To(Succeed()) + Expect(apiextensionsv1.AddToScheme(scheme)).To(Succeed()) + Expect(crewv1.AddToScheme(scheme)).To(Succeed()) + Expect(crewv2.AddToScheme(scheme)).To(Succeed()) + + // Set up test environment. + env := &envtest.Environment{ + Scheme: scheme, + CRDInstallOptions: envtest.CRDInstallOptions{ + CRDs: []*apiextensionsv1.CustomResourceDefinition{driverCRD}, + }, + } + cfg, err := env.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + defer func() { + Expect(env.Stop()).To(Succeed()) + }() + c, err := client.New(cfg, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + + // Create driver CR (which is stored as v1). + driverV1 := &unstructured.Unstructured{} + driverV1.SetGroupVersionKind(crewv1.GroupVersion.WithKind("Driver")) + driverV1.SetName("driver1") + driverV1.SetNamespace(metav1.NamespaceDefault) + Expect(c.Create(ctx, driverV1)).To(Succeed()) + + // Update driver CRD to make v2 the storage version. + driverCRDV2Storage := driverCRD.DeepCopy() + driverCRDV2Storage.Spec.Versions[0].Storage = false + driverCRDV2Storage.Spec.Versions[0].Storage = true + Expect(c.Patch(ctx, driverCRDV2Storage, client.MergeFrom(driverCRD))).To(Succeed()) + + // Set up Manager. + ctrl.SetLogger(zap.New()) + mgr, err := manager.New(env.Config, manager.Options{ + Scheme: scheme, + HealthProbeBindAddress: ":0", + // Disable metrics to avoid port conflicts. + MetricsBindAddress: "0", + WebhookServer: webhook.NewServer(webhook.Options{ + Port: env.WebhookInstallOptions.LocalServingPort, + Host: env.WebhookInstallOptions.LocalServingHost, + CertDir: env.WebhookInstallOptions.LocalServingCertDir, + }), + }) + Expect(err).NotTo(HaveOccurred()) + + // Configure health probes. + Expect(mgr.AddReadyzCheck("webhook", mgr.GetWebhookServer().StartedChecker())).To(Succeed()) + Expect(mgr.AddHealthzCheck("webhook", mgr.GetWebhookServer().StartedChecker())).To(Succeed()) + + // Set up Driver reconciler. + driverReconciler := &DriverReconciler{ + Client: mgr.GetClient(), + } + Expect(ctrl.NewControllerManagedBy(mgr).For(&crewv1.Driver{}).Complete(driverReconciler)).To(Succeed()) + + // Set up a conversion webhook. + conversionWebhook := createConversionWebhook(mgr) + mgr.GetWebhookServer().Register("/convert", conversionWebhook) + + // Add an index on v2 Driver. + Expect(mgr.GetCache().IndexField(ctx, &crewv2.Driver{}, "name", func(object client.Object) []string { + return []string{object.GetName()} + })).To(Succeed()) + + // Start the Manager. + ctx, cancel := context.WithCancel(ctx) + go func() { + defer GinkgoRecover() + Expect(mgr.Start(ctx)).NotTo(HaveOccurred()) + }() + + // Verify manager.Start successfully started health probes, webhooks, caches (including informers) and reconcilers. + <-mgr.Elected() + + // Verify the reconciler reconciles. + Eventually(func(g Gomega) { + g.Expect(atomic.LoadUint64(&driverReconciler.ReconcileCount)).Should(BeNumerically(">", 0)) + }, 10*time.Second).Should(Succeed()) + + // Verify conversion webhook was called. + Expect(atomic.LoadUint64(&conversionWebhook.ConversionCount)).Should(BeNumerically(">", 0)) + + // Verify the conversion webhook works. + driverV2 := &unstructured.Unstructured{} + driverV2.SetGroupVersionKind(crewv2.GroupVersion.WithKind("Driver")) + driverV2.SetName("driver1") + driverV2.SetNamespace(metav1.NamespaceDefault) + Expect(c.Get(ctx, client.ObjectKeyFromObject(driverV2), driverV2)).To(Succeed()) + + // Shutdown the server + cancel() + }) +}) + +type DriverReconciler struct { + Client client.Client + ReconcileCount uint64 +} + +func (r *DriverReconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { + log := ctrl.LoggerFrom(ctx) + log.Info("Reconciling") + + // Fetch the Driver instance. + cluster := &crewv2.Driver{} + if err := r.Client.Get(ctx, req.NamespacedName, cluster); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + + // Error reading the object - requeue the request. + return ctrl.Result{}, err + } + + atomic.AddUint64(&r.ReconcileCount, 1) + + return reconcile.Result{}, nil +} + +type ConversionWebhook struct { + httpClient http.Client + conversionHandler http.Handler + readinessEndpoint string + ConversionCount uint64 +} + +func createConversionWebhook(mgr manager.Manager) *ConversionWebhook { + conversionHandler := conversion.NewWebhookHandler(mgr.GetScheme()) + httpClient := http.Client{ + // Setting a timeout to not get stuck when calling the readiness probe. + Timeout: 5 * time.Second, + } + + // Read the unexported healthProbeListener field of the manager to get the listener address. + // This is a hack but it's better than using a hard-coded port. + v := reflect.ValueOf(mgr).Elem() + field := v.FieldByName("healthProbeListener") + healthProbeListener := *(*net.Listener)(unsafe.Pointer(field.UnsafeAddr())) //nolint:gosec + readinessEndpoint := fmt.Sprint("http://", healthProbeListener.Addr().String(), "/readyz") + + return &ConversionWebhook{ + httpClient: httpClient, + conversionHandler: conversionHandler, + readinessEndpoint: readinessEndpoint, + } +} + +func (c *ConversionWebhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { + resp, err := c.httpClient.Get(c.readinessEndpoint) + if err != nil { + logf.Log.WithName("conversion-webhook").Error(err, "failed to serve conversion: readiness endpoint is not up") + w.WriteHeader(http.StatusInternalServerError) + return + } + + Expect(err).NotTo(HaveOccurred()) + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + // This simulates the behavior in Kubernetes that conversion webhooks are only served after + // the controller is ready (and thus the Kubernetes service sends requests to the controller). + logf.Log.WithName("conversion-webhook").Info("failed to serve conversion: controller is not ready yet") + w.WriteHeader(http.StatusInternalServerError) + return + } + + atomic.AddUint64(&c.ConversionCount, 1) + c.conversionHandler.ServeHTTP(w, r) +} diff --git a/pkg/manager/integration/v1/driver_types.go b/pkg/manager/integration/v1/driver_types.go new file mode 100644 index 0000000000..9182ed4cc8 --- /dev/null +++ b/pkg/manager/integration/v1/driver_types.go @@ -0,0 +1,93 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +// Driver is a test type. +type Driver struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` +} + +// DriverList is a list of Drivers. +type DriverList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []Driver `json:"items"` +} + +func init() { + SchemeBuilder.Register(&Driver{}, &DriverList{}) +} + +// DeepCopyInto deep copies into the given Driver. +func (d *Driver) DeepCopyInto(out *Driver) { + *out = *d + out.TypeMeta = d.TypeMeta + d.ObjectMeta.DeepCopyInto(&out.ObjectMeta) +} + +// DeepCopy returns a copy of Driver. +func (d *Driver) DeepCopy() *Driver { + if d == nil { + return nil + } + out := new(Driver) + d.DeepCopyInto(out) + return out +} + +// DeepCopyObject returns a copy of Driver as runtime.Object. +func (d *Driver) DeepCopyObject() runtime.Object { + return d.DeepCopy() +} + +// DeepCopyInto deep copies into the given DriverList. +func (in *DriverList) DeepCopyInto(out *DriverList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]Driver, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy returns a copy of DriverList. +func (in *DriverList) DeepCopy() *DriverList { + if in == nil { + return nil + } + out := new(DriverList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject returns a copy of DriverList as runtime.Object. +func (in *DriverList) DeepCopyObject() runtime.Object { + return in.DeepCopy() +} + +// Hub marks Driver as a Hub for conversion. +func (*Driver) Hub() {} diff --git a/pkg/manager/integration/v1/groupversion_info.go b/pkg/manager/integration/v1/groupversion_info.go new file mode 100644 index 0000000000..3986a6d023 --- /dev/null +++ b/pkg/manager/integration/v1/groupversion_info.go @@ -0,0 +1,34 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import ( + "k8s.io/apimachinery/pkg/runtime/schema" + + "sigs.k8s.io/controller-runtime/pkg/scheme" +) + +var ( + // GroupVersion is group version used to register these objects. + GroupVersion = schema.GroupVersion{Group: "crew.example.com", Version: "v1"} + + // SchemeBuilder is used to add go types to the GroupVersionKind scheme. + SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion} + + // AddToScheme adds the types in this group-version to the given scheme. + AddToScheme = SchemeBuilder.AddToScheme +) diff --git a/pkg/manager/integration/v2/driver_types.go b/pkg/manager/integration/v2/driver_types.go new file mode 100644 index 0000000000..de1d4fb293 --- /dev/null +++ b/pkg/manager/integration/v2/driver_types.go @@ -0,0 +1,111 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v2 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + + "sigs.k8s.io/controller-runtime/pkg/conversion" + v1 "sigs.k8s.io/controller-runtime/pkg/manager/integration/v1" +) + +// Driver is a test type. +type Driver struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` +} + +// DriverList is a list of Drivers. +type DriverList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []Driver `json:"items"` +} + +func init() { + SchemeBuilder.Register(&Driver{}, &DriverList{}) +} + +// DeepCopyInto deep copies into the given Driver. +func (d *Driver) DeepCopyInto(out *Driver) { + *out = *d + out.TypeMeta = d.TypeMeta + d.ObjectMeta.DeepCopyInto(&out.ObjectMeta) +} + +// DeepCopy returns a copy of Driver. +func (d *Driver) DeepCopy() *Driver { + if d == nil { + return nil + } + out := new(Driver) + d.DeepCopyInto(out) + return out +} + +// DeepCopyObject returns a copy of Driver as runtime.Object. +func (d *Driver) DeepCopyObject() runtime.Object { + return d.DeepCopy() +} + +// DeepCopyInto deep copies into the given DriverList. +func (in *DriverList) DeepCopyInto(out *DriverList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]Driver, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy returns a copy of DriverList. +func (in *DriverList) DeepCopy() *DriverList { + if in == nil { + return nil + } + out := new(DriverList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject returns a copy of DriverList as runtime.Object. +func (in *DriverList) DeepCopyObject() runtime.Object { + return in.DeepCopy() +} + +// ConvertTo converts Driver to the Hub version of driver. +func (d *Driver) ConvertTo(dstRaw conversion.Hub) error { + dst := dstRaw.(*v1.Driver) + dst.Name = d.Name + dst.Namespace = d.Namespace + dst.UID = d.UID + return nil +} + +// ConvertFrom converts Driver from the Hub version of driver. +func (d *Driver) ConvertFrom(srcRaw conversion.Hub) error { + src := srcRaw.(*v1.Driver) + d.Name = src.Name + d.Namespace = src.Namespace + d.UID = src.UID + return nil +} diff --git a/pkg/manager/integration/v2/groupversion_info.go b/pkg/manager/integration/v2/groupversion_info.go new file mode 100644 index 0000000000..218a742793 --- /dev/null +++ b/pkg/manager/integration/v2/groupversion_info.go @@ -0,0 +1,34 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v2 + +import ( + "k8s.io/apimachinery/pkg/runtime/schema" + + "sigs.k8s.io/controller-runtime/pkg/scheme" +) + +var ( + // GroupVersion is group version used to register these objects. + GroupVersion = schema.GroupVersion{Group: "crew.example.com", Version: "v2"} + + // SchemeBuilder is used to add go types to the GroupVersionKind scheme. + SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion} + + // AddToScheme adds the types in this group-version to the given scheme. + AddToScheme = SchemeBuilder.AddToScheme +) From 3ecb530e1270b9a2fd5ad12eececdbe39885dc59 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 24 Jul 2023 19:05:44 +0200 Subject: [PATCH 2/2] fix review findings --- .../integration/api}/v1/driver_types.go | 0 .../integration/api}/v1/groupversion_info.go | 0 .../integration/api}/v2/driver_types.go | 2 +- .../integration/api}/v2/groupversion_info.go | 0 .../integration/manager_suite_test.go | 2 +- .../integration/manager_test.go | 52 +++++++++++-------- 6 files changed, 32 insertions(+), 24 deletions(-) rename pkg/manager/{integration => internal/integration/api}/v1/driver_types.go (100%) rename pkg/manager/{integration => internal/integration/api}/v1/groupversion_info.go (100%) rename pkg/manager/{integration => internal/integration/api}/v2/driver_types.go (97%) rename pkg/manager/{integration => internal/integration/api}/v2/groupversion_info.go (100%) rename pkg/manager/{ => internal}/integration/manager_suite_test.go (95%) rename pkg/manager/{ => internal}/integration/manager_test.go (83%) diff --git a/pkg/manager/integration/v1/driver_types.go b/pkg/manager/internal/integration/api/v1/driver_types.go similarity index 100% rename from pkg/manager/integration/v1/driver_types.go rename to pkg/manager/internal/integration/api/v1/driver_types.go diff --git a/pkg/manager/integration/v1/groupversion_info.go b/pkg/manager/internal/integration/api/v1/groupversion_info.go similarity index 100% rename from pkg/manager/integration/v1/groupversion_info.go rename to pkg/manager/internal/integration/api/v1/groupversion_info.go diff --git a/pkg/manager/integration/v2/driver_types.go b/pkg/manager/internal/integration/api/v2/driver_types.go similarity index 97% rename from pkg/manager/integration/v2/driver_types.go rename to pkg/manager/internal/integration/api/v2/driver_types.go index de1d4fb293..64012ac749 100644 --- a/pkg/manager/integration/v2/driver_types.go +++ b/pkg/manager/internal/integration/api/v2/driver_types.go @@ -21,7 +21,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/conversion" - v1 "sigs.k8s.io/controller-runtime/pkg/manager/integration/v1" + v1 "sigs.k8s.io/controller-runtime/pkg/manager/internal/integration/api/v1" ) // Driver is a test type. diff --git a/pkg/manager/integration/v2/groupversion_info.go b/pkg/manager/internal/integration/api/v2/groupversion_info.go similarity index 100% rename from pkg/manager/integration/v2/groupversion_info.go rename to pkg/manager/internal/integration/api/v2/groupversion_info.go diff --git a/pkg/manager/integration/manager_suite_test.go b/pkg/manager/internal/integration/manager_suite_test.go similarity index 95% rename from pkg/manager/integration/manager_suite_test.go rename to pkg/manager/internal/integration/manager_suite_test.go index cc8a728794..1a5a20d5a5 100644 --- a/pkg/manager/integration/manager_suite_test.go +++ b/pkg/manager/internal/integration/manager_suite_test.go @@ -23,7 +23,7 @@ import ( . "github.com/onsi/gomega" ) -func TestSource(t *testing.T) { +func TestManager(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Manager Integration Suite") } diff --git a/pkg/manager/integration/manager_test.go b/pkg/manager/internal/integration/manager_test.go similarity index 83% rename from pkg/manager/integration/manager_test.go rename to pkg/manager/internal/integration/manager_test.go index 938f786dc8..5c315d170f 100644 --- a/pkg/manager/integration/manager_test.go +++ b/pkg/manager/internal/integration/manager_test.go @@ -41,8 +41,8 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/manager" - crewv1 "sigs.k8s.io/controller-runtime/pkg/manager/integration/v1" - crewv2 "sigs.k8s.io/controller-runtime/pkg/manager/integration/v2" + crewv1 "sigs.k8s.io/controller-runtime/pkg/manager/internal/integration/api/v1" + crewv2 "sigs.k8s.io/controller-runtime/pkg/manager/internal/integration/api/v2" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/conversion" @@ -67,8 +67,8 @@ var ( { Name: crewv1.GroupVersion.Version, Served: true, - // At creation v1 will be the storage version. - // During the test v2 will become the storage version. + // v1 will be the storage version. + // Reconciler and index will use v2 so we can validate the conversion webhook works. Storage: true, Schema: &apiextensionsv1.CustomResourceValidation{ OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ @@ -91,7 +91,7 @@ var ( } ) -var _ = Describe("manger.Manager", func() { +var _ = Describe("manger.Manager Start", func() { // This test ensure the Manager starts without running into any deadlocks as it can be very tricky // to start health probes, webhooks, caches (including informers) and reconcilers in the right order. // @@ -99,15 +99,14 @@ var _ = Describe("manger.Manager", func() { // * Ensure Informer sync requires a functioning conversion webhook (and thus readiness probe) // * Driver CRD is deployed with v1 as storage version // * A Driver CR is created and stored in the v1 version - // * The CRD is updated to make v2 the storage version - // * This ensures every Driver list call goes through conversion. // * Setup manager: // * Set up health probes - // * Set up a Driver reconciler to verify reconciliation works - // * Set up a conversion webhook which only works if readiness probe succeeds (just like a Kubernetes service) + // * Set up a Driver v2 reconciler to verify reconciliation works + // * Set up a conversion webhook which only works if readiness probe succeeds (just like via a Kubernetes service) // * Add an index on v2 Driver to ensure we start and wait for an informer during cache.Start (as part of manager.Start) // * Note: cache.Start would fail if the conversion webhook doesn't work (which in turn depends on the readiness probe) - Describe("Start should start all components without deadlock", func() { + // * Note: Adding the index for v2 ensures the Driver list call during Informer sync goes through conversion. + It("should start all components without deadlock", func() { ctx := ctrl.SetupSignalHandler() // Set up schema. @@ -123,6 +122,7 @@ var _ = Describe("manger.Manager", func() { CRDs: []*apiextensionsv1.CustomResourceDefinition{driverCRD}, }, } + // Note: The test env configures a conversion webhook on driverCRD during Start. cfg, err := env.Start() Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) @@ -139,12 +139,6 @@ var _ = Describe("manger.Manager", func() { driverV1.SetNamespace(metav1.NamespaceDefault) Expect(c.Create(ctx, driverV1)).To(Succeed()) - // Update driver CRD to make v2 the storage version. - driverCRDV2Storage := driverCRD.DeepCopy() - driverCRDV2Storage.Spec.Versions[0].Storage = false - driverCRDV2Storage.Spec.Versions[0].Storage = true - Expect(c.Patch(ctx, driverCRDV2Storage, client.MergeFrom(driverCRD))).To(Succeed()) - // Set up Manager. ctrl.SetLogger(zap.New()) mgr, err := manager.New(env.Config, manager.Options{ @@ -164,17 +158,18 @@ var _ = Describe("manger.Manager", func() { Expect(mgr.AddReadyzCheck("webhook", mgr.GetWebhookServer().StartedChecker())).To(Succeed()) Expect(mgr.AddHealthzCheck("webhook", mgr.GetWebhookServer().StartedChecker())).To(Succeed()) - // Set up Driver reconciler. + // Set up Driver reconciler (using v2). driverReconciler := &DriverReconciler{ Client: mgr.GetClient(), } - Expect(ctrl.NewControllerManagedBy(mgr).For(&crewv1.Driver{}).Complete(driverReconciler)).To(Succeed()) + Expect(ctrl.NewControllerManagedBy(mgr).For(&crewv2.Driver{}).Complete(driverReconciler)).To(Succeed()) // Set up a conversion webhook. conversionWebhook := createConversionWebhook(mgr) mgr.GetWebhookServer().Register("/convert", conversionWebhook) - // Add an index on v2 Driver. + // Add an index on Driver (using v2). + // Note: This triggers the creation of an Informer for Driver v2. Expect(mgr.GetCache().IndexField(ctx, &crewv2.Driver{}, "name", func(object client.Object) []string { return []string{object.GetName()} })).To(Succeed()) @@ -183,11 +178,19 @@ var _ = Describe("manger.Manager", func() { ctx, cancel := context.WithCancel(ctx) go func() { defer GinkgoRecover() - Expect(mgr.Start(ctx)).NotTo(HaveOccurred()) + Expect(mgr.Start(ctx)).To(Succeed()) }() // Verify manager.Start successfully started health probes, webhooks, caches (including informers) and reconcilers. - <-mgr.Elected() + // Notes: + // * The cache will only start successfully if the informer for v2 Driver is synced. + // * The informer for v2 Driver will only sync if a list on v2 Driver succeeds (which requires a working conversion webhook) + select { + case <-time.After(30 * time.Second): + // Don't wait forever if the manager doesn't come up. + Fail("Manager didn't start in time") + case <-mgr.Elected(): + } // Verify the reconciler reconciles. Eventually(func(g Gomega) { @@ -197,7 +200,8 @@ var _ = Describe("manger.Manager", func() { // Verify conversion webhook was called. Expect(atomic.LoadUint64(&conversionWebhook.ConversionCount)).Should(BeNumerically(">", 0)) - // Verify the conversion webhook works. + // Verify the conversion webhook works by getting the Driver as v1 and v2. + Expect(c.Get(ctx, client.ObjectKeyFromObject(driverV1), driverV1)).To(Succeed()) driverV2 := &unstructured.Unstructured{} driverV2.SetGroupVersionKind(crewv2.GroupVersion.WithKind("Driver")) driverV2.SetName("driver1") @@ -234,6 +238,10 @@ func (r *DriverReconciler) Reconcile(ctx context.Context, req reconcile.Request) return reconcile.Result{}, nil } +// ConversionWebhook is just a shim around the conversion handler from +// the webhook package. We use it to simulate the behavior of a conversion +// webhook in a real cluster, i.e. the conversion webhook only works after the +// controller Pod is ready (the readiness probe is up). type ConversionWebhook struct { httpClient http.Client conversionHandler http.Handler