From c6d5cc5e005c20821119355729f182240203a1d2 Mon Sep 17 00:00:00 2001 From: Antonin Stefanutti Date: Wed, 17 Apr 2024 15:28:28 +0200 Subject: [PATCH 1/2] Use a RoleBinding to bind the system:auth-delegator ClusterRole --- config/rbac/role.yaml | 8 +-- pkg/controllers/raycluster_controller.go | 57 +++---------------- pkg/controllers/raycluster_controller_test.go | 24 +------- 3 files changed, 11 insertions(+), 78 deletions(-) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 70a6a861d..4c392adb3 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -68,12 +68,6 @@ rules: - patch - update - watch -- apiGroups: - - ray.io - resources: - - rayclusters/finalizers - verbs: - - update - apiGroups: - ray.io resources: @@ -85,7 +79,7 @@ rules: - apiGroups: - rbac.authorization.k8s.io resources: - - clusterrolebindings + - rolebindings verbs: - create - delete diff --git a/pkg/controllers/raycluster_controller.go b/pkg/controllers/raycluster_controller.go index 551367e49..d499602f7 100644 --- a/pkg/controllers/raycluster_controller.go +++ b/pkg/controllers/raycluster_controller.go @@ -25,7 +25,6 @@ import ( rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1" corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -36,7 +35,6 @@ 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" routev1 "github.com/openshift/api/route/v1" routeapply "github.com/openshift/client-go/route/applyconfigurations/route/v1" @@ -60,27 +58,20 @@ type RayClusterReconciler struct { const ( requeueTime = 10 controllerName = "codeflare-raycluster-controller" - oAuthFinalizer = "ray.openshift.ai/oauth-finalizer" oAuthServicePort = 443 oAuthServicePortName = "oauth-proxy" ingressServicePortName = "dashboard" logRequeueing = "requeueing" ) -var ( - deletePolicy = metav1.DeletePropagationForeground - deleteOptions = client.DeleteOptions{PropagationPolicy: &deletePolicy} -) - // +kubebuilder:rbac:groups=ray.io,resources=rayclusters,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=ray.io,resources=rayclusters/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=ray.io,resources=rayclusters/finalizers,verbs=update // +kubebuilder:rbac:groups=route.openshift.io,resources=routes;routes/custom-host,verbs=get;create;update;patch;delete // +kubebuilder:rbac:groups=networking.k8s.io,resources=ingresses,verbs=get;create;update;patch;delete // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;create;patch;delete;get // +kubebuilder:rbac:groups=core,resources=services,verbs=get;create;update;patch;delete // +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=get;create;update;patch;delete -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,verbs=get;create;update;patch;delete +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;create;update;patch;delete // +kubebuilder:rbac:groups=authentication.k8s.io,resources=tokenreviews,verbs=create; // +kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create; @@ -111,39 +102,6 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) r.IsOpenShiftInitialized = true } - if cluster.ObjectMeta.DeletionTimestamp.IsZero() { - if !controllerutil.ContainsFinalizer(&cluster, oAuthFinalizer) { - logger.Info("Add a finalizer", "finalizer", oAuthFinalizer) - controllerutil.AddFinalizer(&cluster, oAuthFinalizer) - if err := r.Update(ctx, &cluster); err != nil { - // this log is info level since errors are not fatal and are expected - logger.Info("WARN: Failed to update RayCluster with finalizer", "error", err.Error(), logRequeueing, true) - return ctrl.Result{RequeueAfter: requeueTime}, err - } - } - } else if controllerutil.ContainsFinalizer(&cluster, oAuthFinalizer) { - err := client.IgnoreNotFound(r.Client.Delete( - ctx, - &rbacv1.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: crbNameFromCluster(&cluster), - }, - }, - &deleteOptions, - )) - if err != nil { - logger.Error(err, "Failed to remove OAuth ClusterRoleBinding.", logRequeueing, true) - return ctrl.Result{RequeueAfter: requeueTime}, err - } - controllerutil.RemoveFinalizer(&cluster, oAuthFinalizer) - if err := r.Update(ctx, &cluster); err != nil { - logger.Error(err, "Failed to remove finalizer from RayCluster", logRequeueing, true) - return ctrl.Result{RequeueAfter: requeueTime}, err - } - logger.Info("Successfully removed finalizer.", logRequeueing, false) - return ctrl.Result{}, nil - } - if cluster.Status.State != "suspended" && r.isRayDashboardOAuthEnabled() && r.IsOpenShift { logger.Info("Creating OAuth Objects") _, err := r.routeClient.Routes(cluster.Namespace).Apply(ctx, desiredClusterRoute(&cluster), metav1.ApplyOptions{FieldManager: controllerName, Force: true}) @@ -170,9 +128,9 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{RequeueAfter: requeueTime}, err } - _, err = r.kubeClient.RbacV1().ClusterRoleBindings().Apply(ctx, desiredOAuthClusterRoleBinding(&cluster), metav1.ApplyOptions{FieldManager: controllerName, Force: true}) + _, err = r.kubeClient.RbacV1().RoleBindings(cluster.Namespace).Apply(ctx, desiredOAuthRoleBinding(&cluster), metav1.ApplyOptions{FieldManager: controllerName, Force: true}) if err != nil { - logger.Error(err, "Failed to update OAuth ClusterRoleBinding") + logger.Error(err, "Failed to update OAuth RoleBinding") return ctrl.Result{RequeueAfter: requeueTime}, err } @@ -213,13 +171,12 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } -func crbNameFromCluster(cluster *rayv1.RayCluster) string { - return cluster.Name + "-" + cluster.Namespace + "-auth" // NOTE: potential naming conflicts ie {name: foo, ns: bar-baz} and {name: foo-bar, ns: baz} +func roleBindingNameFromCluster(cluster *rayv1.RayCluster) string { + return cluster.Name + "-" + cluster.Namespace + "-auth" } -func desiredOAuthClusterRoleBinding(cluster *rayv1.RayCluster) *rbacapply.ClusterRoleBindingApplyConfiguration { - return rbacapply.ClusterRoleBinding( - crbNameFromCluster(cluster)). +func desiredOAuthRoleBinding(cluster *rayv1.RayCluster) *rbacapply.RoleBindingApplyConfiguration { + return rbacapply.RoleBinding(roleBindingNameFromCluster(cluster), cluster.Namespace). WithLabels(map[string]string{"ray.io/cluster-name": cluster.Name}). WithSubjects( rbacapply.Subject(). diff --git a/pkg/controllers/raycluster_controller_test.go b/pkg/controllers/raycluster_controller_test.go index 6592aa292..62b77aab9 100644 --- a/pkg/controllers/raycluster_controller_test.go +++ b/pkg/controllers/raycluster_controller_test.go @@ -34,15 +34,6 @@ import ( routev1 "github.com/openshift/api/route/v1" ) -func stringInList(l []string, s string) bool { - for _, i := range l { - if i == s { - return true - } - } - return false -} - var letters = []rune("abcdefghijklmnopqrstuvwxyz") var r = rand.New(rand.NewSource(time.Now().UnixNano())) @@ -116,15 +107,6 @@ var _ = Describe("RayCluster controller", func() { }, SpecTimeout(time.Second*10)).Should(Equal(true)) }) - It("should have oauth finalizer set", func() { - foundRayCluster := rayv1.RayCluster{} - Eventually(func() bool { - err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) - Expect(err).To(Not(HaveOccurred())) - return stringInList(foundRayCluster.Finalizers, oAuthFinalizer) - }, SpecTimeout(time.Second*10)).Should(Equal(true)) - }) - It("should create all oauth resources", func() { Eventually(func() error { foundRayCluster := rayv1.RayCluster{} @@ -144,7 +126,7 @@ var _ = Describe("RayCluster controller", func() { if err != nil { return err } - err = k8sClient.Get(ctx, types.NamespacedName{Name: crbNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{}) + err = k8sClient.Get(ctx, types.NamespacedName{Name: roleBindingNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{}) if err != nil { return err } @@ -166,7 +148,7 @@ var _ = Describe("RayCluster controller", func() { Expect(err).To(Not(HaveOccurred())) err = k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &corev1.ServiceAccount{}) Expect(err).To(Not(HaveOccurred())) - err = k8sClient.Get(ctx, types.NamespacedName{Name: crbNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{}) + err = k8sClient.Get(ctx, types.NamespacedName{Name: roleBindingNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{}) Expect(err).To(Not(HaveOccurred())) err = k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &routev1.Route{}) Expect(err).To(Not(HaveOccurred())) @@ -179,7 +161,7 @@ var _ = Describe("RayCluster controller", func() { err = k8sClient.Delete(ctx, &foundRayCluster) Expect(err).To(Not(HaveOccurred())) Eventually(func() bool { - return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: crbNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{})) + return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: roleBindingNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{})) }, SpecTimeout(time.Second*10)).Should(Equal(true)) }) }) From 5a7acef8cb32862db8f0721ee1cae63a20c2c393 Mon Sep 17 00:00:00 2001 From: Antonin Stefanutti Date: Wed, 17 Apr 2024 15:39:00 +0200 Subject: [PATCH 2/2] Add OwnerReference to OAuth RoleBinding --- pkg/controllers/raycluster_controller.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/controllers/raycluster_controller.go b/pkg/controllers/raycluster_controller.go index d499602f7..9bac2646f 100644 --- a/pkg/controllers/raycluster_controller.go +++ b/pkg/controllers/raycluster_controller.go @@ -189,7 +189,12 @@ func desiredOAuthRoleBinding(cluster *rayv1.RayCluster) *rbacapply.RoleBindingAp WithAPIGroup("rbac.authorization.k8s.io"). WithKind("ClusterRole"). WithName("system:auth-delegator"), - ) + ). + WithOwnerReferences(v1.OwnerReference(). + WithUID(cluster.UID). + WithName(cluster.Name). + WithKind(cluster.Kind). + WithAPIVersion(cluster.APIVersion)) } func oauthServiceAccountNameFromCluster(cluster *rayv1.RayCluster) string {