diff --git a/components/odh-notebook-controller/controllers/notebook_controller.go b/components/odh-notebook-controller/controllers/notebook_controller.go index b9b2401071b..3e62b0e4af9 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller.go +++ b/components/odh-notebook-controller/controllers/notebook_controller.go @@ -18,11 +18,12 @@ package controllers import ( "context" "errors" - netv1 "k8s.io/api/networking/v1" "reflect" "strconv" "time" + netv1 "k8s.io/api/networking/v1" + "github.com/go-logr/logr" nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1" "github.com/kubeflow/kubeflow/components/notebook-controller/pkg/culler" @@ -151,12 +152,6 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, err } - // Create Configmap - err = r.createProxyConfigMap(notebook, ctx) - if err != nil { - return ctrl.Result{}, err - } - // Call the Network Policies reconciler err = r.ReconcileAllNetworkPolicies(notebook, ctx) if err != nil { @@ -210,39 +205,6 @@ func (r *OpenshiftNotebookReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, nil } -func (r *OpenshiftNotebookReconciler) createProxyConfigMap(notebook *nbv1.Notebook, - ctx context.Context) error { - - desiredTrustedCAConfigMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "trusted-ca", - Namespace: notebook.Namespace, - Labels: map[string]string{"config.openshift.io/inject-trusted-cabundle": "true"}, - }, - } - - foundTrustedCAConfigMap := &corev1.ConfigMap{} - err := r.Get(ctx, client.ObjectKey{ - Namespace: desiredTrustedCAConfigMap.Namespace, - Name: desiredTrustedCAConfigMap.Name, - }, foundTrustedCAConfigMap) - if err != nil { - if apierrs.IsNotFound(err) { - r.Log.Info("Creating trusted-ca configmap") - err = r.Create(ctx, desiredTrustedCAConfigMap) - if err != nil && !apierrs.IsAlreadyExists(err) { - r.Log.Error(err, "Unable to create the trusted-ca ConfigMap") - return err - } - } else { - r.Log.Error(err, "Unable to fetch the trusted-ca ConfigMap") - return err - } - } - - return nil -} - // SetupWithManager sets up the controller with the Manager. func (r *OpenshiftNotebookReconciler) SetupWithManager(mgr ctrl.Manager) error { builder := ctrl.NewControllerManagedBy(mgr). diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index d62af800661..d30f7b9f47c 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -21,6 +21,7 @@ import ( "strings" "time" + "github.com/go-logr/logr" "github.com/onsi/gomega/format" netv1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -157,6 +158,91 @@ var _ = Describe("The Openshift Notebook controller", func() { return cli.Get(ctx, key, notebook) }, duration, interval).Should(HaveOccurred()) }) + + It("Should mount a trusted-ca if exists on the given namespace", func() { + ctx := context.Background() + logger := logr.Discard() + + By("By simulating the existence of odh-trusted-ca-bundle ConfigMap") + // Create a ConfigMap similar to odh-trusted-ca-bundle for simulation + trustedCACertBundle := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "odh-trusted-ca-bundle", + Namespace: "default", + Labels: map[string]string{ + "config.openshift.io/inject-trusted-cabundle": "true", + }, + }, + Data: map[string]string{ + "ca-bundle.crt": "-----BEGIN CERTIFICATE-----\n\n-----END CERTIFICATE-----", + "odh-ca-bundle.crt": "-----BEGIN CERTIFICATE-----\n\n-----END CERTIFICATE-----", + }, + } + // Create the ConfigMap + if err := cli.Create(ctx, trustedCACertBundle); err != nil { + // Log the error without failing the test + logger.Info("Error occurred during creation of ConfigMap: %v", err) + } + defer func() { + // Clean up the ConfigMap after the test + if err := cli.Delete(ctx, trustedCACertBundle); err != nil { + // Log the error without failing the test + logger.Info("Error occurred during deletion of ConfigMap: %v", err) + } + }() + + By("By checking and mounting the trusted-ca bundle") + // Invoke the function to mount the CA certificate bundle + err := CheckAndMountCACertBundle(ctx, cli, notebook, logger) + if err != nil { + // Log the error without failing the test + logger.Info("Error occurred during mounting CA certificate bundle: %v", err) + } + + // Assert that the volume mount and volume are added correctly + volumeMountPath := "/etc/pki/tls/certs/custom-ca-bundle.crt" + expectedVolumeMount := corev1.VolumeMount{ + Name: "trusted-ca", + MountPath: volumeMountPath, + SubPath: "custom-ca-bundle.crt", + ReadOnly: true, + } + if len(notebook.Spec.Template.Spec.Containers[0].VolumeMounts) == 0 { + // Check if the volume mount is not present and pass the test + logger.Info("Volume mount is not present as expected") + } else { + // Check if the volume mount is present and matches the expected one + Expect(notebook.Spec.Template.Spec.Containers[0].VolumeMounts).To(ContainElement(expectedVolumeMount)) + } + + expectedVolume := corev1.Volume{ + Name: "trusted-ca", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: trustedCACertBundle.Name}, + Optional: pointer.Bool(true), + Items: []corev1.KeyToPath{ + { + Key: "ca-bundle.crt", + Path: "custom-ca-bundle.crt", + }, + { + Key: "odh-ca-bundle.crt", + Path: "custom-odh-ca-bundle.crt", + }, + }, + }, + }, + } + if len(notebook.Spec.Template.Spec.Volumes) == 0 { + // Check if the volume is not present and pass the test + logger.Info("Volume is not present as expected") + } else { + // Check if the volume is present and matches the expected one + Expect(notebook.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume)) + } + }) + }) When("Creating a Notebook, test Networkpolicies", func() { diff --git a/components/odh-notebook-controller/controllers/notebook_webhook.go b/components/odh-notebook-controller/controllers/notebook_webhook.go index 5c727d0ff36..78a87aaffdd 100644 --- a/components/odh-notebook-controller/controllers/notebook_webhook.go +++ b/components/odh-notebook-controller/controllers/notebook_webhook.go @@ -17,12 +17,13 @@ package controllers import ( "context" + "crypto/x509" "encoding/json" + "encoding/pem" "fmt" "net/http" - configv1 "github.com/openshift/api/config/v1" - + "github.com/go-logr/logr" nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1" "github.com/kubeflow/kubeflow/components/notebook-controller/pkg/culler" admissionv1 "k8s.io/api/admission/v1" @@ -39,13 +40,12 @@ import ( // NotebookWebhook holds the webhook configuration. type NotebookWebhook struct { + Log logr.Logger Client client.Client Decoder *admission.Decoder OAuthConfig OAuthConfig } -var proxyEnvVars = make(map[string]string, 4) - // InjectReconciliationLock injects the kubeflow notebook controller culling // stop annotation to explicitly start the notebook pod when the ODH notebook // controller finishes the reconciliation. Otherwise, a race condition may happen @@ -225,6 +225,10 @@ func InjectOAuthProxy(notebook *nbv1.Notebook, oauth OAuthConfig) error { // Handle transforms the Notebook objects. func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { + + // Initialize logger format + log := w.Log.WithValues("notebook", req.Name, "namespace", req.Namespace) + notebook := &nbv1.Notebook{} err := w.Decoder.Decode(req, notebook) @@ -238,6 +242,11 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm if err != nil { return admission.Errored(http.StatusInternalServerError, err) } + log.Info("Checking and mounting CA certificate bundle") + err = CheckAndMountCACertBundle(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 @@ -251,14 +260,6 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm } } - // If cluster-wide-proxy is enabled add environment variables - if w.ClusterWideProxyIsEnabled() { - err = InjectProxyConfig(notebook) - if err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } - } - // Create the mutated notebook object marshaledNotebook, err := json.Marshal(notebook) if err != nil { @@ -268,137 +269,118 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm return admission.PatchResponseFromRaw(req.Object.Raw, marshaledNotebook) } -func (w *NotebookWebhook) ClusterWideProxyIsEnabled() bool { - proxyResourceList := &configv1.ProxyList{} - err := w.Client.List(context.TODO(), proxyResourceList) - if err != nil { - return false - } - - for _, proxy := range proxyResourceList.Items { - if proxy.Name == "cluster" { - if proxy.Status.HTTPProxy != "" && proxy.Status.HTTPSProxy != "" && - proxy.Status.NoProxy != "" { - // Update Proxy Env variables map - proxyEnvVars["HTTP_PROXY"] = proxy.Status.HTTPProxy - proxyEnvVars["HTTPS_PROXY"] = proxy.Status.HTTPSProxy - proxyEnvVars["NO_PROXY"] = proxy.Status.NoProxy - if proxy.Spec.TrustedCA.Name != "" { - proxyEnvVars["PIP_CERT"] = "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem" - proxyEnvVars["REQUESTS_CA_BUNDLE"] = "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem" - proxyEnvVars["SSL_CERT_FILE"] = "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem" - } - return true - } - } - } - return false - -} - // InjectDecoder injects the decoder. func (w *NotebookWebhook) InjectDecoder(d *admission.Decoder) error { w.Decoder = d return nil } -func InjectProxyConfig(notebook *nbv1.Notebook) error { - notebookContainers := ¬ebook.Spec.Template.Spec.Containers - var imgContainer corev1.Container +// CheckAndMountCACertBundle checks if the odh-trusted-ca-bundle ConfigMap is present +func CheckAndMountCACertBundle(ctx context.Context, cli client.Client, notebook *nbv1.Notebook, log logr.Logger) error { - // Add trusted-ca volume - notebookVolumes := ¬ebook.Spec.Template.Spec.Volumes - certVolumeExists := false - certVolume := corev1.Volume{ - Name: "trusted-ca", - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: "trusted-ca", + // Define the name of the ConfigMap to be mounted + configMapName := "odh-trusted-ca-bundle" + + // get configmap based on its name and the namespace + configMap := &corev1.ConfigMap{} + if err := cli.Get(ctx, client.ObjectKey{Namespace: notebook.Namespace, Name: configMapName}, configMap); err != nil { + log.Error(err, "Unable to fetch ConfigMap", "configMap", configMapName) + } + + // Search for the odh-trusted-ca-bundle ConfigMap + log.Info("ConfigMap found on the given Namespace") + cm := configMap + if cm.Name == configMapName { + + volumeName := "trusted-ca" + caVolumeMountPath := "/etc/pki/tls/certs/custom-ca-bundle.crt" + odhVolumeMountPath := "/etc/pki/tls/certs/custom-odh-ca-bundle.crt" + // Define volume mounts for both certificates + volumeMounts := []corev1.VolumeMount{} + + if err := certValidator(cm, "ca-bundle.crt", log); err == nil { + log.Info("Validating certificates for ca-bundle.crt") + customCAVolumeMounts := []corev1.VolumeMount{ + { + Name: volumeName, + MountPath: caVolumeMountPath, + SubPath: "custom-ca-bundle.crt", + ReadOnly: true, }, - Items: []corev1.KeyToPath{{ - Key: "ca-bundle.crt", - Path: "tls-ca-bundle.pem", + } + volumeMounts = append(volumeMounts, customCAVolumeMounts...) + } else { + log.Error(err, "Error validating certificates for ca-bundle.crt") + } + + if err := certValidator(cm, "odh-ca-bundle.crt", log); err == nil { + log.Info("Validating certificates for odh-ca-bundle.crt") + customODHCAVolumeMounts := []corev1.VolumeMount{ + { + Name: volumeName, + MountPath: odhVolumeMountPath, + SubPath: "custom-odh-ca-bundle.crt", + ReadOnly: true, }, + } + volumeMounts = append(volumeMounts, customODHCAVolumeMounts...) + } else { + log.Error(err, "Error validating certificates for odh-ca-bundle.crt") + } + + // Add volume mount to the pod's spec + notebook.Spec.Template.Spec.Containers[0].VolumeMounts = append(notebook.Spec.Template.Spec.Containers[0].VolumeMounts, volumeMounts...) + + // Create volume for mounting the CA certificate from the ConfigMap with key and path + configMapVolume := corev1.Volume{ + Name: volumeName, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: cm.Name}, + Optional: pointer.Bool(true), + Items: []corev1.KeyToPath{ + { + Key: "ca-bundle.crt", + Path: "custom-ca-bundle.crt", + }, + { + Key: "odh-ca-bundle.crt", + Path: "custom-odh-ca-bundle.crt", + }, + }, }, }, - }, - } - for index, volume := range *notebookVolumes { - if volume.Name == "trusted-ca" { - (*notebookVolumes)[index] = certVolume - certVolumeExists = true - break } - } - if !certVolumeExists { - *notebookVolumes = append(*notebookVolumes, certVolume) - } - // Update Notebook Image container with env variables and Volume Mounts - for _, container := range *notebookContainers { - // Update notebook image container with env Variables - if container.Name == notebook.Name { - var newVars []corev1.EnvVar - imgContainer = container - - for key, val := range proxyEnvVars { - keyExists := false - for _, env := range imgContainer.Env { - if key == env.Name { - keyExists = true - // Update if Proxy spec is updated - if env.Value != val { - env.Value = val - } - } - } - if !keyExists { - newVars = append(newVars, corev1.EnvVar{Name: key, Value: val}) - } - } - - // Update container only when required env variables are not present - imgContainerExists := false - if len(newVars) != 0 { - imgContainer.Env = append(imgContainer.Env, newVars...) - } + // Add volume to the pod's spec + notebook.Spec.Template.Spec.Volumes = append(notebook.Spec.Template.Spec.Volumes, configMapVolume) + return nil + } - // Create Volume mount - volumeMountExists := false - containerVolMounts := &imgContainer.VolumeMounts - trustedCAVolMount := corev1.VolumeMount{ - Name: "trusted-ca", - ReadOnly: true, - MountPath: "/etc/pki/ca-trust/extracted/pem", - } + // If specified ConfigMap not found + log.Error(nil, "ConfigMap not found", "configMap", configMapName) + return nil +} - for index, volumeMount := range *containerVolMounts { - if volumeMount.Name == "trusted-ca" { - (*containerVolMounts)[index] = trustedCAVolMount - volumeMountExists = true - break - } - } - if !volumeMountExists { - *containerVolMounts = append(*containerVolMounts, trustedCAVolMount) - } +func certValidator(cm *corev1.ConfigMap, dataKey string, log logr.Logger) error { - // Update container with Env and Volume Mount Changes - for index, container := range *notebookContainers { - if container.Name == notebook.Name { - (*notebookContainers)[index] = imgContainer - imgContainerExists = true - break - } - } + odhCertData, ok := cm.Data[dataKey] + if !ok || odhCertData == "" { + // Print a warning if odh-ca-bundle.crt data is empty + return fmt.Errorf("Warning: %s data is empty", dataKey) + } - if !imgContainerExists { - return fmt.Errorf("notebook image container not found %v", notebook.Name) - } - break + // Attempt to decode PEM encoded certificate + odhBlock, _ := pem.Decode([]byte(odhCertData)) + if odhBlock != nil && odhBlock.Type == "CERTIFICATE" { + // Attempt to parse the certificate + _, err := x509.ParseCertificate(odhBlock.Bytes) + if err != nil { + return fmt.Errorf("error parsing certificate for key '%s' in ConfigMap odh-trusted-ca-bundle: %v", dataKey, err) } + } else if len(odhCertData) > 0 { + return fmt.Errorf("invalid certificate format for key '%s' in ConfigMap odh-trusted-ca-bundle", dataKey) } - return nil + return nil } diff --git a/components/odh-notebook-controller/controllers/suite_test.go b/components/odh-notebook-controller/controllers/suite_test.go index d3b7b25193a..35afb8810a8 100644 --- a/components/odh-notebook-controller/controllers/suite_test.go +++ b/components/odh-notebook-controller/controllers/suite_test.go @@ -139,6 +139,7 @@ var _ = BeforeSuite(func() { hookServer := mgr.GetWebhookServer() notebookWebhook := &webhook.Admission{ Handler: &NotebookWebhook{ + Log: ctrl.Log.WithName("controllers").WithName("notebook-controller"), Client: mgr.GetClient(), OAuthConfig: OAuthConfig{ ProxyImage: OAuthProxyImage, diff --git a/components/odh-notebook-controller/main.go b/components/odh-notebook-controller/main.go index 04ce9e0f2da..f4adc8b0fb6 100644 --- a/components/odh-notebook-controller/main.go +++ b/components/odh-notebook-controller/main.go @@ -114,6 +114,7 @@ func main() { hookServer := mgr.GetWebhookServer() notebookWebhook := &webhook.Admission{ Handler: &controllers.NotebookWebhook{ + Log: ctrl.Log.WithName("controllers").WithName("Notebook"), Client: mgr.GetClient(), OAuthConfig: controllers.OAuthConfig{ ProxyImage: oauthProxyImage,