From bdbb577335a44b76ce3bde40388af80ea38f3a71 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Tue, 1 Aug 2023 10:57:11 -0400 Subject: [PATCH 1/2] use Storage interface in Catalog controller Signed-off-by: Bryce Palmer --- cmd/manager/main.go | 11 ++ config/default/manager_auth_proxy_patch.yaml | 2 +- internal/storage/storage.go | 46 ++++++ internal/storage/temp.go | 41 +++++ pkg/controllers/core/catalog_controller.go | 8 + .../core/catalog_controller_test.go | 140 ++++++++++++++++++ pkg/features/features.go | 2 + 7 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 internal/storage/storage.go create mode 100644 internal/storage/temp.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 0214b526..6c291cd1 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -35,6 +35,7 @@ import ( "github.com/spf13/pflag" "github.com/operator-framework/catalogd/internal/source" + "github.com/operator-framework/catalogd/internal/storage" "github.com/operator-framework/catalogd/internal/version" corecontrollers "github.com/operator-framework/catalogd/pkg/controllers/core" "github.com/operator-framework/catalogd/pkg/features" @@ -116,9 +117,12 @@ func main() { os.Exit(1) } + store := &storage.TempStorage{} + if err = (&corecontrollers.CatalogReconciler{ Client: mgr.GetClient(), Unpacker: unpacker, + Storage: store, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Catalog") os.Exit(1) @@ -134,6 +138,13 @@ func main() { os.Exit(1) } + if features.CatalogdFeatureGate.Enabled(features.HttpServer) { + if err := mgr.AddMetricsExtraHandler("/storage/", store); err != nil { + setupLog.Error(err, "unable to set up storage") + os.Exit(1) + } + } + if profiling { pprofer := profile.NewPprofer() if err := pprofer.ConfigureControllerManager(mgr); err != nil { diff --git a/config/default/manager_auth_proxy_patch.yaml b/config/default/manager_auth_proxy_patch.yaml index f15c2cb9..a38d65e0 100644 --- a/config/default/manager_auth_proxy_patch.yaml +++ b/config/default/manager_auth_proxy_patch.yaml @@ -50,5 +50,5 @@ spec: - "--health-probe-bind-address=:8081" - "--metrics-bind-address=127.0.0.1:8080" - "--leader-elect" - - "--feature-gates=PackagesBundleMetadataAPIs=true" + - "--feature-gates=HttpServer=true" diff --git a/internal/storage/storage.go b/internal/storage/storage.go new file mode 100644 index 00000000..cedc99a1 --- /dev/null +++ b/internal/storage/storage.go @@ -0,0 +1,46 @@ +package storage + +import ( + "context" + "io/fs" + "net/http" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type Storage interface { + Loader + Storer +} + +type Loader interface { + Load(ctx context.Context, owner client.Object) (fs.FS, error) +} + +type Storer interface { + Store(ctx context.Context, owner client.Object, bundle fs.FS) error + Delete(ctx context.Context, owner client.Object) error + + http.Handler + URLFor(ctx context.Context, owner client.Object) (string, error) +} + +type fallbackLoaderStorage struct { + Storage + fallbackLoader Loader +} + +func WithFallbackLoader(s Storage, fallback Loader) Storage { + return &fallbackLoaderStorage{ + Storage: s, + fallbackLoader: fallback, + } +} + +func (s *fallbackLoaderStorage) Load(ctx context.Context, owner client.Object) (fs.FS, error) { + fsys, err := s.Storage.Load(ctx, owner) + if err != nil { + return s.fallbackLoader.Load(ctx, owner) + } + return fsys, nil +} diff --git a/internal/storage/temp.go b/internal/storage/temp.go new file mode 100644 index 00000000..0150a4b2 --- /dev/null +++ b/internal/storage/temp.go @@ -0,0 +1,41 @@ +package storage + +import ( + "context" + "fmt" + "io/fs" + "log" + "net/http" + "testing/fstest" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ Storage = &TempStorage{} + +var _ http.Handler = &TempStorage{} + +type TempStorage struct{} + +func (ts *TempStorage) Store(ctx context.Context, owner client.Object, bundle fs.FS) error { + log.Printf("Storing contents for %s", owner.GetName()) + return nil +} + +func (ts *TempStorage) Delete(ctx context.Context, owner client.Object) error { + log.Printf("Deleting contents for %s", owner.GetName()) + return nil +} + +func (ts *TempStorage) URLFor(ctx context.Context, owner client.Object) (string, error) { + return fmt.Sprintf("%s-tempstorage-url", owner.GetName()), nil +} + +func (ts *TempStorage) ServeHTTP(rw http.ResponseWriter, req *http.Request) { + // Eat any errors since this is temporary + rw.Write([]byte("TempStorage serving some HTTP!")) +} + +func (ts *TempStorage) Load(ctx context.Context, owner client.Object) (fs.FS, error) { + return &fstest.MapFS{}, nil +} diff --git a/pkg/controllers/core/catalog_controller.go b/pkg/controllers/core/catalog_controller.go index c3a873e9..81d8d9ac 100644 --- a/pkg/controllers/core/catalog_controller.go +++ b/pkg/controllers/core/catalog_controller.go @@ -41,6 +41,7 @@ import ( "github.com/operator-framework/catalogd/api/core/v1alpha1" "github.com/operator-framework/catalogd/internal/source" + "github.com/operator-framework/catalogd/internal/storage" "github.com/operator-framework/catalogd/pkg/features" ) @@ -50,6 +51,7 @@ import ( type CatalogReconciler struct { client.Client Unpacker source.Unpacker + Storage storage.Storage } //+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=catalogs,verbs=get;list;watch;create;update;patch;delete @@ -164,6 +166,12 @@ func (r *CatalogReconciler) reconcile(ctx context.Context, catalog *v1alpha1.Cat } } + if features.CatalogdFeatureGate.Enabled(features.HttpServer) { + if err = r.Storage.Store(ctx, catalog, unpackResult.FS); err != nil { + return ctrl.Result{}, updateStatusUnpackFailing(&catalog.Status, fmt.Errorf("storing catalog: %v", err)) + } + } + updateStatusUnpacked(&catalog.Status, unpackResult) return ctrl.Result{}, nil default: diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index b14ce245..0526cf0e 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -506,6 +506,146 @@ var _ = Describe("Catalogd Controller Test", func() { }) }) }) + + 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{} + Expect(cl.List(ctx, catalogMetadatas)).To(Succeed()) + Expect(catalogMetadatas.Items).To(HaveLen(3)) + for _, catalogMetadata := range catalogMetadatas.Items { + Expect(catalogMetadata.Name).To(ContainSubstring(catalogKey.Name)) + Expect(catalogMetadata.Kind).To(Equal("CatalogMetadata")) + Expect(catalogMetadata.OwnerReferences).To(HaveLen(1)) + Expect(catalogMetadata.OwnerReferences[0].Name).To(Equal(catalogKey.Name)) + Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(catalogKey.Name)) + } + }) + + When("creating another Catalog", func() { + var ( + tempCatalog *v1alpha1.Catalog + ) + 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, tempCatalog)).To(Succeed()) + res, err := reconciler.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.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.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) + break + } + } + } + } + }) + }) + + // TODO(everettraven): Implement proper testing logic when there is an implementation to test + When("the HttpServer feature gate is enabled", func() { + BeforeEach(func() { + Expect(features.CatalogdFeatureGate.SetFromMap(map[string]bool{ + string(features.HttpServer): 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.HttpServer): false, + })).NotTo(HaveOccurred()) + }) + + It("should do something", func() {}) + + When("creating another Catalog", func() { + var ( + // tempCatalog *v1alpha1.Catalog + ) + 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, tempCatalog)).To(Succeed()) + // res, err := reconciler.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 do something", func() {}) + }) + }) + }) }) }) }) diff --git a/pkg/features/features.go b/pkg/features/features.go index c5ded7db..c39121d2 100644 --- a/pkg/features/features.go +++ b/pkg/features/features.go @@ -11,6 +11,7 @@ const ( CatalogMetadataAPI featuregate.Feature = "CatalogMetadataAPI" PackagesBundleMetadataAPIs featuregate.Feature = "PackagesBundleMetadataAPIs" + HttpServer featuregate.Feature = "HttpServer" ) var catalogdFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ @@ -19,6 +20,7 @@ var catalogdFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ PackagesBundleMetadataAPIs: {Default: false, PreRelease: featuregate.Deprecated}, CatalogMetadataAPI: {Default: false, PreRelease: featuregate.Alpha}, + HttpServer: {Default: false, PreRelease: featuregate.Alpha}, } var CatalogdFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate() From 970f06aeaf5eca5f9c7b3678c6a1e7e504a8e37c Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Tue, 1 Aug 2023 12:38:45 -0400 Subject: [PATCH 2/2] remove accidental test duplication Signed-off-by: Bryce Palmer --- .../core/catalog_controller_test.go | 87 ------------------- 1 file changed, 87 deletions(-) diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index 0526cf0e..a7bc3467 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -505,93 +505,6 @@ var _ = Describe("Catalogd Controller Test", func() { }) }) }) - }) - - 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{} - Expect(cl.List(ctx, catalogMetadatas)).To(Succeed()) - Expect(catalogMetadatas.Items).To(HaveLen(3)) - for _, catalogMetadata := range catalogMetadatas.Items { - Expect(catalogMetadata.Name).To(ContainSubstring(catalogKey.Name)) - Expect(catalogMetadata.Kind).To(Equal("CatalogMetadata")) - Expect(catalogMetadata.OwnerReferences).To(HaveLen(1)) - Expect(catalogMetadata.OwnerReferences[0].Name).To(Equal(catalogKey.Name)) - Expect(catalogMetadata.Spec.Catalog.Name).To(Equal(catalogKey.Name)) - } - }) - - When("creating another Catalog", func() { - var ( - tempCatalog *v1alpha1.Catalog - ) - 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, tempCatalog)).To(Succeed()) - res, err := reconciler.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.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.Spec.Catalog.Name).To(Equal(tempCatalog.Name)) - break - } - } - } - } - }) - }) - // TODO(everettraven): Implement proper testing logic when there is an implementation to test When("the HttpServer feature gate is enabled", func() { BeforeEach(func() {