From 3df6119fe6d8f806527c323aecc1187d98529cad Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Fri, 11 Aug 2023 08:59:05 -0400 Subject: [PATCH] Fixed CatalogMetadata syncing issues related to metadata.name and etcd size constraints Signed-off-by: Joe Lanford --- internal/k8sutil/k8sutil.go | 17 +++++++++ pkg/controllers/core/catalog_controller.go | 35 +++++++++++++++---- .../core/catalog_controller_test.go | 33 ++++++----------- 3 files changed, 57 insertions(+), 28 deletions(-) create mode 100644 internal/k8sutil/k8sutil.go diff --git a/internal/k8sutil/k8sutil.go b/internal/k8sutil/k8sutil.go new file mode 100644 index 00000000..fb3d04a3 --- /dev/null +++ b/internal/k8sutil/k8sutil.go @@ -0,0 +1,17 @@ +package k8sutil + +import ( + "regexp" + + "k8s.io/apimachinery/pkg/util/validation" +) + +var invalidDNSChars = regexp.MustCompile(`[^a-zA-Z0-9-]`) + +// MetadataName replaces all invalid DNS characters with a dash. If the result +// is not a valid DNS subdomain, returns `result, false`. Otherwise, returns the +// `result, true`. +func MetadataName(name string) (string, bool) { + result := invalidDNSChars.ReplaceAllString(name, "-") + return result, validation.IsDNS1123Subdomain(result) == nil +} diff --git a/pkg/controllers/core/catalog_controller.go b/pkg/controllers/core/catalog_controller.go index c3a873e9..ec11a16e 100644 --- a/pkg/controllers/core/catalog_controller.go +++ b/pkg/controllers/core/catalog_controller.go @@ -18,14 +18,14 @@ package core import ( "context" - // #nosec - "crypto/sha1" + "crypto/sha1" // #nosec "encoding/hex" "encoding/json" "fmt" "io/fs" "github.com/operator-framework/operator-registry/alpha/declcfg" + "github.com/operator-framework/operator-registry/alpha/property" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" @@ -40,6 +40,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "github.com/operator-framework/catalogd/api/core/v1alpha1" + "github.com/operator-framework/catalogd/internal/k8sutil" "github.com/operator-framework/catalogd/internal/source" "github.com/operator-framework/catalogd/pkg/features" ) @@ -223,7 +224,7 @@ func (r *CatalogReconciler) syncBundleMetadata(ctx context.Context, declCfg *dec newBundles := map[string]*v1alpha1.BundleMetadata{} for _, bundle := range declCfg.Bundles { - bundleName := fmt.Sprintf("%s-%s", catalog.Name, bundle.Name) + bundleName, _ := k8sutil.MetadataName(fmt.Sprintf("%s-%s", catalog.Name, bundle.Name)) bundleMeta := v1alpha1.BundleMetadata{ TypeMeta: metav1.TypeMeta{ @@ -303,7 +304,7 @@ func (r *CatalogReconciler) syncPackages(ctx context.Context, declCfg *declcfg.D newPkgs := map[string]*v1alpha1.Package{} for _, pkg := range declCfg.Packages { - name := fmt.Sprintf("%s-%s", catalog.Name, pkg.Name) + name, _ := k8sutil.MetadataName(fmt.Sprintf("%s-%s", catalog.Name, pkg.Name)) var icon *v1alpha1.Icon if pkg.Icon != nil { icon = &v1alpha1.Icon{ @@ -342,7 +343,7 @@ func (r *CatalogReconciler) syncPackages(ctx context.Context, declCfg *declcfg.D } for _, ch := range declCfg.Channels { - pkgName := fmt.Sprintf("%s-%s", catalog.Name, ch.Package) + pkgName, _ := k8sutil.MetadataName(fmt.Sprintf("%s-%s", catalog.Name, ch.Package)) pkg, ok := newPkgs[pkgName] if !ok { return fmt.Errorf("channel %q references package %q which does not exist", ch.Name, ch.Package) @@ -404,6 +405,26 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS, return fmt.Errorf("error in generating catalog metadata name: %w", err) } + blob := meta.Blob + if meta.Schema == declcfg.SchemaBundle { + var b declcfg.Bundle + if err := json.Unmarshal(blob, &b); err != nil { + return fmt.Errorf("error unmarshalling bundle: %w", err) + } + properties := b.Properties[:0] + for _, p := range b.Properties { + if p.Type == property.TypeBundleObject { + continue + } + properties = append(properties, p) + } + b.Properties = properties + blob, err = json.Marshal(b) + if err != nil { + return fmt.Errorf("error marshalling bundle: %w", err) + } + } + catalogMetadata := &v1alpha1.CatalogMetadata{ TypeMeta: metav1.TypeMeta{ APIVersion: v1alpha1.GroupVersion.String(), @@ -432,7 +453,7 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS, Name: meta.Name, Schema: meta.Schema, Package: meta.Package, - Content: meta.Blob, + Content: blob, }, } @@ -474,6 +495,7 @@ func (r *CatalogReconciler) syncCatalogMetadata(ctx context.Context, fsys fs.FS, // In the place of the empty `meta.Name`, it computes a hash of `meta.Blob` to prevent multiple FBC blobs colliding on the the object name. // Possible outcomes are: "{catalogName}-{meta.Schema}-{meta.Name}", "{catalogName}-{meta.Schema}-{meta.Package}-{meta.Name}", // "{catalogName}-{meta.Schema}-{hash{meta.Blob}}", "{catalogName}-{meta.Schema}-{meta.Package}-{hash{meta.Blob}}". +// Characters that would result in an invalid DNS name are replaced with dashes. func generateCatalogMetadataName(_ context.Context, catalogName string, meta *declcfg.Meta) (string, error) { objName := fmt.Sprintf("%s-%s", catalogName, meta.Schema) if meta.Package != "" { @@ -491,5 +513,6 @@ func generateCatalogMetadataName(_ context.Context, catalogName string, meta *de h.Write(metaJSON) objName = fmt.Sprintf("%s-%s", objName, hex.EncodeToString(h.Sum(nil))) } + objName, _ = k8sutil.MetadataName(objName) return objName, nil } diff --git a/pkg/controllers/core/catalog_controller_test.go b/pkg/controllers/core/catalog_controller_test.go index b14ce245..e4b82385 100644 --- a/pkg/controllers/core/catalog_controller_test.go +++ b/pkg/controllers/core/catalog_controller_test.go @@ -19,6 +19,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "github.com/operator-framework/catalogd/api/core/v1alpha1" + "github.com/operator-framework/catalogd/internal/k8sutil" "github.com/operator-framework/catalogd/internal/source" "github.com/operator-framework/catalogd/pkg/controllers/core" "github.com/operator-framework/catalogd/pkg/features" @@ -222,14 +223,14 @@ var _ = Describe("Catalogd Controller Test", func() { When("unpacker returns source.Result with state == 'Unpacked'", func() { var ( - testBundleName = "webhook-operator.v0.0.1" + 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" - testPackageName = "webhook-operator" - testChannelName = "preview" + 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) @@ -238,8 +239,8 @@ var _ = Describe("Catalogd Controller Test", func() { testPackageMetaName string ) BeforeEach(func() { - testBundleMetaName = fmt.Sprintf("%s-%s", catalog.Name, testBundleName) - testPackageMetaName = fmt.Sprintf("%s-%s", catalog.Name, testPackageName) + testBundleMetaName, _ = k8sutil.MetadataName(fmt.Sprintf("%s-%s", catalog.Name, testBundleName)) + testPackageMetaName, _ = k8sutil.MetadataName(fmt.Sprintf("%s-%s", catalog.Name, testPackageName)) filesys := &fstest.MapFS{ "bundle.yaml": &fstest.MapFile{Data: []byte(testBundle), Mode: os.ModePerm}, @@ -286,21 +287,9 @@ var _ = Describe("Catalogd Controller Test", func() { }) AfterEach(func() { - // clean up package - pkg := &v1alpha1.Package{ - ObjectMeta: metav1.ObjectMeta{ - Name: testPackageMetaName, - }, - } - Expect(cl.Delete(ctx, pkg)).To(Succeed()) + Expect(cl.DeleteAllOf(ctx, &v1alpha1.Package{})).To(Succeed()) + Expect(cl.DeleteAllOf(ctx, &v1alpha1.BundleMetadata{})).To(Succeed()) - // clean up bundlemetadata - bm := &v1alpha1.BundleMetadata{ - ObjectMeta: metav1.ObjectMeta{ - Name: testBundleMetaName, - }, - } - Expect(cl.Delete(ctx, bm)).To(Succeed()) Expect(features.CatalogdFeatureGate.SetFromMap(map[string]bool{ string(features.PackagesBundleMetadataAPIs): false, string(features.CatalogMetadataAPI): false, @@ -358,8 +347,8 @@ var _ = Describe("Catalogd Controller Test", func() { }, } - tempTestBundleMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, testBundleName) - tempTestPackageMetaName = fmt.Sprintf("%s-%s", tempCatalog.Name, testPackageName) + tempTestBundleMetaName, _ = k8sutil.MetadataName(fmt.Sprintf("%s-%s", tempCatalog.Name, testBundleName)) + tempTestPackageMetaName, _ = k8sutil.MetadataName(fmt.Sprintf("%s-%s", tempCatalog.Name, testPackageName)) Expect(cl.Create(ctx, tempCatalog)).To(Succeed()) res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "tempedout"}})