RHOAI-4148: Re-adjust the statefulset, route, service based on generateName (#539)

* Adjust the route url to maintain the sub-domain length less than 63 char

Signed-off-by: Harshad Reddy Nalla <[email protected]>

* Adjust the statefulset naming based on the length

Signed-off-by: Harshad Reddy Nalla <[email protected]>

* Use constant vars for routelengthmax and namespacemax

Signed-off-by: Harshad Reddy Nalla <[email protected]>

* Include the env test case for the route generateName

Signed-off-by: Harshad Reddy Nalla <[email protected]>

* Check limit for entire sub-domain instead of name and namespace

Signed-off-by: Harshad Reddy Nalla <[email protected]>

* adjust the finding of route for oauthclient

Signed-off-by: Harshad Reddy Nalla <[email protected]>

* Update components/notebook-controller/controllers/notebook_controller.go

Co-authored-by: Jan Stourac <[email protected]>

* Fix the lint issue
- ST1023: should omit type bool from declaration; it will be inferred from the right-hand side (staticcheck)
- QF1008: could remove embedded field "ObjectMeta" from selector (staticcheck)

Signed-off-by: Harshad Reddy Nalla <[email protected]>

---------

Signed-off-by: Harshad Reddy Nalla <[email protected]>
Co-authored-by: Jan Stourac <[email protected]>

Author: harshad16

components/notebook-controller/controllers/notebook_controller.go

@@ -55,6 +55,9 @@ const WorkbenchLabel = "opendatahub.io/workbenches"
 
 const PrefixEnvVar = "NB_PREFIX"
 
+// Statefulset name should be less than 52https://github.com/kubernetes/kubernetes/issues/64023
+const MaxStatefulsetNameLength = 52
+
 // The default fsGroup of PodSecurityContext.
 // https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.11/#podsecuritycontext-v1-core
 const DefaultFSGroup = int64(100)
@@ -136,16 +139,38 @@ func (r *NotebookReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return ctrl.Result{}, nil
 	}
 
+	// check if Notebook Name length is greater than 52
+	// as controller-service include as hash (len 11) as label
+	// controller-service-hash that shouldn't exceed 63 chars.
+	isGenerateName := false
+	if len(instance.Name) > MaxStatefulsetNameLength {
+		log.Info("Notebook name is too long, it can be {MaxStatefulsetNameLength} chars long at most")
+		isGenerateName = true
+	}
+
 	// Reconcile StatefulSet
-	ss := generateStatefulSet(instance)
+	ss := generateStatefulSet(instance, isGenerateName)
 	if err := ctrl.SetControllerReference(instance, ss, r.Scheme); err != nil {
 		return ctrl.Result{}, err
 	}
 	// Check if the StatefulSet already exists
 	foundStateful := &appsv1.StatefulSet{}
+	namespacedStatefulSets := &appsv1.StatefulSetList{}
+	err := r.List(ctx, namespacedStatefulSets, client.InNamespace(ss.Namespace))
+	if err != nil {
+		log.Error(err, "error listing StatefulSets")
+		return ctrl.Result{}, err
+	}
+
+	for _, sts := range namespacedStatefulSets.Items {
+		if metav1.IsControlledBy(&sts, instance) {
+			foundStateful = &sts
+			break
+		}
+	}
+
 	justCreated := false
-	err := r.Get(ctx, types.NamespacedName{Name: ss.Name, Namespace: ss.Namespace}, foundStateful)
-	if err != nil && apierrs.IsNotFound(err) {
+	if foundStateful.Name == "" && foundStateful.Namespace == "" {
 		log.Info("Creating StatefulSet", "namespace", ss.Namespace, "name", ss.Name)
 		r.Metrics.NotebookCreation.WithLabelValues(ss.Namespace).Inc()
 		err = r.Create(ctx, ss)
@@ -405,17 +430,25 @@ func setPrefixEnvVar(instance *v1beta1.Notebook, container *corev1.Container) {
 	})
 }
 
-func generateStatefulSet(instance *v1beta1.Notebook) *appsv1.StatefulSet {
+func generateStatefulSet(instance *v1beta1.Notebook, isGenerateName bool) *appsv1.StatefulSet {
 	replicas := int32(1)
 	if metav1.HasAnnotation(instance.ObjectMeta, "kubeflow-resource-stopped") {
 		replicas = 0
 	}
 
+	ssObjectMeta := metav1.ObjectMeta{
+		Name:      instance.Name,
+		Namespace: instance.Namespace,
+	}
+	if isGenerateName {
+		ssObjectMeta = metav1.ObjectMeta{
+			GenerateName: "nb-",
+			Namespace:    instance.Namespace,
+		}
+	}
+
 	ss := &appsv1.StatefulSet{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      instance.Name,
-			Namespace: instance.Namespace,
-		},
+		ObjectMeta: ssObjectMeta,
 		Spec: appsv1.StatefulSetSpec{
 			Replicas: &replicas,
 			Selector: &metav1.LabelSelector{
@@ -501,7 +534,7 @@ func generateService(instance *v1beta1.Notebook) *corev1.Service {
 			Ports: []corev1.ServicePort{
 				{
 					// Make port name follow Istio pattern so it can be managed by istio rbac
-					Name:       "http-" + instance.Name,
+					Name:       "http-notebook",
 					Port:       DefaultServingPort,
 					TargetPort: intstr.FromInt(port),
 					Protocol:   "TCP",

components/odh-notebook-controller/controllers/notebook_controller_test.go

@@ -19,6 +19,7 @@ import (
 	"context"
 	"crypto/x509"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"os"
 	"time"
@@ -313,6 +314,131 @@ var _ = Describe("The Openshift Notebook controller", func() {
 
 	})
 
+	// New test case for notebook creation with long name
+	When("Creating a long named Notebook", func() {
+
+		// With the work done https://issues.redhat.com/browse/RHOAIENG-4148,
+		// 48 characters is the maximum length for a notebook name.
+		// This would the extent of the test.
+		// TODO: Update the test to use the maximum length when the work is done.
+		const (
+			Name      = "test-notebook-with-a-very-long-name-thats-48char"
+			Namespace = "default"
+		)
+
+		notebook := createNotebook(Name, Namespace)
+
+		expectedRoute := routev1.Route{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "nb-",
+				Namespace:    Namespace,
+				Labels: map[string]string{
+					"notebook-name": Name,
+				},
+			},
+			Spec: routev1.RouteSpec{
+				To: routev1.RouteTargetReference{
+					Kind:   "Service",
+					Name:   Name,
+					Weight: ptr.To[int32](100),
+				},
+				Port: &routev1.RoutePort{
+					TargetPort: intstr.FromString("http-" + Name),
+				},
+				TLS: &routev1.TLSConfig{
+					Termination:                   routev1.TLSTerminationEdge,
+					InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
+				},
+				WildcardPolicy: routev1.WildcardPolicyNone,
+			},
+			Status: routev1.RouteStatus{
+				Ingress: []routev1.RouteIngress{},
+			},
+		}
+
+		route := &routev1.Route{}
+
+		It("Should create a Route to expose the traffic externally", func() {
+			ctx := context.Background()
+
+			By("By creating a new Notebook")
+			Expect(cli.Create(ctx, notebook)).Should(Succeed())
+			time.Sleep(interval)
+
+			By("By checking that the controller has created the Route")
+			Eventually(func() error {
+				route, err := getRouteFromList(route, notebook, Name, Namespace)
+				if route == nil {
+					return err
+				}
+				return nil
+			}, duration, interval).Should(Succeed())
+			Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
+		})
+
+		It("Should reconcile the Route when modified", func() {
+			By("By simulating a manual Route modification")
+			patch := client.RawPatch(types.MergePatchType, []byte(`{"spec":{"to":{"name":"foo"}}}`))
+			Expect(cli.Patch(ctx, route, patch)).Should(Succeed())
+			time.Sleep(interval)
+
+			By("By checking that the controller has restored the Route spec")
+			Eventually(func() (string, error) {
+				route, err := getRouteFromList(route, notebook, Name, Namespace)
+				if route == nil {
+					return "", err
+				}
+				return route.Spec.To.Name, nil
+			}, duration, interval).Should(Equal(Name))
+			Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
+		})
+
+		It("Should recreate the Route when deleted", func() {
+			By("By deleting the notebook route")
+			Expect(cli.Delete(ctx, route)).Should(Succeed())
+			time.Sleep(interval)
+
+			By("By checking that the controller has recreated the Route")
+			Eventually(func() error {
+				route, err := getRouteFromList(route, notebook, Name, Namespace)
+				if route == nil {
+					return err
+				}
+				return nil
+			}, duration, interval).Should(Succeed())
+			Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
+		})
+
+		It("Should delete the Openshift Route", func() {
+			// Testenv cluster does not implement Kubernetes GC:
+			// https://book.kubebuilder.io/reference/envtest.html#testing-considerations
+			// To test that the deletion lifecycle works, test the ownership
+			// instead of asserting on existence.
+			expectedOwnerReference := metav1.OwnerReference{
+				APIVersion:         "kubeflow.org/v1",
+				Kind:               "Notebook",
+				Name:               Name,
+				UID:                notebook.GetObjectMeta().GetUID(),
+				Controller:         ptr.To(true),
+				BlockOwnerDeletion: ptr.To(true),
+			}
+
+			By("By checking that the Notebook owns the Route object")
+			Expect(route.GetObjectMeta().GetOwnerReferences()).To(ContainElement(expectedOwnerReference))
+
+			By("By deleting the recently created Notebook")
+			Expect(cli.Delete(ctx, notebook)).Should(Succeed())
+			time.Sleep(interval)
+
+			By("By checking that the Notebook is deleted")
+			Eventually(func() error {
+				key := types.NamespacedName{Name: Name, Namespace: Namespace}
+				return cli.Get(ctx, key, notebook)
+			}, duration, interval).Should(HaveOccurred())
+		})
+
+	})
+
 	// New test case for notebook update
 	When("Updating a Notebook", func() {
 		const (
@@ -972,6 +1098,28 @@ func createNotebook(name, namespace string) *nbv1.Notebook {
 	}
 }
 
+func getRouteFromList(route *routev1.Route, notebook *nbv1.Notebook, name, namespace string) (*routev1.Route, error) {
+	routeList := &routev1.RouteList{}
+	opts := []client.ListOption{
+		client.InNamespace(namespace),
+		client.MatchingLabels{"notebook-name": name},
+	}
+
+	err := cli.List(ctx, routeList, opts...)
+	if err != nil {
+		return nil, err
+	}
+
+	// Get the route from the list
+	for _, nRoute := range routeList.Items {
+		if metav1.IsControlledBy(&nRoute, notebook) {
+			*route = nRoute
+			return route, nil
+		}
+	}
+	return nil, errors.New("Route not found")
+}
+
 func createOAuthServiceAccount(name, namespace string) corev1.ServiceAccount {
 	return corev1.ServiceAccount{
 		ObjectMeta: metav1.ObjectMeta{

components/odh-notebook-controller/controllers/notebook_oauth.go

@@ -275,18 +275,32 @@ func (r *OpenshiftNotebookReconciler) ReconcileOAuthSecret(notebook *nbv1.Notebo
 func (r *OpenshiftNotebookReconciler) ReconcileOAuthClient(notebook *nbv1.Notebook, ctx context.Context) error {
 	log := logf.FromContext(ctx)
 
+	// Create the route if it does not already exist
 	route := &routev1.Route{}
-	err := r.Get(ctx, types.NamespacedName{
-		Name:      notebook.Name,
-		Namespace: notebook.Namespace,
-	}, route)
+	routeList := &routev1.RouteList{}
+
+	// List the routes in the notebook namespace with the notebook name label
+	opts := []client.ListOption{
+		client.InNamespace(notebook.Namespace),
+		client.MatchingLabels{"notebook-name": notebook.Name},
+	}
+
+	err := r.List(ctx, routeList, opts...)
 	if err != nil {
-		if apierrs.IsNotFound(err) {
-			log.Info("Route not found, cannot create OAuthClient yet", "route", notebook.Name)
-			return nil
+		log.Error(err, "Unable to list the Route")
+	}
+
+	// Get the route from the list
+	for _, nRoute := range routeList.Items {
+		if metav1.IsControlledBy(&nRoute, notebook) {
+			route = &nRoute
+			break
 		}
-		log.Error(err, "Failed to get Route for OAuthClient")
-		return err
+	}
+
+	if route.Name == "" && route.Namespace != notebook.Namespace {
+		log.Info("Route not found, cannot create OAuthClient yet", "route", notebook.Name)
+		return nil
 	}
 
 	err = r.createOAuthClient(notebook, ctx)
@@ -335,8 +349,8 @@ func (r *OpenshiftNotebookReconciler) createSecret(notebook *nbv1.Notebook, ctx
 }
 
 // NewNotebookOAuthRoute defines the desired OAuth route object
-func NewNotebookOAuthRoute(notebook *nbv1.Notebook) *routev1.Route {
-	route := NewNotebookRoute(notebook)
+func NewNotebookOAuthRoute(notebook *nbv1.Notebook, isGenerateName bool) *routev1.Route {
+	route := NewNotebookRoute(notebook, isGenerateName)
 	route.Spec.To.Name = notebook.Name + "-tls"
 	route.Spec.Port.TargetPort = intstr.FromString(OAuthServicePortName)
 	route.Spec.TLS.Termination = routev1.TLSTerminationReencrypt