✨ OPRUN-4221: Use operator-controller SA by default, make SA field optional #2333
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Changes the ClusterExtension API field spec.ServiceAccount to be optional. Operator-controller will use its own service account by default unless the spec.ServiceAccount field is set. RBAC PreAuthorization only happens if the optional SA field is set, as well. Give operator-controller's SA cluster-admin by default.
trgeiger
force-pushed
the
sa-optional
branch
from
920432f to
435dc41
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2333 +/- ##
==========================================
- Coverage 74.42% 74.28% -0.14%
==========================================
Files 91 91
Lines 7057 7085 +28
==========================================
+ Hits 5252 5263 +11
- Misses 1393 1406 +13
- Partials 412 416 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
trgeiger
changed the title
|
Due to the changes to the clusterrole, I don't think this is ever going to pass upgrade-e2e since it can't change the clusterrole in place. What's the cleanest way of handling this, versioning our clusterrole moving forward? Just make a new one like the existing boxcutter experimental config does? |
trgeiger
changed the title
| // [RFC 1123]: https://tools.ietf.org/html/rfc1123 | ||
| // | ||
| // +kubebuilder:validation:MaxLength:=253 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" |
There was a problem hiding this comment.
Should we allow a CE to unset the SA now that it is optional? Open question. I think we need to discuss.
|
That's a great question. Does it make sense to hash that out in the RFC?
…On Tue, Nov 18, 2025, 4:37 PM Joe Lanford ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In api/v1/clusterextension_types.go
<#2333 (comment)>
:
> @@ -403,7 +403,7 @@ type ServiceAccountReference struct {
// +kubebuilder:validation:MaxLength:=253
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable"
Should we allow a CE to unset the SA now that it is optional? Open
question. I think we need to discuss.
—
Reply to this email directly, view it on GitHub
<#2333 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AENY3BO7TJJNPRRO5ZKIQKD35ONRNAVCNFSM6AAAAACMBQXGOWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTINZZHE2DINRYGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***
.com>
|
|
We need to add an e2e that checks that we can correctly install a ClusterExtension that does not have a service account configured. The JSON serialization also seems a bit finicky, so I would also suggest adding a unit test that serializes a ClusterExtension lacking a service account into JSON and checks to ensure that there is no |
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Optional |
There was a problem hiding this comment.
| // +kubebuilder:validation:Optional | |
| // +optional |
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Optional | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
| Name string `json:"name"` | |
| Name string `json:"name,omitempty"` |
2 participants
Changes the ClusterExtension API field spec.ServiceAccount to be optional. Operator-controller will use its own service account by default unless the spec.ServiceAccount field is set. RBAC PreAuthorization only happens if the optional SA field is set, as well.
Give operator-controller's SA cluster-admin by default.
Addresses OPRUN-4221
Wasn't sure if I should mark this major or minor change.
Description
Reviewer Checklist