From eaca4f4de8d1829adc546a60311187459a2d2604 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Thu, 13 Jul 2023 09:14:10 -0400 Subject: [PATCH 1/7] add label selector to sync list queries to prevent overwriting of resources not belonging to the Catalog currently being reconciled Signed-off-by: Bryce Palmer --- pkg/controllers/core/catalog_controller.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/controllers/core/catalog_controller.go b/pkg/controllers/core/catalog_controller.go index 2958bf7f..4a715f3f 100644 --- a/pkg/controllers/core/catalog_controller.go +++ b/pkg/controllers/core/catalog_controller.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" apimacherrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/pointer" @@ -272,8 +273,9 @@ func (r *CatalogReconciler) syncBundleMetadata(ctx context.Context, declCfg *dec newBundles[bundleName] = &bundleMeta } + listOpts := &client.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{"catalog": catalog.Name})} var existingBundles v1alpha1.BundleMetadataList - if err := r.List(ctx, &existingBundles); err != nil { + if err := r.List(ctx, &existingBundles, listOpts); err != nil { return fmt.Errorf("list existing bundle metadatas: %v", err) } for i := range existingBundles.Items { @@ -359,8 +361,9 @@ func (r *CatalogReconciler) syncPackages(ctx context.Context, declCfg *declcfg.D pkg.Spec.Channels = append(pkg.Spec.Channels, pkgChannel) } + listOpts := &client.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{"catalog": catalog.Name})} var existingPkgs v1alpha1.PackageList - if err := r.List(ctx, &existingPkgs); err != nil { + if err := r.List(ctx, &existingPkgs, listOpts); err != nil { return fmt.Errorf("list existing packages: %v", err) } for i := range existingPkgs.Items { @@ -444,8 +447,9 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS, return fmt.Errorf("unable to parse declarative config into CatalogMetadata API: %w", err) } + listOpts := &client.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{"catalog": catalog.Name})} var existingCatalogMetadataObjs v1alpha1.CatalogMetadataList - if err := r.List(ctx, &existingCatalogMetadataObjs); err != nil { + if err := r.List(ctx, &existingCatalogMetadataObjs, listOpts); err != nil { return fmt.Errorf("list existing catalog metadata: %v", err) } for i, existingCatalogMetadata := range existingCatalogMetadataObjs.Items { From 617174aeca59b11d221d338ef58ca8263e5846cf Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Thu, 13 Jul 2023 17:02:18 -0400 Subject: [PATCH 2/7] add unit test and update them slightly as it seemed like the feature gates were not appropriately enabled resulting in tests not running Signed-off-by: Bryce Palmer --- .../core/catalog_controller_test.go | 179 +++++++++++++++--- 1 file changed, 156 insertions(+), 23 deletions(-) diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index 94ca3b6f..9cb3b7dc 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -9,11 +9,14 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" 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" @@ -42,6 +45,7 @@ func (ms *MockSource) Unpack(_ context.Context, _ *v1alpha1.Catalog) (*source.Re } var _ = Describe("Catalogd Controller Test", func() { + format.MaxLength = 0 var ( ctx context.Context reconciler *core.CatalogReconciler @@ -49,8 +53,6 @@ var _ = Describe("Catalogd Controller Test", func() { ) BeforeEach(func() { ctx = context.Background() - Expect(features.CatalogdFeatureGate.Set("PackagesBundleMetadataAPIs=true")).To(Succeed()) - Expect(features.CatalogdFeatureGate.Set("CatalogMetadataAPI=true")).To(Succeed()) mockSource = &MockSource{} reconciler = &core.CatalogReconciler{ Client: cl, @@ -252,15 +254,39 @@ var _ = Describe("Catalogd Controller Test", func() { 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)) }) - AfterEach(func() { - if features.CatalogdFeatureGate.Enabled(features.PackagesBundleMetadataAPIs) { + When("PackagesBundleMetadataAPIs feature gate is enabled", func() { + BeforeEach(func() { + Expect(features.CatalogdFeatureGate.SetFromMap(map[string]bool{ + string(features.PackagesBundleMetadataAPIs): true, + string(features.CatalogMetadataAPI): false, + })).NotTo(HaveOccurred()) + + // reconcile + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) + Expect(res).To(Equal(ctrl.Result{})) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { // clean up package pkg := &v1alpha1.Package{ ObjectMeta: metav1.ObjectMeta{ @@ -276,23 +302,12 @@ var _ = Describe("Catalogd Controller Test", func() { }, } Expect(cl.Delete(ctx, bm)).To(Succeed()) - } - }) - - It("should set unpacking status to 'unpacked'", func() { - // 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)) - }) + Expect(features.CatalogdFeatureGate.SetFromMap(map[string]bool{ + string(features.PackagesBundleMetadataAPIs): false, + string(features.CatalogMetadataAPI): false, + })).NotTo(HaveOccurred()) + }) - if features.CatalogdFeatureGate.Enabled(features.PackagesBundleMetadataAPIs) { - // TODO (rashmigottipati): Add testing of BundleMetadata sync process. It("should create BundleMetadata resources", func() { // validate bundlemetadata resources bundlemetadatas := &v1alpha1.BundleMetadataList{} @@ -324,9 +339,101 @@ var _ = Describe("Catalogd Controller Test", func() { Expect(pack.Spec.Channels[0].Entries).To(HaveLen(1)) Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(testBundleName)) }) - } - if features.CatalogdFeatureGate.Enabled(features.CatalogMetadataAPI) { + It("should not delete BundleMetadata belonging to a different catalog", func() { + bm := &v1alpha1.BundleMetadata{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foobar", + Labels: map[string]string{ + "catalog": "notyours", + }, + }, + Spec: v1alpha1.BundleMetadataSpec{ + Catalog: v1.LocalObjectReference{Name: "foobar"}, + Package: "barfoo", + Image: "notreal:latest", + Properties: []v1alpha1.Property{}, + RelatedImages: []v1alpha1.RelatedImage{}, + }, + } + + Expect(cl.Create(ctx, bm)).NotTo(HaveOccurred()) + + // reconcile again to ensure the bundlemetadata still exists + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) + Expect(res).To(Equal(ctrl.Result{})) + Expect(err).ToNot(HaveOccurred()) + + expectedBm := &v1alpha1.BundleMetadata{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(bm), expectedBm)).NotTo(HaveOccurred()) + Expect(expectedBm).Should(BeEquivalentTo(bm)) + + // clean up + Expect(cl.Delete(ctx, bm)).NotTo(HaveOccurred()) + }) + + It("should not delete Packages belonging to a different catalog", func() { + pack := &v1alpha1.Package{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foobar", + Labels: map[string]string{ + "catalog": "notyours", + }, + }, + Spec: v1alpha1.PackageSpec{ + Catalog: v1.LocalObjectReference{Name: "foobar"}, + Name: "barfoo", + Description: "", + DefaultChannel: "alpha", + Channels: []v1alpha1.PackageChannel{ + { + Name: "alpha", + Entries: []v1alpha1.ChannelEntry{ + { + Name: "barfoo.v1.0.0", + }, + }, + }, + }, + }, + } + + Expect(cl.Create(ctx, pack)).NotTo(HaveOccurred()) + + // reconcile again to ensure the bundlemetadata still exists + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) + Expect(res).To(Equal(ctrl.Result{})) + Expect(err).ToNot(HaveOccurred()) + + expectedPack := &v1alpha1.Package{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(pack), expectedPack)).NotTo(HaveOccurred()) + Expect(expectedPack).Should(BeEquivalentTo(pack)) + + // clean up + Expect(cl.Delete(ctx, pack)).NotTo(HaveOccurred()) + }) + }) + + When("the CatalogMetadataAPI feature gate is enabled", func() { + BeforeEach(func() { + Expect(features.CatalogdFeatureGate.SetFromMap(map[string]bool{ + string(features.CatalogMetadataAPI): true, + })).NotTo(HaveOccurred()) + + // reconcile + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) + Expect(res).To(Equal(ctrl.Result{})) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + // clean up catalogmetadata + Expect(cl.DeleteAllOf(ctx, &v1alpha1.CatalogMetadata{})).To(Succeed()) + Expect(features.CatalogdFeatureGate.SetFromMap(map[string]bool{ + string(features.CatalogMetadataAPI): false, + })).NotTo(HaveOccurred()) + }) + // TODO (rashmigottipati): Add testing of CatalogMetadata sync process. It("should create CatalogMetadata resources", func() { catalogMetadatas := &v1alpha1.CatalogMetadataList{} @@ -341,8 +448,34 @@ var _ = Describe("Catalogd Controller Test", func() { Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(catalogKey.Name)) } }) - } + It("should not delete CatalogMetadata belonging to a different catalog", func() { + cm := &v1alpha1.CatalogMetadata{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foobar", + Labels: map[string]string{ + "catalog": "notyours", + }, + }, + Spec: v1alpha1.CatalogMetadataSpec{ + Catalog: v1.LocalObjectReference{Name: "foobar"}, + Name: "barfoo", + Schema: "catalogd.test", + }, + } + + Expect(cl.Create(ctx, cm)).NotTo(HaveOccurred()) + + // reconcile again to ensure the bundlemetadata still exists + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) + Expect(res).To(Equal(ctrl.Result{})) + Expect(err).ToNot(HaveOccurred()) + + expectedCm := &v1alpha1.CatalogMetadata{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), expectedCm)).NotTo(HaveOccurred()) + Expect(expectedCm).Should(BeEquivalentTo(cm)) + }) + }) }) }) From d0be28792a054f80e15187115320c4fe03a8afcf Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 14 Jul 2023 09:55:52 -0400 Subject: [PATCH 3/7] fix lint issues Signed-off-by: Bryce Palmer --- pkg/controllers/core/catalog_controller_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index 9cb3b7dc..2c8b58e6 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -10,7 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/format" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -349,7 +349,7 @@ var _ = Describe("Catalogd Controller Test", func() { }, }, Spec: v1alpha1.BundleMetadataSpec{ - Catalog: v1.LocalObjectReference{Name: "foobar"}, + Catalog: corev1.LocalObjectReference{Name: "foobar"}, Package: "barfoo", Image: "notreal:latest", Properties: []v1alpha1.Property{}, @@ -381,7 +381,7 @@ var _ = Describe("Catalogd Controller Test", func() { }, }, Spec: v1alpha1.PackageSpec{ - Catalog: v1.LocalObjectReference{Name: "foobar"}, + Catalog: corev1.LocalObjectReference{Name: "foobar"}, Name: "barfoo", Description: "", DefaultChannel: "alpha", @@ -458,7 +458,7 @@ var _ = Describe("Catalogd Controller Test", func() { }, }, Spec: v1alpha1.CatalogMetadataSpec{ - Catalog: v1.LocalObjectReference{Name: "foobar"}, + Catalog: corev1.LocalObjectReference{Name: "foobar"}, Name: "barfoo", Schema: "catalogd.test", }, From 0434e04dea15f573a1bb8062ebec2053856354d1 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 14 Jul 2023 10:52:11 -0400 Subject: [PATCH 4/7] harden the sync test case by using multiple catalogs instead of creating an indivdual resource. Signed-off-by: Bryce Palmer --- pkg/controllers/core/catalog_controller.go | 10 +- .../core/catalog_controller_test.go | 289 ++++++++++++------ 2 files changed, 198 insertions(+), 101 deletions(-) diff --git a/pkg/controllers/core/catalog_controller.go b/pkg/controllers/core/catalog_controller.go index 4a715f3f..c3a873e9 100644 --- a/pkg/controllers/core/catalog_controller.go +++ b/pkg/controllers/core/catalog_controller.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" apimacherrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/pointer" @@ -273,9 +272,8 @@ func (r *CatalogReconciler) syncBundleMetadata(ctx context.Context, declCfg *dec newBundles[bundleName] = &bundleMeta } - listOpts := &client.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{"catalog": catalog.Name})} var existingBundles v1alpha1.BundleMetadataList - if err := r.List(ctx, &existingBundles, listOpts); err != nil { + if err := r.List(ctx, &existingBundles, &client.MatchingLabels{"catalog": catalog.Name}); err != nil { return fmt.Errorf("list existing bundle metadatas: %v", err) } for i := range existingBundles.Items { @@ -361,9 +359,8 @@ func (r *CatalogReconciler) syncPackages(ctx context.Context, declCfg *declcfg.D pkg.Spec.Channels = append(pkg.Spec.Channels, pkgChannel) } - listOpts := &client.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{"catalog": catalog.Name})} var existingPkgs v1alpha1.PackageList - if err := r.List(ctx, &existingPkgs, listOpts); err != nil { + if err := r.List(ctx, &existingPkgs, &client.MatchingLabels{"catalog": catalog.Name}); err != nil { return fmt.Errorf("list existing packages: %v", err) } for i := range existingPkgs.Items { @@ -447,9 +444,8 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS, return fmt.Errorf("unable to parse declarative config into CatalogMetadata API: %w", err) } - listOpts := &client.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{"catalog": catalog.Name})} var existingCatalogMetadataObjs v1alpha1.CatalogMetadataList - if err := r.List(ctx, &existingCatalogMetadataObjs, listOpts); err != nil { + if err := r.List(ctx, &existingCatalogMetadataObjs, &client.MatchingLabels{"catalog": catalog.Name}); err != nil { return fmt.Errorf("list existing catalog metadata: %v", err) } for i, existingCatalogMetadata := range existingCatalogMetadataObjs.Items { diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index 2c8b58e6..bd1c7402 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -10,7 +10,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/format" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -340,77 +339,119 @@ var _ = Describe("Catalogd Controller Test", func() { Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(testBundleName)) }) - It("should not delete BundleMetadata belonging to a different catalog", func() { - bm := &v1alpha1.BundleMetadata{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foobar", - Labels: map[string]string{ - "catalog": "notyours", - }, - }, - Spec: v1alpha1.BundleMetadataSpec{ - Catalog: corev1.LocalObjectReference{Name: "foobar"}, - Package: "barfoo", - Image: "notreal:latest", - Properties: []v1alpha1.Property{}, - RelatedImages: []v1alpha1.RelatedImage{}, - }, - } - - Expect(cl.Create(ctx, bm)).NotTo(HaveOccurred()) - - // reconcile again to ensure the bundlemetadata still exists - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - - expectedBm := &v1alpha1.BundleMetadata{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(bm), expectedBm)).NotTo(HaveOccurred()) - Expect(expectedBm).Should(BeEquivalentTo(bm)) - - // clean up - Expect(cl.Delete(ctx, bm)).NotTo(HaveOccurred()) - }) - - It("should not delete Packages belonging to a different catalog", func() { - pack := &v1alpha1.Package{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foobar", - Labels: map[string]string{ - "catalog": "notyours", - }, - }, - Spec: v1alpha1.PackageSpec{ - Catalog: corev1.LocalObjectReference{Name: "foobar"}, - Name: "barfoo", - Description: "", - DefaultChannel: "alpha", - Channels: []v1alpha1.PackageChannel{ - { - Name: "alpha", - Entries: []v1alpha1.ChannelEntry{ - { - Name: "barfoo.v1.0.0", - }, + When("creating another Catalog", func() { + var ( + tempTestBundleName = "temp-webhook-operator.v0.0.1" + tempTestBundleImage = "quay.io/olmtest/temp-webhook-operator-bundle:0.0.3" + tempTestBundleRelatedImageName = "temp-test" + tempTestBundleRelatedImageImage = "temp-testimage:latest" + tempTestBundleObjectData = "dW5pbXBvcnRhbnQK" + tempTestPackageDefaultChannel = "temp-preview" + tempTestPackageName = "temp-webhook-operator" + tempTestChannelName = "temp-preview" + tempTestPackage = fmt.Sprintf(testPackageTemplate, tempTestPackageDefaultChannel, tempTestPackageName) + tempTestBundle = fmt.Sprintf(testBundleTemplate, tempTestBundleImage, tempTestBundleName, tempTestPackageName, tempTestBundleRelatedImageName, tempTestBundleRelatedImageImage, tempTestBundleObjectData) + tempTestChannel = fmt.Sprintf(testChannelTemplate, tempTestPackageName, tempTestChannelName, tempTestBundleName) + tempCatalog *v1alpha1.Catalog + tempMs *MockSource + tempRec *core.CatalogReconciler + tempTestBundleMetaName string + tempTestPackageMetaName string + ) + BeforeEach(func() { + tempCatalog = &v1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{Name: "tempedout"}, + Spec: v1alpha1.CatalogSpec{ + Source: v1alpha1.CatalogSource{ + Type: "image", + Image: &v1alpha1.ImageSource{ + Ref: "somecatalog:latest", }, }, }, - }, - } - - Expect(cl.Create(ctx, pack)).NotTo(HaveOccurred()) - - // reconcile again to ensure the bundlemetadata still exists - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - - expectedPack := &v1alpha1.Package{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(pack), expectedPack)).NotTo(HaveOccurred()) - Expect(expectedPack).Should(BeEquivalentTo(pack)) - - // clean up - Expect(cl.Delete(ctx, pack)).NotTo(HaveOccurred()) + } + + tempTestBundleMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, tempTestBundleName) + tempTestPackageMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, tempTestPackageName) + + tempMs = &MockSource{} + filesys := &fstest.MapFS{ + "bundle.yaml": &fstest.MapFile{Data: []byte(tempTestBundle), Mode: os.ModePerm}, + "package.yaml": &fstest.MapFile{Data: []byte(tempTestPackage), Mode: os.ModePerm}, + "channel.yaml": &fstest.MapFile{Data: []byte(tempTestChannel), Mode: os.ModePerm}, + } + + tempMs.shouldError = false + tempMs.result = &source.Result{ + ResolvedSource: &catalog.Spec.Source, + State: source.StateUnpacked, + FS: filesys, + } + + tempRec = &core.CatalogReconciler{ + Client: cl, + Unpacker: source.NewUnpacker( + map[v1alpha1.SourceType]source.Unpacker{ + v1alpha1.SourceTypeImage: tempMs, + }, + ), + } + Expect(cl.Create(ctx, tempCatalog)).To(Succeed()) + res, err := tempRec.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "tempedout"}}) + Expect(res).To(Equal(ctrl.Result{})) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + Expect(cl.Delete(ctx, tempCatalog)).NotTo(HaveOccurred()) + }) + + It("should not delete BundleMetadata belonging to a different catalog", func() { + bundlemetadata := &v1alpha1.BundleMetadata{} + Expect(cl.Get(ctx, client.ObjectKey{Name: testBundleMetaName}, bundlemetadata)).To(Succeed()) + Expect(bundlemetadata.Name).To(Equal(testBundleMetaName)) + Expect(bundlemetadata.Spec.Image).To(Equal(testBundleImage)) + Expect(bundlemetadata.Spec.Catalog.Name).To(Equal(catalog.Name)) + Expect(bundlemetadata.Spec.Package).To(Equal(testPackageName)) + Expect(bundlemetadata.Spec.RelatedImages).To(HaveLen(1)) + Expect(bundlemetadata.Spec.RelatedImages[0].Name).To(Equal(testBundleRelatedImageName)) + Expect(bundlemetadata.Spec.RelatedImages[0].Image).To(Equal(testBundleRelatedImageImage)) + Expect(bundlemetadata.Spec.Properties).To(HaveLen(1)) + + bundlemetadata = &v1alpha1.BundleMetadata{} + Expect(cl.Get(ctx, client.ObjectKey{Name: tempTestBundleMetaName}, bundlemetadata)).To(Succeed()) + Expect(bundlemetadata.Name).To(Equal(tempTestBundleMetaName)) + Expect(bundlemetadata.Spec.Image).To(Equal(tempTestBundleImage)) + Expect(bundlemetadata.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) + Expect(bundlemetadata.Spec.Package).To(Equal(tempTestPackageName)) + Expect(bundlemetadata.Spec.RelatedImages).To(HaveLen(1)) + Expect(bundlemetadata.Spec.RelatedImages[0].Name).To(Equal(tempTestBundleRelatedImageName)) + Expect(bundlemetadata.Spec.RelatedImages[0].Image).To(Equal(tempTestBundleRelatedImageImage)) + Expect(bundlemetadata.Spec.Properties).To(HaveLen(1)) + }) + + It("should not delete Packages belonging to a different catalog", func() { + // validate package resources + pack := &v1alpha1.Package{} + Expect(cl.Get(ctx, client.ObjectKey{Name: testPackageMetaName}, pack)).To(Succeed()) + Expect(pack.Name).To(Equal(testPackageMetaName)) + Expect(pack.Spec.DefaultChannel).To(Equal(testPackageDefaultChannel)) + Expect(pack.Spec.Catalog.Name).To(Equal(catalog.Name)) + Expect(pack.Spec.Channels).To(HaveLen(1)) + Expect(pack.Spec.Channels[0].Name).To(Equal(testChannelName)) + Expect(pack.Spec.Channels[0].Entries).To(HaveLen(1)) + Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(testBundleName)) + + pack = &v1alpha1.Package{} + Expect(cl.Get(ctx, client.ObjectKey{Name: tempTestPackageMetaName}, pack)).To(Succeed()) + Expect(pack.Name).To(Equal(tempTestPackageMetaName)) + Expect(pack.Spec.DefaultChannel).To(Equal(tempTestPackageDefaultChannel)) + Expect(pack.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) + Expect(pack.Spec.Channels).To(HaveLen(1)) + Expect(pack.Spec.Channels[0].Name).To(Equal(tempTestChannelName)) + Expect(pack.Spec.Channels[0].Entries).To(HaveLen(1)) + Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(tempTestBundleName)) + }) }) }) @@ -449,36 +490,96 @@ var _ = Describe("Catalogd Controller Test", func() { } }) - It("should not delete CatalogMetadata belonging to a different catalog", func() { - cm := &v1alpha1.CatalogMetadata{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foobar", - Labels: map[string]string{ - "catalog": "notyours", + When("creating another Catalog", func() { + var ( + tempTestBundleName = "temp-webhook-operator.v0.0.1" + tempTestBundleImage = "quay.io/olmtest/temp-webhook-operator-bundle:0.0.3" + tempTestBundleRelatedImageName = "temp-test" + tempTestBundleRelatedImageImage = "temp-testimage:latest" + tempTestBundleObjectData = "dW5pbXBvcnRhbnQK" + tempTestPackageDefaultChannel = "temp-preview" + tempTestPackageName = "temp-webhook-operator" + tempTestChannelName = "temp-preview" + tempTestPackage = fmt.Sprintf(testPackageTemplate, tempTestPackageDefaultChannel, tempTestPackageName) + tempTestBundle = fmt.Sprintf(testBundleTemplate, tempTestBundleImage, tempTestBundleName, tempTestPackageName, tempTestBundleRelatedImageName, tempTestBundleRelatedImageImage, tempTestBundleObjectData) + tempTestChannel = fmt.Sprintf(testChannelTemplate, tempTestPackageName, tempTestChannelName, tempTestBundleName) + tempCatalog *v1alpha1.Catalog + tempMs *MockSource + tempRec *core.CatalogReconciler + ) + BeforeEach(func() { + tempCatalog = &v1alpha1.Catalog{ + ObjectMeta: metav1.ObjectMeta{Name: "tempedout"}, + Spec: v1alpha1.CatalogSpec{ + Source: v1alpha1.CatalogSource{ + Type: "image", + Image: &v1alpha1.ImageSource{ + Ref: "somecatalog:latest", + }, + }, }, - }, - Spec: v1alpha1.CatalogMetadataSpec{ - Catalog: corev1.LocalObjectReference{Name: "foobar"}, - Name: "barfoo", - Schema: "catalogd.test", - }, - } - - Expect(cl.Create(ctx, cm)).NotTo(HaveOccurred()) - - // reconcile again to ensure the bundlemetadata still exists - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) - Expect(res).To(Equal(ctrl.Result{})) - Expect(err).ToNot(HaveOccurred()) - - expectedCm := &v1alpha1.CatalogMetadata{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), expectedCm)).NotTo(HaveOccurred()) - Expect(expectedCm).Should(BeEquivalentTo(cm)) + } + + tempMs = &MockSource{} + filesys := &fstest.MapFS{ + "bundle.yaml": &fstest.MapFile{Data: []byte(tempTestBundle), Mode: os.ModePerm}, + "package.yaml": &fstest.MapFile{Data: []byte(tempTestPackage), Mode: os.ModePerm}, + "channel.yaml": &fstest.MapFile{Data: []byte(tempTestChannel), Mode: os.ModePerm}, + } + + tempMs.shouldError = false + tempMs.result = &source.Result{ + ResolvedSource: &catalog.Spec.Source, + State: source.StateUnpacked, + FS: filesys, + } + + tempRec = &core.CatalogReconciler{ + Client: cl, + Unpacker: source.NewUnpacker( + map[v1alpha1.SourceType]source.Unpacker{ + v1alpha1.SourceTypeImage: tempMs, + }, + ), + } + Expect(cl.Create(ctx, tempCatalog)).To(Succeed()) + res, err := tempRec.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "tempedout"}}) + Expect(res).To(Equal(ctrl.Result{})) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + Expect(cl.Delete(ctx, tempCatalog)).NotTo(HaveOccurred()) + }) + + It("should not delete CatalogMetadata belonging to a different catalog", func() { + catalogMetadatas := &v1alpha1.CatalogMetadataList{} + Expect(cl.List(ctx, catalogMetadatas)).To(Succeed()) + Expect(catalogMetadatas.Items).To(HaveLen(6)) + for _, catalogMetadata := range catalogMetadatas.Items { + for _, or := range catalogMetadata.GetOwnerReferences() { + if or.Kind == "Catalog" { + if or.Name == catalogKey.Name { + Expect(catalogMetadata.Name).To(ContainSubstring(catalogKey.Name)) + Expect(catalogMetadata.Kind).To(Equal("CatalogMetadata")) + Expect(catalogMetadata.GetLabels()).To(HaveLen(5)) + Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(catalogKey.Name)) + break + } else if or.Name == tempCatalog.Name { + Expect(catalogMetadata.Name).To(ContainSubstring(tempCatalog.Name)) + Expect(catalogMetadata.Kind).To(Equal("CatalogMetadata")) + Expect(catalogMetadata.GetLabels()).To(HaveLen(5)) + Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) + break + } + } + } + } + }) }) }) }) }) - }) }) From f6fe216ca64613297bc072886e1c825e44fac6bc Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 14 Jul 2023 11:00:18 -0400 Subject: [PATCH 5/7] clean up test implementation Signed-off-by: Bryce Palmer --- .../core/catalog_controller_test.go | 100 +++--------------- 1 file changed, 15 insertions(+), 85 deletions(-) diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index bd1c7402..715c536c 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -341,22 +341,9 @@ var _ = Describe("Catalogd Controller Test", func() { When("creating another Catalog", func() { var ( - tempTestBundleName = "temp-webhook-operator.v0.0.1" - tempTestBundleImage = "quay.io/olmtest/temp-webhook-operator-bundle:0.0.3" - tempTestBundleRelatedImageName = "temp-test" - tempTestBundleRelatedImageImage = "temp-testimage:latest" - tempTestBundleObjectData = "dW5pbXBvcnRhbnQK" - tempTestPackageDefaultChannel = "temp-preview" - tempTestPackageName = "temp-webhook-operator" - tempTestChannelName = "temp-preview" - tempTestPackage = fmt.Sprintf(testPackageTemplate, tempTestPackageDefaultChannel, tempTestPackageName) - tempTestBundle = fmt.Sprintf(testBundleTemplate, tempTestBundleImage, tempTestBundleName, tempTestPackageName, tempTestBundleRelatedImageName, tempTestBundleRelatedImageImage, tempTestBundleObjectData) - tempTestChannel = fmt.Sprintf(testChannelTemplate, tempTestPackageName, tempTestChannelName, tempTestBundleName) - tempCatalog *v1alpha1.Catalog - tempMs *MockSource - tempRec *core.CatalogReconciler - tempTestBundleMetaName string - tempTestPackageMetaName string + tempCatalog *v1alpha1.Catalog + tempTestBundleMetaName string + tempTestPackageMetaName string ) BeforeEach(func() { tempCatalog = &v1alpha1.Catalog{ @@ -371,33 +358,11 @@ var _ = Describe("Catalogd Controller Test", func() { }, } - tempTestBundleMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, tempTestBundleName) - tempTestPackageMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, tempTestPackageName) + tempTestBundleMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, testBundleName) + tempTestPackageMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, testPackageName) - tempMs = &MockSource{} - filesys := &fstest.MapFS{ - "bundle.yaml": &fstest.MapFile{Data: []byte(tempTestBundle), Mode: os.ModePerm}, - "package.yaml": &fstest.MapFile{Data: []byte(tempTestPackage), Mode: os.ModePerm}, - "channel.yaml": &fstest.MapFile{Data: []byte(tempTestChannel), Mode: os.ModePerm}, - } - - tempMs.shouldError = false - tempMs.result = &source.Result{ - ResolvedSource: &catalog.Spec.Source, - State: source.StateUnpacked, - FS: filesys, - } - - tempRec = &core.CatalogReconciler{ - Client: cl, - Unpacker: source.NewUnpacker( - map[v1alpha1.SourceType]source.Unpacker{ - v1alpha1.SourceTypeImage: tempMs, - }, - ), - } Expect(cl.Create(ctx, tempCatalog)).To(Succeed()) - res, err := tempRec.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "tempedout"}}) + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "tempedout"}}) Expect(res).To(Equal(ctrl.Result{})) Expect(err).ToNot(HaveOccurred()) }) @@ -421,12 +386,12 @@ var _ = Describe("Catalogd Controller Test", func() { bundlemetadata = &v1alpha1.BundleMetadata{} Expect(cl.Get(ctx, client.ObjectKey{Name: tempTestBundleMetaName}, bundlemetadata)).To(Succeed()) Expect(bundlemetadata.Name).To(Equal(tempTestBundleMetaName)) - Expect(bundlemetadata.Spec.Image).To(Equal(tempTestBundleImage)) + Expect(bundlemetadata.Spec.Image).To(Equal(testBundleImage)) Expect(bundlemetadata.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) - Expect(bundlemetadata.Spec.Package).To(Equal(tempTestPackageName)) + Expect(bundlemetadata.Spec.Package).To(Equal(testPackageName)) Expect(bundlemetadata.Spec.RelatedImages).To(HaveLen(1)) - Expect(bundlemetadata.Spec.RelatedImages[0].Name).To(Equal(tempTestBundleRelatedImageName)) - Expect(bundlemetadata.Spec.RelatedImages[0].Image).To(Equal(tempTestBundleRelatedImageImage)) + Expect(bundlemetadata.Spec.RelatedImages[0].Name).To(Equal(testBundleRelatedImageName)) + Expect(bundlemetadata.Spec.RelatedImages[0].Image).To(Equal(testBundleRelatedImageImage)) Expect(bundlemetadata.Spec.Properties).To(HaveLen(1)) }) @@ -445,12 +410,12 @@ var _ = Describe("Catalogd Controller Test", func() { pack = &v1alpha1.Package{} Expect(cl.Get(ctx, client.ObjectKey{Name: tempTestPackageMetaName}, pack)).To(Succeed()) Expect(pack.Name).To(Equal(tempTestPackageMetaName)) - Expect(pack.Spec.DefaultChannel).To(Equal(tempTestPackageDefaultChannel)) + Expect(pack.Spec.DefaultChannel).To(Equal(testPackageDefaultChannel)) Expect(pack.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) Expect(pack.Spec.Channels).To(HaveLen(1)) - Expect(pack.Spec.Channels[0].Name).To(Equal(tempTestChannelName)) + Expect(pack.Spec.Channels[0].Name).To(Equal(testChannelName)) Expect(pack.Spec.Channels[0].Entries).To(HaveLen(1)) - Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(tempTestBundleName)) + Expect(pack.Spec.Channels[0].Entries[0].Name).To(Equal(testBundleName)) }) }) }) @@ -492,20 +457,7 @@ var _ = Describe("Catalogd Controller Test", func() { When("creating another Catalog", func() { var ( - tempTestBundleName = "temp-webhook-operator.v0.0.1" - tempTestBundleImage = "quay.io/olmtest/temp-webhook-operator-bundle:0.0.3" - tempTestBundleRelatedImageName = "temp-test" - tempTestBundleRelatedImageImage = "temp-testimage:latest" - tempTestBundleObjectData = "dW5pbXBvcnRhbnQK" - tempTestPackageDefaultChannel = "temp-preview" - tempTestPackageName = "temp-webhook-operator" - tempTestChannelName = "temp-preview" - tempTestPackage = fmt.Sprintf(testPackageTemplate, tempTestPackageDefaultChannel, tempTestPackageName) - tempTestBundle = fmt.Sprintf(testBundleTemplate, tempTestBundleImage, tempTestBundleName, tempTestPackageName, tempTestBundleRelatedImageName, tempTestBundleRelatedImageImage, tempTestBundleObjectData) - tempTestChannel = fmt.Sprintf(testChannelTemplate, tempTestPackageName, tempTestChannelName, tempTestBundleName) - tempCatalog *v1alpha1.Catalog - tempMs *MockSource - tempRec *core.CatalogReconciler + tempCatalog *v1alpha1.Catalog ) BeforeEach(func() { tempCatalog = &v1alpha1.Catalog{ @@ -520,30 +472,8 @@ var _ = Describe("Catalogd Controller Test", func() { }, } - tempMs = &MockSource{} - filesys := &fstest.MapFS{ - "bundle.yaml": &fstest.MapFile{Data: []byte(tempTestBundle), Mode: os.ModePerm}, - "package.yaml": &fstest.MapFile{Data: []byte(tempTestPackage), Mode: os.ModePerm}, - "channel.yaml": &fstest.MapFile{Data: []byte(tempTestChannel), Mode: os.ModePerm}, - } - - tempMs.shouldError = false - tempMs.result = &source.Result{ - ResolvedSource: &catalog.Spec.Source, - State: source.StateUnpacked, - FS: filesys, - } - - tempRec = &core.CatalogReconciler{ - Client: cl, - Unpacker: source.NewUnpacker( - map[v1alpha1.SourceType]source.Unpacker{ - v1alpha1.SourceTypeImage: tempMs, - }, - ), - } Expect(cl.Create(ctx, tempCatalog)).To(Succeed()) - res, err := tempRec.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "tempedout"}}) + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "tempedout"}}) Expect(res).To(Equal(ctrl.Result{})) Expect(err).ToNot(HaveOccurred()) }) From 7b408efd15e141c05408cf1c26341b1191fe3cbb Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 14 Jul 2023 13:16:01 -0400 Subject: [PATCH 6/7] address review comments Signed-off-by: Bryce Palmer --- pkg/controllers/core/catalog_controller_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index 715c536c..5b3b4fd1 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -492,13 +492,11 @@ var _ = Describe("Catalogd Controller Test", func() { if or.Name == catalogKey.Name { Expect(catalogMetadata.Name).To(ContainSubstring(catalogKey.Name)) Expect(catalogMetadata.Kind).To(Equal("CatalogMetadata")) - Expect(catalogMetadata.GetLabels()).To(HaveLen(5)) Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(catalogKey.Name)) break } else if or.Name == tempCatalog.Name { Expect(catalogMetadata.Name).To(ContainSubstring(tempCatalog.Name)) Expect(catalogMetadata.Kind).To(Equal("CatalogMetadata")) - Expect(catalogMetadata.GetLabels()).To(HaveLen(5)) Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) break } From 098fbaa0761030eaddb8384622a4bb16344ca75e Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 14 Jul 2023 13:16:27 -0400 Subject: [PATCH 7/7] remove all label len checks Signed-off-by: Bryce Palmer --- pkg/controllers/core/catalog_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index 5b3b4fd1..b14ce245 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -448,7 +448,6 @@ var _ = Describe("Catalogd Controller Test", func() { for _, catalogMetadata := range catalogMetadatas.Items { Expect(catalogMetadata.Name).To(ContainSubstring(catalogKey.Name)) Expect(catalogMetadata.Kind).To(Equal("CatalogMetadata")) - Expect(catalogMetadata.GetLabels()).To(HaveLen(5)) Expect(catalogMetadata.OwnerReferences).To(HaveLen(1)) Expect(catalogMetadata.OwnerReferences[0].Name).To(Equal(catalogKey.Name)) Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(catalogKey.Name))