feat: update project level auth config with kserve hosts #130

aslakknutsen · 2023-12-08T19:12:47Z

Description

Add Authorino AuthConfig for KServe InferenceService hosts in ODHProject Namespaces.

The InferenceService can be annotated with enable-auth to trigger the creation of an AuthConfig that secures the InferenceService access. The secure access require a kubernetes token and access to get the InferenceService resources in the target Namespace.

If enable-auth is not present, an AuthConfig for anonymous access will be created instead.

How Has This Been Tested?

enable-auth:

grpc|curl -H "Authorization: Bearer $(oc whoami -t)" ...

no auth:

grpc|curl ...

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

Dependencies

openshift-ci · 2023-12-08T19:12:52Z

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

go.mod

spolti · 2023-12-08T19:29:02Z

controllers/reconcilers/kserve_authconfig_reconciler.go

+func (r *KserveAuthConfigReconciler) Reconcile(ctx context.Context, log logr.Logger, isvc *kservev1beta1.InferenceService) error {
+
+	if isvc.Status.URL == nil {
+		log.V(1).Info("Inference Service not ready yet, waiting for URL")


imho, it should be a warning.

Could make it a warning. It's part of the normal flow to reconcile a few times before the Status.URL show up

So, log does not have a warning func. Only Info and Error.
We have V(1) in the middle to raise verbosity, so I think this is good enough.

controllers/resources/authconfig.go

controllers/resources/authconfig_test.go

spolti · 2023-12-08T19:38:09Z

controllers/utils/converter.go

+func ConvertToStructuredResource(yamlContent []byte, out runtime.Object, opts ...manifestival.Option) error {
+
+	s := runtime.NewScheme()
+	RegisterSchemes(s)


not sure if I followed, why is this needed here?

So, this is creating an empty Scheme which is needed by the decoder a few lines later.
I guess, the decoder won't work correctly if the Schemes/CRDs are not present in the Scheme.

controllers/reconcilers/kserve_authconfig_reconciler.go

controllers/resources/authconfig.go

controllers/resources/template/authconfig_userdefined.yaml

controllers/resources/authconfig.go

controllers/reconcilers/kserve_authconfig_reconciler.go

israel-hdez · 2024-01-18T00:13:09Z

So, I was trying to make this work.
Looks like everything seems to be correctly "wired up" in my cluster. Despite so, the request still goes through even when annotating the ISVC with enable-auth=true.

@aslakknutsen @bartoszmajsak I may meet with you soon to see if we can debug this.

config/rbac/role.yaml

israel-hdez · 2024-01-26T21:12:39Z

/retest

aslakknutsen · 2024-01-26T21:16:57Z

/test unit

controllers/reconcilers/kserve_authconfig_reconciler.go

controllers/utils/converter.go

main.go

controllers/resources/template/authconfig_userdefined.yaml

controllers/kserve_inferenceservice_controller_authconfig_test.go

israel-hdez · 2024-01-29T23:19:53Z

controllers/kserve_inferenceservice_controller_authconfig_test.go

+
+func disableAuth(isvc *kservev1beta1.InferenceService) error {
+	delete(isvc.Annotations, "enable-auth")
+	delete(isvc.Annotations, "security.opendatahub.io/enable-auth")


Maybe, these strings should be a const.

Fixed a6c333a

Seeing them side by side I wonder if maybe we should have called it something like security.opendatahub.io/authorization instead...?

It could be. Although, for enabling and disabling, I'm not sure if it is that useful to make distinction between authorization and authentication.

But you tell, I let you do the renaming if you think it is better.

I was more thinking the short v "auth" vs "auth..." and "auth-x" vs "x-auth" order. (not authentication vs authorization)

security.opendatahub.io/authorization-group
security.opendatahub.io/enable-auth

security.opendatahub.io/authorization-group
security.opendatahub.io/authorization-enabled

The security.opendatahub.io/authorization-enabled is the one that I see as potentially noisy for later. Right now, we are dealing only with authZ and the naming looks fine. But once we start looking at proper SSO, we may want to expand this with the authN part and add back to the AuthConfig the redirect that we removed previously. Then, this annotation would work to enable/disable both authN and authZ together, and its name will no longer fully reflect what it is doing.

This is what I was meaning about making the distinction.

is security....io/enable sufficient? It hints at securing the Model. Would there be other types of 'security' that could make it confusing in the future at some point?

is security....io/enable sufficient?

Uhm... I'm OK with it, but...

Would there be other types of 'security'

I don't know....
The only other thing that has been a topic lately is around TLS certs. But IDK if we will use annotations or labels for that.

israel-hdez · 2024-02-02T16:54:52Z

/retest

- Add HostExtractor interface. Hosts are defined differently between KServe and ModelMesh - Remove webhook CRD convertion. Not relevant to test cases. - Update controllers/reconcilers/kserve_authconfig_reconciler.go Co-authored-by: Bartosz Majsak <[email protected]> Co-authored-by: Edgar Hernández <[email protected]> Signed-off-by: Aslak Knutsen <[email protected]>

Signed-off-by: Aslak Knutsen <[email protected]>

This fixes a reconcile loop. The `AuthConfig` from the template would end with `subresource.value` and `name.value` fields set to `null`, while the cluster stores these fields with empty strings. This would be detected as a diff at reconciliation and the `AuthConfig` would be re-applied causing a loop both in `odh-model-controller` and `authorino` controller. Adding the empty fields to the template stops the loop. Signed-off-by: Edgar Hernández <[email protected]>

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

This is to have parity with Model Mesh. Signed-off-by: Edgar Hernández <[email protected]>

Signed-off-by: Aslak Knutsen <[email protected]>

heyselbi · 2024-02-16T18:55:20Z

/lgtm

israel-hdez · 2024-02-16T20:24:06Z

/approve

openshift-ci · 2024-02-16T20:24:13Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

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