Skip to content

🐛 Fix: installed bundle provider no longer requires catalog #916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2024
Merged

🐛 Fix: installed bundle provider no longer requires catalog #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func main() {
Unpacker: unpacker,
Storage: localStorage,
Handler: handler.HandlerFunc(handler.HandleClusterExtension),
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{},
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
os.Exit(1)
Expand Down
9 changes: 5 additions & 4 deletions internal/catalogmetadata/filter/bundle_predicates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/operator-framework/operator-registry/alpha/declcfg"

ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/catalogmetadata"
)

Expand Down Expand Up @@ -66,7 +67,7 @@ func WithBundleName(bundleName string) Predicate[catalogmetadata.Bundle] {
}
}

func LegacySuccessor(installedBundle *catalogmetadata.Bundle) Predicate[catalogmetadata.Bundle] {
func LegacySuccessor(installedBundle *ocv1alpha1.BundleMetadata) Predicate[catalogmetadata.Bundle] {
isSuccessor := func(candidateBundleEntry declcfg.ChannelEntry) bool {
if candidateBundleEntry.Replaces == installedBundle.Name {
return true
Expand All @@ -77,9 +78,9 @@ func LegacySuccessor(installedBundle *catalogmetadata.Bundle) Predicate[catalogm
}
}
if candidateBundleEntry.SkipRange != "" {
installedBundleVersion, _ := installedBundle.Version()
skipRange, _ := bsemver.ParseRange(candidateBundleEntry.SkipRange)
if installedBundleVersion != nil && skipRange != nil && skipRange(*installedBundleVersion) {
installedBundleVersion, vErr := bsemver.Parse(installedBundle.Version)
skipRange, srErr := bsemver.ParseRange(candidateBundleEntry.SkipRange)
if vErr == nil && srErr == nil && skipRange(installedBundleVersion) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the before and after code just silently ignore edges if the installed bundle version or the skipRange cannot be parsed.

I'm tempted to actually fail here if the bundle version or skip range fail to parse.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the impact to non-semver versions if we fail here? Do we know how many packages use versioning schemes incompatible with semver in the commonly used catalogs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure FBC requires valid semver and opm validate checks both places. So in theory, this should never fail, and if it does, it is because someone has invalid FBC.

Which is why I think I'd rather just fail here. If we ignore it, it's this sorta weird fence-straddling position of accepting it, but then not doing anything with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is validation that the versions must be valid semver then I am fine with failing here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it would be a significant change to bubble errors up through the predicate API. Probably better to handle this in a follow-up. I think we should do one of the following:

  1. Update the predicate API to return (bool, error). This is a big change because lots of stuff implements the predicate interface.
  2. Update the bundle-related predicates to use a types where the bundle version and channel skipRanges are pre-parsed. That way we eliminate the calls inside the predicate that could return an error.

return true
}
}
Expand Down
11 changes: 4 additions & 7 deletions internal/catalogmetadata/filter/bundle_predicates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/operator-framework/operator-registry/alpha/declcfg"
"github.com/operator-framework/operator-registry/alpha/property"

ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/catalogmetadata"
"github.com/operator-framework/operator-controller/internal/catalogmetadata/filter"
)
Expand Down Expand Up @@ -150,13 +151,9 @@ func TestLegacySuccessor(t *testing.T) {
},
},
}
installedBundle := &catalogmetadata.Bundle{
Bundle: declcfg.Bundle{
Name: "package1.v0.0.1",
Properties: []property.Property{
{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "0.0.1"}`)},
},
},
installedBundle := &ocv1alpha1.BundleMetadata{
Name: "package1.v0.0.1",
Version: "0.0.1",
}

b2 := &catalogmetadata.Bundle{
Expand Down
44 changes: 15 additions & 29 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ type ClusterExtensionReconciler struct {
}

type InstalledBundleGetter interface {
GetInstalledBundle(ctx context.Context, acg helmclient.ActionClientGetter, allBundles []*catalogmetadata.Bundle, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error)
GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error)
}

//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions,verbs=get;list;watch
Expand Down Expand Up @@ -370,7 +370,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1
channelName := ext.Spec.Channel
versionRange := ext.Spec.Version

installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, r.ActionClientGetter, allBundles, &ext)
installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, &ext)
if err != nil {
return nil, err
}
Expand All @@ -392,7 +392,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1
}

if ext.Spec.UpgradeConstraintPolicy != ocv1alpha1.UpgradeConstraintPolicyIgnore && installedBundle != nil {
upgradePredicate, err := SuccessorsPredicate(installedBundle)
upgradePredicate, err := SuccessorsPredicate(ext.Spec.PackageName, installedBundle)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be covered separately/later, but I wonder if the successors predicate should actually care about the package name?

I could envision a separate predicate that gets to decide which package names are acceptable to be upgraded from, where the default would be what it is now (i.e. "spec.packageName").

Something like that may make it easier for us if we wanted to help users do an upgrade between bundles that come from a different package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But perhaps upgradeConstraintPolicy: Ignore is enough of an escape hatch for that situation....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on covering later. I think there is some additional nuance here of deciding the ordering of bundles between different packages. How would you decide between a newer version of a bundle in the same package versus a different one?

I think for now the current escape hatch should work?

if err != nil {
return nil, err
}
Expand All @@ -404,7 +404,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1

var upgradeErrorPrefix string
if installedBundle != nil {
installedBundleVersion, err := installedBundle.Version()
installedBundleVersion, err := mmsemver.NewVersion(installedBundle.Version)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -568,6 +568,7 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
r.controller = controller
r.cache = mgr.GetCache()
r.dynamicWatchGVKs = map[schema.GroupVersionKind]struct{}{}

return nil
}

Expand Down Expand Up @@ -663,10 +664,12 @@ func (r *ClusterExtensionReconciler) getReleaseState(cl helmclient.ActionInterfa
return currentRelease, stateUnchanged, nil
}

type DefaultInstalledBundleGetter struct{}
type DefaultInstalledBundleGetter struct {
helmclient.ActionClientGetter
}

func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, acg helmclient.ActionClientGetter, allBundles []*catalogmetadata.Bundle, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) {
cl, err := acg.ActionClientFor(ctx, ext)
func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) {
cl, err := d.ActionClientFor(ctx, ext)
if err != nil {
return nil, err
}
Expand All @@ -679,27 +682,10 @@ func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, a
return nil, nil
}

// Bundle must match installed version exactly
vr, err := mmsemver.NewConstraint(release.Labels[labels.BundleVersionKey])
if err != nil {
return nil, err
}

// find corresponding bundle for the installed content
resultSet := catalogfilter.Filter(allBundles, catalogfilter.And(
catalogfilter.WithPackageName(release.Labels[labels.PackageNameKey]),
catalogfilter.WithBundleName(release.Labels[labels.BundleNameKey]),
catalogfilter.InMastermindsSemverRange(vr),
))
if len(resultSet) == 0 {
return nil, fmt.Errorf("bundle %q for package %q not found in available catalogs but is currently installed in namespace %q", release.Labels[labels.BundleNameKey], ext.Spec.PackageName, release.Namespace)
}

sort.SliceStable(resultSet, func(i, j int) bool {
return catalogsort.ByVersion(resultSet[i], resultSet[j])
})

return resultSet[0], nil
return &ocv1alpha1.BundleMetadata{
Name: release.Labels[labels.BundleNameKey],
Version: release.Labels[labels.BundleVersionKey],
}, nil
}

type postrenderer struct {
Expand Down Expand Up @@ -749,7 +735,7 @@ func bundleMetadataFor(bundle *catalogmetadata.Bundle) *ocv1alpha1.BundleMetadat
}

func (r *ClusterExtensionReconciler) validateBundle(bundle *catalogmetadata.Bundle) error {
unsupportedProps := sets.New(
unsupportedProps := sets.New[string](
property.TypePackageRequired,
property.TypeGVKRequired,
property.TypeConstraint,
Expand Down
Loading
Loading