From 29d9b9ed798b113b9e7d2c1bc919b8d6eac1210d Mon Sep 17 00:00:00 2001 From: atheo89 Date: Fri, 17 May 2024 11:24:36 +0200 Subject: [PATCH 1/3] Add logic on nbc-webhook to pickup internal or external registry --- .../controllers/notebook_webhook.go | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index 5dde927cef0..0a94bd5ac27 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -20,6 +20,10 @@ import ( "encoding/json" "fmt" "net/http" + "os" + "path/filepath" + "sort" + "strings" "github.com/go-logr/logr" nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1" @@ -28,7 +32,11 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/clientcmd" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -246,6 +254,12 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm if err != nil { return admission.Errored(http.StatusInternalServerError, err) } + + // Check Imagestream Info + err = SetContainerImageFromRegistry(ctx, w.Client, notebook, log) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } } // Inject the OAuth proxy if the annotation is present but only if Service Mesh is disabled @@ -438,3 +452,112 @@ func InjectCertConfig(notebook *nbv1.Notebook, configMapName string) error { } return nil } + +// This function checks if there is an internal registry and takes the corresponding actions to set the container.image value. +// 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, cli client.Client, notebook *nbv1.Notebook, log logr.Logger) error { + + // Load kubeconfig + config, err := rest.InClusterConfig() + if err != nil { + kubeconfig := os.Getenv("KUBECONFIG") + if kubeconfig == "" { + kubeconfig = filepath.Join(os.Getenv("HOME"), ".kube", "config") + } + config, err = clientcmd.BuildConfigFromFlags("", kubeconfig) + if err != nil { + log.Error(err, "Error creating config") + return err + } + } + + // Create a dynamic client + dynamicClient, err := dynamic.NewForConfig(config) + if err != nil { + log.Error(err, "Error creating dynamic client") + return err + } + // Specify the GroupVersionResource for imagestreams + ims := schema.GroupVersionResource{ + Group: "image.openshift.io", + Version: "v1", + Resource: "imagestreams", + } + + annotations := notebook.GetAnnotations() + if annotations != nil { + if imageSelection, exists := annotations["notebooks.opendatahub.io/last-image-selection"]; exists { + + // Check if the image selection has an internal registry, if so will pickup this. This value constructed on the initialization of the Notebook CR. + if strings.Contains(notebook.Spec.Template.Spec.Containers[0].Image, "image-registry.openshift-image-registry.svc:5000") { + log.Info("Internal registry found. Will pickup the default value from image field.") + return nil + } else { + // Split the imageSelection to imagestream and tag + parts := strings.Split(imageSelection, ":") + if len(parts) != 2 { + log.Error(nil, "Invalid image selection format") + return fmt.Errorf("invalid image selection format") + } + + imagestreamName := parts[0] + tag := parts[1] + + // Specify the namespaces to search in + namespaces := []string{"opendatahub", "redhat-ods-applications"} + + 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.Error(err, "Cannot list imagestreams", "namespace", namespace) + continue + } + + // Iterate through the imagestreams to find matches + for _, item := range imagestreams.Items { + metadata := item.Object["metadata"].(map[string]interface{}) + name := metadata["name"].(string) + + if name == imagestreamName { + status := item.Object["status"].(map[string]interface{}) + + log.Info("No Internal registry found, pick up imageHash from status.tag.dockerImageReference") + + tags := status["tags"].([]interface{}) + for _, t := range tags { + tagMap := t.(map[string]interface{}) + tagName := tagMap["tag"].(string) + if tagName == tag { + 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 + }) + imageHash := items[0].(map[string]interface{})["dockerImageReference"].(string) + notebook.Spec.Template.Spec.Containers[0].Image = imageHash + // Update the JUPYTER_IMAGE environment variable + for i, envVar := range notebook.Spec.Template.Spec.Containers[0].Env { + if envVar.Name == "JUPYTER_IMAGE" { + notebook.Spec.Template.Spec.Containers[0].Env[i].Value = imageHash + break + } + } + return nil + } + } + } + } + } + } + } + } + } + + return nil +} From f83181ac4ec1d66f0bd8396a51191ef522728910 Mon Sep 17 00:00:00 2001 From: atheo89 Date: Mon, 20 May 2024 12:23:39 +0200 Subject: [PATCH 2/3] create a temporary kubeconfig file for the test setup --- .../controllers/suite_test.go | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 35afb8810a8..8ffc955746e 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -1,5 +1,4 @@ /* - Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at @@ -19,7 +18,9 @@ import ( "context" "crypto/tls" "fmt" + "io/ioutil" "net" + "os" "path/filepath" "testing" "time" @@ -83,6 +84,36 @@ var _ = BeforeSuite(func() { } logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseFlagOptions(&opts))) + // Create a temporary kubeconfig file + kubeconfigContent := []byte(` +apiVersion: v1 +clusters: +- cluster: + server: https://localhost:6443 + name: local +contexts: +- context: + cluster: local + user: user + name: local +current-context: local +kind: Config +preferences: {} +users: +- name: user + user: + token: fake-token +`) + tmpDir, err := ioutil.TempDir("", "kubeconfig") + Expect(err).NotTo(HaveOccurred()) + + kubeconfigPath := filepath.Join(tmpDir, "config") + err = ioutil.WriteFile(kubeconfigPath, kubeconfigContent, 0644) + Expect(err).NotTo(HaveOccurred()) + + // Set the KUBECONFIG environment variable + os.Setenv("KUBECONFIG", kubeconfigPath) + // Initiliaze test environment: // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/envtest#Environment.Start By("Bootstrapping test environment") @@ -98,7 +129,7 @@ var _ = BeforeSuite(func() { }, } - cfg, err := envTest.Start() + cfg, err = envTest.Start() Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) From 1f4b6230d46278cd9c88e4480f700368a3b9bbee Mon Sep 17 00:00:00 2001 From: atheo89 Date: Tue, 21 May 2024 11:02:03 +0200 Subject: [PATCH 3/3] Pass kubeconfig as parameter,and handle the scenario of no namespaces that include the imagestream --- .../controllers/notebook_webhook.go | 208 +++++++++--------- .../controllers/suite_test.go | 35 +-- components/odh-notebook-controller/main.go | 1 + 3 files changed, 103 insertions(+), 141 deletions(-) diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index 0a94bd5ac27..5e7cee5acf1 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -20,8 +20,6 @@ import ( "encoding/json" "fmt" "net/http" - "os" - "path/filepath" "sort" "strings" @@ -36,7 +34,6 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/dynamic" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -48,6 +45,7 @@ import ( type NotebookWebhook struct { Log logr.Logger Client client.Client + Config *rest.Config Decoder *admission.Decoder OAuthConfig OAuthConfig } @@ -256,7 +254,7 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm } // Check Imagestream Info - err = SetContainerImageFromRegistry(ctx, w.Client, notebook, log) + err = SetContainerImageFromRegistry(ctx, w.Config, notebook, log) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } @@ -453,111 +451,105 @@ func InjectCertConfig(notebook *nbv1.Notebook, configMapName string) error { return nil } -// This function checks if there is an internal registry and takes the corresponding actions to set the container.image value. +// SetContainerImageFromRegistry checks if there is an internal registry and takes the corresponding actions to set the container.image value. // 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, cli client.Client, notebook *nbv1.Notebook, log logr.Logger) error { - - // Load kubeconfig - config, err := rest.InClusterConfig() - if err != nil { - kubeconfig := os.Getenv("KUBECONFIG") - if kubeconfig == "" { - kubeconfig = filepath.Join(os.Getenv("HOME"), ".kube", "config") - } - config, err = clientcmd.BuildConfigFromFlags("", kubeconfig) - if err != nil { - log.Error(err, "Error creating config") - return err - } - } - - // Create a dynamic client - dynamicClient, err := dynamic.NewForConfig(config) - if err != nil { - log.Error(err, "Error creating dynamic client") - return err - } - // Specify the GroupVersionResource for imagestreams - ims := schema.GroupVersionResource{ - Group: "image.openshift.io", - Version: "v1", - Resource: "imagestreams", - } - - annotations := notebook.GetAnnotations() - if annotations != nil { - if imageSelection, exists := annotations["notebooks.opendatahub.io/last-image-selection"]; exists { - - // Check if the image selection has an internal registry, if so will pickup this. This value constructed on the initialization of the Notebook CR. - if strings.Contains(notebook.Spec.Template.Spec.Containers[0].Image, "image-registry.openshift-image-registry.svc:5000") { - log.Info("Internal registry found. Will pickup the default value from image field.") - return nil - } else { - // Split the imageSelection to imagestream and tag - parts := strings.Split(imageSelection, ":") - if len(parts) != 2 { - log.Error(nil, "Invalid image selection format") - return fmt.Errorf("invalid image selection format") - } - - imagestreamName := parts[0] - tag := parts[1] - - // Specify the namespaces to search in - namespaces := []string{"opendatahub", "redhat-ods-applications"} - - 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.Error(err, "Cannot list imagestreams", "namespace", namespace) - continue - } - - // Iterate through the imagestreams to find matches - for _, item := range imagestreams.Items { - metadata := item.Object["metadata"].(map[string]interface{}) - name := metadata["name"].(string) - - if name == imagestreamName { - status := item.Object["status"].(map[string]interface{}) - - log.Info("No Internal registry found, pick up imageHash from status.tag.dockerImageReference") - - tags := status["tags"].([]interface{}) - for _, t := range tags { - tagMap := t.(map[string]interface{}) - tagName := tagMap["tag"].(string) - if tagName == tag { - 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 - }) - imageHash := items[0].(map[string]interface{})["dockerImageReference"].(string) - notebook.Spec.Template.Spec.Containers[0].Image = imageHash - // Update the JUPYTER_IMAGE environment variable - for i, envVar := range notebook.Spec.Template.Spec.Containers[0].Env { - if envVar.Name == "JUPYTER_IMAGE" { - notebook.Spec.Template.Spec.Containers[0].Env[i].Value = imageHash - break - } - } - return nil - } - } - } - } - } - } - } - } - } - - return nil +func SetContainerImageFromRegistry(ctx context.Context, config *rest.Config, notebook *nbv1.Notebook, log logr.Logger) error { + // Create a dynamic client + dynamicClient, err := dynamic.NewForConfig(config) + if err != nil { + log.Error(err, "Error creating dynamic client") + return err + } + // Specify the GroupVersionResource for imagestreams + ims := schema.GroupVersionResource{ + Group: "image.openshift.io", + Version: "v1", + Resource: "imagestreams", + } + + annotations := notebook.GetAnnotations() + if annotations != nil { + if imageSelection, exists := annotations["notebooks.opendatahub.io/last-image-selection"]; exists { + // Check if the image selection has an internal registry, if so will pickup this. This value constructed on the initialization of the Notebook CR. + if strings.Contains(notebook.Spec.Template.Spec.Containers[0].Image, "image-registry.openshift-image-registry.svc:5000") { + log.Info("Internal registry found. Will pickup the default value from image field.") + return nil + } else { + // Split the imageSelection to imagestream and tag + parts := strings.Split(imageSelection, ":") + if len(parts) != 2 { + log.Error(nil, "Invalid image selection format") + return fmt.Errorf("invalid image selection format") + } + + imagestreamName := parts[0] + tag := parts[1] + + // 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.Error(err, "Cannot list imagestreams", "namespace", namespace) + continue + } + + // Iterate through the imagestreams to find matches + for _, item := range imagestreams.Items { + metadata := item.Object["metadata"].(map[string]interface{}) + name := metadata["name"].(string) + + if name == imagestreamName { + status := item.Object["status"].(map[string]interface{}) + + log.Info("No Internal registry found, pick up imageHash from status.tag.dockerImageReference") + + tags := status["tags"].([]interface{}) + for _, t := range tags { + tagMap := t.(map[string]interface{}) + tagName := tagMap["tag"].(string) + if tagName == tag { + 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) + notebook.Spec.Template.Spec.Containers[0].Image = imageHash + // Update the JUPYTER_IMAGE environment variable + for i, envVar := range notebook.Spec.Template.Spec.Containers[0].Env { + if envVar.Name == "JUPYTER_IMAGE" { + notebook.Spec.Template.Spec.Containers[0].Env[i].Value = imageHash + break + } + } + imagestreamFound = true + break + } + } + } + } + } + if imagestreamFound { + break + } + } + + if !imagestreamFound { + log.Info("Imagestream not found in any of the specified namespaces", "imagestreamName", imagestreamName, "tag", tag) + } + } + } + } + + return nil } diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index 8ffc955746e..9e8decba191 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -18,9 +18,7 @@ import ( "context" "crypto/tls" "fmt" - "io/ioutil" "net" - "os" "path/filepath" "testing" "time" @@ -84,36 +82,6 @@ var _ = BeforeSuite(func() { } logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseFlagOptions(&opts))) - // Create a temporary kubeconfig file - kubeconfigContent := []byte(` -apiVersion: v1 -clusters: -- cluster: - server: https://localhost:6443 - name: local -contexts: -- context: - cluster: local - user: user - name: local -current-context: local -kind: Config -preferences: {} -users: -- name: user - user: - token: fake-token -`) - tmpDir, err := ioutil.TempDir("", "kubeconfig") - Expect(err).NotTo(HaveOccurred()) - - kubeconfigPath := filepath.Join(tmpDir, "config") - err = ioutil.WriteFile(kubeconfigPath, kubeconfigContent, 0644) - Expect(err).NotTo(HaveOccurred()) - - // Set the KUBECONFIG environment variable - os.Setenv("KUBECONFIG", kubeconfigPath) - // Initiliaze test environment: // https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/envtest#Environment.Start By("Bootstrapping test environment") @@ -129,7 +97,7 @@ users: }, } - cfg, err = envTest.Start() + cfg, err := envTest.Start() Expect(err).NotTo(HaveOccurred()) Expect(cfg).NotTo(BeNil()) @@ -172,6 +140,7 @@ users: Handler: &NotebookWebhook{ Log: ctrl.Log.WithName("controllers").WithName("notebook-controller"), Client: mgr.GetClient(), + Config: mgr.GetConfig(), OAuthConfig: OAuthConfig{ ProxyImage: OAuthProxyImage, }, diff --git a/components/odh-notebook-controller/main.go b/components/odh-notebook-controller/main.go index f4adc8b0fb6..bef4615c3b0 100644 --- a/components/odh-notebook-controller/main.go +++ b/components/odh-notebook-controller/main.go @@ -116,6 +116,7 @@ func main() { Handler: &controllers.NotebookWebhook{ Log: ctrl.Log.WithName("controllers").WithName("Notebook"), Client: mgr.GetClient(), + Config: mgr.GetConfig(), OAuthConfig: controllers.OAuthConfig{ ProxyImage: oauthProxyImage, },