🐛 Fix: installed bundle provider no longer requires catalog #916
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
The installed bundle provider, which is necessary for safely handling package upgrades, was too strict. It required the currently installed bundle to exist in the catalog in order to "find" the installed bundle. This is problematic in several situations: - If a user receives a catalog update that no longer contains their installed bundle (perhaps its was pulled from the catalog for security or policy reasons) - If a user is trying to transition to a different catalog that provides that package - If a bundle was directly installed, and the user is trying to have a catalog-backed ClusterExtension adopt it. This change simply returns the name and version of the installed bundle, and makes some minor changes in the successors functions to handle the simplified installed bundle metadata.
joelanford
force-pushed
the
installed-bundle
branch
from
b264baa
to
c250d2d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #916 +/- ##
==========================================
+ Coverage 73.54% 73.57% +0.02%
==========================================
Files 18 18
Lines 1221 1211 -10
==========================================
- Hits 898 891 -7
+ Misses 248 246 -2
+ Partials 75 74 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Sounds like a great change, nicely simplifies some sections of code. LGTM after a quick look, but can look more closely later |
| 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If there is validation that the versions must be valid semver then I am fine with failing here
There was a problem hiding this comment.
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:
- Update the predicate API to return
(bool, error). This is a big change because lots of stuff implements the predicate interface. - 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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But perhaps upgradeConstraintPolicy: Ignore is enough of an escape hatch for that situation....
There was a problem hiding this comment.
+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?
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
The installed bundle provider, which is necessary for safely handling package upgrades, was too strict. It required the currently installed bundle to exist in the catalog in order to "find" the installed bundle.
This is problematic in several situations:
This change simply returns the name and version of the installed bundle, and makes some minor changes in the successors functions to handle the simplified installed bundle metadata.
Description
Reviewer Checklist