Skip to content

Commit f35e2cf

Browse files
hongkailiuhongkailiu
committed
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)
1 parent 54dc4ef commit f35e2cf

4 files changed

Lines changed: 116 additions & 24 deletions

File tree

‎lib/validation/validation.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
configv1 "github.com/openshift/api/config/v1"
1414
)
1515

16-
func ValidateClusterVersion(config *configv1.ClusterVersion) field.ErrorList {
16+
func ValidateClusterVersion(config *configv1.ClusterVersion, shouldReconcileAcceptRisks bool) field.ErrorList {
1717
errs := apivalidation.ValidateObjectMeta(&config.ObjectMeta, false, apivalidation.NameIsDNS1035Label, nil)
1818

1919
if len(config.Spec.Upstream) > 0 {
@@ -32,8 +32,12 @@ func ValidateClusterVersion(config *configv1.ClusterVersion) field.ErrorList {
3232
}
3333
if u := config.Spec.DesiredUpdate; u != nil {
3434
switch {
35-
case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0:
36-
errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or image"))
35+
case !shouldReconcileAcceptRisks && len(u.AcceptRisks) > 0:
36+
errs = append(errs, field.Forbidden(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified"))
37+
case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && !shouldReconcileAcceptRisks:
38+
errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or acceptRisks"))
39+
case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && len(u.AcceptRisks) == 0 && shouldReconcileAcceptRisks:
40+
errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks"))
3741
case len(u.Version) > 0 && !validSemVer(u.Version):
3842
errs = append(errs, field.Invalid(field.NewPath("spec", "desiredUpdate", "version"), u.Version,
3943
"must be a semantic version (1.2.3[-...])"))

‎lib/validation/validation_test.go‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
package validation
2+
3+
import (
4+
"testing"
5+
6+
"github.com/google/go-cmp/cmp"
7+
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
"k8s.io/apimachinery/pkg/util/validation/field"
10+
11+
configv1 "github.com/openshift/api/config/v1"
12+
)
13+
14+
func TestValidateClusterVersion(t *testing.T) {
15+
tests := []struct {
16+
name string
17+
config *configv1.ClusterVersion
18+
shouldReconcileAcceptRisks bool
19+
expect field.ErrorList
20+
}{
21+
{
22+
name: "allow accept risks if shouldReconcileAcceptRisks is true",
23+
shouldReconcileAcceptRisks: true,
24+
config: &configv1.ClusterVersion{
25+
ObjectMeta: metav1.ObjectMeta{
26+
Name: "version",
27+
},
28+
Spec: configv1.ClusterVersionSpec{
29+
DesiredUpdate: &configv1.Update{
30+
AcceptRisks: []configv1.AcceptRisk{{
31+
Name: "SomeInfrastructureThing",
32+
}, {
33+
Name: "SomeChannelThing",
34+
}},
35+
},
36+
},
37+
},
38+
},
39+
{
40+
name: "forbid accept risks if shouldReconcileAcceptRisks is false",
41+
config: &configv1.ClusterVersion{
42+
ObjectMeta: metav1.ObjectMeta{
43+
Name: "version",
44+
},
45+
Spec: configv1.ClusterVersionSpec{
46+
DesiredUpdate: &configv1.Update{
47+
AcceptRisks: []configv1.AcceptRisk{{
48+
Name: "SomeInfrastructureThing",
49+
}, {
50+
Name: "SomeChannelThing",
51+
}},
52+
},
53+
},
54+
},
55+
expect: append(make(field.ErrorList, 0), field.Forbidden(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified")),
56+
},
57+
{
58+
name: "empty desired update is not allowed if shouldReconcileAcceptRisks is true",
59+
shouldReconcileAcceptRisks: true,
60+
config: &configv1.ClusterVersion{
61+
ObjectMeta: metav1.ObjectMeta{
62+
Name: "version",
63+
},
64+
Spec: configv1.ClusterVersionSpec{DesiredUpdate: &configv1.Update{}},
65+
},
66+
expect: append(make(field.ErrorList, 0), field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks")),
67+
},
68+
{
69+
name: "empty desired update is not allowed if shouldReconcileAcceptRisks is false",
70+
config: &configv1.ClusterVersion{
71+
ObjectMeta: metav1.ObjectMeta{
72+
Name: "version",
73+
},
74+
Spec: configv1.ClusterVersionSpec{DesiredUpdate: &configv1.Update{}},
75+
},
76+
expect: append(make(field.ErrorList, 0), field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or acceptRisks")),
77+
},
78+
}
79+
for _, test := range tests {
80+
t.Run(test.name, func(t *testing.T) {
81+
actual := ValidateClusterVersion(test.config, test.shouldReconcileAcceptRisks)
82+
if diff := cmp.Diff(test.expect, actual); diff != "" {
83+
t.Errorf("(-want, +got): %s", diff)
84+
}
85+
})
86+
}
87+
}

‎pkg/cvo/cvo.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -748,7 +748,7 @@ func (optr *Operator) sync(ctx context.Context, key string) error {
748748
optr.uid = original.UID
749749

750750
// ensure that the object we do have is valid
751-
errs := validation.ValidateClusterVersion(original)
751+
errs := validation.ValidateClusterVersion(original, optr.shouldReconcileAcceptRisks())
752752
// for fields that have meaning that are incomplete, clear them
753753
// prevents us from loading clearly malformed payloads
754754
config := validation.ClearInvalidFields(original, errs)
@@ -839,7 +839,7 @@ func (optr *Operator) upgradeableSyncFunc(ignoreThrottlePeriod bool) func(_ cont
839839
if err != nil {
840840
return err
841841
}
842-
if errs := validation.ValidateClusterVersion(config); len(errs) > 0 {
842+
if errs := validation.ValidateClusterVersion(config, optr.shouldReconcileAcceptRisks()); len(errs) > 0 {
843843
return nil
844844
}
845845

‎pkg/cvo/cvo_test.go‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3453,12 +3453,12 @@ func TestOperator_upgradeableSync(t *testing.T) {
34533453
"ack-4.9-kube-122-api-removals-in-4.9": "Description 2."},
34543454
},
34553455
ackCm: &defaultAckCm, expectedResult: &upgradeable{
3456-
Conditions: []configv1.ClusterOperatorStatusCondition{{
3457-
Type: configv1.OperatorUpgradeable,
3458-
Status: configv1.ConditionFalse,
3459-
Reason: "AdminAckRequired",
3460-
Message: "Description."}},
3461-
},
3456+
Conditions: []configv1.ClusterOperatorStatusCondition{{
3457+
Type: configv1.OperatorUpgradeable,
3458+
Status: configv1.ConditionFalse,
3459+
Reason: "AdminAckRequired",
3460+
Message: "Description."}},
3461+
},
34623462
},
34633463
{
34643464
name: "admin ack required and configmap gate does not have value",
@@ -3497,12 +3497,12 @@ func TestOperator_upgradeableSync(t *testing.T) {
34973497
"ack-4.8-foo": ""},
34983498
},
34993499
ackCm: &defaultAckCm, expectedResult: &upgradeable{
3500-
Conditions: []configv1.ClusterOperatorStatusCondition{{
3501-
Type: configv1.OperatorUpgradeable,
3502-
Status: configv1.ConditionFalse,
3503-
Reason: "MultipleReasons",
3504-
Message: "Description. admin-gates configmap gate ack-4.8-foo must contain a non-empty value."}},
3505-
},
3500+
Conditions: []configv1.ClusterOperatorStatusCondition{{
3501+
Type: configv1.OperatorUpgradeable,
3502+
Status: configv1.ConditionFalse,
3503+
Reason: "MultipleReasons",
3504+
Message: "Description. admin-gates configmap gate ack-4.8-foo must contain a non-empty value."}},
3505+
},
35063506
},
35073507
{
35083508
name: "multiple admin acks required",
@@ -3542,12 +3542,12 @@ func TestOperator_upgradeableSync(t *testing.T) {
35423542
"ack-4.8-bar": "Description 3."},
35433543
},
35443544
ackCm: &defaultAckCm, expectedResult: &upgradeable{
3545-
Conditions: []configv1.ClusterOperatorStatusCondition{{
3546-
Type: configv1.OperatorUpgradeable,
3547-
Status: configv1.ConditionFalse,
3548-
Reason: "AdminAckRequired",
3549-
Message: "Description 1. Description 2. Description 3."}},
3550-
},
3545+
Conditions: []configv1.ClusterOperatorStatusCondition{{
3546+
Type: configv1.OperatorUpgradeable,
3547+
Status: configv1.ConditionFalse,
3548+
Reason: "AdminAckRequired",
3549+
Message: "Description 1. Description 2. Description 3."}},
3550+
},
35513551
},
35523552
{
35533553
name: "admin ack found",
@@ -3845,6 +3845,7 @@ func TestOperator_upgradeableSync(t *testing.T) {
38453845

38463846
optr.upgradeableChecks = optr.defaultUpgradeableChecks()
38473847
optr.eventRecorder = record.NewFakeRecorder(100)
3848+
optr.enabledCVOFeatureGates = featuregates.DefaultCvoGates("version")
38483849

38493850
if tt.gateCm != nil {
38503851
if tt.gateCm.Name == "delete" {
@@ -4055,7 +4056,7 @@ func expectUpdateStatus(t *testing.T, a ktesting.Action, resource, namespace str
40554056
// checkStatus is a generic function used to verify only a portion of a CreateAction as defined by the
40564057
// generic type arguments.
40574058
func checkStatus[S configv1.ClusterVersionCapabilitiesStatus | []configv1.ClusterOperatorStatusCondition | []configv1.UpdateHistory |
4058-
*configv1.ClusterVersion](t *testing.T, a ktesting.Action, verb string, resource, subresource, namespace string, expect S) {
4059+
*configv1.ClusterVersion](t *testing.T, a ktesting.Action, verb string, resource, subresource, namespace string, expect S) {
40594060

40604061
t.Helper()
40614062
if verb != a.GetVerb() {

0 commit comments

Comments
 (0)