Implement authorization for Raw InferenceGraphs #499

israel-hdez · 2025-02-18T00:02:51Z

What this PR does / why we need it:

Authorization is implemented by using the TokenReview and the SubjectAccessReview Kubernetes APIs. A Middleware function is setup when some arguments are specified that trigger plugging-in the middleware func.

Some additional reconciliation is added toInferenceGraph controller to:

Switch to a different ServiceAccount so that privileges for using the cluster APIs are granted.
Creating the needed ServiceAccount for the auth-protected InferenceGraph to run.
Managing a ClusterRoleBinding to give the required privileges for auth verification.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes https://issues.redhat.com/browse/RHOAIENG-17832

Type of changes
Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Feature/Issue validation/testing:

Create an auth-enabled InfrerenceGraph. Notice the logs of the IG pod mention that auth is enabled.
Try to send a non-authenticated request to the IG. Notice the request is rejected.
Try to send an authenticated request but without enough privileges. Notice the request is rejected.
Try to send an authenticated request with enough privileges. Notice the request is accepted.

Checklist:

Have you added unit/e2e tests that prove your fix is effective or that this feature works?
Has code been commented, particularly in hard-to-understand areas?
Have you made corresponding changes to the documentation?

openshift-ci · 2025-02-18T00:02:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

VedantMahabaleshwarkar

added some comments

VedantMahabaleshwarkar · 2025-02-20T15:31:34Z

pkg/controller/v1alpha1/inferencegraph/raw_ig.go

+		}
+
+		// Bind the required privileges to the Service Account
+		err = addAuthPrivilegesToGraphServiceAccount(ctx, clientset, graph)


nit:

Suggested change

err = addAuthPrivilegesToGraphServiceAccount(ctx, clientset, graph)

err = addAuthPrivilegesToGraphServiceAccount(ctx, clientset, graph, saName)

So, do you prefer sending it as an argument rather than re-getting it inside the func?

yes, would avoid having to re-get. but it's only a nit :)

VedantMahabaleshwarkar · 2025-02-20T15:32:09Z

pkg/controller/v1alpha1/inferencegraph/raw_ig.go

+			return err
+		}
+	} else {
+		err := removeAuthPrivilegesFromGraphServiceAccount(ctx, clientset, graph)


nit:

Suggested change

err := removeAuthPrivilegesFromGraphServiceAccount(ctx, clientset, graph)

err := removeAuthPrivilegesFromGraphServiceAccount(ctx, clientset, graph, saName)

VedantMahabaleshwarkar · 2025-02-20T15:32:27Z

pkg/controller/v1alpha1/inferencegraph/raw_ig.go

+			return err
+		}
+
+		err = deleteGraphServiceAccount(ctx, clientset, graph)


nit:

Suggested change

err = deleteGraphServiceAccount(ctx, clientset, graph)

err = deleteGraphServiceAccount(ctx, clientset, graph, saName)

VedantMahabaleshwarkar · 2025-02-20T15:38:58Z

pkg/controller/v1alpha1/inferencegraph/controller.go

+		if err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "fails to reconcile resources for auth verification")
+		}
+
 		// Create inference graph resources such as deployment, service, hpa in raw deployment mode
 		deployment, url, err := handleInferenceGraphRawDeployment(r.Client, r.Clientset, r.Scheme, graph, routerConfig)


Looking at the controller flow from here, handleInferenceGraphRawDeployment -> NewRawKubeReconciler -> NewDeploymentReconciler -> createRawDeploymentODH where we have

if val, ok := componentMeta.Labels[constants.ODHKserveRawAuth]; ok && val == "true" { err := addOauthContainerToDeployment(clientset, deployment, componentMeta, componentExt, podSpec) if err != nil { return nil, err } }

Does this mean that if the graph has the ODHKserveRawAuth label, the graph deployment will get the oauth-proxy container. Not sure if I'm missing something 🤔

In my testings it didn't get oauth-proxy. It could be because of RHOAIENG-20326 which we discussed today.

Most likely, when RHOAIENG-20326 is fixed, the IG will get the proxy. If that's the case, I'll appreciate you create a follow-up ticket for fixing it (although you'd be kind if you fix it as part of RHOAIENG-20326 😄 ).

To me this feels like more in scope with this PR whereas https://issues.redhat.com/browse/RHOAIENG-20326 should IMO be limited to making the switch from label to annotation

It does not fit here, nor in RHOAIENG-20326. With the current code, there is nothing to fix in this PR. This is my motivation for that to be a follow-up Jira.

By doing any fix here, I'd need to ensure no regression, and also, I'd need to implement RHOAIENG-20326, just to ensure that the fix will actually work.

Issue created: https://issues.redhat.com/browse/RHOAIENG-21300

VedantMahabaleshwarkar · 2025-02-20T15:41:23Z

cmd/router/main.go

+// checkRequestIsAuthorized verifies that the user in the provided tokenReviewResult has privileges to query the
+// Kubernetes API and get the InferenceGraph resource that belongs to this pod. If so, the request is considered
+// as allowed and `true` is returned. Otherwise, the HTTP response is sent rejecting the request and setting
+// a meaningful status code along with a reason (if available).


Question:
What happens if the token has get privileges for the IG but not 1 or more of the ISVCs in the IG? Should we be verifying that the token has the correct privileges for the IG + all the ISVCs?

The token needs to have privileges for both the IG and the ISVC. The IG is doing only its check.

Later, the token is forwarded to the ISVC, so that it also can do its own check. This, effectively, delegates such check to the ISVC.

This may not be optimal, if the auth-protected ISVC is the last one on the IG and the previous ones are not protected. The request would fail after wasting resources. But I think current implementation is good enough, given we are not sure how users will use InferenceGraph. So, I'd say that we should optimize once we are sure.

spolti · 2025-02-20T19:01:53Z

pkg/controller/v1alpha1/inferencegraph/controller.go

@@ -173,9 +178,53 @@ func (r *InferenceGraphReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return reconcile.Result{}, errors.Wrapf(err, "fails to create DeployConfig")
 	}

+	// name of our custom finalizer
+	finalizerName := "inferencegraph.finalizers"


maybe move it to the constants section.

It is moved to a constant.

pkg/controller/v1alpha1/inferencegraph/raw_ig.go

cmd/router/main.go

pkg/controller/v1alpha1/inferencegraph/controller.go

pkg/controller/v1alpha1/inferencegraph/controller_test.go

pkg/controller/v1alpha1/inferencegraph/raw_ig.go

spolti · 2025-03-06T18:25:44Z

@Jooho do you want to take a look before merging?

Jooho

Looks good to me.

I just leave something not directly related to this PR :) so I can defer to Vedant because his comments are not solved yet.

cmd/router/main.go

pkg/controller/v1alpha1/inferencegraph/controller.go

israel-hdez · 2025-03-12T21:18:00Z

/retest

VedantMahabaleshwarkar

/approve

openshift-ci · 2025-03-13T18:27:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, VedantMahabaleshwarkar

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~~ [VedantMahabaleshwarkar,israel-hdez]

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

Jooho · 2025-03-13T18:58:56Z

/lgtm

openshift-ci · 2025-03-13T22:03:21Z

@israel-hdez: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-raw	`9068707`	link	true	`/test e2e-raw`
ci/prow/e2e-graph	`9068707`	link	true	`/test e2e-graph`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Authorization is implemented by using the TokenReview and the SubjectAccessReview Kubernetes APIs. A Middleware function is setup when some arguments are specified that trigger plugging-in the middleware func. Some additional reconciliation is added toInferenceGraph controller to: * Switch to a different ServiceAccount so that privileges for using the cluster APIs are granted. * Creating the needed ServiceAccount for the auth-protected InferenceGraph to run. * Managing a ClusterRoleBinding to give the required privileges for auth verification. Signed-off-by: Edgar Hernández <[email protected]>

Fix comment

Signed-off-by: Edgar Hernández <[email protected]>

Jooho · 2025-03-14T15:29:58Z

/lgtm

* Implement authorization for Raw InferenceGraphs Authorization is implemented by using the TokenReview and the SubjectAccessReview Kubernetes APIs. A Middleware function is setup when some arguments are specified that trigger plugging-in the middleware func. Some additional reconciliation is added toInferenceGraph controller to: * Switch to a different ServiceAccount so that privileges for using the cluster APIs are granted. * Creating the needed ServiceAccount for the auth-protected InferenceGraph to run. * Managing a ClusterRoleBinding to give the required privileges for auth verification. Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Jooho Fix comment * Fix unit test Signed-off-by: Edgar Hernández <[email protected]> --------- Signed-off-by: Edgar Hernández <[email protected]>

* Implement authorization for Raw InferenceGraphs Authorization is implemented by using the TokenReview and the SubjectAccessReview Kubernetes APIs. A Middleware function is setup when some arguments are specified that trigger plugging-in the middleware func. Some additional reconciliation is added toInferenceGraph controller to: * Switch to a different ServiceAccount so that privileges for using the cluster APIs are granted. * Creating the needed ServiceAccount for the auth-protected InferenceGraph to run. * Managing a ClusterRoleBinding to give the required privileges for auth verification. * Feedback: Jooho Fix comment * Fix unit test --------- Signed-off-by: Edgar Hernández <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress label Feb 18, 2025

openshift-ci bot added the approved label Feb 18, 2025

israel-hdez force-pushed the j17832-raw-ig-auth branch 6 times, most recently from cb3c634 to 9068707 Compare February 20, 2025 00:35

israel-hdez marked this pull request as ready for review February 20, 2025 00:39

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 20, 2025

israel-hdez requested review from VedantMahabaleshwarkar, Jooho and spolti February 20, 2025 00:39

VedantMahabaleshwarkar reviewed Feb 20, 2025

View reviewed changes