Skip to content

🌱 OPRUN-4138 ClusterExtensionRevision .spec.revision must be positive #2231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 24, 2025
Merged

🌱 OPRUN-4138 ClusterExtensionRevision .spec.revision must be positive #2231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion api/v1/clusterextensionrevision_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +53 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can validate uniqueness if we require the metadata.name to have a pattern that matches <ceName>-<revNumber>, right?

But I assume there's no way to guarantee "new rev must be previous rev +1"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory you could add a webhook, but I'd like to avoid webhooks as much as possible.
I don't think you can add a validation rule to .metadata.name, but we could add a CEL transition rule where .spec.revision must be equal to -digit suffix of the name. 🤔

But I'd add that in a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could probably do that, but for that we need a validation webhook where we could fetch all existing revisions, sort them by creation date and compare their revisions with the one we got in the payload. Not sure if the effort would be justified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to have a CEL validation rule for metadata.name. You just have to declare the validation against the root struct that embeds ObjectMeta.

We could use ValidationAdmissionPolicy, which is now preferred over webhooks in general, but there's still the problem that when there are multiple apiserver replicas, they are each acting independently when making validation decisions and (I'm pretty sure) without holding a lock or transaction against etcd for all the data used to make the validation decision.

//
// +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.
Expand Down
52 changes: 50 additions & 2 deletions api/v1/clusterextensionrevision_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
{
Expand All @@ -55,6 +58,7 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) {
},
"phases are immutable if not empty": {
spec: ClusterExtensionRevisionSpec{
Revision: 1,
Phases: []ClusterExtensionRevisionPhase{
{
Name: "foo",
Expand Down Expand Up @@ -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)
}
})
}
}
7 changes: 5 additions & 2 deletions ...rator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions manifests/experimental-e2e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions manifests/experimental.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading