ClusterExtensionRevision .spec.revision must be positive (#2231)

Added the validation rules and the unit tests.

Author: pedjak

api/v1/clusterextensionrevision_types.go

@@ -49,9 +49,12 @@ type ClusterExtensionRevisionSpec struct {
 	// +kubebuilder:validation:Enum=Active;Paused;Archived
 	// +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Paused' || oldSelf == 'Archived' && oldSelf == self", message="can not un-archive"
 	LifecycleState ClusterExtensionRevisionLifecycleState `json:"lifecycleState,omitempty"`
-	// Revision number orders changes over time, must always be previous revision +1.
+	// Revision is a sequence number representing a specific revision of the ClusterExtension instance.
+	// Must be positive. Each ClusterExtensionRevision of the same parent ClusterExtension needs to have
+	// a unique value assigned. It is immutable after creation. The new revision number must always be previous revision +1.
 	//
 	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:Minimum:=1
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="revision is immutable"
 	Revision int64 `json:"revision"`
 	// Phases are groups of objects that will be applied at the same time.

api/v1/clusterextensionrevision_types_test.go

@@ -29,7 +29,8 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
 		},
 		"phases may be initially empty": {
 			spec: ClusterExtensionRevisionSpec{
-				Phases: []ClusterExtensionRevisionPhase{},
+				Revision: 1,
+				Phases:   []ClusterExtensionRevisionPhase{},
 			},
 			updateFunc: func(cer *ClusterExtensionRevision) {
 				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
@@ -42,7 +43,9 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
 			allowed: true,
 		},
 		"phases may be initially unset": {
-			spec: ClusterExtensionRevisionSpec{},
+			spec: ClusterExtensionRevisionSpec{
+				Revision: 1,
+			},
 			updateFunc: func(cer *ClusterExtensionRevision) {
 				cer.Spec.Phases = []ClusterExtensionRevisionPhase{
 					{
@@ -55,6 +58,7 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
 		},
 		"phases are immutable if not empty": {
 			spec: ClusterExtensionRevisionSpec{
+				Revision: 1,
 				Phases: []ClusterExtensionRevisionPhase{
 					{
 						Name:    "foo",
@@ -92,3 +96,47 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
 		})
 	}
 }
+
+func TestClusterExtensionRevisionValidity(t *testing.T) {
+	c := newClient(t)
+	ctx := context.Background()
+	i := 0
+	for name, tc := range map[string]struct {
+		spec  ClusterExtensionRevisionSpec
+		valid bool
+	}{
+		"revision cannot be negative": {
+			spec: ClusterExtensionRevisionSpec{
+				Revision: -1,
+			},
+			valid: false,
+		},
+		"revision cannot be zero": {
+			spec:  ClusterExtensionRevisionSpec{},
+			valid: false,
+		},
+		"revision must be positive": {
+			spec: ClusterExtensionRevisionSpec{
+				Revision: 1,
+			},
+			valid: true,
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			cer := &ClusterExtensionRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf("bar%d", i),
+				},
+				Spec: tc.spec,
+			}
+			i = i + 1
+			err := c.Create(ctx, cer)
+			if tc.valid && err != nil {
+				t.Fatal("expected create to succeed, but got:", err)
+			}
+			if !tc.valid && !errors.IsInvalid(err) {
+				t.Fatal("expected create to fail due to invalid payload, but got:", err)
+			}
+		})
+	}
+}

helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml

@@ -122,9 +122,12 @@ spec:
                 - message: previous is immutable
                   rule: self == oldSelf
               revision:
-                description: Revision number orders changes over time, must always
-                  be previous revision +1.
+                description: |-
+                  Revision is a sequence number representing a specific revision of the ClusterExtension instance.
+                  Must be positive. Each ClusterExtensionRevision of the same parent ClusterExtension needs to have
+                  a unique value assigned. It is immutable after creation. The new revision number must always be previous revision +1.
                 format: int64
+                minimum: 1
                 type: integer
                 x-kubernetes-validations:
                 - message: revision is immutable

manifests/experimental-e2e.yaml

@@ -713,9 +713,12 @@ spec:
                 - message: previous is immutable
                   rule: self == oldSelf
               revision:
-                description: Revision number orders changes over time, must always
-                  be previous revision +1.
+                description: |-
+                  Revision is a sequence number representing a specific revision of the ClusterExtension instance.
+                  Must be positive. Each ClusterExtensionRevision of the same parent ClusterExtension needs to have
+                  a unique value assigned. It is immutable after creation. The new revision number must always be previous revision +1.
                 format: int64
+                minimum: 1
                 type: integer
                 x-kubernetes-validations:
                 - message: revision is immutable

manifests/experimental.yaml

@@ -678,9 +678,12 @@ spec:
                 - message: previous is immutable
                   rule: self == oldSelf
               revision:
-                description: Revision number orders changes over time, must always
-                  be previous revision +1.
+                description: |-
+                  Revision is a sequence number representing a specific revision of the ClusterExtension instance.
+                  Must be positive. Each ClusterExtensionRevision of the same parent ClusterExtension needs to have
+                  a unique value assigned. It is immutable after creation. The new revision number must always be previous revision +1.
                 format: int64
+                minimum: 1
                 type: integer
                 x-kubernetes-validations:
                 - message: revision is immutable