fix: address review feedback and upgrade test failure

- Remove unused saClient parameter from ServiceAccountDeprecationWarning
- Relax serviceAccount.name immutability rule to allow clearing the
  deprecated field (self == oldSelf || size(self) == 0)
- Delete old ClusterRoleBinding before kubectl apply during upgrade —
  Kubernetes does not allow changing roleRef on existing CRBs

Co-Authored-By: Claude <noreply@anthropic.com>

Author: pedjak

api/v1/clusterextension_types.go

@@ -400,7 +400,7 @@ type ServiceAccountReference struct {
 	// [RFC 1123]: https://tools.ietf.org/html/rfc1123
 	//
 	// +kubebuilder:validation:MaxLength:=253
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable"
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf || size(self) == 0",message="name is immutable once set, but may be cleared"
 	// +kubebuilder:validation:XValidation:rule="size(self) == 0 || 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"
 	// +optional
 	Name string `json:"name"`

cmd/operator-controller/main.go

@@ -652,7 +652,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
 	ceReconciler.ReconcileSteps = []controllers.ReconcileStepFunc{
 		controllers.HandleFinalizers(c.finalizers),
 		controllers.ValidateClusterExtension(
-			controllers.ServiceAccountDeprecationWarning(coreClient),
+			controllers.ServiceAccountDeprecationWarning(),
 		),
 		controllers.MigrateStorage(storageMigrator),
 		controllers.RetrieveRevisionStates(revisionStatesGetter),
@@ -740,7 +740,7 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster
 	ceReconciler.ReconcileSteps = []controllers.ReconcileStepFunc{
 		controllers.HandleFinalizers(c.finalizers),
 		controllers.ValidateClusterExtension(
-			controllers.ServiceAccountDeprecationWarning(coreClient),
+			controllers.ServiceAccountDeprecationWarning(),
 		),
 		controllers.RetrieveRevisionStates(revisionStatesGetter),
 		controllers.ResolveBundle(c.resolver, c.mgr.GetClient()),

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

@@ -206,8 +206,8 @@ spec:
                     maxLength: 253
                     type: string
                     x-kubernetes-validations:
-                    - message: name is immutable
-                      rule: self == oldSelf
+                    - message: name is immutable once set, but may be cleared
+                      rule: self == oldSelf || size(self) == 0
                     - 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

helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml

@@ -158,8 +158,8 @@ spec:
                     maxLength: 253
                     type: string
                     x-kubernetes-validations:
-                    - message: name is immutable
-                      rule: self == oldSelf
+                    - message: name is immutable once set, but may be cleared
+                      rule: self == oldSelf || size(self) == 0
                     - 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

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

@@ -23,7 +23,6 @@ import (
 
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/finalizer"
@@ -99,7 +98,7 @@ func ValidateClusterExtension(validators ...ClusterExtensionValidator) Reconcile
 
 // ServiceAccountDeprecationWarning returns a validator that checks if the deprecated
 // serviceAccount field is populated and logs a warning if found.
-func ServiceAccountDeprecationWarning(saClient corev1client.ServiceAccountsGetter) ClusterExtensionValidator {
+func ServiceAccountDeprecationWarning() ClusterExtensionValidator {
 	return func(ctx context.Context, ext *ocv1.ClusterExtension) error {
 		l := log.FromContext(ctx)
 		if len(ext.Spec.ServiceAccount.Name) > 0 { //nolint:staticcheck // intentional read of deprecated field to emit deprecation warning

manifests/experimental-e2e.yaml

@@ -820,8 +820,8 @@ spec:
                     maxLength: 253
                     type: string
                     x-kubernetes-validations:
-                    - message: name is immutable
-                      rule: self == oldSelf
+                    - message: name is immutable once set, but may be cleared
+                      rule: self == oldSelf || size(self) == 0
                     - 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

manifests/experimental.yaml

@@ -781,8 +781,8 @@ spec:
                     maxLength: 253
                     type: string
                     x-kubernetes-validations:
-                    - message: name is immutable
-                      rule: self == oldSelf
+                    - message: name is immutable once set, but may be cleared
+                      rule: self == oldSelf || size(self) == 0
                     - 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

manifests/standard-e2e.yaml

@@ -772,8 +772,8 @@ spec:
                     maxLength: 253
                     type: string
                     x-kubernetes-validations:
-                    - message: name is immutable
-                      rule: self == oldSelf
+                    - message: name is immutable once set, but may be cleared
+                      rule: self == oldSelf || size(self) == 0
                     - 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

manifests/standard.yaml

@@ -733,8 +733,8 @@ spec:
                     maxLength: 253
                     type: string
                     x-kubernetes-validations:
-                    - message: name is immutable
-                      rule: self == oldSelf
+                    - message: name is immutable once set, but may be cleared
+                      rule: self == oldSelf || size(self) == 0
                     - 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

scripts/install.tpl.sh

@@ -116,6 +116,10 @@ if [ -f "${olmv1_manifest}" ]; then
     olmv1_manifest=file://localhost$(realpath ${olmv1_manifest})
 fi
 
+# Delete the old ClusterRoleBinding if it exists — Kubernetes does not allow
+# changing roleRef via kubectl apply, so we must delete before re-creating.
+kubectl delete clusterrolebinding operator-controller-manager-rolebinding --ignore-not-found
+
 curl -L -s "${olmv1_manifest}" | sed "s/olmv1-system/${olmv1_namespace}/g" | kubectl apply -f -
 # Wait for the rollout, and then wait for the deployment to be Available
 kubectl_wait_rollout "${olmv1_namespace}" "deployment/catalogd-controller-manager" "60s"