From 4f0c9c1df24fe712c43d718f1340dd1c5f95825f Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 2 Apr 2024 10:09:46 -0400 Subject: [PATCH] make oauth part of config and remove annotation check Signed-off-by: Kevin --- controllers/raycluster_controller.go | 48 +------- controllers/raycluster_controller_test.go | 132 ++++++++-------------- main.go | 4 +- pkg/config/config.go | 2 + 4 files changed, 54 insertions(+), 132 deletions(-) diff --git a/controllers/raycluster_controller.go b/controllers/raycluster_controller.go index 00d7d559..54ebf07a 100644 --- a/controllers/raycluster_controller.go +++ b/controllers/raycluster_controller.go @@ -21,7 +21,6 @@ import ( "crypto/rand" "crypto/sha1" "encoding/base64" - "strconv" rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1" @@ -128,52 +127,7 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } - val, ok := cluster.ObjectMeta.Annotations["codeflare.dev/oauth"] - boolVal, err := strconv.ParseBool(val) - if err != nil { - logger.Error(err, "Could not convert codeflare.dev/oauth value to bool", "codeflare.dev/oauth", val) - } - if !ok || err != nil || !boolVal { - logger.Info("Removing all OAuth Objects") - err := r.deleteIfNotExist( - ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: oauthSecretNameFromCluster(&cluster)}, &corev1.Secret{}, - ) - if err != nil { - logger.Error(err, "Error deleting OAuth Secret, retrying", logRequeueing, strTrue) - return ctrl.Result{RequeueAfter: requeueTime}, nil - } - err = r.deleteIfNotExist( - ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: oauthServiceNameFromCluster(&cluster)}, &corev1.Service{}, - ) - if err != nil { - logger.Error(err, "Error deleting OAuth Service, retrying", logRequeueing, strTrue) - return ctrl.Result{RequeueAfter: requeueTime}, nil - } - err = r.deleteIfNotExist( - ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: oauthServiceAccountNameFromCluster(&cluster)}, &corev1.ServiceAccount{}, - ) - if err != nil { - logger.Error(err, "Error deleting OAuth ServiceAccount, retrying", logRequeueing, strTrue) - return ctrl.Result{RequeueAfter: requeueTime}, nil - } - err = r.deleteIfNotExist( - ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: crbNameFromCluster(&cluster)}, &rbacv1.ClusterRoleBinding{}, - ) - if err != nil { - logger.Error(err, "Error deleting OAuth CRB, retrying", logRequeueing, strTrue) - return ctrl.Result{RequeueAfter: requeueTime}, nil - } - err = r.deleteIfNotExist( - ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: routeNameFromCluster(&cluster)}, &routev1.Route{}, - ) - if err != nil { - logger.Error(err, "Error deleting OAuth Route, retrying", logRequeueing, strTrue) - return ctrl.Result{RequeueAfter: requeueTime}, nil - } - return ctrl.Result{}, nil - } - - _, err = r.routeClient.Routes(cluster.Namespace).Apply(ctx, desiredClusterRoute(&cluster), metav1.ApplyOptions{FieldManager: controllerName, Force: true}) + _, err := r.routeClient.Routes(cluster.Namespace).Apply(ctx, desiredClusterRoute(&cluster), metav1.ApplyOptions{FieldManager: controllerName, Force: true}) if err != nil { logger.Error(err, "Failed to update OAuth Route") } diff --git a/controllers/raycluster_controller_test.go b/controllers/raycluster_controller_test.go index 0facb0cb..fbbaaa46 100644 --- a/controllers/raycluster_controller_test.go +++ b/controllers/raycluster_controller_test.go @@ -125,98 +125,62 @@ var _ = Describe("RayCluster controller", func() { }, SpecTimeout(time.Second*10)).Should(Equal(true)) }) - Context("Cluster has OAuth annotation", func() { - BeforeEach(func() { - By("adding OAuth annotation to RayCluster") - Eventually(func() error { - foundRayCluster := rayv1.RayCluster{} - err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) - Expect(err).To(Not(HaveOccurred())) - if foundRayCluster.Annotations == nil { - foundRayCluster.Annotations = map[string]string{"codeflare.dev/oauth": "true"} - } else { - foundRayCluster.Annotations["codeflare.dev/oauth"] = "'true'" - } - return k8sClient.Update(ctx, &foundRayCluster) - }, SpecTimeout(time.Second*10)).Should(Not(HaveOccurred())) - By("waiting for dependent resources to be created") - Eventually(func() error { - foundRayCluster := rayv1.RayCluster{} - err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) - if err != nil { - return err - } - err = k8sClient.Get(ctx, types.NamespacedName{Name: oauthSecretNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Secret{}) - if err != nil { - return err - } - err = k8sClient.Get(ctx, types.NamespacedName{Name: oauthServiceNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Service{}) - if err != nil { - return err - } - err = k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &corev1.ServiceAccount{}) - if err != nil { - return err - } - err = k8sClient.Get(ctx, types.NamespacedName{Name: crbNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{}) - if err != nil { - return err - } - err = k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &routev1.Route{}) - if err != nil { - return err - } - return nil - }, SpecTimeout(time.Second*10)).Should(Not(HaveOccurred())) - }) - - It("should set owner references for all resources", func() { + It("should create all oauth resources", func() { + Eventually(func() error { foundRayCluster := rayv1.RayCluster{} err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) - Expect(err).ToNot(HaveOccurred()) + if err != nil { + return err + } err = k8sClient.Get(ctx, types.NamespacedName{Name: oauthSecretNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Secret{}) - Expect(err).To(Not(HaveOccurred())) + if err != nil { + return err + } err = k8sClient.Get(ctx, types.NamespacedName{Name: oauthServiceNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Service{}) - Expect(err).To(Not(HaveOccurred())) + if err != nil { + return err + } err = k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &corev1.ServiceAccount{}) - Expect(err).To(Not(HaveOccurred())) + if err != nil { + return err + } err = k8sClient.Get(ctx, types.NamespacedName{Name: crbNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{}) - Expect(err).To(Not(HaveOccurred())) + if err != nil { + return err + } err = k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &routev1.Route{}) - Expect(err).To(Not(HaveOccurred())) - }) + if err != nil { + return err + } + return nil + }, SpecTimeout(time.Second*10)).Should(Not(HaveOccurred())) + }) - It("should delete OAuth resources when annotation is removed", func() { - foundRayCluster := rayv1.RayCluster{} - err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) - Expect(err).To(Not(HaveOccurred())) - delete(foundRayCluster.Annotations, "codeflare.dev/oauth") - err = k8sClient.Update(ctx, &foundRayCluster) - Expect(err).To(Not(HaveOccurred())) - Eventually(func() bool { - return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: oauthSecretNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Secret{})) - }, SpecTimeout(time.Second*10)).Should(Equal(true)) - Eventually(func() bool { - return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: oauthServiceNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Service{})) - }, SpecTimeout(time.Second*10)).Should(Equal(true)) - Eventually(func() bool { - return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &corev1.ServiceAccount{})) - }, SpecTimeout(time.Second*10)).Should(Equal(true)) - Eventually(func() bool { - return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: crbNameFromCluster(&foundRayCluster)}, &rbacv1.ClusterRoleBinding{})) - }, SpecTimeout(time.Second*10)).Should(Equal(true)) - }) - - It("should remove CRB when the RayCluster is deleted", func() { - foundRayCluster := rayv1.RayCluster{} - err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) - Expect(err).To(Not(HaveOccurred())) - 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{})) - }, SpecTimeout(time.Second*10)).Should(Equal(true)) - }) + It("should set owner references for all resources", func() { + foundRayCluster := rayv1.RayCluster{} + err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) + Expect(err).ToNot(HaveOccurred()) + err = k8sClient.Get(ctx, types.NamespacedName{Name: oauthSecretNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Secret{}) + Expect(err).To(Not(HaveOccurred())) + err = k8sClient.Get(ctx, types.NamespacedName{Name: oauthServiceNameFromCluster(&foundRayCluster), Namespace: foundRayCluster.Namespace}, &corev1.Service{}) + 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{}) + Expect(err).To(Not(HaveOccurred())) + err = k8sClient.Get(ctx, types.NamespacedName{Name: foundRayCluster.Name, Namespace: foundRayCluster.Namespace}, &routev1.Route{}) + Expect(err).To(Not(HaveOccurred())) + }) + + It("should remove CRB when the RayCluster is deleted", func() { + foundRayCluster := rayv1.RayCluster{} + err := k8sClient.Get(ctx, typeNamespaceName, &foundRayCluster) + Expect(err).To(Not(HaveOccurred())) + 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{})) + }, SpecTimeout(time.Second*10)).Should(Equal(true)) }) }) }) diff --git a/main.go b/main.go index 58aa0289..951dbf98 100644 --- a/main.go +++ b/main.go @@ -134,6 +134,7 @@ func main() { MaxScaleoutAllowed: 5, }, }, + RayClusterOAuth: pointer.Bool(true), } kubeConfig, err := ctrl.GetConfig() @@ -180,7 +181,8 @@ func main() { exitOnError(instaScaleController.SetupWithManager(context.Background(), mgr), "Error setting up InstaScale controller") } - if v, err := HasAPIResourceForGVK(kubeClient.DiscoveryClient, rayv1.GroupVersion.WithKind("RayCluster")); v { + v, err := HasAPIResourceForGVK(kubeClient.DiscoveryClient, rayv1.GroupVersion.WithKind("RayCluster")) + if v && *cfg.RayClusterOAuth { rayClusterController := cfoControllers.RayClusterReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()} exitOnError(rayClusterController.SetupWithManager(mgr), "Error setting up RayCluster controller") } else if err != nil { diff --git a/pkg/config/config.go b/pkg/config/config.go index 295e3e00..79a944bf 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -36,6 +36,8 @@ type CodeFlareOperatorConfiguration struct { // The InstaScale controller configuration InstaScale *InstaScaleConfiguration `json:"instascale,omitempty"` + + RayClusterOAuth *bool `json:"rayClusterOAuth,omitempty"` } type InstaScaleConfiguration struct {