-
Notifications
You must be signed in to change notification settings - Fork 32
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,6 +19,7 @@ import ( | |||||
"context" | ||||||
"encoding/json" | ||||||
"fmt" | ||||||
configv1 "github.com/openshift/api/config/v1" | ||||||
"net/http" | ||||||
"sort" | ||||||
"strings" | ||||||
|
@@ -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 | ||||||
|
@@ -227,6 +230,29 @@ func InjectOAuthProxy(notebook *nbv1.Notebook, oauth OAuthConfig) error { | |||||
return nil | ||||||
} | ||||||
|
||||||
func (w *NotebookWebhook) ClusterWideProxyIsEnabled() bool { | ||||||
proxyResourceList := &configv1.ProxyList{} | ||||||
err := w.Client.List(context.TODO(), proxyResourceList) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's already some context available in kubeflow/components/odh-notebook-controller/controllers/notebook_webhook.go Lines 250 to 251 in 7401baf
why don't you pass it in as parameter and use that, instead of creating a TODO context? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||||
|
||||||
|
@@ -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 { | ||||||
|
@@ -289,6 +323,58 @@ func (w *NotebookWebhook) InjectDecoder(d *admission.Decoder) error { | |||||
return nil | ||||||
} | ||||||
|
||||||
func InjectProxyConfigEnvVars(notebook *nbv1.Notebook) error { | ||||||
notebookContainers := ¬ebook.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 { | ||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that
proxyEnvVars
does not need to be a global variable, especially since every call toClusterWideProxyIsEnabled
creates its content anew. Can you return the proxy env vars from the function?There was a problem hiding this comment.
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