From 9f91fc504b4eacd1faa4a744087e818ee4ee4e5c Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 11 Oct 2023 12:28:53 -0400 Subject: [PATCH] (techdebt): refactor catalog controller unit tests (#196) * (techdebt): refactor catalog controller unit tests to no longer use Ginkgo and instead use the native Go testing and testify Signed-off-by: Bryce Palmer * remove rebase detritus, unnecessary IIFE, and featuregate comments/blocks. goimports. Signed-off-by: Bryce Palmer --------- Signed-off-by: Bryce Palmer --- cmd/manager/main.go | 54 +- config/manager/manager.yaml | 1 - pkg/controllers/core/catalog_controller.go | 15 +- .../core/catalog_controller_test.go | 831 ++++++++++-------- pkg/controllers/core/suite_test.go | 79 -- pkg/features/features.go | 14 +- 6 files changed, 506 insertions(+), 488 deletions(-) delete mode 100644 pkg/controllers/core/suite_test.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index d4c5c1b4..0a2646c3 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -140,38 +140,36 @@ func main() { } var localStorage storage.Instance - if features.CatalogdFeatureGate.Enabled(features.HTTPServer) { - metrics.Registry.MustRegister(catalogdmetrics.RequestDurationMetric) + metrics.Registry.MustRegister(catalogdmetrics.RequestDurationMetric) - storeDir := filepath.Join(cacheDir, storageDir) - if err := os.MkdirAll(storeDir, 0700); err != nil { - setupLog.Error(err, "unable to create storage directory for catalogs") - os.Exit(1) - } + storeDir := filepath.Join(cacheDir, storageDir) + if err := os.MkdirAll(storeDir, 0700); err != nil { + setupLog.Error(err, "unable to create storage directory for catalogs") + os.Exit(1) + } - baseStorageURL, err := url.Parse(fmt.Sprintf("%s/catalogs/", httpExternalAddr)) - if err != nil { - setupLog.Error(err, "unable to create base storage URL") - os.Exit(1) - } + baseStorageURL, err := url.Parse(fmt.Sprintf("%s/catalogs/", httpExternalAddr)) + if err != nil { + setupLog.Error(err, "unable to create base storage URL") + os.Exit(1) + } - localStorage = storage.LocalDir{RootDir: storeDir, BaseURL: baseStorageURL} - shutdownTimeout := 30 * time.Second - catalogServer := server.Server{ - Kind: "catalogs", - Server: &http.Server{ - Addr: catalogServerAddr, - Handler: catalogdmetrics.AddMetricsToHandler(localStorage.StorageServerHandler()), - ReadTimeout: 5 * time.Second, - WriteTimeout: 10 * time.Second, - }, - ShutdownTimeout: &shutdownTimeout, - } + localStorage = storage.LocalDir{RootDir: storeDir, BaseURL: baseStorageURL} + shutdownTimeout := 30 * time.Second + catalogServer := server.Server{ + Kind: "catalogs", + Server: &http.Server{ + Addr: catalogServerAddr, + Handler: catalogdmetrics.AddMetricsToHandler(localStorage.StorageServerHandler()), + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + }, + ShutdownTimeout: &shutdownTimeout, + } - if err := mgr.Add(&catalogServer); err != nil { - setupLog.Error(err, "unable to start catalog server") - os.Exit(1) - } + if err := mgr.Add(&catalogServer); err != nil { + setupLog.Error(err, "unable to start catalog server") + os.Exit(1) } if err = (&corecontrollers.CatalogReconciler{ diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 31343abc..b9905046 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -74,7 +74,6 @@ spec: args: - --leader-elect - --metrics-bind-address=127.0.0.1:8080 - - --feature-gates=HTTPServer=true - --http-external-address=http://catalogd-catalogserver.catalogd-system.svc image: controller:latest name: manager diff --git a/pkg/controllers/core/catalog_controller.go b/pkg/controllers/core/catalog_controller.go index 441e6ab7..303ea6ff 100644 --- a/pkg/controllers/core/catalog_controller.go +++ b/pkg/controllers/core/catalog_controller.go @@ -34,7 +34,6 @@ import ( "github.com/operator-framework/catalogd/api/core/v1alpha1" "github.com/operator-framework/catalogd/internal/source" catalogderrors "github.com/operator-framework/catalogd/pkg/errors" - "github.com/operator-framework/catalogd/pkg/features" "github.com/operator-framework/catalogd/pkg/storage" ) @@ -112,11 +111,11 @@ func (r *CatalogReconciler) SetupWithManager(mgr ctrl.Manager) error { // linting from the linter that was fussing about this. // nolint:unparam func (r *CatalogReconciler) reconcile(ctx context.Context, catalog *v1alpha1.Catalog) (ctrl.Result, error) { - if features.CatalogdFeatureGate.Enabled(features.HTTPServer) && catalog.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(catalog, fbcDeletionFinalizer) { + if catalog.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(catalog, fbcDeletionFinalizer) { controllerutil.AddFinalizer(catalog, fbcDeletionFinalizer) return ctrl.Result{}, nil } - if features.CatalogdFeatureGate.Enabled(features.HTTPServer) && !catalog.DeletionTimestamp.IsZero() { + if !catalog.DeletionTimestamp.IsZero() { if err := r.Storage.Delete(catalog.Name); err != nil { return ctrl.Result{}, updateStatusStorageDeleteError(&catalog.Status, err) } @@ -143,13 +142,11 @@ func (r *CatalogReconciler) reconcile(ctx context.Context, catalog *v1alpha1.Cat // TODO: We should check to see if the unpacked result has the same content // as the already unpacked content. If it does, we should skip this rest // of the unpacking steps. - if features.CatalogdFeatureGate.Enabled(features.HTTPServer) { - err := r.Storage.Store(catalog.Name, unpackResult.FS) - if err != nil { - return ctrl.Result{}, updateStatusStorageError(&catalog.Status, fmt.Errorf("error storing fbc: %v", err)) - } - contentURL = r.Storage.ContentURL(catalog.Name) + err := r.Storage.Store(catalog.Name, unpackResult.FS) + if err != nil { + return ctrl.Result{}, updateStatusStorageError(&catalog.Status, fmt.Errorf("error storing fbc: %v", err)) } + contentURL = r.Storage.ContentURL(catalog.Name) updateStatusUnpacked(&catalog.Status, unpackResult, contentURL) return ctrl.Result{}, nil diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index 759ce165..31fe413b 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -1,31 +1,23 @@ -package core_test +package core import ( "context" "errors" - "fmt" "io/fs" "net/http" - "os" + "testing" "testing/fstest" + "time" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "github.com/onsi/gomega/format" - "k8s.io/apimachinery/pkg/api/meta" + "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/rand" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/manager" - "github.com/operator-framework/catalogd/api/core/v1alpha1" + "github.com/google/go-cmp/cmp/cmpopts" + + catalogdv1alpha1 "github.com/operator-framework/catalogd/api/core/v1alpha1" "github.com/operator-framework/catalogd/internal/source" - "github.com/operator-framework/catalogd/pkg/controllers/core" - "github.com/operator-framework/catalogd/pkg/features" "github.com/operator-framework/catalogd/pkg/storage" ) @@ -40,7 +32,7 @@ type MockSource struct { shouldError bool } -func (ms *MockSource) Unpack(_ context.Context, _ *v1alpha1.Catalog) (*source.Result, error) { +func (ms *MockSource) Unpack(_ context.Context, _ *catalogdv1alpha1.Catalog) (*source.Result, error) { if ms.shouldError { return nil, errors.New("mocksource error") } @@ -48,7 +40,7 @@ func (ms *MockSource) Unpack(_ context.Context, _ *v1alpha1.Catalog) (*source.Re return ms.result, nil } -func (ms *MockSource) Cleanup(_ context.Context, _ *v1alpha1.Catalog) error { +func (ms *MockSource) Cleanup(_ context.Context, _ *catalogdv1alpha1.Catalog) error { if ms.shouldError { return errors.New("mocksource error") } @@ -84,349 +76,472 @@ func (m MockStore) StorageServerHandler() http.Handler { panic("not needed") } -var _ = Describe("Catalogd Controller Test", func() { - format.MaxLength = 0 - var ( - ctx context.Context - reconciler *core.CatalogReconciler - mockSource *MockSource - mockStore *MockStore - ) - BeforeEach(func() { - ctx = context.Background() - mockSource = &MockSource{} - mockStore = &MockStore{} - reconciler = &core.CatalogReconciler{ - Client: cl, - Unpacker: source.NewUnpacker( - map[v1alpha1.SourceType]source.Unpacker{ - v1alpha1.SourceTypeImage: mockSource, - }, - ), - Storage: mockStore, - } - }) - When("the catalog does not exist", func() { - It("returns no error", func() { - res, err := reconciler.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: "non-existent"}}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).NotTo(HaveOccurred()) - }) - }) - - When("setting up with controller manager", func() { - var mgr ctrl.Manager - BeforeEach(func() { - var err error - mgr, err = ctrl.NewManager(cfg, manager.Options{Scheme: sch}) - Expect(mgr).ToNot(BeNil()) - Expect(err).ToNot(HaveOccurred()) - }) - It("returns no error", func() { - Expect(reconciler.SetupWithManager(mgr)).To(Succeed()) - }) - }) - - When("the catalog exists", func() { - var ( - catalog *v1alpha1.Catalog - catalogKey types.NamespacedName - ) - BeforeEach(func() { - catalogKey = types.NamespacedName{Name: fmt.Sprintf("catalogd-test-%s", rand.String(8))} - }) - - When("the catalog specifies an invalid source", func() { - BeforeEach(func() { - By("initializing cluster state") - catalog = &v1alpha1.Catalog{ - ObjectMeta: metav1.ObjectMeta{Name: catalogKey.Name}, - Spec: v1alpha1.CatalogSpec{ - Source: v1alpha1.CatalogSource{ - Type: "invalid-source", +func TestCatalogdControllerReconcile(t *testing.T) { + for _, tt := range []struct { + name string + catalog *catalogdv1alpha1.Catalog + shouldErr bool + expectedCatalog *catalogdv1alpha1.Catalog + source source.Unpacker + store storage.Instance + }{ + { + name: "invalid source type, returns error", + source: &MockSource{}, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "invalid", + }, + }, + }, + shouldErr: true, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "invalid", + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Phase: catalogdv1alpha1.PhaseFailing, + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonUnpackFailed, }, }, - } - Expect(cl.Create(ctx, catalog)).To(Succeed()) - }) - - AfterEach(func() { - By("tearing down cluster state") - Expect(cl.Delete(ctx, catalog)).To(Succeed()) - }) - - It("should set unpacking status to failed and return an error", func() { - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).To(HaveOccurred()) - - // get the catalog and ensure status is set properly - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - Expect(cat.Status.ResolvedSource).To(BeNil()) - Expect(cat.Status.Phase).To(Equal(v1alpha1.PhaseFailing)) - cond := meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeUnpacked) - Expect(cond).ToNot(BeNil()) - Expect(cond.Reason).To(Equal(v1alpha1.ReasonUnpackFailed)) - Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - }) - }) - - When("the catalog specifies a valid source", func() { - BeforeEach(func() { - By("initializing cluster state") - catalog = &v1alpha1.Catalog{ - ObjectMeta: metav1.ObjectMeta{Name: catalogKey.Name}, - Spec: v1alpha1.CatalogSpec{ - Source: v1alpha1.CatalogSource{ - Type: "image", - Image: &v1alpha1.ImageSource{ - Ref: "somecatalog:latest", - }, + }, + }, + }, + { + name: "valid source type, unpack state == Pending, unpack state is reflected in status", + source: &MockSource{ + result: &source.Result{State: source.StatePending}, + }, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", }, }, - } - Expect(cl.Create(ctx, catalog)).To(Succeed()) - }) - - AfterEach(func() { - By("tearing down cluster state") - Expect(client.IgnoreNotFound(cl.Delete(ctx, catalog))).To(Succeed()) - }) - - When("unpacker returns source.Result with state == 'Pending'", func() { - BeforeEach(func() { - mockSource.shouldError = false - mockSource.result = &source.Result{State: source.StatePending} - }) - - It("should update status to reflect the pending state", func() { - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - - // get the catalog and ensure status is set properly - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - Expect(cat.Status.ResolvedSource).To(BeNil()) - Expect(cat.Status.Phase).To(Equal(v1alpha1.PhasePending)) - cond := meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeUnpacked) - Expect(cond).ToNot(BeNil()) - Expect(cond.Reason).To(Equal(v1alpha1.ReasonUnpackPending)) - Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - }) - }) - - When("unpacker returns source.Result with state == 'Unpacking'", func() { - BeforeEach(func() { - mockSource.shouldError = false - mockSource.result = &source.Result{State: source.StateUnpacking} - }) - - It("should update status to reflect the unpacking state", func() { - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - - // get the catalog and ensure status is set properly - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - Expect(cat.Status.ResolvedSource).To(BeNil()) - Expect(cat.Status.Phase).To(Equal(v1alpha1.PhaseUnpacking)) - cond := meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeUnpacked) - Expect(cond).ToNot(BeNil()) - Expect(cond.Reason).To(Equal(v1alpha1.ReasonUnpacking)) - Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - }) - }) - - When("unpacker returns source.Result with unknown state", func() { - BeforeEach(func() { - mockSource.shouldError = false - mockSource.result = &source.Result{State: "unknown"} - }) - - It("should set unpacking status to failed and return an error", func() { - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).To(HaveOccurred()) - - // get the catalog and ensure status is set properly - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - Expect(cat.Status.ResolvedSource).To(BeNil()) - Expect(cat.Status.Phase).To(Equal(v1alpha1.PhaseFailing)) - cond := meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeUnpacked) - Expect(cond).ToNot(BeNil()) - Expect(cond.Reason).To(Equal(v1alpha1.ReasonUnpackFailed)) - Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - }) - }) - - When("unpacker returns source.Result with state == 'Unpacked'", func() { - var ( - testBundleName = "webhook_operator.v0.0.1" - testBundleImage = "quay.io/olmtest/webhook-operator-bundle:0.0.3" - testBundleRelatedImageName = "test" - testBundleRelatedImageImage = "testimage:latest" - testBundleObjectData = "dW5pbXBvcnRhbnQK" - testPackageDefaultChannel = "preview_test" - testPackageName = "webhook_operator_test" - testChannelName = "preview_test" - testPackage = fmt.Sprintf(testPackageTemplate, testPackageDefaultChannel, testPackageName) - testBundle = fmt.Sprintf(testBundleTemplate, testBundleImage, testBundleName, testPackageName, testBundleRelatedImageName, testBundleRelatedImageImage, testBundleObjectData) - testChannel = fmt.Sprintf(testChannelTemplate, testPackageName, testChannelName, testBundleName) - ) - BeforeEach(func() { - - filesys := &fstest.MapFS{ - "bundle.yaml": &fstest.MapFile{Data: []byte(testBundle), Mode: os.ModePerm}, - "package.yaml": &fstest.MapFile{Data: []byte(testPackage), Mode: os.ModePerm}, - "channel.yaml": &fstest.MapFile{Data: []byte(testChannel), Mode: os.ModePerm}, - } - - mockSource.shouldError = false - mockSource.result = &source.Result{ - ResolvedSource: &catalog.Spec.Source, - State: source.StateUnpacked, - FS: filesys, - } - }) - It("should set unpacking status to 'unpacked'", func() { - // reconcile - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - - // get the catalog and ensure status is set properly - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - Expect(cat.Status.ResolvedSource).ToNot(BeNil()) - Expect(cat.Status.Phase).To(Equal(v1alpha1.PhaseUnpacked)) - cond := meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeUnpacked) - Expect(cond).ToNot(BeNil()) - Expect(cond.Reason).To(Equal(v1alpha1.ReasonUnpackSuccessful)) - Expect(cond.Status).To(Equal(metav1.ConditionTrue)) - }) - - When("HTTPServer feature gate is enabled", func() { - BeforeEach(func() { - Expect(features.CatalogdFeatureGate.SetFromMap(map[string]bool{ - string(features.HTTPServer): true, - })).NotTo(HaveOccurred()) - // call reconciler so that initial finalizer setup is done here - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - }) - When("there is no error in storing the fbc", func() { - BeforeEach(func() { - By("setting up mockStore to return no error", func() { - mockStore.shouldError = false - }) - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - }) - It("should reflect in the status condition", func() { - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - Expect(cat.Status.Phase).To(Equal(v1alpha1.PhaseUnpacked)) - diff := cmp.Diff(meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeUnpacked), &metav1.Condition{ - Reason: v1alpha1.ReasonUnpackSuccessful, - Status: metav1.ConditionTrue, - }, cmpopts.IgnoreFields(metav1.Condition{}, "Type", "ObservedGeneration", "LastTransitionTime", "Message")) - Expect(diff).To(Equal("")) - }) - - When("the catalog is deleted but there is an error deleting the stored FBC", func() { - BeforeEach(func() { - By("setting up mockStore to return an error", func() { - mockStore.shouldError = true - }) - Expect(cl.Delete(ctx, catalog)).To(Succeed()) - // call reconciler so that MockStore can send an error on deletion attempt - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).To(HaveOccurred()) - }) - It("should set status condition to reflect the error", func() { - // get the catalog and ensure status is set properly - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - cond := meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeDelete) - Expect(cond).To(Not(BeNil())) - Expect(cond.Reason).To(Equal(v1alpha1.ReasonStorageDeleteFailed)) - Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - }) - }) - }) - - When("there is an error storing the fbc", func() { - BeforeEach(func() { - By("setting up mockStore to return an error", func() { - mockStore.shouldError = true - }) - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).To(HaveOccurred()) - }) - It("should set status condition to reflect that storage error", func() { - cat := &v1alpha1.Catalog{} - Expect(cl.Get(ctx, catalogKey, cat)).To(Succeed()) - Expect(cat.Status.ResolvedSource).To(BeNil()) - Expect(cat.Status.Phase).To(Equal(v1alpha1.PhaseFailing)) - diff := cmp.Diff(meta.FindStatusCondition(cat.Status.Conditions, v1alpha1.TypeUnpacked), &metav1.Condition{ - Reason: v1alpha1.ReasonStorageFailed, - Status: metav1.ConditionFalse, - }, cmpopts.IgnoreFields(metav1.Condition{}, "Type", "ObservedGeneration", "LastTransitionTime", "Message")) - Expect(diff).To(Equal("")) - }) - }) - }) - }) + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Phase: catalogdv1alpha1.PhasePending, + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonUnpackPending, + }, + }, + }, + }, + }, + { + name: "valid source type, unpack state == Unpacking, unpack state is reflected in status", + source: &MockSource{ + result: &source.Result{State: source.StateUnpacking}, + }, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Phase: catalogdv1alpha1.PhaseUnpacking, + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonUnpacking, + }, + }, + }, + }, + }, + { + name: "valid source type, unpack state is unknown, unpack state is reflected in status and error is returned", + shouldErr: true, + source: &MockSource{ + result: &source.Result{State: "unknown"}, + }, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Phase: catalogdv1alpha1.PhaseFailing, + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonUnpackFailed, + }, + }, + }, + }, + }, + { + name: "valid source type, unpack returns error, status updated to reflect error state and error is returned", + shouldErr: true, + source: &MockSource{ + shouldError: true, + }, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Phase: catalogdv1alpha1.PhaseFailing, + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonUnpackFailed, + }, + }, + }, + }, + }, + { + name: "valid source type, unpack state == Unpacked, unpack state is reflected in status", + source: &MockSource{ + result: &source.Result{ + State: source.StateUnpacked, + FS: &fstest.MapFS{}, + }, + }, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Phase: catalogdv1alpha1.PhaseUnpacked, + ContentURL: "URL", + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionTrue, + Reason: catalogdv1alpha1.ReasonUnpackSuccessful, + }, + }, + }, + }, + }, + { + name: "valid source type, unpack state == Unpacked, storage fails, failure reflected in status and error returned", + shouldErr: true, + source: &MockSource{ + result: &source.Result{ + State: source.StateUnpacked, + FS: &fstest.MapFS{}, + }, + }, + store: &MockStore{ + shouldError: true, + }, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Phase: catalogdv1alpha1.PhaseFailing, + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonStorageFailed, + }, + }, + }, + }, + }, + { + name: "storage finalizer not set, storage finalizer gets set", + source: &MockSource{}, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + }, + { + name: "storage finalizer set, catalog deletion timestamp is not zero (or nil), finalizer removed", + source: &MockSource{}, + store: &MockStore{}, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{}, + DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + }, + { + name: "storage finalizer set, catalog deletion timestamp is not zero (or nil), storage delete failed, error returned and finalizer not removed", + shouldErr: true, + source: &MockSource{}, + store: &MockStore{ + shouldError: true, + }, + catalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + }, + expectedCatalog: &catalogdv1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{fbcDeletionFinalizer}, + DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, + }, + Spec: catalogdv1alpha1.CatalogSpec{ + Source: catalogdv1alpha1.CatalogSource{ + Type: "image", + Image: &catalogdv1alpha1.ImageSource{ + Ref: "someimage:latest", + }, + }, + }, + Status: catalogdv1alpha1.CatalogStatus{ + Conditions: []metav1.Condition{ + { + Type: catalogdv1alpha1.TypeDelete, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonStorageDeleteFailed, + }, + }, + }, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + reconciler := &CatalogReconciler{ + Client: nil, + Unpacker: source.NewUnpacker( + map[catalogdv1alpha1.SourceType]source.Unpacker{ + catalogdv1alpha1.SourceTypeImage: tt.source, + }, + ), + Storage: tt.store, + } + ctx := context.Background() + res, err := reconciler.reconcile(ctx, tt.catalog) + assert.Equal(t, ctrl.Result{}, res) + + if !tt.shouldErr { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + + diff := cmp.Diff(tt.expectedCatalog, tt.catalog, cmpopts.IgnoreFields(metav1.Condition{}, "Message", "LastTransitionTime")) + assert.Empty(t, diff, "comparing the expected Catalog") }) - }) -}) - -// The below string templates each represent a YAML file consisting -// of file-based catalog objects to build a minimal catalog consisting of -// one package, with one channel, and one bundle in that channel. -// To learn more about File-Based Catalogs and the different objects, view the -// documentation at https://olm.operatorframework.io/docs/reference/file-based-catalogs/. -// The reasoning behind having these as a template is to parameterize different -// fields to use custom values during testing and verifying to ensure that the catalog -// metadata served after the Catalog resource has been reconciled have the appropriate values. -// Having the parameterized fields allows us to easily change the values that are used in -// the tests by changing them in one place as opposed to manually changing many string literals -// throughout the code. -const testBundleTemplate = `--- -image: %s -name: %s -schema: olm.bundle -package: %s -relatedImages: - - name: %s - image: %s -properties: - - type: olm.bundle.object - value: - data: %s - - type: some.other - value: - data: arbitrary-info -` - -const testPackageTemplate = `--- -defaultChannel: %s -name: %s -schema: olm.package -` - -const testChannelTemplate = `--- -schema: olm.channel -package: %s -name: %s -entries: - - name: %s -` + } +} diff --git a/pkg/controllers/core/suite_test.go b/pkg/controllers/core/suite_test.go deleted file mode 100644 index cf2146fc..00000000 --- a/pkg/controllers/core/suite_test.go +++ /dev/null @@ -1,79 +0,0 @@ -/* -Copyright 2023. - -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 core_test - -import ( - "path/filepath" - "testing" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/rest" - "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" - - catalogdv1alpha1 "github.com/operator-framework/catalogd/api/core/v1alpha1" -) - -// These tests use Ginkgo (BDD-style Go testing framework). Refer to -// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. - -var ( - cfg *rest.Config - cl client.Client - sch *runtime.Scheme - testEnv *envtest.Environment -) - -func TestAPIs(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Controller Suite") -} - -var _ = BeforeSuite(func() { - logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) - - By("bootstrapping test environment") - testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{ - filepath.Join("..", "..", "..", "config", "crd", "bases")}, - ErrorIfCRDPathMissing: true, - } - - var err error - // cfg is defined in this file globally. - cfg, err = testEnv.Start() - Expect(err).NotTo(HaveOccurred()) - Expect(cfg).NotTo(BeNil()) - - sch = runtime.NewScheme() - Expect(catalogdv1alpha1.AddToScheme(sch)).To(Succeed()) - Expect(corev1.AddToScheme(sch)).To(Succeed()) - - cl, err = client.New(cfg, client.Options{Scheme: sch}) - Expect(err).NotTo(HaveOccurred()) - Expect(cl).NotTo(BeNil()) -}) - -var _ = AfterSuite(func() { - By("tearing down the test environment") - Expect(testEnv.Stop()).To(Succeed()) -}) diff --git a/pkg/features/features.go b/pkg/features/features.go index 2e20f5ac..8f67b168 100644 --- a/pkg/features/features.go +++ b/pkg/features/features.go @@ -5,19 +5,7 @@ import ( "k8s.io/component-base/featuregate" ) -const ( - // Add new feature gates constants (strings) - // Ex: SomeFeature featuregate.Feature = "SomeFeature" - - HTTPServer featuregate.Feature = "HTTPServer" -) - -var catalogdFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - // Add new feature gate definitions - // Ex: SomeFeature: {...} - - HTTPServer: {Default: false, PreRelease: featuregate.Alpha}, -} +var catalogdFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{} var CatalogdFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate()