loosen upgradeable condition to allow z-level upgrades

If current and desired have the same 4.y, then the upgrade is allowed even
if upgradeable==false.  If the 4.y is different, then upgrades are not allowed.
This permits CVE updates and fixes the current service-catalog upgrade problem.

Author: deads2k

pkg/cvo/sync_test.go

@@ -453,7 +453,7 @@ func (pf *testPrecondition) Name() string {
 	return fmt.Sprintf("TestPrecondition SuccessAfter: %d", pf.SuccessAfter)
 }
 
-func (pf *testPrecondition) Run(_ context.Context) error {
+func (pf *testPrecondition) Run(_ context.Context, _ precondition.ReleaseContext) error {
 	if pf.SuccessAfter == 0 {
 		return nil
 	}

pkg/cvo/sync_worker.go

@@ -521,7 +521,7 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in
 				Actual:      update,
 				Verified:    info.Verified,
 			})
-			if err := precondition.Summarize(w.preconditions.RunAll(ctx)); err != nil && !update.Force {
+			if err := precondition.Summarize(w.preconditions.RunAll(ctx, precondition.ReleaseContext{DesiredVersion: payloadUpdate.ReleaseVersion})); err != nil && !update.Force {
 				reporter.Report(SyncWorkerStatus{
 					Generation:  work.Generation,
 					Failure:     err,

pkg/payload/precondition/clusterversion/upgradable_test.go

@@ -0,0 +1,155 @@
+package clusterversion
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/openshift/cluster-version-operator/pkg/payload/precondition"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	configv1 "github.com/openshift/api/config/v1"
+	configv1listers "github.com/openshift/client-go/config/listers/config/v1"
+
+	"k8s.io/client-go/tools/cache"
+)
+
+func TestGetEffectiveMinor(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "empty",
+			input:    "",
+			expected: "",
+		},
+		{
+			name:     "invalid",
+			input:    "something@very-differe",
+			expected: "",
+		},
+		{
+			name:     "multidot",
+			input:    "v4.7.12.3+foo",
+			expected: "7",
+		},
+		{
+			name:     "single",
+			input:    "v4.7",
+			expected: "7",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := getEffectiveMinor(tc.input)
+			if tc.expected != actual {
+				t.Error(actual)
+			}
+		})
+	}
+}
+
+func TestUpgradeableRun(t *testing.T) {
+	ptr := func(status configv1.ConditionStatus) *configv1.ConditionStatus {
+		return &status
+	}
+
+	tests := []struct {
+		name           string
+		upgradeable    *configv1.ConditionStatus
+		currVersion    string
+		desiredVersion string
+		expected       string
+	}{
+		{
+			name:           "first",
+			desiredVersion: "4.2",
+			expected:       "",
+		},
+		{
+			name:           "move-y, no condition",
+			currVersion:    "4.1",
+			desiredVersion: "4.2",
+			expected:       "",
+		},
+		{
+			name:           "move-y, with true condition",
+			upgradeable:    ptr(configv1.ConditionTrue),
+			currVersion:    "4.1",
+			desiredVersion: "4.2",
+			expected:       "",
+		},
+		{
+			name:           "move-y, with unknown condition",
+			upgradeable:    ptr(configv1.ConditionUnknown),
+			currVersion:    "4.1",
+			desiredVersion: "4.2",
+			expected:       "",
+		},
+		{
+			name:           "move-y, with false condition",
+			upgradeable:    ptr(configv1.ConditionFalse),
+			currVersion:    "4.1",
+			desiredVersion: "4.2",
+			expected:       "set to False",
+		},
+		{
+			name:           "move-z, with false condition",
+			upgradeable:    ptr(configv1.ConditionFalse),
+			currVersion:    "4.1.3",
+			desiredVersion: "4.1.4",
+			expected:       "",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			clusterVersion := &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "version"},
+				Spec:       configv1.ClusterVersionSpec{},
+				Status: configv1.ClusterVersionStatus{
+					History: []configv1.UpdateHistory{},
+				},
+			}
+			if len(tc.currVersion) > 0 {
+				clusterVersion.Status.History = append(clusterVersion.Status.History, configv1.UpdateHistory{Version: tc.currVersion})
+			}
+			if tc.upgradeable != nil {
+				clusterVersion.Status.Conditions = append(clusterVersion.Status.Conditions, configv1.ClusterOperatorStatusCondition{
+					Type:    configv1.OperatorUpgradeable,
+					Status:  *tc.upgradeable,
+					Message: fmt.Sprintf("set to %v", *tc.upgradeable),
+				})
+			}
+			cvLister := fakeClusterVersionLister(clusterVersion)
+			instance := NewUpgradeable(cvLister)
+
+			err := instance.Run(context.TODO(), precondition.ReleaseContext{DesiredVersion: tc.desiredVersion})
+			switch {
+			case err != nil && len(tc.expected) == 0:
+				t.Error(err)
+			case err != nil && err.Error() == tc.expected:
+			case err != nil && err.Error() != tc.expected:
+				t.Error(err)
+			case err == nil && len(tc.expected) == 0:
+			case err == nil && len(tc.expected) != 0:
+				t.Error(err)
+			}
+
+		})
+	}
+}
+
+func fakeClusterVersionLister(clusterVersion *configv1.ClusterVersion) configv1listers.ClusterVersionLister {
+	indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+	if clusterVersion == nil {
+		return configv1listers.NewClusterVersionLister(indexer)
+	}
+
+	indexer.Add(clusterVersion)
+	return configv1listers.NewClusterVersionLister(indexer)
+}

pkg/payload/precondition/clusterversion/upgradeable.go

@@ -2,6 +2,7 @@ package clusterversion
 
 import (
 	"context"
+	"strings"
 
 	configv1 "github.com/openshift/api/config/v1"
 	configv1listers "github.com/openshift/client-go/config/listers/config/v1"
@@ -29,7 +30,7 @@ func NewUpgradeable(lister configv1listers.ClusterVersionLister) *Upgradeable {
 // Run runs the Upgradeable precondition.
 // If the feature gate `key` is not found, or the api for clusterversion doesn't exist, this check is inert and always returns nil error.
 // Otherwise, if Upgradeable condition is set to false in the object, it returns an PreconditionError when possible.
-func (pf *Upgradeable) Run(ctx context.Context) error {
+func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.ReleaseContext) error {
 	cv, err := pf.lister.Get(pf.key)
 	if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) {
 		return nil
@@ -42,16 +43,43 @@ func (pf *Upgradeable) Run(ctx context.Context) error {
 			Name:    pf.Name(),
 		}
 	}
-	if up := resourcemerge.FindOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable); up != nil && up.Status == configv1.ConditionFalse {
-		return &precondition.Error{
-			Nested:  err,
-			Reason:  up.Reason,
-			Message: up.Message,
-			Name:    pf.Name(),
-		}
+
+	// if we are upgradeable==true we can always upgrade
+	up := resourcemerge.FindOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable)
+	if up == nil || up.Status != configv1.ConditionFalse {
+		return nil
+	}
+
+	// we can always allow the upgrade if there isn't a version already installed
+	if len(cv.Status.History) == 0 {
+		return nil
+	}
+
+	currentMinor := getEffectiveMinor(cv.Status.History[0].Version)
+	desiredMinor := getEffectiveMinor(releaseContext.DesiredVersion)
+
+	// if there is no difference in the minor version (4.y.z where 4.y is the same for current and desired), then we can still upgrade
+	if currentMinor == desiredMinor {
+		return nil
+	}
+
+	return &precondition.Error{
+		Nested:  err,
+		Reason:  up.Reason,
+		Message: up.Message,
+		Name:    pf.Name(),
 	}
-	return nil
 }
 
 // Name returns Name for the precondition.
 func (pf *Upgradeable) Name() string { return "ClusterVersionUpgradeable" }
+
+// getEffectiveMinor attempts to do a simple parse of the version provided.  If it does not parse, the value is considered
+// empty string, which works for the comparison done here for equivalence.
+func getEffectiveMinor(version string) string {
+	splits := strings.Split(version, ".")
+	if len(splits) < 2 {
+		return ""
+	}
+	return splits[1]
+}

pkg/payload/precondition/precondition.go

@@ -28,10 +28,20 @@ func (e *Error) Cause() error {
 	return e.Nested
 }
 
+// ReleaseContext holds information about the update being considered
+type ReleaseContext struct {
+	// DesiredVersion is the version of the payload being considered.
+	// While this might be a semantic version, consumers should not
+	// require SemVer validity so they can handle custom releases
+	// where the author decided to use a different naming scheme, or
+	// to leave the version completely unset.
+	DesiredVersion string
+}
+
 // Precondition defines the precondition check for a payload.
 type Precondition interface {
 	// Run executes the precondition checks ands returns an error when the precondition fails.
-	Run(context.Context) error
+	Run(ctx context.Context, releaseContext ReleaseContext) error
 
 	// Name returns a human friendly name for the precondition.
 	Name() string
@@ -42,10 +52,10 @@ type List []Precondition
 
 // RunAll runs all the reflight checks in order, returning a list of errors if any.
 // All checks are run, regardless if any one precondition fails.
-func (pfList List) RunAll(ctx context.Context) []error {
+func (pfList List) RunAll(ctx context.Context, releaseContext ReleaseContext) []error {
 	var errs []error
 	for _, pf := range pfList {
-		if err := pf.Run(ctx); err != nil {
+		if err := pf.Run(ctx, releaseContext); err != nil {
 			klog.Errorf("Precondition %q failed: %v", pf.Name(), err)
 			errs = append(errs, err)
 		}