Skip to content

Commit

Permalink
Enable CatalogMetadataAPI via explicit flag, fix syncing issues (#138)
Browse files Browse the repository at this point in the history
  • Loading branch information
joelanford committed Aug 14, 2023
1 parent 64a9fe5 commit f1067da
Show file tree
Hide file tree
Showing 7 changed files with 124 additions and 29 deletions.
2 changes: 1 addition & 1 deletion config/default/manager_auth_proxy_patch.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true"

2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand All @@ -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=
Expand Down
17 changes: 17 additions & 0 deletions internal/k8sutil/k8sutil.go
Original file line number Diff line number Diff line change
@@ -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
}
62 changes: 62 additions & 0 deletions internal/k8sutil/k8sutil_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
}
35 changes: 29 additions & 6 deletions pkg/controllers/core/catalog_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
)
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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,
},
}

Expand Down Expand Up @@ -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 != "" {
Expand All @@ -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
}
33 changes: 11 additions & 22 deletions pkg/controllers/core/catalog_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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},
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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"}})
Expand Down

0 comments on commit f1067da

Please sign in to comment.