odh notebook controller imagestreams logic: do not hard-code namespac…

…e to look in. Take namespace controller runs in

getControllerNamespace redundantly defined in network and networkpolicy code as well. Coordinate with @bartoszmajsak on fallback scenario

notebook_network use logr

fix

fix

fix

controlruntime.GetConfig instead of just inClusterConfig to also cover unit tests correctly

controlruntime.GetConfig without using it

fix

move controller namespace determination to main, pass on as Webhook Struct field to be used later

fix

fix

fix

fix

fix

fix

fix

fix

fix

fix

fix

fix

fix

fix

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

@@ -54,6 +54,7 @@ const (
 // OpenshiftNotebookReconciler holds the controller configuration.
 type OpenshiftNotebookReconciler struct {
 	client.Client
+	Namespace string
 	Scheme *runtime.Scheme
 	Log    logr.Logger
 }

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

@@ -19,10 +19,9 @@ import (
 	"context"
 	corev1 "k8s.io/api/core/v1"
 	netv1 "k8s.io/api/networking/v1"
-	"os"
 	"reflect"
-	"strings"
 
+	"github.com/go-logr/logr"
 	nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -44,7 +43,7 @@ func (r *OpenshiftNotebookReconciler) ReconcileAllNetworkPolicies(notebook *nbv1
 	log := r.Log.WithValues("notebook", notebook.Name, "namespace", notebook.Namespace)
 
 	// Generate the desired Network Policies
-	desiredNotebookNetworkPolicy := NewNotebookNetworkPolicy(notebook)
+	desiredNotebookNetworkPolicy := NewNotebookNetworkPolicy(notebook, log, r.Namespace)
 
 	// Create Network Policies if they do not already exist
 	err := r.reconcileNetworkPolicy(desiredNotebookNetworkPolicy, ctx, notebook)
@@ -129,11 +128,12 @@ func CompareNotebookNetworkPolicies(np1 netv1.NetworkPolicy, np2 netv1.NetworkPo
 }
 
 // NewNotebookNetworkPolicy defines the desired network policy for Notebook port
-func NewNotebookNetworkPolicy(notebook *nbv1.Notebook) *netv1.NetworkPolicy {
+func NewNotebookNetworkPolicy(notebook *nbv1.Notebook, log logr.Logger, namespace string) *netv1.NetworkPolicy {
 	npProtocol := corev1.ProtocolTCP
+	log.Info("Controller is running in namespace", "namespace", namespace)
 	namespaceSel := metav1.LabelSelector{
 		MatchLabels: map[string]string{
-			"kubernetes.io/metadata.name": getControllerNamespace(),
+			"kubernetes.io/metadata.name": namespace,
 		},
 	}
 	// Create a Kubernetes NetworkPolicy resource that allows all traffic to the oauth port of a notebook
@@ -209,15 +209,3 @@ func NewOAuthNetworkPolicy(notebook *nbv1.Notebook) *netv1.NetworkPolicy {
 		},
 	}
 }
-
-func getControllerNamespace() string {
-	// TODO:Add env variable that stores namespace for both controllers.
-	if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {
-		if ns := strings.TrimSpace(string(data)); len(ns) > 0 {
-			return ns
-		}
-	}
-
-	// Fallback to default namespace, keep default as redhat-ods-applications
-	return "redhat-ods-applications"
-}

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

@@ -48,6 +48,8 @@ type NotebookWebhook struct {
 	Config      *rest.Config
 	Decoder     *admission.Decoder
 	OAuthConfig OAuthConfig
+	// controller namespace
+	Namespace string
 }
 
 // InjectReconciliationLock injects the kubeflow notebook controller culling
@@ -257,7 +259,7 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm
 	// Check Imagestream Info both on create and update operations
 	if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update {
 		// Check Imagestream Info
-		err = SetContainerImageFromRegistry(ctx, w.Config, notebook, log)
+		err = SetContainerImageFromRegistry(ctx, w.Config, notebook, log, w.Namespace)
 		if err != nil {
 			return admission.Errored(http.StatusInternalServerError, err)
 		}
@@ -458,7 +460,7 @@ func InjectCertConfig(notebook *nbv1.Notebook, configMapName string) error {
 // If an internal registry is detected, it uses the default values specified in the Notebook Custom Resource (CR).
 // Otherwise, it checks the last-image-selection annotation to find the image stream and fetches the image from status.dockerImageReference,
 // assigning it to the container.image value.
-func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, notebook *nbv1.Notebook, log logr.Logger) error {
+func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, notebook *nbv1.Notebook, log logr.Logger, namespace string) error {
 	// Create a dynamic client
 	dynamicClient, err := dynamic.NewForConfig(config)
 	if err != nil {
@@ -497,63 +499,56 @@ func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, not
 							return fmt.Errorf("invalid image selection format")
 						}
 
-						// Specify the namespaces to search in
-						namespaces := []string{"opendatahub", "redhat-ods-applications"}
 						imagestreamFound := false
-						for _, namespace := range namespaces {
-							// List imagestreams in the specified namespace
-							imagestreams, err := dynamicClient.Resource(ims).Namespace(namespace).List(ctx, metav1.ListOptions{})
-							if err != nil {
-								log.Info("Cannot list imagestreams", "error", err)
-								continue
-							}
+						// List imagestreams in the specified namespace
+						imagestreams, err := dynamicClient.Resource(ims).Namespace(namespace).List(ctx, metav1.ListOptions{})
+						if err != nil {
+							log.Info("Cannot list imagestreams", "error", err)
+							continue
+						}
 
-							// Iterate through the imagestreams to find matches
-							for _, item := range imagestreams.Items {
-								metadata := item.Object["metadata"].(map[string]interface{})
-								name := metadata["name"].(string)
-
-								// Match with the ImageStream name
-								if name == imageSelected[0] {
-									status := item.Object["status"].(map[string]interface{})
-
-									// Match to the corresponding tag of the image
-									tags := status["tags"].([]interface{})
-									for _, t := range tags {
-										tagMap := t.(map[string]interface{})
-										tagName := tagMap["tag"].(string)
-										if tagName == imageSelected[1] {
-											items := tagMap["items"].([]interface{})
-											if len(items) > 0 {
-												// Sort items by creationTimestamp to get the most recent one
-												sort.Slice(items, func(i, j int) bool {
-													iTime := items[i].(map[string]interface{})["created"].(string)
-													jTime := items[j].(map[string]interface{})["created"].(string)
-													return iTime > jTime // Lexicographical comparison of RFC3339 timestamps
-												})
-												imageHash := items[0].(map[string]interface{})["dockerImageReference"].(string)
-												// Update the Containers[i].Image value
-												notebook.Spec.Template.Spec.Containers[i].Image = imageHash
-												// Update the JUPYTER_IMAGE environment variable with the image selection for example "jupyter-datascience-notebook:2023.2"
-												for i, envVar := range container.Env {
-													if envVar.Name == "JUPYTER_IMAGE" {
-														container.Env[i].Value = imageSelection
-														break
-													}
+						// Iterate through the imagestreams to find matches
+						for _, item := range imagestreams.Items {
+							metadata := item.Object["metadata"].(map[string]interface{})
+							name := metadata["name"].(string)
+
+							// Match with the ImageStream name
+							if name == imageSelected[0] {
+								status := item.Object["status"].(map[string]interface{})
+
+								// Match to the corresponding tag of the image
+								tags := status["tags"].([]interface{})
+								for _, t := range tags {
+									tagMap := t.(map[string]interface{})
+									tagName := tagMap["tag"].(string)
+									if tagName == imageSelected[1] {
+										items := tagMap["items"].([]interface{})
+										if len(items) > 0 {
+											// Sort items by creationTimestamp to get the most recent one
+											sort.Slice(items, func(i, j int) bool {
+												iTime := items[i].(map[string]interface{})["created"].(string)
+												jTime := items[j].(map[string]interface{})["created"].(string)
+												return iTime > jTime // Lexicographical comparison of RFC3339 timestamps
+											})
+											imageHash := items[0].(map[string]interface{})["dockerImageReference"].(string)
+											// Update the Containers[i].Image value
+											notebook.Spec.Template.Spec.Containers[i].Image = imageHash
+											// Update the JUPYTER_IMAGE environment variable with the image selection for example "jupyter-datascience-notebook:2023.2"
+											for i, envVar := range container.Env {
+												if envVar.Name == "JUPYTER_IMAGE" {
+													container.Env[i].Value = imageSelection
+													break
 												}
-												imagestreamFound = true
-												break
 											}
+											imagestreamFound = true
+											break
 										}
 									}
 								}
 							}
-							if imagestreamFound {
-								break
-							}
 						}
 						if !imagestreamFound {
-							log.Error(nil, "Imagestream not found in any of the specified namespaces", "imageSelected", imageSelected[0], "tag", imageSelected[1])
+							log.Error(nil, "Imagestream not found in main controller namespace", "imageSelected", imageSelected[0], "tag", imageSelected[1], "namespace", namespace)
 						}
 					}
 				}

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

@@ -66,6 +66,7 @@ var (
 const (
 	timeout  = time.Second * 10
 	interval = time.Second * 2
+	odhNotebookControllerTestNamespace = "redhat-ods-applications"
 )
 
 func TestAPIs(t *testing.T) {
@@ -134,6 +135,7 @@ var _ = BeforeSuite(func() {
 		Client: mgr.GetClient(),
 		Log:    ctrl.Log.WithName("controllers").WithName("notebook-controller"),
 		Scheme: mgr.GetScheme(),
+		Namespace: odhNotebookControllerTestNamespace,
 	}).SetupWithManager(mgr)
 	Expect(err).ToNot(HaveOccurred())
 
@@ -144,6 +146,7 @@ var _ = BeforeSuite(func() {
 			Log:    ctrl.Log.WithName("controllers").WithName("notebook-controller"),
 			Client: mgr.GetClient(),
 			Config: mgr.GetConfig(),
+			Namespace: odhNotebookControllerTestNamespace,
 			OAuthConfig: OAuthConfig{
 				ProxyImage: OAuthProxyImage,
 			},

components/odh-notebook-controller/main.go

@@ -17,6 +17,7 @@ package main
 
 import (
 	"flag"
+	"fmt"
 	"os"
 	"time"
 
@@ -59,6 +60,17 @@ func init() {
 	//+kubebuilder:scaffold:scheme
 }
 
+func getControllerNamespace() (string, error) {
+	// Try to get the namespace from the service account secret
+	if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {
+		if ns := string(data); len(ns) > 0 {
+			return ns, nil
+		}
+	}
+
+	return "", fmt.Errorf("unable to determine the namespace")
+}
+
 func main() {
 	var metricsAddr, probeAddr, oauthProxyImage string
 	var webhookPort int
@@ -104,9 +116,17 @@ func main() {
 	}
 
 	// Setup notebook controller
+	// determine and set the controller namespace
+	namespace, err := getControllerNamespace()
+	if err != nil {
+		setupLog.Error(err, "Error during determining controller / main namespace")
+		os.Exit(1)
+	}
+	setupLog.Info("Controller is running in namespace", "namespace", namespace)
 	if err = (&controllers.OpenshiftNotebookReconciler{
 		Client: mgr.GetClient(),
 		Log:    ctrl.Log.WithName("controllers").WithName("Notebook"),
+		Namespace: namespace,
 		Scheme: mgr.GetScheme(),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "Notebook")
@@ -120,6 +140,7 @@ func main() {
 			Log:    ctrl.Log.WithName("controllers").WithName("Notebook"),
 			Client: mgr.GetClient(),
 			Config: mgr.GetConfig(),
+			Namespace: namespace,
 			OAuthConfig: controllers.OAuthConfig{
 				ProxyImage: oauthProxyImage,
 			},