diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index daac2b87bce..e8b77ace91f 100644 --- 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 index 41e35ee800a..2908b7fae9b 100644 --- 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 index 4a42e5ceacf..47169d84e9c 100644 --- 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 index 0f3163bd758..5b0db331f81 100644 --- 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 index a9c93337db4..1cdc8ce65a4 100644 --- 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,9 +174,10 @@ 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()) @@ -183,9 +185,10 @@ var _ = BeforeSuite(func() { 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 index 698bca75a88..85fa78e0801 100644 --- 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, },