only create CRB for raw if isvc has auth

config/rbac/role.yaml

@@ -239,10 +239,22 @@ rules:
   - serving.kserve.io
   resources:
   - inferenceservices
+  verbs:
+  - get
+  - list
+  - patch
+  - update
+  - watch
+- apiGroups:
+  - serving.kserve.io
+  resources:
   - inferenceservices/finalizers
   verbs:
+  - create
+  - delete
   - get
   - list
+  - patch
   - update
   - watch
 - apiGroups:

internal/controller/constants/constants.go

@@ -18,7 +18,8 @@ package constants
 type KServeDeploymentMode string
 
 const (
-	InferenceServiceKind = "InferenceService"
+	InferenceServiceKind             = "InferenceService"
+	InferenceServiceODHFinalizerName = "odh.inferenceservice.finalizers"
 
 	ServiceMeshMemberRollName        = "default"
 	ServiceMeshMemberName            = "default"

internal/controller/serving/inferenceservice_controller.go

@@ -19,8 +19,11 @@ package serving
 import (
 	"context"
 	"errors"
+	"fmt"
+	"strings"
 
 	"github.com/go-logr/logr"
+	"github.com/hashicorp/go-multierror"
 	kservev1alpha1 "github.com/kserve/kserve/pkg/apis/serving/v1alpha1"
 	kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
 	routev1 "github.com/openshift/api/route/v1"
@@ -34,9 +37,11 @@ import (
 	"k8s.io/client-go/kubernetes"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/yaml"
 
 	"github.com/opendatahub-io/odh-model-controller/internal/controller/constants"
 	"github.com/opendatahub-io/odh-model-controller/internal/controller/serving/reconcilers"
@@ -84,8 +89,8 @@ func NewInferenceServiceReconciler(setupLog logr.Logger, client client.Client, s
 	return isvcReconciler
 }
 
-// +kubebuilder:rbac:groups=serving.kserve.io,resources=inferenceservices,verbs=get;list;watch;update
-// +kubebuilder:rbac:groups=serving.kserve.io,resources=inferenceservices/finalizers,verbs=get;list;watch;update
+// +kubebuilder:rbac:groups=serving.kserve.io,resources=inferenceservices,verbs=get;list;watch;update;patch
+// +kubebuilder:rbac:groups=serving.kserve.io,resources=inferenceservices/finalizers,verbs=get;list;watch;update;create;patch;delete
 
 // +kubebuilder:rbac:groups=networking.istio.io,resources=virtualservices,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=networking.istio.io,resources=virtualservices/finalizers,verbs=get;list;watch;create;update;patch;delete
@@ -132,8 +137,41 @@ func (r *InferenceServiceReconciler) ReconcileServing(ctx context.Context, req c
 		return ctrl.Result{}, err
 	}
 
-	if isvc.GetDeletionTimestamp() != nil {
-		return reconcile.Result{}, r.onDeletion(ctx, logger, isvc)
+	if isvc.GetDeletionTimestamp() == nil {
+		// The object is not being deleted, so if it does not have our finalizer,
+		// then lets add the finalizer and update the object. This is equivalent
+		// registering our finalizer.
+		logger.Info("Adding Finalizer")
+		if !controllerutil.ContainsFinalizer(isvc, constants.InferenceServiceODHFinalizerName) {
+			controllerutil.AddFinalizer(isvc, constants.InferenceServiceODHFinalizerName)
+			patchYaml := "metadata:\n  finalizers: [" + strings.Join(isvc.ObjectMeta.Finalizers, ",") + "]"
+			patchJson, _ := yaml.YAMLToJSON([]byte(patchYaml))
+			if err := r.Patch(ctx, isvc, client.RawPatch(types.MergePatchType, patchJson)); err != nil {
+				return reconcile.Result{}, err
+			}
+		}
+	} else {
+		var deleteErrors *multierror.Error
+		logger.Info("InferenceService being deleted")
+		if controllerutil.ContainsFinalizer(isvc, constants.InferenceServiceODHFinalizerName) {
+			err := r.onDeletion(ctx, logger, isvc)
+			if err != nil {
+				deleteErrors = multierror.Append(deleteErrors, err)
+			}
+			// Check if we need to also perform cleanup on the namespace
+			err1 := r.DeleteResourcesIfNoIsvcExists(ctx, logger, req.Namespace)
+			if err1 != nil {
+				deleteErrors = multierror.Append(deleteErrors, err1)
+			}
+			controllerutil.RemoveFinalizer(isvc, constants.InferenceServiceODHFinalizerName)
+			patchYaml := "metadata:\n  finalizers: [" + strings.Join(isvc.ObjectMeta.Finalizers, ",") + "]"
+			patchJson, _ := yaml.YAMLToJSON([]byte(patchYaml))
+			if err := r.Patch(ctx, isvc, client.RawPatch(types.MergePatchType, patchJson)); err != nil {
+				return reconcile.Result{}, err
+			}
+
+		}
+		return reconcile.Result{}, deleteErrors.ErrorOrNil()
 	}
 
 	// Check what deployment mode is used by the InferenceService. We have differing reconciliation logic for Kserve and ModelMesh
@@ -259,7 +297,8 @@ func (r *InferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 // general clean-up, mostly resources in different namespaces from kservev1beta1.InferenceService
 func (r *InferenceServiceReconciler) onDeletion(ctx context.Context, log logr.Logger, inferenceService *kservev1beta1.InferenceService) error {
-	log.V(1).Info("Running cleanup logic")
+
+	log.V(1).Info("Triggering Delete for InferenceService: " + inferenceService.Name)
 
 	IsvcDeploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, r.Client, inferenceService.GetAnnotations())
 	if err != nil {
@@ -277,18 +316,58 @@ func (r *InferenceServiceReconciler) onDeletion(ctx context.Context, log logr.Lo
 }
 
 func (r *InferenceServiceReconciler) DeleteResourcesIfNoIsvcExists(ctx context.Context, log logr.Logger, namespace string) error {
+	inferenceServiceList := &kservev1beta1.InferenceServiceList{}
+	if err := r.Client.List(ctx, inferenceServiceList, client.InNamespace(namespace)); err != nil {
+		return err
+	}
+
+	var existingServerlessIsvcs []kservev1beta1.InferenceService
+	var existingRawIsvcs []kservev1beta1.InferenceService
+	var existingModelMeshIsvcs []kservev1beta1.InferenceService
+	for _, isvc := range inferenceServiceList.Items {
+		isvcDeploymentMode, err := utils.GetDeploymentModeForKServeResource(ctx, r.Client, isvc.GetAnnotations())
+		if err != nil {
+			return err
+		}
+		switch isvcDeploymentMode {
+		case constants.Serverless:
+			if isvc.GetDeletionTimestamp() == nil {
+				existingServerlessIsvcs = append(existingServerlessIsvcs, isvc)
+			}
+		case constants.RawDeployment:
+			if isvc.GetDeletionTimestamp() == nil {
+				existingRawIsvcs = append(existingRawIsvcs, isvc)
+			}
+		case constants.ModelMesh:
+			if isvc.GetDeletionTimestamp() == nil {
+				existingModelMeshIsvcs = append(existingModelMeshIsvcs, isvc)
+			}
+		default:
+			return fmt.Errorf("Unknown deployment mode for InferenceService: %v", isvc)
+		}
+	}
 	// If there are no ISVCs left, all 3 of these functions get called. Issue is that when the Serverless reconciler gets called
 	// and there is no servicemesh installed (which is the case with RawDeployment) the Serverless reconciler will fail when it calls
 	// the subresourcereconciler Cleanup function. This is because the Serverless reconciler will try to get the ServiceMesh resources and clean them up.
 	// Adding a no match error check will prevent this from erroring out due to that.
-	if err := r.kserveServerlessISVCReconciler.CleanupNamespaceIfNoKserveIsvcExists(ctx, log, namespace); err != nil {
-		return err
+
+	if len(existingServerlessIsvcs) == 0 {
+		log.V(1).Info("Triggering Serverless Cleanup for Namespace: " + namespace)
+		if err := r.kserveServerlessISVCReconciler.CleanupNamespaceIfNoKserveIsvcExists(ctx, log, namespace); err != nil {
+			return err
+		}
 	}
-	if err := r.mmISVCReconciler.DeleteModelMeshResourcesIfNoMMIsvcExists(ctx, log, namespace); err != nil {
-		return err
+	if len(existingRawIsvcs) == 0 {
+		log.V(1).Info("Triggering RawDeployment Cleanup for Namespace: " + namespace)
+		if err := r.kserveRawISVCReconciler.CleanupNamespaceIfNoRawKserveIsvcExists(ctx, log, namespace); err != nil {
+			return err
+		}
 	}
-	if err := r.kserveRawISVCReconciler.CleanupNamespaceIfNoKserveIsvcExists(ctx, log, namespace); err != nil {
-		return err
+	if len(existingModelMeshIsvcs) == 0 {
+		log.V(1).Info("Triggering ModelMesh Cleanup for Namespace: " + namespace)
+		if err := r.mmISVCReconciler.DeleteModelMeshResourcesIfNoMMIsvcExists(ctx, log, namespace); err != nil {
+			return err
+		}
 	}
 	return nil
 }

internal/controller/serving/inferenceservice_controller_test.go

@@ -1039,6 +1039,8 @@ var _ = Describe("InferenceService Controller", func() {
 			It("it should create a default clusterrolebinding for auth", func() {
 				_ = createServingRuntime(testNs, KserveServingRuntimePath1)
 				inferenceService := createInferenceService(testNs, KserveOvmsInferenceServiceName, KserveInferenceServicePath1)
+				inferenceService.Labels = map[string]string{}
+				inferenceService.Labels[constants.LabelEnableAuthODH] = "true"
 				if err := k8sClient.Create(ctx, inferenceService); err != nil && !k8sErrors.IsAlreadyExists(err) {
 					Expect(err).NotTo(HaveOccurred())
 				}
@@ -1060,6 +1062,8 @@ var _ = Describe("InferenceService Controller", func() {
 				serviceAccountName := "custom-sa"
 				_ = createServingRuntime(testNs, KserveServingRuntimePath1)
 				inferenceService := createInferenceService(testNs, KserveOvmsInferenceServiceName, KserveInferenceServicePath1)
+				inferenceService.Labels = map[string]string{}
+				inferenceService.Labels[constants.LabelEnableAuthODH] = "true"
 				inferenceService.Spec.Predictor.ServiceAccountName = serviceAccountName
 				if err := k8sClient.Create(ctx, inferenceService); err != nil && !k8sErrors.IsAlreadyExists(err) {
 					Expect(err).NotTo(HaveOccurred())
@@ -1187,6 +1191,88 @@ var _ = Describe("InferenceService Controller", func() {
 					return k8sClient.Get(ctx, key, serviceMonitor)
 				}, timeout, interval).Should(HaveOccurred())
 			})
+			It("CRB is deleted only when all associated isvcs are deleted", func() {
+				customServiceAccountName := "custom-sa"
+				_ = createServingRuntime(testNs, KserveServingRuntimePath1)
+				// create 2 isvcs with no SA (i.e default) and 2 with a custom SA
+				defaultIsvc1 := createInferenceService(testNs, "default-1", KserveInferenceServicePath1)
+				defaultIsvc1.Labels = map[string]string{}
+				defaultIsvc1.Labels[constants.LabelEnableAuthODH] = "true"
+				if err := k8sClient.Create(ctx, defaultIsvc1); err != nil {
+					Expect(err).NotTo(HaveOccurred())
+				}
+				defaultIsvc2 := createInferenceService(testNs, "default-2", KserveInferenceServicePath1)
+				defaultIsvc2.Labels = map[string]string{}
+				defaultIsvc2.Labels[constants.LabelEnableAuthODH] = "true"
+				if err := k8sClient.Create(ctx, defaultIsvc2); err != nil {
+					Expect(err).NotTo(HaveOccurred())
+				}
+				customIsvc1 := createInferenceService(testNs, "custom-1", KserveInferenceServicePath1)
+				customIsvc1.Labels = map[string]string{}
+				customIsvc1.Labels[constants.LabelEnableAuthODH] = "true"
+				customIsvc1.Spec.Predictor.ServiceAccountName = customServiceAccountName
+				if err := k8sClient.Create(ctx, customIsvc1); err != nil {
+					Expect(err).NotTo(HaveOccurred())
+				}
+				customIsvc2 := createInferenceService(testNs, "custom-2", KserveInferenceServicePath1)
+				customIsvc2.Labels = map[string]string{}
+				customIsvc2.Labels[constants.LabelEnableAuthODH] = "true"
+				customIsvc2.Spec.Predictor.ServiceAccountName = customServiceAccountName
+				if err := k8sClient.Create(ctx, customIsvc2); err != nil {
+					Expect(err).NotTo(HaveOccurred())
+				}
+				// confirm that default CRB exists
+				crb := &rbacv1.ClusterRoleBinding{}
+				Eventually(func() error {
+					key := types.NamespacedName{Name: defaultIsvc1.Namespace + "-" + constants.KserveServiceAccountName + "-auth-delegator",
+						Namespace: defaultIsvc1.Namespace}
+					return k8sClient.Get(ctx, key, crb)
+				}, timeout, interval).ShouldNot(HaveOccurred())
+				// confirm that custom CRB exists
+				customCrb := &rbacv1.ClusterRoleBinding{}
+				Eventually(func() error {
+					key := types.NamespacedName{Name: defaultIsvc1.Namespace + "-" + customServiceAccountName + "-auth-delegator",
+						Namespace: defaultIsvc1.Namespace}
+					return k8sClient.Get(ctx, key, customCrb)
+				}, timeout, interval).ShouldNot(HaveOccurred())
+
+				// Delete isvc and isvc2 (one with default SA and one with custom SA)
+				Expect(k8sClient.Delete(ctx, defaultIsvc1)).Should(Succeed())
+				Expect(k8sClient.Delete(ctx, customIsvc1)).Should(Succeed())
+
+				// confirm that CRBs are not deleted
+				Consistently(func() error {
+					key := types.NamespacedName{Name: defaultIsvc1.Namespace + "-" + constants.KserveServiceAccountName + "-auth-delegator",
+						Namespace: defaultIsvc1.Namespace}
+					return k8sClient.Get(ctx, key, crb)
+				}, timeout, interval).ShouldNot(HaveOccurred())
+				Consistently(func() error {
+					key := types.NamespacedName{Name: defaultIsvc1.Namespace + "-" + customServiceAccountName + "-auth-delegator",
+						Namespace: defaultIsvc1.Namespace}
+					return k8sClient.Get(ctx, key, customCrb)
+				}, timeout, interval).ShouldNot(HaveOccurred())
+
+				// Delete rest of the isvcs
+				Expect(k8sClient.Delete(ctx, defaultIsvc2)).Should(Succeed())
+				Expect(k8sClient.Delete(ctx, customIsvc2)).Should(Succeed())
+
+				crblist := &rbacv1.ClusterRoleBindingList{}
+				listOpts := client.ListOptions{Namespace: testNs}
+				if err := k8sClient.List(ctx, crblist, &listOpts); err != nil {
+					Fail(err.Error())
+				}
+
+				Eventually(func() error {
+					crb := &rbacv1.ClusterRoleBinding{}
+					key := types.NamespacedName{Name: defaultIsvc1.Namespace + "-" + constants.KserveServiceAccountName + "-auth-delegator", Namespace: defaultIsvc2.Namespace}
+					return k8sClient.Get(ctx, key, crb)
+				}, timeout, interval).Should(HaveOccurred())
+				Eventually(func() error {
+					customCrb := &rbacv1.ClusterRoleBinding{}
+					key := types.NamespacedName{Name: defaultIsvc1.Namespace + "-" + customServiceAccountName + "-auth-delegator", Namespace: customIsvc2.Namespace}
+					return k8sClient.Get(ctx, key, customCrb)
+				}, timeout, interval).Should(HaveOccurred())
+			})
 		})
 		When("namespace no longer has any RawDeployment models", func() {
 			It("should delete the default clusterrolebinding", func() {
@@ -1318,25 +1404,52 @@ func createBasicISVC(namespace string) *kservev1beta1.InferenceService {
 }
 
 func updateISVCStatus(isvc *kservev1beta1.InferenceService) error {
+	latestISVC := isvc.DeepCopy()
+	// Construct the URL and update the status
 	url, _ := apis.ParseURL("http://iscv-" + isvc.Namespace + "ns.apps.openshift.ai")
-	isvc.Status = kservev1beta1.InferenceServiceStatus{
-		URL: url,
+	latestISVC.Status.URL = url
+	// Patch the status to avoid conflicts
+	err := k8sClient.Status().Patch(context.Background(), latestISVC, client.MergeFrom(isvc))
+	if err != nil {
+		if k8sErrors.IsConflict(err) {
+			// Retry on conflict
+			return updateISVCStatus(isvc)
+		}
+		return err
 	}
-	return k8sClient.Status().Update(context.Background(), isvc)
+	return nil
 }
 
 func disableAuth(isvc *kservev1beta1.InferenceService) error {
-	delete(isvc.Annotations, constants.LabelEnableAuth)
-	delete(isvc.Annotations, constants.LabelEnableAuthODH)
-	return k8sClient.Update(context.Background(), isvc)
+	// Retrieve the latest version of the InferenceService
+	latestISVC := &kservev1beta1.InferenceService{}
+	err := k8sClient.Get(context.Background(), types.NamespacedName{
+		Name:      isvc.Name,
+		Namespace: isvc.Namespace,
+	}, latestISVC)
+	if err != nil {
+		return err
+	}
+	delete(latestISVC.Annotations, constants.LabelEnableAuth)
+	delete(latestISVC.Annotations, constants.LabelEnableAuthODH)
+	return k8sClient.Update(context.Background(), latestISVC)
 }
 
 func enableAuth(isvc *kservev1beta1.InferenceService) error {
-	if isvc.Annotations == nil {
-		isvc.Annotations = map[string]string{}
+	// Retrieve the latest version of the InferenceService
+	latestISVC := &kservev1beta1.InferenceService{}
+	err := k8sClient.Get(context.Background(), types.NamespacedName{
+		Name:      isvc.Name,
+		Namespace: isvc.Namespace,
+	}, latestISVC)
+	if err != nil {
+		return err
+	}
+	if latestISVC.Annotations == nil {
+		latestISVC.Annotations = map[string]string{}
 	}
-	isvc.Annotations[constants.LabelEnableAuthODH] = "true"
-	return k8sClient.Update(context.Background(), isvc)
+	latestISVC.Annotations[constants.LabelEnableAuthODH] = "true"
+	return k8sClient.Update(context.Background(), latestISVC)
 }
 
 func createDSCI(_ string) error {

internal/controller/serving/reconcilers/kserve_istio_smm_reconciler.go

@@ -52,6 +52,7 @@ func (r *KserveServiceMeshMemberReconciler) Reconcile(ctx context.Context, log l
 }
 
 func (r *KserveServiceMeshMemberReconciler) Cleanup(ctx context.Context, log logr.Logger, namespace string) error {
+	log.V(1).Info("Deleting Istio Telemetry object for target namespace")
 	existingSMM, getError := r.getExistingResource(ctx, log, namespace)
 	if getError != nil {
 		return getError