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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Aug 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion components/odh-notebook-controller/config/manager/manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ spec:
imagePullPolicy: Always
command:
- /manager
args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy:v4.10"]
args: ["--oauth-proxy-image", "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46"]
jiridanek marked this conversation as resolved.
Show resolved Hide resolved
securityContext:
allowPrivilegeEscalation: false
ports:
Expand Down
5 changes: 4 additions & 1 deletion components/odh-notebook-controller/controllers/notebook_oauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ import (
const (
OAuthServicePort = 443
OAuthServicePortName = "oauth-proxy"
OAuthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy:latest"
// OAuthProxyImage uses sha256 manifest list digest value of v4.8 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?image=6306f12280cc9b3291272668&architecture=amd64&container-tabs=overview
// and kept in sync with the manifests here and in ClusterServiceVersion metadata of opendatahub operator
OAuthProxyImage = "registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46"
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

@shalberd shalberd Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

yup, nice

)

type OAuthConfig struct {
Expand Down