Skip to content

Commit 130c987

Browse files
dtfranzdtfranz
authored
ClusterExtensionRevision API Updates (#2491)
Fixing some missing flags and godoc comments brought up via API review. Signed-off-by: Daniel Franz <dfranz@redhat.com>
1 parent 23dd207 commit 130c987

File tree

8 files changed

+173
-38
lines changed

8 files changed

+173
-38
lines changed

‎api/v1/clusterextensionrevision_types.go‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,18 @@ const (
4242
type ClusterExtensionRevisionSpec struct {
4343
// lifecycleState specifies the lifecycle state of the ClusterExtensionRevision.
4444
//
45-
// When set to "Active" (the default), the revision is actively managed and reconciled.
45+
// When set to "Active", the revision is actively managed and reconciled.
4646
// When set to "Archived", the revision is inactive and any resources not managed by a subsequent revision are deleted.
4747
// The revision is removed from the owner list of all objects previously under management.
4848
// All objects that did not transition to a succeeding revision are deleted.
4949
//
5050
// Once a revision is set to "Archived", it cannot be un-archived.
5151
//
52-
// +kubebuilder:default="Active"
52+
// It is possible for more than one revision to be "Active" simultaneously. This will occur when
53+
// moving from one revision to another. The old revision will not be set to "Archived" until the
54+
// new revision has been completely rolled out.
55+
//
56+
// +required
5357
// +kubebuilder:validation:Enum=Active;Archived
5458
// +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Archived' && oldSelf == self", message="cannot un-archive"
5559
LifecycleState ClusterExtensionRevisionLifecycleState `json:"lifecycleState,omitempty"`
@@ -82,7 +86,10 @@ type ClusterExtensionRevisionSpec struct {
8286
//
8387
// Once set, even if empty, the phases field is immutable.
8488
//
89+
// Each phase in the list must have a unique name. The maximum number of phases is 20.
90+
//
8591
// +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf.size() == 0", message="phases is immutable"
92+
// +kubebuilder:validation:MaxItems=20
8693
// +listType=map
8794
// +listMapKey=name
8895
// +optional
@@ -125,13 +132,17 @@ type ClusterExtensionRevisionPhase struct {
125132
//
126133
// [RFC 1123]: https://tools.ietf.org/html/rfc1123
127134
//
135+
// +required
136+
// +kubebuilder:validation:MinLength=1
128137
// +kubebuilder:validation:MaxLength=63
129-
// +kubebuilder:validation:Pattern=`^[a-z]([-a-z0-9]*[a-z0-9])?$`
138+
// +kubebuilder:validation:XValidation:rule=`!format.dns1123Label().validate(self).hasValue()`,message="the value must consist of only lowercase alphanumeric characters and hyphens, and must start with an alphabetic character and end with an alphanumeric character."
130139
Name string `json:"name"`
131140

132141
// objects is a required list of all Kubernetes objects that belong to this phase.
133142
//
134-
// All objects in this list are applied to the cluster in no particular order.
143+
// All objects in this list are applied to the cluster in no particular order. The maximum number of objects per phase is 50.
144+
// +required
145+
// +kubebuilder:validation:MaxItems=50
135146
Objects []ClusterExtensionRevisionObject `json:"objects"`
136147
}
137148

@@ -149,7 +160,9 @@ type ClusterExtensionRevisionObject struct {
149160
// collisionProtection controls whether the operator can adopt and modify objects
150161
// that already exist on the cluster.
151162
//
152-
// When set to "Prevent" (the default), the operator only manages objects it created itself.
163+
// Allowed values are: "Prevent", "IfNoController", and "None".
164+
//
165+
// When set to "Prevent", the operator only manages objects it created itself.
153166
// This prevents ownership collisions.
154167
//
155168
// When set to "IfNoController", the operator can adopt and modify pre-existing objects
@@ -161,9 +174,8 @@ type ClusterExtensionRevisionObject struct {
161174
// Use this setting with extreme caution as it may cause multiple controllers to fight over
162175
// the same resource, resulting in increased load on the API server and etcd.
163176
//
164-
// +kubebuilder:default="Prevent"
177+
// +required
165178
// +kubebuilder:validation:Enum=Prevent;IfNoController;None
166-
// +optional
167179
CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"`
168180
}
169181

‎api/v1/clusterextensionrevision_types_test.go‎

Lines changed: 79 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,18 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
2121
}{
2222
"revision is immutable": {
2323
spec: ClusterExtensionRevisionSpec{
24-
Revision: 1,
24+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
25+
Revision: 1,
2526
},
2627
updateFunc: func(cer *ClusterExtensionRevision) {
2728
cer.Spec.Revision = 2
2829
},
2930
},
3031
"phases may be initially empty": {
3132
spec: ClusterExtensionRevisionSpec{
32-
Revision: 1,
33-
Phases: []ClusterExtensionRevisionPhase{},
33+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
34+
Revision: 1,
35+
Phases: []ClusterExtensionRevisionPhase{},
3436
},
3537
updateFunc: func(cer *ClusterExtensionRevision) {
3638
cer.Spec.Phases = []ClusterExtensionRevisionPhase{
@@ -44,7 +46,8 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
4446
},
4547
"phases may be initially unset": {
4648
spec: ClusterExtensionRevisionSpec{
47-
Revision: 1,
49+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
50+
Revision: 1,
4851
},
4952
updateFunc: func(cer *ClusterExtensionRevision) {
5053
cer.Spec.Phases = []ClusterExtensionRevisionPhase{
@@ -58,7 +61,8 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
5861
},
5962
"phases are immutable if not empty": {
6063
spec: ClusterExtensionRevisionSpec{
61-
Revision: 1,
64+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
65+
Revision: 1,
6266
Phases: []ClusterExtensionRevisionPhase{
6367
{
6468
Name: "foo",
@@ -107,20 +111,87 @@ func TestClusterExtensionRevisionValidity(t *testing.T) {
107111
}{
108112
"revision cannot be negative": {
109113
spec: ClusterExtensionRevisionSpec{
110-
Revision: -1,
114+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
115+
Revision: -1,
111116
},
112117
valid: false,
113118
},
114119
"revision cannot be zero": {
115-
spec: ClusterExtensionRevisionSpec{},
120+
spec: ClusterExtensionRevisionSpec{
121+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
122+
},
116123
valid: false,
117124
},
118125
"revision must be positive": {
119126
spec: ClusterExtensionRevisionSpec{
120-
Revision: 1,
127+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
128+
Revision: 1,
121129
},
122130
valid: true,
123131
},
132+
"lifecycleState must be set": {
133+
spec: ClusterExtensionRevisionSpec{
134+
Revision: 1,
135+
},
136+
valid: false,
137+
},
138+
"phases must have no more than 20 phases": {
139+
spec: ClusterExtensionRevisionSpec{
140+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
141+
Revision: 1,
142+
Phases: make([]ClusterExtensionRevisionPhase, 21),
143+
},
144+
valid: false,
145+
},
146+
"phases entries must have no more than 50 objects": {
147+
spec: ClusterExtensionRevisionSpec{
148+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
149+
Revision: 1,
150+
Phases: []ClusterExtensionRevisionPhase{
151+
{
152+
Name: "too-many-objects",
153+
Objects: make([]ClusterExtensionRevisionObject, 51),
154+
},
155+
},
156+
},
157+
valid: false,
158+
},
159+
"phases entry names cannot be empty": {
160+
spec: ClusterExtensionRevisionSpec{
161+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
162+
Revision: 1,
163+
Phases: []ClusterExtensionRevisionPhase{
164+
{
165+
Name: "",
166+
},
167+
},
168+
},
169+
valid: false,
170+
},
171+
"phases entry names cannot start with symbols": {
172+
spec: ClusterExtensionRevisionSpec{
173+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
174+
Revision: 1,
175+
Phases: []ClusterExtensionRevisionPhase{
176+
{
177+
Name: "-invalid",
178+
},
179+
},
180+
},
181+
valid: false,
182+
},
183+
"phases entry names cannot start with numeric characters": {
184+
spec: ClusterExtensionRevisionSpec{
185+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
186+
Revision: 1,
187+
Phases: []ClusterExtensionRevisionPhase{
188+
{
189+
Name: "1-invalid",
190+
},
191+
},
192+
},
193+
valid: false,
194+
},
124195
} {
125196
t.Run(name, func(t *testing.T) {
126197
cer := &ClusterExtensionRevision{

‎api/v1/validation_test.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ func TestValidate(t *testing.T) {
9191
"ClusterExtensionRevision: invalid progress deadline < 10": {
9292
args: args{
9393
object: ClusterExtensionRevisionSpec{
94+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
9495
ProgressDeadlineMinutes: 9,
9596
},
9697
},
@@ -99,6 +100,7 @@ func TestValidate(t *testing.T) {
99100
"ClusterExtensionRevision: valid progress deadline = 10": {
100101
args: args{
101102
object: ClusterExtensionRevisionSpec{
103+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
102104
ProgressDeadlineMinutes: 10,
103105
},
104106
},
@@ -107,6 +109,7 @@ func TestValidate(t *testing.T) {
107109
"ClusterExtensionRevision: valid progress deadline = 360": {
108110
args: args{
109111
object: ClusterExtensionRevisionSpec{
112+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
110113
ProgressDeadlineMinutes: 360,
111114
},
112115
},
@@ -115,6 +118,7 @@ func TestValidate(t *testing.T) {
115118
"ClusterExtensionRevision: valid progress deadline = 720": {
116119
args: args{
117120
object: ClusterExtensionRevisionSpec{
121+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
118122
ProgressDeadlineMinutes: 720,
119123
},
120124
},
@@ -123,14 +127,17 @@ func TestValidate(t *testing.T) {
123127
"ClusterExtensionRevision: invalid progress deadline > 720": {
124128
args: args{
125129
object: ClusterExtensionRevisionSpec{
130+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
126131
ProgressDeadlineMinutes: 721,
127132
},
128133
},
129134
want: want{valid: false},
130135
},
131136
"ClusterExtensionRevision: no progress deadline set": {
132137
args: args{
133-
object: ClusterExtensionRevisionSpec{},
138+
object: ClusterExtensionRevisionSpec{
139+
LifecycleState: ClusterExtensionRevisionLifecycleStateActive,
140+
},
134141
},
135142
want: want{valid: true},
136143
},

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

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,16 +57,19 @@ spec:
5757
description: spec defines the desired state of the ClusterExtensionRevision.
5858
properties:
5959
lifecycleState:
60-
default: Active
6160
description: |-
6261
lifecycleState specifies the lifecycle state of the ClusterExtensionRevision.
6362
64-
When set to "Active" (the default), the revision is actively managed and reconciled.
63+
When set to "Active", the revision is actively managed and reconciled.
6564
When set to "Archived", the revision is inactive and any resources not managed by a subsequent revision are deleted.
6665
The revision is removed from the owner list of all objects previously under management.
6766
All objects that did not transition to a succeeding revision are deleted.
6867
6968
Once a revision is set to "Archived", it cannot be un-archived.
69+
70+
It is possible for more than one revision to be "Active" simultaneously. This will occur when
71+
moving from one revision to another. The old revision will not be set to "Archived" until the
72+
new revision has been completely rolled out.
7073
enum:
7174
- Active
7275
- Archived
@@ -92,6 +95,8 @@ spec:
9295
The revision progresses to the next phase only after all objects in the current phase pass their readiness probes.
9396
9497
Once set, even if empty, the phases field is immutable.
98+
99+
Each phase in the list must have a unique name. The maximum number of phases is 20.
95100
items:
96101
description: |-
97102
ClusterExtensionRevisionPhase represents a group of objects that are applied together. The phase is considered
@@ -109,25 +114,31 @@ spec:
109114
110115
[RFC 1123]: https://tools.ietf.org/html/rfc1123
111116
maxLength: 63
112-
pattern: ^[a-z]([-a-z0-9]*[a-z0-9])?$
117+
minLength: 1
113118
type: string
119+
x-kubernetes-validations:
120+
- message: the value must consist of only lowercase alphanumeric
121+
characters and hyphens, and must start with an alphabetic
122+
character and end with an alphanumeric character.
123+
rule: '!format.dns1123Label().validate(self).hasValue()'
114124
objects:
115125
description: |-
116126
objects is a required list of all Kubernetes objects that belong to this phase.
117127
118-
All objects in this list are applied to the cluster in no particular order.
128+
All objects in this list are applied to the cluster in no particular order. The maximum number of objects per phase is 50.
119129
items:
120130
description: |-
121131
ClusterExtensionRevisionObject represents a Kubernetes object to be applied as part
122132
of a phase, along with its collision protection settings.
123133
properties:
124134
collisionProtection:
125-
default: Prevent
126135
description: |-
127136
collisionProtection controls whether the operator can adopt and modify objects
128137
that already exist on the cluster.
129138
130-
When set to "Prevent" (the default), the operator only manages objects it created itself.
139+
Allowed values are: "Prevent", "IfNoController", and "None".
140+
141+
When set to "Prevent", the operator only manages objects it created itself.
131142
This prevents ownership collisions.
132143
133144
When set to "IfNoController", the operator can adopt and modify pre-existing objects
@@ -152,13 +163,16 @@ spec:
152163
x-kubernetes-embedded-resource: true
153164
x-kubernetes-preserve-unknown-fields: true
154165
required:
166+
- collisionProtection
155167
- object
156168
type: object
169+
maxItems: 50
157170
type: array
158171
required:
159172
- name
160173
- objects
161174
type: object
175+
maxItems: 20
162176
type: array
163177
x-kubernetes-list-map-keys:
164178
- name
@@ -191,6 +205,7 @@ spec:
191205
- message: revision is immutable
192206
rule: self == oldSelf
193207
required:
208+
- lifecycleState
194209
- revision
195210
type: object
196211
status:

‎internal/operator-controller/applier/boxcutter.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,8 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
147147
sanitizedUnstructured(ctx, &unstr)
148148

149149
objs = append(objs, ocv1.ClusterExtensionRevisionObject{
150-
Object: unstr,
150+
Object: unstr,
151+
CollisionProtection: ocv1.CollisionProtectionPrevent,
151152
})
152153
}
153154
return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil
@@ -215,9 +216,6 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
215216
},
216217
},
217218
Spec: ocv1.ClusterExtensionRevisionSpec{
218-
// Explicitly set LifecycleState to Active. While the CRD has a default,
219-
// being explicit here ensures all code paths are clear and doesn't rely
220-
// on API server defaulting behavior.
221219
LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
222220
Phases: PhaseSort(objects),
223221
},

‎internal/operator-controller/applier/boxcutter_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
231231
"spec": map[string]interface{}{},
232232
},
233233
},
234+
CollisionProtection: ocv1.CollisionProtectionPrevent,
234235
},
235236
{
236237
Object: unstructured.Unstructured{
@@ -259,6 +260,7 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
259260
},
260261
},
261262
},
263+
CollisionProtection: ocv1.CollisionProtectionPrevent,
262264
},
263265
},
264266
},

0 commit comments

Comments
 (0)