RHOAIENG-11155: Better explanation of 'Authorize Access' UI #449

daniellutz · 2024-11-14T00:16:36Z

This feature will improve the user experience in a way that the user required OAuth scope will change from a UI showing the scopes to a simple login confirmation page, according to https://issues.redhat.com/browse/RHOAIENG-11155

To test this PR, params.env have been changed to point to another generated image, so it will pick the correct content due to the changes made, so the test would be recommended to be with the devFlags instead of pointing directly to the PR generated image

Description

There is an option to inject the OAuth scope into the proxy sidecar container, in a way that it will be required only for the user to confirm his login to accept it, instead of showing up a page with confusing permissions and a bad user experience.

Not only the OAuth scope need to be passed on, but also a volume need to be mounted to gather the OAuth client secret, in a way that the application understands who is authenticating properly.

How Has This Been Tested?

Manual tests have been executed, using devFlags and with a clean test running in OpenShift Local environment.

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

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-11-14T00:16:46Z

[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 caponetto 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

atheo89 · 2024-11-14T09:18:24Z

Thank for opening this PR!
Seems that in my end there is no oauth-client data in order to be mounted and start properly the oauth proxy.

Steps to reproduce:

Replace odh-notebook-controller image with the one that generated from this PR.
Create a new notebook
Then I got:
Logs from oauth conteiner show:

2024/11/14 08:31:30 provider.go:120: Defaulting client-id to system:serviceaccount:test:q2
2024/11/14 08:31:30 provider.go:125: Defaulting client-secret to service account token /var/run/secrets/kubernetes.io/serviceaccount/token
2024/11/14 08:31:30 main.go:140: Invalid configuration:
  cannot read client-secret-file: open /etc/oauth/client/secret: no such file or directory

atheodor@fedora:~-$ oc get secrets
NAME                       TYPE                      DATA   AGE
builder-dockercfg-jkztm    kubernetes.io/dockercfg   1      7d20h
default-dockercfg-knsk6    kubernetes.io/dockercfg   1      7d20h
deployer-dockercfg-9ngd6   kubernetes.io/dockercfg   1      7d20h
pipeline-dockercfg-5fn4m   kubernetes.io/dockercfg   1      7d20h
q2-dockercfg-2x9gd         kubernetes.io/dockercfg   1      35m
q2-oauth-client            Opaque                    0      35m      <---- Problematic secret
q2-oauth-config            Opaque                    1      35m
q2-tls                     kubernetes.io/tls         2      35m

Oauth-client didn't populated data secret and thats why failed

What i missed?

jiridanek · 2024-11-14T12:39:22Z

components/odh-notebook-controller/config/base/params.env

@@ -1 +1 @@
-odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-3f931d2
+odh-notebook-controller-image=quay.io/dlutz/odh-notebook-controller:authorize-access


testing purposes? where? what?

This reverts commit 8e285fd.

jiridanek · 2024-11-15T07:13:39Z

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

+								VolumeSource: corev1.VolumeSource{
+									Secret: &corev1.SecretVolumeSource{
+										SecretName:  Name + "-oauth-client-generated",
+										DefaultMode: pointer.Int32Ptr(420),


@daniellutz the idea here is that the linter only runs on new code, so that's why it is not flagging the existing usages of pointer.Int32Ptr. What you should do here is to simply use ptr.To instead of this, and it will work just fine. The idea is that ptr.To is a better replacement for the old functions, and it could not be used before because it requires go 1.18+ features https://pkg.go.dev/k8s.io/utils/ptr#section-readme

jiridanek · 2024-11-15T07:49:39Z

@daniellutz regarding ODH Notebook Controller Integration Test / build (pull_request) Failing after 14m

  Warning  FailedMount  43s                kubelet            Unable to attach or mount volumes: unmounted volumes=[oauth-client], unattached volumes=[oauth-client oauth-config tls-certificates kube-api-access-mn78k]: timed out waiting for the condition

That's never going to pass because in the test we are running in a KinD cluster (https://kind.sigs.k8s.io/) and we only have the notebook controller and no other components of rhoai, most importantly we don't have rhods-operator that would create your oauth secret for the pod; so the solution should be to create the secret in test setup. I'll take a look r.n.

edit: got it resolved; but when i copied actual random secret from my cluster, I got yelled at by prodsec code scanning tool that I am leaking secrets into github. need to get some example secret that will not trigger their bots

atheo89 · 2024-11-15T08:31:21Z

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

+				"notebook-name": notebook.Name,
+			},
+			Annotations: map[string]string{
+				"secret-generator.opendatahub.io/name":               "secret",


FYI: This specific annotations used by rhoai-operator to create a secret <secret-name>-generated

https://github.com/opendatahub-io/opendatahub-operator/tree/84d22f35a620f43f0e8b397b1b45e2bdb25a8f46/controllers/secretgenerator#basic-usage

…r to do it for us

…being made elsewhere * redhat-best-practices-for-k8s/certsuite-qe#194

harshad16

Based on the discussion with platform, team
we got to know, it would better if the logic of secret is on own side, so lets adjust that

harshad16 · 2024-11-21T17:57:04Z

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

@@ -209,6 +209,26 @@ func NewNotebookOAuthSecret(notebook *nbv1.Notebook) *corev1.Secret {
 	}
 }

+// NewNotebookOAuthClientSecret defines the desired OAuth client secret object
+func NewNotebookOAuthClientSecret(notebook *nbv1.Notebook) *corev1.Secret {


As we would not be utilizing, opendatahub-operator secretgenerator,
lets change the logic here, and write our own secret create

please take this as reference:

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

Line 190 in 30afc15

func NewNotebookOAuthSecret(notebook *nbv1.Notebook) *corev1.Secret {

and adjust this function , and for secret generation,
utilize the logic of random function from here: https://github.com/opendatahub-io/opendatahub-operator/blob/c1671ab5fd11baea814f8acdee1bc448d502fb1c/controllers/secretgenerator/secret.go#L91

Suggested change

func NewNotebookOAuthClientSecret(notebook *nbv1.Notebook) *corev1.Secret {

func NewNotebookOAuthClientSecret(notebook *nbv1.Notebook) *corev1.Secret {

// Generate the client secret for the OAuth proxy

randomValue := make([]byte, 32)

for i := 0; i < secret.Complexity; i++ {

num, err := rand.Int(rand.Reader, big.NewInt(int64(len(letterRunes))))

if err != nil {

return err

}

randomValue[i] = letterRunes[num.Int64()]

}

// Create a Kubernetes secret to store the cookie secret

return &corev1.Secret{

ObjectMeta: metav1.ObjectMeta{

Name: notebook.Name + "-oauth-client",

Namespace: notebook.Namespace,

Labels: map[string]string{

"notebook-name": notebook.Name,

},

},

StringData: map[string]string{

"secret": string(randomValue),

},

}

and adjust the oauth-proxy to directly pick value from this secret

daniellutz added 2 commits November 13, 2024 21:04

Add scope for oauth authentication

d9fb7b7

Refactor code and fix tests

2914a5f

daniellutz self-assigned this Nov 14, 2024

openshift-ci bot requested review from caponetto and paulovmr November 14, 2024 00:16

daniellutz changed the title ~~Better explanation of 'Authorize Access' UI~~ RHOAIENG-11155: Better explanation of 'Authorize Access' UI Nov 14, 2024

atheo89 added the tide/merge-method-squash label Nov 14, 2024

daniellutz added 2 commits November 14, 2024 09:24

Add the -generated to oauth-client volume name

55ddef2

Add params.env for testing purposes

009b144

jiridanek reviewed Nov 14, 2024

View reviewed changes

Remove the -generated to oauth-client volume name

8e285fd

daniellutz force-pushed the authorize-access-ui branch from 0a53f60 to 8e285fd Compare November 14, 2024 13:19

Revert "Remove the -generated to oauth-client volume name"

3047718

This reverts commit 8e285fd.

openshift-merge-robot added the needs-rebase The PR needs a rebase or there are conflicts label Nov 14, 2024

Merge branch 'main' into authorize-access-ui

c9147f7

openshift-merge-robot removed the needs-rebase The PR needs a rebase or there are conflicts label Nov 15, 2024

jiridanek reviewed Nov 15, 2024

View reviewed changes

atheo89 reviewed Nov 15, 2024

View reviewed changes

jiridanek added 2 commits November 15, 2024 10:00

Create the oauth secrets in tests because we don't have rhods-operato…

fefa76e

…r to do it for us

fixup, use nondeprecated pointer func alternative, see similar fixes …

b492194

…being made elsewhere * redhat-best-practices-for-k8s/certsuite-qe#194

harshad16 requested changes Nov 21, 2024

View reviewed changes

openshift-ci bot assigned harshad16 Nov 21, 2024

jiridanek mentioned this pull request Nov 26, 2024

RHOAIENG-8390: feat(odh-nbc): search for imagestreams only in the namespace the controller runs in #375

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHOAIENG-11155: Better explanation of 'Authorize Access' UI #449

RHOAIENG-11155: Better explanation of 'Authorize Access' UI #449

daniellutz commented Nov 14, 2024 •

edited by jiridanek

Loading

openshift-ci bot commented Nov 14, 2024

atheo89 commented Nov 14, 2024

jiridanek Nov 14, 2024

jiridanek Nov 15, 2024

jiridanek commented Nov 15, 2024 •

edited

Loading

atheo89 Nov 15, 2024 •

edited

Loading

harshad16 left a comment

harshad16 Nov 21, 2024

		@@ -1 +1 @@
		odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-3f931d2
		odh-notebook-controller-image=quay.io/dlutz/odh-notebook-controller:authorize-access

-func NewNotebookOAuthClientSecret(notebook *nbv1.Notebook) *corev1.Secret {
+func NewNotebookOAuthClientSecret(notebook *nbv1.Notebook) *corev1.Secret {
+  // Generate the client secret for the OAuth proxy
+	randomValue := make([]byte, 32)
+		for i := 0; i < secret.Complexity; i++ {
+			num, err := rand.Int(rand.Reader, big.NewInt(int64(len(letterRunes))))
+			if err != nil {
+				return err
+			}
+			randomValue[i] = letterRunes[num.Int64()]
+		}
+	// Create a Kubernetes secret to store the cookie secret
+	return &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      notebook.Name + "-oauth-client",
+			Namespace: notebook.Namespace,
+			Labels: map[string]string{
+				"notebook-name": notebook.Name,
+			},
+		},
+		StringData: map[string]string{
+			"secret": string(randomValue),
+		},
+	}

RHOAIENG-11155: Better explanation of 'Authorize Access' UI #449

Are you sure you want to change the base?

RHOAIENG-11155: Better explanation of 'Authorize Access' UI #449

Conversation

daniellutz commented Nov 14, 2024 • edited by jiridanek Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Nov 14, 2024

atheo89 commented Nov 14, 2024

jiridanek Nov 14, 2024

Choose a reason for hiding this comment

jiridanek Nov 15, 2024

Choose a reason for hiding this comment

jiridanek commented Nov 15, 2024 • edited Loading

atheo89 Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

harshad16 Nov 21, 2024

Choose a reason for hiding this comment

daniellutz commented Nov 14, 2024 •

edited by jiridanek

Loading

jiridanek commented Nov 15, 2024 •

edited

Loading

atheo89 Nov 15, 2024 •

edited

Loading