RHOAIENG-8390: feat(odh-nbc): search for imagestreams only in the nam…

…espace the controller runs in (#375) * [Fix] RHOAIENG-8390: feat(odh-nbc): search for imagestreams only in the namespace the controller runs in (#375) * [Fix] search for imagestreams only in the namespace the controller runs in. - Take namespace that the controller runs in - Log that dynamically-determined namespace once at startup. - Use hard-coded namespace for unit tests. * review suggestion, fix comment in tests --------- Co-authored-by: Jiri Daněk <[email protected]>
opendatahub-io · Nov 27, 2024 · bb2dd26 · bb2dd26
1 parent f1be2a4
commit bb2dd26
Show file tree

Hide file tree

Showing 6 changed files with 93 additions and 93 deletions.
diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go
@@ -57,8 +57,9 @@ const (
 // OpenshiftNotebookReconciler holds the controller configuration.
 type OpenshiftNotebookReconciler struct {
 	client.Client
-	Scheme *runtime.Scheme
-	Log    logr.Logger
+	Namespace string
+	Scheme    *runtime.Scheme
+	Log       logr.Logger
 }
 
 // ClusterRole permissions

diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go
@@ -20,9 +20,7 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
-	"io/ioutil"
 	"os"
-	"strings"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -432,12 +430,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
 		notebook := createNotebook(Name, Namespace)
 
 		npProtocol := corev1.ProtocolTCP
-		testPodNamespace := "redhat-ods-applications"
-		if data, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {
-			if ns := strings.TrimSpace(string(data)); len(ns) > 0 {
-				testPodNamespace = ns
-			}
-		}
+		testPodNamespace := odhNotebookControllerTestNamespace
 
 		expectedNotebookNetworkPolicy := netv1.NetworkPolicy{
 			ObjectMeta: metav1.ObjectMeta{
@@ -462,9 +455,9 @@ var _ = Describe("The Openshift Notebook controller", func() {
 						},
 						From: []netv1.NetworkPolicyPeer{
 							{
-								// Since for unit tests we do not have context,
-								// namespace will fallback to test pod namespace
-								// if run in CI or `redhat-ods-applications` if run locally
+								// Since for unit tests the controller does not run in a cluster pod,
+								// it cannot detect its own pod's namespace. Therefore, we define it
+								// to be `redhat-ods-applications` (in suite_test.go)
 								NamespaceSelector: &metav1.LabelSelector{
 									MatchLabels: map[string]string{
 										"kubernetes.io/metadata.name": testPodNamespace,

diff --git a/components/odh-notebook-controller/controllers/notebook_network.go b/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,11 @@ 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
 	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 +208,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"
-}
diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go
@@ -49,6 +49,8 @@ type NotebookWebhook struct {
 	Config      *rest.Config
 	Decoder     *admission.Decoder
 	OAuthConfig OAuthConfig
+	// controller namespace
+	Namespace string
 }
 
 // InjectReconciliationLock injects the kubeflow notebook controller culling
@@ -254,7 +256,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)
 		}
@@ -536,7 +538,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 {
@@ -575,63 +577,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)
 						}
 					}
 				}

diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go
@@ -69,8 +69,9 @@ var (
 )
 
 const (
-	timeout  = time.Second * 10
-	interval = time.Second * 2
+	timeout                            = time.Second * 10
+	interval                           = time.Second * 2
+	odhNotebookControllerTestNamespace = "redhat-ods-applications"
 )
 
 func TestAPIs(t *testing.T) {
@@ -173,19 +174,21 @@ var _ = BeforeSuite(func() {
 
 	// Setup notebook controller
 	err = (&OpenshiftNotebookReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("notebook-controller"),
-		Scheme: mgr.GetScheme(),
+		Client:    mgr.GetClient(),
+		Log:       ctrl.Log.WithName("controllers").WithName("notebook-controller"),
+		Scheme:    mgr.GetScheme(),
+		Namespace: odhNotebookControllerTestNamespace,
 	}).SetupWithManager(mgr)
 	Expect(err).ToNot(HaveOccurred())
 
 	// Setup notebook mutating webhook
 	hookServer := mgr.GetWebhookServer()
 	notebookWebhook := &webhook.Admission{
 		Handler: &NotebookWebhook{
-			Log:    ctrl.Log.WithName("controllers").WithName("notebook-controller"),
-			Client: mgr.GetClient(),
-			Config: mgr.GetConfig(),
+			Log:       ctrl.Log.WithName("controllers").WithName("notebook-controller"),
+			Client:    mgr.GetClient(),
+			Config:    mgr.GetConfig(),
+			Namespace: odhNotebookControllerTestNamespace,
 			OAuthConfig: OAuthConfig{
 				ProxyImage: OAuthProxyImage,
 			},

diff --git a/components/odh-notebook-controller/main.go b/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,10 +116,18 @@ 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"),
-		Scheme: mgr.GetScheme(),
+		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")
 		os.Exit(1)
@@ -117,9 +137,10 @@ func main() {
 	hookServer := mgr.GetWebhookServer()
 	notebookWebhook := &webhook.Admission{
 		Handler: &controllers.NotebookWebhook{
-			Log:    ctrl.Log.WithName("controllers").WithName("Notebook"),
-			Client: mgr.GetClient(),
-			Config: mgr.GetConfig(),
+			Log:       ctrl.Log.WithName("controllers").WithName("Notebook"),
+			Client:    mgr.GetClient(),
+			Config:    mgr.GetConfig(),
+			Namespace: namespace,
 			OAuthConfig: controllers.OAuthConfig{
 				ProxyImage: oauthProxyImage,
 			},