Skip to content
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

Jump to bottom

✨ Filter out bundle versions lower than installed #711

Merged
merged 1 commit into from
Apr 9, 2024
Merged

✨ Filter out bundle versions lower than installed #711

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
35 changes: 0 additions & 35 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package main

import (
"flag"
"fmt"
"net/http"
"os"
"time"
Expand All @@ -28,13 +27,10 @@ import (
rukpakv1alpha2 "github.com/operator-framework/rukpak/api/v1alpha2"
"github.com/spf13/pflag"
"go.uber.org/zap/zapcore"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/discovery"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
_ "k8s.io/client-go/plugin/pkg/client/auth"
"k8s.io/client-go/rest"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
Expand Down Expand Up @@ -131,16 +127,9 @@ func main() {
os.Exit(1)
}

hasKappApis, err := hasKappApis(mgr.GetConfig())
if err != nil {
setupLog.Error(err, "unable to evaluate if App needs to be created")
os.Exit(1)
}

if err = (&controllers.ExtensionReconciler{
Client: cl,
BundleProvider: catalogClient,
HasKappApis: hasKappApis,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "Extension")
os.Exit(1)
Expand All @@ -162,27 +151,3 @@ func main() {
os.Exit(1)
}
}

// hasKappApis checks whether the cluster has Kapp APIs installed in the cluster.
// This does not guarantee that the controller is present to reconcile the App CRs.
func hasKappApis(config *rest.Config) (bool, error) {
discoveryClient, err := discovery.NewDiscoveryClientForConfig(config)
if err != nil {
return false, fmt.Errorf("creating discovery client: %v", err)
}
apiResourceList, err := discoveryClient.ServerResourcesForGroupVersion(carvelv1alpha1.SchemeGroupVersion.String())
if err != nil && !errors.IsNotFound(err) {
return false, fmt.Errorf("listing resource APIs: %v", err)
}

if apiResourceList == nil {
return false, nil
}

for _, resource := range apiResourceList.APIResources {
if resource.Kind == "App" {
return true, nil
}
}
return false, nil
}
Comment on lines -168 to -188
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was taken from #690 , in which Varsha explained that the check is not needed due to the feature gate.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, why is it in yours? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That PR is undergoing a refactor to combine the tests into a test loop, which is likely going to take some time given its relative priority. This particular change is valuable though since it made doing these tests much easier.

13 changes: 13 additions & 0 deletions internal/catalogmetadata/filter/bundle_predicates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,19 @@ func InMastermindsSemverRange(semverRange *mmsemver.Constraints) Predicate[catal
}
}

func HigherBundleVersion(currentVersion *bsemver.Version) Predicate[catalogmetadata.Bundle] {
Copy link
Member

Choose a reason for hiding this comment

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

This is a good start, but the end state needs to be the same as the current behavior of the ClusterExtension, which is one of:

  1. Semver semantics (if the "force semver" feature gate is enabled)
  2. Replaces, skips, skipRange semantics (which finds other bundles that have skips, replaces, skipRange that include the currently installed bundle)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm not sure ClusterExtension handles skips and skipRange for the second case. But it needs to there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joelanford Coming back to this after a v0 distraction and had a follow-up question here: for the second case, are we looking for bundles that do all three (replaces and skips and skipRange) or any one of the three?

Copy link
Member

@joelanford joelanford Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Any, and then of all the matches, choose the one with the highest semver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @joelanford ! I was just about finished adding the feature gating and replaces/skips/skipsRange then realized it was going to make the PR significantly larger (mostly due to tests). How do you feel about me doing that as a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

+1 on wrapping this and capturing a follow-up issue.

I've implemented this for ClusterExtension here: #743

Given the recent change roadmap updates, I think we should try to wrap up work on Extension API for now, and capture a "current state of the world" of the Extension API and behavior.

return func(bundle *catalogmetadata.Bundle) bool {
if currentVersion == nil {
return false
}
bundleVersion, err := bundle.Version()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, just a though. I wonder if we should validate the bundle version on creation/ingress rather than on egress (i.e. bundle.Version() shouldn't return an error). So, a bundle object is always validated and correct.

Copy link
Member

Choose a reason for hiding this comment

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

I've had this thought too. If it didn't break any assumptions, it would be nice to get something into the declcfg.Bundle type for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes great sense and also would make it a lot easier to grab.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll create a discussion upstream - see what ppl think - or would a ticket suffice?

if err != nil {
return false
}
return bundleVersion.GTE(*currentVersion)
}
}

func InBlangSemverRange(semverRange bsemver.Range) Predicate[catalogmetadata.Bundle] {
return func(bundle *catalogmetadata.Bundle) bool {
bundleVersion, err := bundle.Version()
Expand Down
62 changes: 62 additions & 0 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 @@ -2,6 +2,7 @@ package filter_test

import (
"encoding/json"
"fmt"
"testing"

mmsemver "github.com/Masterminds/semver/v3"
Expand Down Expand Up @@ -62,6 +63,67 @@ func TestInMastermindsSemverRange(t *testing.T) {
assert.False(t, f(b3))
}

func TestHigherBundleVersion(t *testing.T) {
testCases := []struct {
name string
requestedVersion string
comparedVersion string
wantResult bool
}{
{
name: "includes equal version",
requestedVersion: "1.0.0",
comparedVersion: "1.0.0",
wantResult: true,
},
{
name: "filters out older version",
requestedVersion: "1.0.0",
comparedVersion: "0.0.1",
wantResult: false,
},
{
name: "includes higher version",
requestedVersion: "1.0.0",
comparedVersion: "2.0.0",
wantResult: true,
},
{
name: "filters out broken version",
requestedVersion: "1.0.0",
comparedVersion: "broken",
wantResult: false,
},
{
name: "filter returns false with nil version",
requestedVersion: "",
comparedVersion: "1.0.0",
wantResult: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
bundle := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{
Properties: []property.Property{
{
Type: property.TypePackage,
Value: json.RawMessage(fmt.Sprintf(`{"packageName": "package1", "version": "%s"}`, tc.comparedVersion)),
},
},
}}
var version *bsemver.Version
if tc.requestedVersion != "" {
parsedVersion := bsemver.MustParse(tc.requestedVersion)
// to test with nil requested version
version = &parsedVersion
}
f := filter.HigherBundleVersion(version)
assert.Equal(t, tc.wantResult, f(bundle))
})
}
}

func TestInBlangSemverRange(t *testing.T) {
b1 := &catalogmetadata.Bundle{Bundle: declcfg.Bundle{
Properties: []property.Property{
Expand Down
82 changes: 59 additions & 23 deletions internal/controllers/extension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package controllers

import (
"context"
"errors"
"fmt"
"sort"
"strings"
Expand All @@ -31,6 +30,7 @@ import (
kappctrlv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand All @@ -55,10 +55,11 @@ import (
type ExtensionReconciler struct {
client.Client
BundleProvider BundleProvider
HasKappApis bool
}

var errkappAPIUnavailable = errors.New("kapp-controller apis unavailable on cluster")
var (
bundleVersionKey = "olm.operatorframework.io/bundleVersion"
)

//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=extensions,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=extensions/status,verbs=update;patch
Expand Down Expand Up @@ -145,17 +146,6 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
return ctrl.Result{}, nil
}

if !r.HasKappApis {
ext.Status.InstalledBundle = nil
setInstalledStatusConditionFailed(&ext.Status.Conditions, errkappAPIUnavailable.Error(), ext.GetGeneration())

ext.Status.ResolvedBundle = nil
setResolvedStatusConditionUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration())

setDeprecationStatusesUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration())
return ctrl.Result{}, errkappAPIUnavailable
}

// TODO: Improve the resolution logic.
bundle, err := r.resolve(ctx, *ext)
if err != nil {
Expand Down Expand Up @@ -189,7 +179,13 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext
return ctrl.Result{}, nil
}

app := r.GenerateExpectedApp(*ext, bundle.Image)
app, err := r.GenerateExpectedApp(*ext, bundle)
if err != nil {
setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
return ctrl.Result{}, err
}

if err := r.ensureApp(ctx, app); err != nil {
// originally Reason: ocv1alpha1.ReasonInstallationFailed
ext.Status.InstalledBundle = nil
Expand Down Expand Up @@ -402,7 +398,13 @@ func extensionRequestsForCatalog(c client.Reader, logger logr.Logger) handler.Ma
}
}

func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundlePath string) *unstructured.Unstructured {
func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundle *catalogmetadata.Bundle) (*unstructured.Unstructured, error) {
bundleVersion, err := bundle.Version()
if err != nil {
return nil, fmt.Errorf("failed to generate App from Extension %q with bundle %q: %w", o.GetName(), bundle.Name, err)
}
bundlePath := bundle.Image

// We use unstructured here to avoid problems of serializing default values when sending patches to the apiserver.
// If you use a typed object, any default values from that struct get serialized into the JSON patch, which could
// cause unrelated fields to be patched back to the default value even though that isn't the intention. Using an
Expand Down Expand Up @@ -436,6 +438,9 @@ func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundle
"metadata": map[string]interface{}{
"name": o.GetName(),
"namespace": o.GetNamespace(),
"annotations": map[string]string{
bundleVersionKey: bundleVersion.String(),
},
},
"spec": spec,
},
Expand All @@ -451,7 +456,24 @@ func (r *ExtensionReconciler) GenerateExpectedApp(o ocv1alpha1.Extension, bundle
BlockOwnerDeletion: ptr.To(true),
},
})
return app
return app, nil
}

func (r *ExtensionReconciler) getInstalledVersion(ctx context.Context, namespacedName types.NamespacedName) (*bsemver.Version, error) {
existingApp, err := r.existingAppUnstructured(ctx, namespacedName.Name, namespacedName.Namespace)
if err != nil {
return nil, err
}
existingVersion, ok := existingApp.GetAnnotations()[bundleVersionKey]
if !ok {
return nil, fmt.Errorf("existing App %q in Namespace %q missing bundle version", namespacedName.Name, namespacedName.Namespace)
}

existingVersionSemver, err := bsemver.New(existingVersion)
if err != nil {
return nil, fmt.Errorf("could not determine bundle version of existing App %q in Namespace %q: %w", namespacedName.Name, namespacedName.Namespace, err)
}
return existingVersionSemver, nil
}

func (r *ExtensionReconciler) resolve(ctx context.Context, extension ocv1alpha1.Extension) (*catalogmetadata.Bundle, error) {
Expand Down Expand Up @@ -480,19 +502,33 @@ func (r *ExtensionReconciler) resolve(ctx context.Context, extension ocv1alpha1.
predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr))
}

var installedVersion string
// Do not include bundle versions older than currently installed unless UpgradeConstraintPolicy = 'Ignore'
if extension.Spec.Source.Package.UpgradeConstraintPolicy != ocv1alpha1.UpgradeConstraintPolicyIgnore {
dtfranz marked this conversation as resolved.
Show resolved Hide resolved
installedVersionSemver, err := r.getInstalledVersion(ctx, types.NamespacedName{Name: extension.GetName(), Namespace: extension.GetNamespace()})
if err != nil && !apierrors.IsNotFound(err) {
return nil, err
}
if installedVersionSemver != nil {
installedVersion = installedVersionSemver.String()
predicates = append(predicates, catalogfilter.HigherBundleVersion(installedVersionSemver))
}
}

resultSet := catalogfilter.Filter(allBundles, catalogfilter.And(predicates...))

if len(resultSet) == 0 {
if versionRange != "" && channelName != "" {
return nil, fmt.Errorf("no package %q matching version %q found in channel %q", packageName, versionRange, channelName)
}
var versionError, channelError, existingVersionError string
if versionRange != "" {
return nil, fmt.Errorf("no package %q matching version %q found", packageName, versionRange)
versionError = fmt.Sprintf(" matching version %q", versionRange)
}
if channelName != "" {
return nil, fmt.Errorf("no package %q found in channel %q", packageName, channelName)
channelError = fmt.Sprintf(" in channel %q", channelName)
}
if installedVersion != "" {
existingVersionError = fmt.Sprintf(" which upgrades currently installed version %q", installedVersion)
}
return nil, fmt.Errorf("no package %q found", packageName)
return nil, fmt.Errorf("no package %q%s%s%s found", packageName, versionError, channelError, existingVersionError)
}

sort.SliceStable(resultSet, func(i, j int) bool {
Expand Down
Loading
Loading