Merge main into v1.10-branch with known resolved conflicts

Author: github-actions[bot]

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/config/rbac/role.yaml

@@ -33,6 +33,7 @@ rules:
   verbs:
   - get
   - list
+  - watch
 - apiGroups:
   - kubeflow.org
   resources:

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

@@ -76,7 +76,7 @@ type OpenshiftNotebookReconciler struct {
 // +kubebuilder:rbac:groups=oauth.openshift.io,resources=oauthclients,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=get;list;watch;create;update;patch
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups="image.openshift.io",resources=imagestreams,verbs=list;get
+// +kubebuilder:rbac:groups="image.openshift.io",resources=imagestreams,verbs=list;get;watch
 
 // CompareNotebooks checks if two notebooks are equal, if not return false.
 func CompareNotebooks(nb1 nbv1.Notebook, nb2 nbv1.Notebook) bool {
@@ -137,7 +137,7 @@ func (r *OpenshiftNotebookReconciler) RemoveReconciliationLock(notebook *nbv1.No
 				return err
 			}
 			if len(serviceAccount.ImagePullSecrets) == 0 {
-				return errors.New("Pull secret not mounted")
+				return errors.New("pull secret not mounted")
 			}
 			return nil
 		},
@@ -358,7 +358,7 @@ func (r *OpenshiftNotebookReconciler) CreateNotebookCertConfigMap(notebook *nbv1
 					r.Log.Info("Created workbench-trusted-ca-bundle ConfigMap", "namespace", notebook.Namespace, "notebook", notebook.Name)
 				}
 			}
-		} else if err == nil && !reflect.DeepEqual(foundTrustedCAConfigMap.Data, desiredTrustedCAConfigMap.Data) {
+		} else if !reflect.DeepEqual(foundTrustedCAConfigMap.Data, desiredTrustedCAConfigMap.Data) {
 			// some data has changed, update the ConfigMap
 			r.Log.Info("Updating workbench-trusted-ca-bundle ConfigMap", "namespace", notebook.Namespace, "notebook", notebook.Name)
 			foundTrustedCAConfigMap.Data = desiredTrustedCAConfigMap.Data

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 (
@@ -939,7 +1065,12 @@ var _ = Describe("The Openshift Notebook controller", func() {
 		})
 
 		It("Should not create OAuth secret", func() {
-			secrets := &corev1.SecretList{}
+			secrets := &corev1.SecretList{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "SecretList",
+					APIVersion: "v1",
+				},
+			}
 			Eventually(func() error {
 				return cli.List(context.Background(), secrets, client.InNamespace(namespace))
 			}, duration, interval).Should(Succeed())
@@ -967,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