From ab7400c1ed1a016ae9817d7bf6e553d11a5c4d6e Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk Date: Mon, 21 Aug 2023 16:36:53 +0100 Subject: [PATCH] Switch to catalogd's `CatalogMetadata` APIs Signed-off-by: Mikalai Radchuk --- cmd/resolutioncli/entity_source.go | 58 +------ config/rbac/role.yaml | 2 +- internal/controllers/operator_controller.go | 2 +- .../entitysources/catalogdsource.go | 148 ++++++++++++------ test/e2e/e2e_suite_test.go | 7 +- .../operator_framework_test.go | 57 ++++--- 6 files changed, 138 insertions(+), 136 deletions(-) diff --git a/cmd/resolutioncli/entity_source.go b/cmd/resolutioncli/entity_source.go index b5d6e1128..664cf9943 100644 --- a/cmd/resolutioncli/entity_source.go +++ b/cmd/resolutioncli/entity_source.go @@ -18,17 +18,12 @@ package main import ( "context" - "encoding/json" - "fmt" "github.com/operator-framework/deppy/pkg/deppy" "github.com/operator-framework/deppy/pkg/deppy/input" "github.com/operator-framework/operator-registry/alpha/action" - "github.com/operator-framework/operator-registry/alpha/declcfg" - "github.com/operator-framework/operator-registry/alpha/model" - "github.com/operator-framework/operator-registry/alpha/property" - olmentity "github.com/operator-framework/operator-controller/internal/resolution/entities" + "github.com/operator-framework/operator-controller/internal/resolution/entitysources" ) type indexRefEntitySource struct { @@ -101,12 +96,8 @@ func (es *indexRefEntitySource) entities(ctx context.Context) (input.EntityList, return nil, err } - model, err := declcfg.ConvertToModel(*cfg) - if err != nil { - return nil, err - } - - entities, err := modelToEntities(model) + // TODO: update empty catalog name string to be catalog name once we support multiple catalogs in CLI + entities, err := entitysources.MetadataToEntities("", cfg.Channels, cfg.Bundles) if err != nil { return nil, err } @@ -116,46 +107,3 @@ func (es *indexRefEntitySource) entities(ctx context.Context) (input.EntityList, return es.entitiesCache, nil } - -// TODO: Reduce code duplication: share a function with catalogdEntitySource (see getEntities) -// We don't want to maintain two functions performing conversion into input.EntityList. -// For this we need some common format. So we need a package which will be able -// to convert from declfcg structs into CRD structs directly or via model.Model. -// One option would be to make this piece of code from catalogd reusable and exportable: -// https://github.com/operator-framework/catalogd/blob/9fe45a628de2e74d9cd73c3650fa2582aaac5213/pkg/controllers/core/catalog_controller.go#L200-L360 -func modelToEntities(model model.Model) (input.EntityList, error) { - entities := input.EntityList{} - - for _, pkg := range model { - for _, ch := range pkg.Channels { - for _, bundle := range ch.Bundles { - props := map[string]string{} - - for _, prop := range bundle.Properties { - switch prop.Type { - case property.TypePackage: - // this is already a json marshalled object, so it doesn't need to be marshalled - // like the other ones - props[property.TypePackage] = string(prop.Value) - } - } - - imgValue, err := json.Marshal(bundle.Image) - if err != nil { - return nil, err - } - props[olmentity.PropertyBundlePath] = string(imgValue) - - channelValue, _ := json.Marshal(property.Channel{ChannelName: ch.Name, Priority: 0}) - props[property.TypeChannel] = string(channelValue) - entity := input.Entity{ - ID: deppy.IdentifierFromString(fmt.Sprintf("%s%s%s", bundle.Name, bundle.Package.Name, ch.Name)), - Properties: props, - } - entities = append(entities, entity) - } - } - } - - return entities, nil -} diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 49d3c2458..5bbc421c2 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -7,7 +7,7 @@ rules: - apiGroups: - catalogd.operatorframework.io resources: - - bundlemetadata + - catalogmetadata verbs: - list - watch diff --git a/internal/controllers/operator_controller.go b/internal/controllers/operator_controller.go index a842b8d20..154f293cf 100644 --- a/internal/controllers/operator_controller.go +++ b/internal/controllers/operator_controller.go @@ -58,9 +58,9 @@ type OperatorReconciler struct { //+kubebuilder:rbac:groups=core.rukpak.io,resources=bundledeployments,verbs=get;list;watch;create;update;patch -//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=bundlemetadata,verbs=list;watch //+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=packages,verbs=list;watch //+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=catalogs,verbs=list;watch +//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=catalogmetadata,verbs=list;watch func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { l := log.FromContext(ctx).WithName("operator-controller") diff --git a/internal/resolution/entitysources/catalogdsource.go b/internal/resolution/entitysources/catalogdsource.go index 6c9460e68..ba838da9b 100644 --- a/internal/resolution/entitysources/catalogdsource.go +++ b/internal/resolution/entitysources/catalogdsource.go @@ -8,6 +8,7 @@ import ( catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" "github.com/operator-framework/deppy/pkg/deppy" "github.com/operator-framework/deppy/pkg/deppy/input" + "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" "sigs.k8s.io/controller-runtime/pkg/client" @@ -71,70 +72,119 @@ func (es *CatalogdEntitySource) Iterate(ctx context.Context, fn input.IteratorFu return nil } -func getEntities(ctx context.Context, client client.Client) (input.EntityList, error) { - entityList := input.EntityList{} - bundleMetadatas, packageMetdatas, err := fetchMetadata(ctx, client) - if err != nil { +func getEntities(ctx context.Context, cl client.Client) (input.EntityList, error) { + allEntitiesList := input.EntityList{} + + var catalogList catalogd.CatalogList + if err := cl.List(ctx, &catalogList); err != nil { return nil, err } - for _, bundle := range bundleMetadatas.Items { - props := map[string]string{} - - // TODO: We should make sure all properties are forwarded - // through and avoid a lossy translation from FBC --> entity - for _, prop := range bundle.Spec.Properties { - switch prop.Type { - case property.TypePackage: - // this is already a json marshalled object, so it doesn't need to be marshalled - // like the other ones - props[property.TypePackage] = string(prop.Value) - case entities.PropertyBundleMediaType: - props[entities.PropertyBundleMediaType] = string(prop.Value) - } + for _, catalog := range catalogList.Items { + channels, bundles, err := fetchCatalogMetadata(ctx, cl, catalog.Name) + if err != nil { + return nil, err } - imgValue, err := json.Marshal(bundle.Spec.Image) + catalogEntitiesList, err := MetadataToEntities(catalog.Name, channels, bundles) if err != nil { return nil, err } - props[entities.PropertyBundlePath] = string(imgValue) - catalogScopedPkgName := fmt.Sprintf("%s-%s", bundle.Spec.Catalog.Name, bundle.Spec.Package) - bundlePkg := packageMetdatas[catalogScopedPkgName] - for _, ch := range bundlePkg.Spec.Channels { - for _, b := range ch.Entries { - catalogScopedEntryName := fmt.Sprintf("%s-%s", bundle.Spec.Catalog.Name, b.Name) - if catalogScopedEntryName == bundle.Name { - channelValue, _ := json.Marshal(property.Channel{ChannelName: ch.Name, Priority: 0}) - props[property.TypeChannel] = string(channelValue) - replacesValue, _ := json.Marshal(entities.ChannelEntry{ - Name: b.Name, - Replaces: b.Replaces, - }) - props[entities.PropertyBundleChannelEntry] = string(replacesValue) - entity := input.Entity{ - ID: deppy.IdentifierFromString(fmt.Sprintf("%s%s%s", bundle.Name, bundle.Spec.Package, ch.Name)), - Properties: props, - } - entityList = append(entityList, entity) + + allEntitiesList = append(allEntitiesList, catalogEntitiesList...) + } + + return allEntitiesList, nil +} + +func MetadataToEntities(catalogName string, channels []declcfg.Channel, bundles []declcfg.Bundle) (input.EntityList, error) { + entityList := input.EntityList{} + + bundlesMap := map[string]*declcfg.Bundle{} + for i := range bundles { + bundleKey := fmt.Sprintf("%s-%s-%s", catalogName, bundles[i].Package, bundles[i].Name) + bundlesMap[bundleKey] = &bundles[i] + } + + for _, ch := range channels { + for _, chEntry := range ch.Entries { + bundleKey := fmt.Sprintf("%s-%s-%s", catalogName, ch.Package, chEntry.Name) + bundle, ok := bundlesMap[bundleKey] + if !ok { + return nil, fmt.Errorf("bundle %q not found in catalog %q (package %q, channel %q)", chEntry.Name, catalogName, ch.Package, ch.Name) + } + + props := map[string]string{} + + for _, prop := range bundle.Properties { + switch prop.Type { + case property.TypePackage: + // this is already a json marshalled object, so it doesn't need to be marshalled + // like the other ones + props[property.TypePackage] = string(prop.Value) + case entities.PropertyBundleMediaType: + props[entities.PropertyBundleMediaType] = string(prop.Value) } } + + imgValue, err := json.Marshal(bundle.Image) + if err != nil { + return nil, err + } + props[entities.PropertyBundlePath] = string(imgValue) + + channelValue, _ := json.Marshal(property.Channel{ChannelName: ch.Name, Priority: 0}) + props[property.TypeChannel] = string(channelValue) + replacesValue, _ := json.Marshal(entities.ChannelEntry{ + Name: bundle.Name, + Replaces: chEntry.Replaces, + }) + props[entities.PropertyBundleChannelEntry] = string(replacesValue) + + catalogScopedEntryName := fmt.Sprintf("%s-%s", catalogName, bundle.Name) + entity := input.Entity{ + ID: deppy.IdentifierFromString(fmt.Sprintf("%s%s%s", catalogScopedEntryName, bundle.Package, ch.Name)), + Properties: props, + } + entityList = append(entityList, entity) } } + return entityList, nil } -func fetchMetadata(ctx context.Context, client client.Client) (catalogd.BundleMetadataList, map[string]catalogd.Package, error) { - packageMetdatas := catalogd.PackageList{} - if err := client.List(ctx, &packageMetdatas); err != nil { - return catalogd.BundleMetadataList{}, nil, err +func fetchCatalogMetadata(ctx context.Context, cl client.Client, catalogName string) ([]declcfg.Channel, []declcfg.Bundle, error) { + channels, err := fetchCatalogMetadataByScheme[declcfg.Channel](ctx, cl, declcfg.SchemaChannel, catalogName) + if err != nil { + return nil, nil, err + } + bundles, err := fetchCatalogMetadataByScheme[declcfg.Bundle](ctx, cl, declcfg.SchemaBundle, catalogName) + if err != nil { + return nil, nil, err } - bundleMetadatas := catalogd.BundleMetadataList{} - if err := client.List(ctx, &bundleMetadatas); err != nil { - return catalogd.BundleMetadataList{}, nil, err + + return channels, bundles, nil +} + +type declcfgSchema interface { + declcfg.Package | declcfg.Bundle | declcfg.Channel +} + +// TODO: Cleanup once https://github.com/golang/go/issues/45380 implemented +// We should be able to get rid of the schema arg and switch based on the type passed to this generic +func fetchCatalogMetadataByScheme[T declcfgSchema](ctx context.Context, cl client.Client, schema, catalogName string) ([]T, error) { + cmList := catalogd.CatalogMetadataList{} + if err := cl.List(ctx, &cmList, client.MatchingLabels{"schema": schema, "catalog": catalogName}); err != nil { + return nil, err } - packages := map[string]catalogd.Package{} - for _, pkg := range packageMetdatas.Items { - packages[pkg.Name] = pkg + + contents := []T{} + for _, cm := range cmList.Items { + var content T + if err := json.Unmarshal(cm.Spec.Content, &content); err != nil { + return nil, err + } + contents = append(contents, content) } - return bundleMetadatas, packages, nil + + return contents, nil } diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index d5dc9d4e0..05cd7eb0a 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -95,8 +95,9 @@ var _ = AfterSuite(func() { }).Should(Succeed()) // speed up delete without waiting for gc - Expect(c.DeleteAllOf(ctx, &catalogd.BundleMetadata{})).To(Succeed()) Expect(c.DeleteAllOf(ctx, &catalogd.Package{})).To(Succeed()) + Expect(c.DeleteAllOf(ctx, &catalogd.BundleMetadata{})).To(Succeed()) + Expect(c.DeleteAllOf(ctx, &catalogd.CatalogMetadata{})).To(Succeed()) Eventually(func(g Gomega) { // ensure resource cleanup @@ -107,6 +108,10 @@ var _ = AfterSuite(func() { bmd := &catalogd.BundleMetadataList{} g.Expect(c.List(ctx, bmd)).To(Succeed()) g.Expect(bmd.Items).To(BeEmpty()) + + cmd := &catalogd.CatalogMetadataList{} + g.Expect(c.List(ctx, cmd)).To(Succeed()) + g.Expect(bmd.Items).To(BeEmpty()) }).Should(Succeed()) }) diff --git a/test/operator-framework-e2e/operator_framework_test.go b/test/operator-framework-e2e/operator_framework_test.go index db2e6a836..822463dbd 100644 --- a/test/operator-framework-e2e/operator_framework_test.go +++ b/test/operator-framework-e2e/operator_framework_test.go @@ -8,7 +8,6 @@ import ( "os" "os/exec" "path/filepath" - "reflect" "strings" "testing" "time" @@ -27,6 +26,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" + "github.com/operator-framework/operator-registry/alpha/declcfg" + "github.com/operator-framework/operator-registry/alpha/property" rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" operatorv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" @@ -833,8 +834,7 @@ func createCatalogCheckResources(operatorCatalog *catalogd.Catalog, catalogDInfo // checking if the bundle metadatas are created By("Eventually checking if bundle metadata is created") Eventually(func(g Gomega) { - err = validateBundleMetadataCreation(operatorCatalog, catalogDInfo.operatorName, bundleVersions) - g.Expect(err).ToNot(HaveOccurred()) + validateCatalogMetadataCreation(g, operatorCatalog, catalogDInfo.operatorName, bundleVersions) }).Should(Succeed()) return operatorCatalog, nil } @@ -886,39 +886,38 @@ func validatePackageCreation(operatorCatalog *catalogd.Catalog, pkgName string) return nil } -// Checks if the bundle metadatas are created from the catalog and returns error if not. +// Checks if the CatalogMetadata was created from the catalog and returns error if not. // The expected pkgNames and their versions are taken as input. This is then compared against the collected bundle versions. // The collected bundle versions are formed by reading the version from "olm.package" property type whose catalog name // matches the catalog name and pkgName matches the pkgName under consideration. -func validateBundleMetadataCreation(operatorCatalog *catalogd.Catalog, pkgName string, versions []string) error { - type Package struct { - PackageName string `json:"packageName"` - Version string `json:"version"` - } - var pkgValue Package - collectedBundleVersions := make([]string, 0) - bmList := &catalogd.BundleMetadataList{} - if err := c.List(ctx, bmList); err != nil { - return fmt.Errorf("Error retrieving the bundle metadata after %v catalog instance creation: %v", operatorCatalog.Name, err) - } - - for _, bm := range bmList.Items { - if bm.Spec.Catalog.Name == operatorCatalog.Name { - for _, prop := range bm.Spec.Properties { - if prop.Type == "olm.package" { - err := json.Unmarshal(prop.Value, &pkgValue) - if err == nil && pkgValue.PackageName == pkgName { - collectedBundleVersions = append(collectedBundleVersions, pkgValue.Version) - } - } +func validateCatalogMetadataCreation(g Gomega, operatorCatalog *catalogd.Catalog, pkgName string, versions []string) { + cmList := catalogd.CatalogMetadataList{} + err := c.List(ctx, &cmList, client.MatchingLabels{ + "schema": declcfg.SchemaBundle, + "catalog": operatorCatalog.Name, + "package": pkgName, + }) + g.Expect(err).ToNot(HaveOccurred()) + + collectedBundleVersions := []string{} + for _, cm := range cmList.Items { + bundle := declcfg.Bundle{} + + err := json.Unmarshal(cm.Spec.Content, &bundle) + g.Expect(err).ToNot(HaveOccurred()) + + for _, prop := range bundle.Properties { + if prop.Type == "olm.package" { + var pkgValue property.Package + err := json.Unmarshal(prop.Value, &pkgValue) + g.Expect(err).ToNot(HaveOccurred()) + + collectedBundleVersions = append(collectedBundleVersions, pkgValue.Version) } } } - if !reflect.DeepEqual(collectedBundleVersions, versions) { - return fmt.Errorf("Package %v for the catalog %v is not created", pkgName, operatorCatalog.Name) - } - return nil + g.Expect(collectedBundleVersions).To(ConsistOf(versions)) } // Checks for a successful resolution and bundle path for the operator and returns error if not.