[WIP] Injected oauth imagepullpolicy change and sha256 digest #119

shalberd · 2023-07-12T21:36:30Z

@atheo89 new PR due to rebase chaos

@harshad16 Currently, the ose-oauth-proxy sidecar container of a running notebook, handled by odh notebook controller, always pulls the image from an external location. Current ose-oauth-proxy image is v4.10.
This always image pulling is problematic from a stability perspective, as, should connectivity to an external repo ever cease, no image pulling on notebook pod start is possible.

Also, providing the image in tag format is not compatible with disconnected cluster installs that use mirroring and soft-linking from source to target. Those on-prem installs need image references to be in sha256 digest format.

Another reason for changing to sha256 digest format: Tag ose-oauth-proxy:v4.10 changes. If imagePullPolicy is set to IfNotPresent, what matters is that an image with that tag is already cached on one of the cluster nodes. So, to keep a unique image version and build status for ose-oauth-proxy, we need to go with sha256 digest format as well.

Description

changed from tag format to digest format in odh notebook controller webhook that adds an ose-oauth-proxy sidecar to a Notebook CR

Manifest link digest of v4.10 from July 6 2023:

ab112105ac37352a2a4916a39d6736f5db6ab4c29bad4467de8d613e80e9bb33

https://catalog.redhat.com/software/containers/openshift4/ose-oauth-proxy/5cdb2133bed8bd5717d5ae64?architecture=amd64&tag=v4.10.0-202306170106.p0.g799d414.assembly.stream&push_date=1688610772000&container-tabs=gti

Put in comments with respect to actual tag used and location to look at for details. Changed imagepullPolicy from Always to IfNotPresent.

Related PR in odh-manifests:

opendatahub-io/odh-manifests#868

How Has This Been Tested?

Please mark with ok to test.

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

…ferences

…ullPolicy

…resent

openshift-ci · 2023-07-12T21:36:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vaishnavihire 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

openshift-ci · 2023-07-12T21:36:42Z

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.

shalberd · 2023-07-12T21:38:29Z

@atheo89 found an odh notebook controller test was looking for old imagePullPolicy and thus did not run though. Furthermore, due to ServiceMesh recent changes, a whole bunch of stuff changed, so I just rebased and then created this new PR. Can you do an ok to test, please?

shalberd · 2023-07-12T21:39:03Z

components/odh-notebook-controller/config/manager/manager.yaml

@@ -25,7 +25,8 @@ spec:
          imagePullPolicy: Always
          command:
            - /manager
-          args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy:v4.10"]
+          # latest v4.10 manifest list digest for architecture AMD64. Used instead of tag format to be compatible with imagePullPolicy: IfNotPresent
+          args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:ab112105ac37352a2a4916a39d6736f5db6ab4c29bad4467de8d613e80e9bb33"]


discussion on manifest list digest vs image digest at opendatahub-io/odh-manifests#868 (comment)

@LaVLaS suggested leaving the notebook controller main management pod oauth proxy image topic (changes in line 28 and 29) out of the PR for now. That makes sense in that this PR here is about the injection of ose-oauth-proxy into Notebook pods under /controllers.

@harshad16 @atheo89 Also, @LaVLaS suggested for odh notebook controller manager itself here:

While we should be updating the ose-oauth-proxy image at a regular cadence, we should not be hardcoding the image digest into the container command argument of the Deployment object. This should be replaced with an environment variable that can be configured at when the kustomize manifest is parsed. This would also allow us to replace with as a CSV relatedImage or modified by the new operator.

opendatahub-io/odh-manifests#868 (comment)

I suggest to handle that in a separate PR. Created an issue #132

shalberd · 2023-07-12T21:40:10Z

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

@@ -740,7 +740,7 @@ func createOAuthContainer(name, namespace string) corev1.Container {
 	return corev1.Container{
 		Name:            "oauth-proxy",
 		Image:           OAuthProxyImage,
-		ImagePullPolicy: corev1.PullAlways,
+		ImagePullPolicy: corev1.PullIfNotPresent,


previously, test did not run thru because it was still assuming imagePullPolicy Always in IS vs MUST BE comparision during testing

atheo89 · 2023-07-13T06:47:41Z

/ok-to-test

shalberd · 2023-07-13T08:05:44Z

components/odh-notebook-controller/config/manager/manager.yaml

@@ -25,7 +25,8 @@ spec:
          imagePullPolicy: Always


kept Always here because this seems to be used internally, not in releases, for testing with the :latest image-tag of odh-notebook-controller, which necessitates imagePullPolicy: Always

shalberd · 2023-07-14T13:37:12Z

@atheo89 @LaVLaS do you know what is going on with test infra? All tests and checks are pending ...

shalberd · 2023-07-23T15:58:55Z

/retest

shalberd · 2023-07-23T16:09:43Z

@atheo89 @harshad16 tests turned out ok

main changes to summarize:

oauth proxy container ref in sha256 digest format, latest v4.10 digest as base.
change imagePullPolicy of injected oauth container of notebook CRs from Always to IfNotPresent

shalberd · 2023-07-24T12:36:47Z

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

-	OAuthProxyImage      = "registry.redhat.io/openshift4/ose-oauth-proxy:latest"
+        // use sha256 manifest link digest value of v4.10 image for AMD64 as default to be compatible with imagePullPolicy: IfNotPresent, overridable 
+        // taken from https://catalog.redhat.com/software/containers/openshift4/ose-oauth-proxy/5cdb2133bed8bd5717d5ae64?architecture=amd64&tag=v4.10.0-202306170106.p0.g799d414.assembly.stream&push_date=1688610772000&container-tabs=gti
+	OAuthProxyImage      = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:ab112105ac37352a2a4916a39d6736f5db6ab4c29bad4467de8d613e80e9bb33"


discussion on manifest list digest vs image digest at opendatahub-io/odh-manifests#868 (comment)

…e not related to oauth sidecar injection

…ng code

shalberd · 2023-07-25T05:46:50Z

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

-	OAuthProxyImage      = "registry.redhat.io/openshift4/ose-oauth-proxy:latest"
+        // use sha256 manifest list digest value of v4.10 image for AMD64 as default to be compatible with imagePullPolicy: IfNotPresent, overridable 
+        // taken from https://catalog.redhat.com/software/containers/openshift4/ose-oauth-proxy/5cdb2133bed8bd5717d5ae64?tag=v4.10.0-202306170106.p0.g799d414.assembly.stream&push_date=1688610772000&gti-tabs=get-the-source&container-tabs=gti
+	OAuthProxyImage      = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:ab112105ac37352a2a4916a39d6736f5db6ab4c29bad4467de8d613e80e9bb33"


@VaishnaviHire @cfchase @harshad16 @atheo89

question: is the value of OAuthProxyImage set or overridden by the argument of odh notebook controller manager?

I am asking because I've never seen ose-oauth-proxy:latest in injected notebook oauth sidecars.

kubeflow/components/odh-notebook-controller/main.go

Line 68 in 92739d6

flag.StringVar(&oauthProxyImage, "oauth-proxy-image", controllers.OAuthProxyImage,

instead, what seems to be used is the version of ose-oauth defined here:

https://github.com/opendatahub-io/kubeflow/blob/master/components/odh-notebook-controller/config/manager/manager.yaml#L24

…lu-labels update LABELs and remove unwanted ARGs for konflux builds

shalberd added 3 commits July 12, 2023 23:16

use v4.10 tag digest from July 6 for all ose-oauth container image re…

042ad6b

…ferences

odh notebook controller test createOAuthContainer IfNotPresent imagrP…

9e50be6

…ullPolicy

odh notebook controller webhook injected oauth imagepullpolicy ifnotp…

990a1b1

…resent

openshift-ci bot requested review from LaVLaS and VaishnaviHire July 12, 2023 21:36

openshift-ci bot added the needs-ok-to-test label Jul 12, 2023

shalberd commented Jul 12, 2023

View reviewed changes

shalberd mentioned this pull request Jul 12, 2023

odh notebook controller change v4.10 ose-oauth image to digest notati… #113

Closed

3 tasks

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Jul 13, 2023

shalberd commented Jul 13, 2023

View reviewed changes

This was referenced Jul 7, 2023

unify all references to ose-oauth-proxy to v4.10 latest digest opendatahub-io/odh-manifests#869

Closed

No Internal OCP registry would fail with current notebook setup. #112

Open

shalberd commented Jul 24, 2023

View reviewed changes

shalberd added 2 commits July 24, 2023 14:54

reverted change to digest in odh notebook controller manager pod caus…

4363e29

…e not related to oauth sidecar injection

changed comment in odh notebook-oauth injection in sha256 format gola…

434756d

…ng code

This was referenced Jul 24, 2023

ose-oauth-proxy image standardize to latest digest value of tag v4.10 opendatahub-io/data-science-pipelines-operator#218

Closed

change oauth proxy digest value to manifest list digest of tag v4.10 opendatahub-io/data-science-pipelines-operator#219

Merged

shalberd commented Jul 25, 2023

View reviewed changes

harshad16 mentioned this pull request Aug 4, 2023

ImageChangeTrigger annotation #104

Open

shalberd changed the title ~~Injected oauth imagepullpolicy change and sha256 digest~~ [WIP] Injected oauth imagepullpolicy change and sha256 digest Oct 25, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Oct 25, 2023

jiridanek mentioned this pull request Aug 1, 2024

RHOAIENG-9935: feat(nbcs): get ose-oauth-proxy image with PullIfNeeded policy and use digest to specify the image #374

Merged

5 tasks

atheo89 pushed a commit to atheo89/kubeflow that referenced this pull request Nov 7, 2024

Merge pull request opendatahub-io#119 from red-hat-data-services/konf…

7fa8201

…lu-labels update LABELs and remove unwanted ARGs for konflux builds

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Injected oauth imagepullpolicy change and sha256 digest #119

[WIP] Injected oauth imagepullpolicy change and sha256 digest #119

shalberd commented Jul 12, 2023 •

edited

Loading

openshift-ci bot commented Jul 12, 2023

openshift-ci bot commented Jul 12, 2023

shalberd commented Jul 12, 2023

shalberd Jul 12, 2023 •

edited

Loading

shalberd Jul 12, 2023

atheo89 commented Jul 13, 2023

shalberd Jul 13, 2023

shalberd commented Jul 14, 2023

shalberd commented Jul 23, 2023

shalberd commented Jul 23, 2023

shalberd Jul 24, 2023

shalberd Jul 25, 2023 •

edited

Loading

[WIP] Injected oauth imagepullpolicy change and sha256 digest #119

Are you sure you want to change the base?

[WIP] Injected oauth imagepullpolicy change and sha256 digest #119

Conversation

shalberd commented Jul 12, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Jul 12, 2023

openshift-ci bot commented Jul 12, 2023

shalberd commented Jul 12, 2023

shalberd Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

shalberd Jul 12, 2023

Choose a reason for hiding this comment

atheo89 commented Jul 13, 2023

shalberd Jul 13, 2023

Choose a reason for hiding this comment

shalberd commented Jul 14, 2023

shalberd commented Jul 23, 2023

shalberd commented Jul 23, 2023

shalberd Jul 24, 2023

Choose a reason for hiding this comment

shalberd Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

shalberd commented Jul 12, 2023 •

edited

Loading

shalberd Jul 12, 2023 •

edited

Loading

shalberd Jul 25, 2023 •

edited

Loading