From 5ea0e2bcdf8e0683f5d2b1e545b6e0faaa78ebd3 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 --- go.mod | 2 + go.sum | 2 + internal/k8sutil/k8sutil.go | 17 +++++ internal/k8sutil/k8sutil_test.go | 62 +++++++++++++++++++ pkg/controllers/core/catalog_controller.go | 35 +++++++++-- .../core/catalog_controller_test.go | 33 ++++------ 6 files changed, 123 insertions(+), 28 deletions(-) create mode 100644 internal/k8sutil/k8sutil.go create mode 100644 internal/k8sutil/k8sutil_test.go diff --git a/go.mod b/go.mod index a9bf77e2..bd971964 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/onsi/gomega v1.27.7 github.com/operator-framework/operator-registry v1.27.1 github.com/spf13/pflag v1.0.5 + github.com/stretchr/testify v1.8.0 k8s.io/api v0.26.1 k8s.io/apimachinery v0.26.1 k8s.io/client-go v0.26.1 @@ -55,6 +56,7 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/operator-framework/api v0.17.4-0.20230223191600-0131a6301e42 // indirect github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_golang v1.14.0 // indirect github.com/prometheus/client_model v0.3.0 // indirect github.com/prometheus/common v0.37.0 // indirect diff --git a/go.sum b/go.sum index b9005601..130830a4 100644 --- a/go.sum +++ b/go.sum @@ -300,6 +300,7 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An github.com/stoewer/go-strcase v1.2.0/go.mod h1:IBiWB2sKIp3wVVQ3Y035++gc+knqhUQag1KpM8ahLw8= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= @@ -308,6 +309,7 @@ github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/xanzy/ssh-agent v0.3.0/go.mod h1:3s9xbODqPuuhK9JV1R321M/FlMZSBvE5aY6eAcqrDh0= github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= diff --git a/internal/k8sutil/k8sutil.go b/internal/k8sutil/k8sutil.go new file mode 100644 index 00000000..dfea1d0d --- /dev/null +++ b/internal/k8sutil/k8sutil.go @@ -0,0 +1,17 @@ +package k8sutil + +import ( + "regexp" + + "k8s.io/apimachinery/pkg/util/validation" +) + +var invalidNameChars = 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 := invalidNameChars.ReplaceAllString(name, "-") + return result, validation.IsDNS1123Subdomain(result) == nil +} diff --git a/internal/k8sutil/k8sutil_test.go b/internal/k8sutil/k8sutil_test.go new file mode 100644 index 00000000..d1b14268 --- /dev/null +++ b/internal/k8sutil/k8sutil_test.go @@ -0,0 +1,62 @@ +package k8sutil + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestMetadataName(t *testing.T) { + type testCase struct { + name string + in string + expectedResult string + expectedValid bool + } + for _, tc := range []testCase{ + { + name: "empty", + in: "", + expectedResult: "", + expectedValid: false, + }, + { + name: "invalid", + in: "foo-bar.123!", + expectedResult: "foo-bar.123-", + expectedValid: false, + }, + { + name: "too long", + in: fmt.Sprintf("foo-bar_%s", strings.Repeat("1234567890", 50)), + expectedResult: fmt.Sprintf("foo-bar-%s", strings.Repeat("1234567890", 50)), + expectedValid: false, + }, + { + name: "valid", + in: "foo-bar.123", + expectedResult: "foo-bar.123", + expectedValid: true, + }, + { + name: "valid with underscore", + in: "foo-bar_123", + expectedResult: "foo-bar-123", + expectedValid: true, + }, + { + name: "valid with colon", + in: "foo-bar:123", + expectedResult: "foo-bar-123", + expectedValid: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + actualResult, actualValid := MetadataName(tc.in) + assert.Equal(t, tc.expectedResult, actualResult) + assert.Equal(t, tc.expectedValid, actualValid) + }) + } +} 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"}})