Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RHOAIENG-8450: feat(odh-notebook-controller): add back notebook container envs var from central proxy configs #326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions components/odh-notebook-controller/controllers/notebook_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"context"
"encoding/json"
"fmt"
configv1 "github.com/openshift/api/config/v1"
"net/http"
"sort"
"strings"
Expand Down Expand Up @@ -50,6 +51,8 @@ type NotebookWebhook struct {
OAuthConfig OAuthConfig
}

var proxyEnvVars = make(map[string]string, 3)

// 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
Expand Down Expand Up @@ -227,6 +230,29 @@ func InjectOAuthProxy(notebook *nbv1.Notebook, oauth OAuthConfig) error {
return nil
}

func (w *NotebookWebhook) ClusterWideProxyIsEnabled() bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (w *NotebookWebhook) ClusterWideProxyIsEnabled() bool {
func (w *NotebookWebhook) ClusterWideProxyIsEnabled() (map[string]string, bool) {

I think that proxyEnvVars does not need to be a global variable, especially since every call to ClusterWideProxyIsEnabled creates its content anew. Can you return the proxy env vars from the function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jiridanek very good point in general. There are certainly improvements possible, I am unsure whether now (adding back previous functionality) or later. I or you could create a new ticket for tracking with your suggestion and link it to issue #291

proxyResourceList := &configv1.ProxyList{}
err := w.Client.List(context.TODO(), proxyResourceList)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already some context available in

// Handle transforms the Notebook objects.
func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) admission.Response {

why don't you pass it in as parameter and use that, instead of creating a TODO context?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also very good point, I am still learning about all the ins and outs / details of odh notebook controller and the possibilities in golang. For here, I was merely adding back @VaishnaviHire code from way back then.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing these on a running notebook will restart the notebook. So if I create a notebook with previous version of controller, and then update to a controller that contains this patch, in a cluster with proxy it will inject the proxy variables and that will restart my running notebooks.

We need to postpone the env vars update to when the notebook is restarted by the user, to prevent unexpected restart.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because there are running pods with notebook container in the podSpec that did not have proxy envs in it, and when anything in a pod spec changes, the container in the pod restarts.
@harshad16 @jiridanek
Is this a huge problem? I mean, where do you have long-running notebook pods? In RHOSDS, there is no central cluster proxy anyways, so the envs would not be set, and as far as I am aware, in the ODH community, nobody ever mentioned a concept of long-running, never to be stopped and restarted, notebook containers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to postpone the env vars update to when the notebook is restarted by the user, to prevent unexpected restart.

If that is really necessary, I would not know how to achieve this. In my view, it is overkill, but I realize you sometimes have differing requirements with environments with long running notebook containers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rewuirement that was discussed someehat in the Notebooks 2.0 designs that @thesuperzapper is leading.

There, admin initiated change to workspace config should display a Restart required notification and the user then decides when to restart and accept the config update.

I never saw this requirement of avoiding pod restarts spelled out explicitly, though.

proxyEnvVars["HTTPS_PROXY"] = proxy.Status.HTTPSProxy
proxyEnvVars["NO_PROXY"] = proxy.Status.NoProxy
return true
}
}
}
return false

}

// Handle transforms the Notebook objects.
func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) admission.Response {

Expand Down Expand Up @@ -274,6 +300,14 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm
}
}

// If cluster-wide-proxy is enabled add environment variables
if w.ClusterWideProxyIsEnabled() {
err = InjectProxyConfigEnvVars(notebook)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}
}

// Create the mutated notebook object
marshaledNotebook, err := json.Marshal(notebook)
if err != nil {
Expand All @@ -289,6 +323,58 @@ func (w *NotebookWebhook) InjectDecoder(d *admission.Decoder) error {
return nil
}

func InjectProxyConfigEnvVars(notebook *nbv1.Notebook) error {
notebookContainers := &notebook.Spec.Template.Spec.Containers
var imgContainer corev1.Container

// Update Notebook Image container with env variables from central cluster proxy config
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...)
}

// Update container with Proxy Env Changes
for index, container := range *notebookContainers {
if container.Name == notebook.Name {
(*notebookContainers)[index] = imgContainer
imgContainerExists = true
break
}
}

if !imgContainerExists {
return fmt.Errorf("notebook image container not found %v", notebook.Name)
}
break
}
}
return nil

}

// 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 {

Expand Down