Allow for risks only desired update

This is to address the issue from [1].

We need to allow for n desired update with
accept risks and without image, version and
arch.

[1]. #1318 (comment)

Author: hongkailiu

lib/validation/validation.go

@@ -13,7 +13,7 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 )
 
-func ValidateClusterVersion(config *configv1.ClusterVersion) field.ErrorList {
+func ValidateClusterVersion(config *configv1.ClusterVersion, shouldReconcileAcceptRisks bool) field.ErrorList {
 	errs := apivalidation.ValidateObjectMeta(&config.ObjectMeta, false, apivalidation.NameIsDNS1035Label, nil)
 
 	if len(config.Spec.Upstream) > 0 {
@@ -32,8 +32,12 @@ func ValidateClusterVersion(config *configv1.ClusterVersion) field.ErrorList {
 	}
 	if u := config.Spec.DesiredUpdate; u != nil {
 		switch {
-		case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0:
+		case !shouldReconcileAcceptRisks && len(u.AcceptRisks) > 0:
+			errs = append(errs, field.Forbidden(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified"))
+		case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && !shouldReconcileAcceptRisks:
 			errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or image"))
+		case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && len(u.AcceptRisks) == 0 && shouldReconcileAcceptRisks:
+			errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks"))
 		case len(u.Version) > 0 && !validSemVer(u.Version):
 			errs = append(errs, field.Invalid(field.NewPath("spec", "desiredUpdate", "version"), u.Version,
 				"must be a semantic version (1.2.3[-...])"))

lib/validation/validation_test.go

@@ -0,0 +1,87 @@
+package validation
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	configv1 "github.com/openshift/api/config/v1"
+)
+
+func TestValidateClusterVersion(t *testing.T) {
+	tests := []struct {
+		name                       string
+		config                     *configv1.ClusterVersion
+		shouldReconcileAcceptRisks bool
+		expect                     field.ErrorList
+	}{
+		{
+			name:                       "allow accept risks if shouldReconcileAcceptRisks is true",
+			shouldReconcileAcceptRisks: true,
+			config: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "version",
+				},
+				Spec: configv1.ClusterVersionSpec{
+					DesiredUpdate: &configv1.Update{
+						AcceptRisks: []configv1.AcceptRisk{{
+							Name: "SomeInfrastructureThing",
+						}, {
+							Name: "SomeChannelThing",
+						}},
+					},
+				},
+			},
+		},
+		{
+			name: "forbid accept risks if shouldReconcileAcceptRisks is false",
+			config: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "version",
+				},
+				Spec: configv1.ClusterVersionSpec{
+					DesiredUpdate: &configv1.Update{
+						AcceptRisks: []configv1.AcceptRisk{{
+							Name: "SomeInfrastructureThing",
+						}, {
+							Name: "SomeChannelThing",
+						}},
+					},
+				},
+			},
+			expect: append(make(field.ErrorList, 0), field.Forbidden(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified")),
+		},
+		{
+			name:                       "empty desired update is not allowed if shouldReconcileAcceptRisks is true",
+			shouldReconcileAcceptRisks: true,
+			config: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "version",
+				},
+				Spec: configv1.ClusterVersionSpec{DesiredUpdate: &configv1.Update{}},
+			},
+			expect: append(make(field.ErrorList, 0), field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks")),
+		},
+		{
+			name: "empty desired update is not allowed if shouldReconcileAcceptRisks is false",
+			config: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "version",
+				},
+				Spec: configv1.ClusterVersionSpec{DesiredUpdate: &configv1.Update{}},
+			},
+			expect: append(make(field.ErrorList, 0), field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or image")),
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			actual := ValidateClusterVersion(test.config, test.shouldReconcileAcceptRisks)
+			if diff := cmp.Diff(test.expect, actual); diff != "" {
+				t.Errorf("(-want, +got): %s", diff)
+			}
+		})
+	}
+}

pkg/cvo/cvo.go

@@ -748,7 +748,7 @@ func (optr *Operator) sync(ctx context.Context, key string) error {
 	optr.uid = original.UID
 
 	// ensure that the object we do have is valid
-	errs := validation.ValidateClusterVersion(original)
+	errs := validation.ValidateClusterVersion(original, optr.shouldReconcileAcceptRisks())
 	// for fields that have meaning that are incomplete, clear them
 	// prevents us from loading clearly malformed payloads
 	config := validation.ClearInvalidFields(original, errs)
@@ -839,7 +839,7 @@ func (optr *Operator) upgradeableSyncFunc(ignoreThrottlePeriod bool) func(_ cont
 		if err != nil {
 			return err
 		}
-		if errs := validation.ValidateClusterVersion(config); len(errs) > 0 {
+		if errs := validation.ValidateClusterVersion(config, optr.shouldReconcileAcceptRisks()); len(errs) > 0 {
 			return nil
 		}
 

pkg/cvo/cvo_test.go

@@ -3845,6 +3845,7 @@ func TestOperator_upgradeableSync(t *testing.T) {
 
 				optr.upgradeableChecks = optr.defaultUpgradeableChecks()
 				optr.eventRecorder = record.NewFakeRecorder(100)
+				optr.enabledCVOFeatureGates = featuregates.DefaultCvoGates("version")
 
 				if tt.gateCm != nil {
 					if tt.gateCm.Name == "delete" {