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

[jiridanek]

https://issues.redhat.com/browse/RHOAIENG-9935

Revival of @shalberd's PR, now targetted against v1.7-branch.

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

I used https://github.com/opendatahub-io/kubeflow/pull/119.patch to get a patch that I applied against v1.7-branch, to avoid having to rebase the original PR branch.

• TODO: create new ticket to update the proxy image we are using, it is truly ancient, https://issues.redhat.com/browse/RHOAIENG-10827

I also incorporated change to the manifests

• change imagePullPolicy for static tags of kubeflow and odh notebook controller odh-manifests#868

to be in sync with https://github.com/red-hat-data-services/kubeflow/blob/master/components/odh-notebook-controller/config/manager/manager.yaml#L28

and with operator annotations at https://github.com/red-hat-data-services/RHOAI-Build-Config/blob/main/catalog/v4.13/rhods-operator/catalog.yaml#L340

• TODO: create a followup issue to change pull policy for the controller image itself; this looks safe, because we always reference it with either digest or with a tag that includes commit hash in itself. also change github workflow (the sed) when making this change

Motivation

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?

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

[jiridanek]

/cherrypick stable

[openshift-cherrypick-robot]

@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]

I'm unsure whether to leave it as :latest here and only set hash through the manifest. It seems sensible to me to leave :latest in the code.

[shalberd]

Agreed, basically, code here to a floating type tag like latest is best for development environment.

Since the value is overriden in manifest manager.yaml with the custom argument containing the hash, we can leave it to "latest" here.

That is what I did, too. I did not change the notebook controller code, I only put the latest digest based on tag v4.14 in my custom manifests / manager.yaml and can confirm it gets injected as specificed in manager.yaml / overridden in the workbenches.

[jiridanek]

I've changed my mind. I feel more comfortable with having the digest here, and also hardcoding the PullIfNotPresent. There's not much of real value in testing with :latest, anyways. Seems to me that using :latest oauth proxy image is almost always an error.

(It does make some sense to have the notebook controller image in :latest, but that's a different question)

For now, I'll keep the PR without making the changes I considered here.

[shalberd]

I agree with @jiridanek on this, i.e. having the digest here as well, and also hardcoding the PullIfNotPresent

[jiridanek]

We'll be going with 4.14 version of the image, #386

[shalberd]

yup, nice

[jiridanek]

If we did not set this, the defaults are Always if image tag is :latest and PullIfNotPresent otherwise, which is very sensible to me. So, maybe it would be best to leave ImagePullPolicy unset?

https://kubernetes.io/docs/concepts/containers/images#updating-images

[shalberd]

interesting, I did not know this. Makes sense, though. Yeah, we can leave it out in that case.
Have not tried this behavior on an Openshift Cluster yet, though. If true, it makes the whole thing a lot easier, especially with regards to "latest".

Default image pull policy
When you (or a controller) submit a new Pod to the API server, your cluster sets the imagePullPolicy field when specific conditions are met:

if you omit the imagePullPolicy field, and you specify the digest for the container image, the imagePullPolicy is automatically set to IfNotPresent.
if you omit the imagePullPolicy field, and the tag for the container image is :latest, imagePullPolicy is automatically set to Always;
if you omit the imagePullPolicy field, and you don't specify the tag for the container image, imagePullPolicy is automatically set to Always;
if you omit the imagePullPolicy field, and you specify the tag for the container image that isn't :latest, the imagePullPolicy is automatically set to IfNotPresent.

[jstourac]

I don't mind keeping it here so that it is visible explicitly, but the truth is that if we will keep it here, and for any reason we'll get back to the :latest tag (hopefully not) in the future again, this will override the default ImagePullPolicy and will cause that only first image download is done... so maybe removing this line completely truly makes sense?

[jiridanek]

Let's discuss this further in https://issues.redhat.com/browse/RHOAIENG-10058.

[jiridanek]

The images are available in

• quay.io/opendatahub/odh-notebook-controller:pr-374
• quay.io/opendatahub/kubeflow-notebook-controller:pr-374

this is written down in openshift-ci logs that are available under the "Artifacts" link, for example at https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/opendatahub-io_kubeflow/374/pull-ci-opendatahub-io-kubeflow-v1.7-branch-odh-notebook-controller-pr-image-mirror/1818967802866831360/artifacts/odh-notebook-controller-pr-image-mirror/opendatahub-io-ci-image-mirror/artifacts/mirror.log

[jiridanek]

/unhold
I played with the :pr-374 images enough that I feel this is ready for review and comments

[jstourac]

Thank you for these changes. I like them in general, though I feel like there are multiple different things being handled in one commit here:

1. sync with the ose-oauth-proxy images in downstream -> effectively means downgrade in all cases here
2. change in the pulling policy

Would be nice to have it done separate, but no big issue.

Regarding the point 1 - I wonder whether since we are touching this, shouldn't update the sha to the final ose-oauth-proxy version that is agreed on so we don't need to change this in the soon future again, WDYT?

[jiridanek]

Would be nice to have it done separate, but no big issue.

Guess I can still do that. @harshad16 wanted to do the pull policy change separately, so I'll let him do it. And having hash with Always policy temporarily until both PRs come in is weird but it does not break anything.

shouldn't update the sha to the final ose-oauth-proxy version that is agreed on

It's not agreed on. We'd need also pick version that handles fips, and so on. It's not a quick and low-risk job, whereas this PR pretty much is all that. And I already have a separate ticket for the update. More tickets, more velocity. Corporate FTW

[harshad16]

Guess I can still do that. harshad16 wanted to do the pull policy change separately, so I'll let him do it. And having hash with Always policy temporarily until both PRs come in is weird but it does not break anything.

Thanks for this , we can have it in a separate PR.

[harshad16]

/lgtm
/approve

thanks 👍

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [harshad16]

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

[openshift-cherrypick-robot]

@jiridanek: new pull request created: #377

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.