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

shalberd · 2024-04-23T09:41:09Z

/fixes #291

notebook containers have HTTP_PROXY, HTTPS_PROXY, NO_PROXY env vars injected if central cluster proxy config exists

Description

During the last round of updates related to CA trust, code was removed that looks for an openshift central proxy config and adds the centrally configured values for HTTP_PROXY, HTTPS_PROXY, NO_PROXY

How Has This Been Tested?

not tested yet, I'd take the built image from quay.io and test whether the three variables appear in my notebooks / workbench containers.

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

openshift-ci · 2024-04-23T09:41:29Z

Hi @shalberd. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

harshad16 · 2024-04-24T04:25:24Z

/ok-to-test

jiridanek · 2024-06-11T10:42:53Z

components/odh-notebook-controller/controllers/notebook_webhook.go

@@ -221,6 +224,29 @@ func InjectOAuthProxy(notebook *nbv1.Notebook, oauth OAuthConfig) error {
 	return nil
 }

+func (w *NotebookWebhook) ClusterWideProxyIsEnabled() bool {


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?

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

jiridanek · 2024-06-11T10:46:55Z

components/odh-notebook-controller/controllers/notebook_webhook.go

@@ -221,6 +224,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's already some context available in

kubeflow/components/odh-notebook-controller/controllers/notebook_webhook.go

Lines 250 to 251 in 7401baf

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

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.

jiridanek

This is adding back previously removed code. So it is IMO fair to ignore the possible "improvements" I suggested, or that others may come up with, and merge it as it is / as it was.

openshift-ci · 2024-06-11T11:48:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jiridanek
Once this PR has been reviewed and has the lgtm label, please assign atheo89 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shalberd · 2024-06-11T12:07:49Z

This is adding back previously removed code. So it is IMO fair to ignore the possible "improvements" I suggested, or that others may come up with, and merge it as it is / as it was.

Agreed, for tracking, I created issue #344

atheo89 · 2024-06-12T08:36:00Z

Hi @shalberd could you rebase your PR when you get a chance please? There have been many changes since it was opened :)

Current built image of this implementation is here: quay.io/opendatahub/odh-notebook-controller:pr-326

shalberd · 2024-06-12T10:38:42Z

Hi @atheo89 I don't get the message that my forked branch is out of date compared to base branch, so I cannot rebase via GUI
.
I mean, it says here "this branch has no conflicts with the base branch"

https://stackoverflow.com/questions/69839124/update-branch-with-rebase-instead-of-merge

Also, changes since April upstream in v1.7 branch (your odh-io/kubeflow commits ahead shown here: shalberd/kubeflow@add_back_default_proxy_env_vars_from_openshift_central_proxy_config...opendatahub-io:kubeflow:v1.7-branch
don't indicate a need for either a merge or a rebase.

Is a rebase really necessary here? If yes, then yes, I'll rebase via command line and in a home network this evening.

shalberd · 2024-06-12T11:16:55Z

/retest

…o notebook containers have HTTP_PROXY, HTTPS_PROXY, NO_PROXY env vars injected if central cluster proxy config exists

shalberd · 2024-06-12T11:57:06Z

Hi could you rebase your PR when you get a chance please? There have been many changes since it was opened :)
Current built image of this implementation is here: quay.io/opendatahub/odh-notebook-controller:pr-326

@atheo89 ok, I see where you're coming from ... the whole pull request image build process and so on.
@jiridanek I did a rebase with v1.7-branch and force-pushed again, so ok now. Tests and image build re-running as of now.

https://quay.io/repository/opendatahub/odh-notebook-controller?tab=tags&tag=pr-326

Current built image by digest:

quay.io/opendatahub/odh-notebook-controller@sha256:8a375c328467bc80c8dcb7a5514add02bfdb2269b819ccb4ccc9a0e158ae9596

i.e. when used by kustomization.yaml, replace newTag line

https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/config/base/kustomization.yaml#L9

with

digest: sha256:8a375c328467bc80c8dcb7a5514add02bfdb2269b819ccb4ccc9a0e158ae9596

there are a couple of golang x/net vulnerabilities related to http2, but that is best done and looked at in a separately.

https://quay.io/repository/opendatahub/odh-notebook-controller/manifest/sha256:8a375c328467bc80c8dcb7a5514add02bfdb2269b819ccb4ccc9a0e158ae9596?tab=vulnerabilities&fixable=true

Ah, I guess depend-a-bot already created a PR #324

jiridanek · 2024-06-13T15:43:12Z

Here's a tracking jira for it, https://issues.redhat.com/browse/RHOAIENG-8450

/lgtm

shalberd · 2024-07-31T09:38:30Z

@harshad16 ready to merge? This PR here is exclusively about proxy env var and value injection

harshad16 · 2024-07-31T15:52:27Z

@harshad16 ready to merge? This PR here is exclusively about proxy env var and value injection

Hey @shalberd , i will get this prioritized for next week 👍

jiridanek · 2024-09-20T07:13:32Z

/cherrypick v1.9-branch

openshift-cherrypick-robot · 2024-09-20T07:13:35Z

@jiridanek: once the present PR merges, I will cherry-pick it on top of v1.9-branch in a new PR and assign it to you.

In response to this:

/cherrypick v1.9-branch

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jiridanek · 2024-09-20T07:13:40Z

/cherrypick stable

openshift-cherrypick-robot · 2024-09-20T07:13:42Z

@jiridanek: once the present PR merges, I will cherry-pick it on top of stable in a new PR and assign it to you.

In response to this:

/cherrypick stable

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jiridanek · 2024-09-26T07:39:50Z

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

shalberd · 2024-10-08T11:18:19Z

shall I change this PR to v1.9 branch?

jiridanek · 2024-10-08T11:19:36Z

no, against the main branch, please

i don't have any news about the odh/rhoai version this will be scheduled for, still

shalberd · 2024-10-08T11:37:05Z

i don't have any news about the odh/rhoai version this will be scheduled for, still

No problem, just wanted to prepare, given that you work with 2 branches now and cherry-picking.
I changed the branch to compare to here to main.

jiridanek · 2024-10-16T06:19:56Z

components/odh-notebook-controller/controllers/notebook_webhook.go

+			if proxy.Status.HTTPProxy != "" && proxy.Status.HTTPSProxy != "" &&
+				proxy.Status.NoProxy != "" {
+				// Update Proxy Env variables map
+				proxyEnvVars["HTTP_PROXY"] = proxy.Status.HTTPProxy


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.

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.

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.

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

Kubeflow Notebooks 2.0 (Kubeflow Workspaces) kubeflow/kubeflow#7156

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.

openshift-ci bot added the needs-ok-to-test label Apr 23, 2024

openshift-ci bot requested review from harshad16 and LaVLaS April 23, 2024 09:41

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Apr 24, 2024

jiridanek reviewed Jun 11, 2024

View reviewed changes

jiridanek approved these changes Jun 11, 2024

View reviewed changes

openshift-ci bot assigned jiridanek Jun 11, 2024

openshift-ci bot added the lgtm label Jun 11, 2024

shalberd mentioned this pull request Jun 11, 2024

detail improvement odh notebook controller proxy context #344

Open

add back notebook container env var from central proxy config logic s…

83f0f33

…o notebook containers have HTTP_PROXY, HTTPS_PROXY, NO_PROXY env vars injected if central cluster proxy config exists

shalberd force-pushed the add_back_default_proxy_env_vars_from_openshift_central_proxy_config branch from 7401baf to 83f0f33 Compare June 12, 2024 11:54

openshift-ci bot removed the lgtm label Jun 12, 2024

openshift-ci bot added the lgtm label Jun 13, 2024

openshift-ci bot changed the title ~~add back notebook container envs var from central proxy configs~~ RHOAIENG-8450: feat(odh-notebook-controller): add back notebook container envs var from central proxy configs Sep 26, 2024

shalberd changed the base branch from v1.7-branch to main October 8, 2024 11:35

jiridanek reviewed Oct 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shalberd commented Apr 23, 2024

openshift-ci bot commented Apr 23, 2024

harshad16 commented Apr 24, 2024

jiridanek Jun 11, 2024

shalberd Jun 11, 2024

jiridanek Jun 11, 2024

shalberd Jun 11, 2024

jiridanek left a comment

openshift-ci bot commented Jun 11, 2024

shalberd commented Jun 11, 2024

atheo89 commented Jun 12, 2024 •

edited

Loading

shalberd commented Jun 12, 2024 •

edited

Loading

shalberd commented Jun 12, 2024

shalberd commented Jun 12, 2024 •

edited

Loading

jiridanek commented Jun 13, 2024

shalberd commented Jul 31, 2024

harshad16 commented Jul 31, 2024

jiridanek commented Sep 20, 2024

openshift-cherrypick-robot commented Sep 20, 2024

jiridanek commented Sep 20, 2024

openshift-cherrypick-robot commented Sep 20, 2024

jiridanek commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

shalberd commented Oct 8, 2024

jiridanek commented Oct 8, 2024 •

edited

Loading

shalberd commented Oct 8, 2024

jiridanek Oct 16, 2024

shalberd Oct 16, 2024

shalberd Oct 16, 2024

jiridanek Oct 16, 2024

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

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

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

Are you sure you want to change the base?

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

Conversation

shalberd commented Apr 23, 2024

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Apr 23, 2024

harshad16 commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiridanek left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 11, 2024

shalberd commented Jun 11, 2024

atheo89 commented Jun 12, 2024 • edited Loading

shalberd commented Jun 12, 2024 • edited Loading

shalberd commented Jun 12, 2024

shalberd commented Jun 12, 2024 • edited Loading

jiridanek commented Jun 13, 2024

shalberd commented Jul 31, 2024

harshad16 commented Jul 31, 2024

jiridanek commented Sep 20, 2024

openshift-cherrypick-robot commented Sep 20, 2024

jiridanek commented Sep 20, 2024

openshift-cherrypick-robot commented Sep 20, 2024

jiridanek commented Sep 26, 2024 • edited by openshift-ci bot Loading

shalberd commented Oct 8, 2024

jiridanek commented Oct 8, 2024 • edited Loading

shalberd commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atheo89 commented Jun 12, 2024 •

edited

Loading

shalberd commented Jun 12, 2024 •

edited

Loading

shalberd commented Jun 12, 2024 •

edited

Loading

jiridanek commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

jiridanek commented Oct 8, 2024 •

edited

Loading