feat: simplify token audiences config #947

cam-garrison · 2024-04-01T17:17:19Z

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

Description

This PR brings automation to populating the audiences field for token review. Previously, if the audiences needed were different than the default kubernetes.default.svc, the user had to configure it manually thru the AuthSpec in the DSCI.

Now, if the user does not specify audiences in their DSCI, we will use an audiences value fetched while bootstrapping the operator. If the user specifies some non-default audiences, that value will be used over what is fetched while bootstrapping the operator.

This specifically should make testing/deploying on ROSA easier, as previously we needed to manually fetch the audiences and set this in the DSCI, and now if you leave it as default the operator will fetch it for you.

How Has This Been Tested?

Tested on ROSA created thru clusterbot - created a DSCI without specifying audiences under .spec.serviceMesh.auth.audiences. Then, checked created ConfigMap auth-refs to ensure that AUTH_AUDIENCE was populated with the correct audience for ROSA cluster, not the default. Then, did this basic test https://gist.github.com/israel-hdez/af374562ef9e5b9d80890aa6f0bce20d to ensure kserve functionality. (note - used odh-model-controller release v.0.12.0 in operator build to test this aspect).

Also, tested with a specified audience to ensure that if the user specifies some audiences in the DSCI authspec, they are not overwritten.

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

pkg/feature/servicemesh/resources.go

controllers/dscinitialization/servicemesh_setup.go

bartoszmajsak

Thanks, this is great improvement.

I shared a few things which we should think through before merging it.

And on the related note: would it be difficult to write a test for it?

controllers/dscinitialization/servicemesh_setup.go

main.go

bartoszmajsak · 2024-04-02T19:56:01Z

controllers/dscinitialization/dscinitialization_controller.go

@@ -67,6 +67,7 @@ type DSCInitializationReconciler struct { //nolint:golint,revive // Readability
 	Log                   logr.Logger
 	Recorder              record.EventRecorder
 	ApplicationsNamespace string
+	Audiences             []string


I am wondering if there's some other way of passing this config down the chain. Somewhat does not feel like this should belong here, it's unrelated to the reconcile at this level. Let's think about possible ways and sync tomorrow.

moved it! here
c99d6d7

Co-authored-by: Bartosz Majsak <[email protected]>

cam-garrison · 2024-04-04T17:56:00Z

@bartoszmajsak added some basic unit tests, not mocking the full functionality here

controllers/dscinitialization/servicemesh_setup.go

controllers/dscinitialization/servicemesh_test.go

controllers/dscinitialization/servicemesh_setup.go

bartoszmajsak · 2024-04-10T08:47:42Z

pkg/cluster/audiences_test.go

I think we should stick to the correct terminology. We are providing canned responses, so this is stubbing, not mocking.

A bit more about it in this great blog post https://martinfowler.com/articles/mocksArentStubs.html

pkg/cluster/audiences_test.go

Co-authored-by: Bartosz Majsak <[email protected]>

openshift-ci · 2024-04-10T16:26:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bartoszmajsak
Once this PR has been reviewed and has the lgtm label, please assign asanzgom 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

openshift-ci · 2024-04-15T13:48:23Z

New changes are detected. LGTM label has been removed.

zdtsw · 2024-04-15T15:10:08Z

I do not have too many opinions on this PR, @israel-hdez if you have time do a review on it?

cam-garrison · 2024-04-15T17:27:34Z

/retest-required

israel-hdez

Leaving some questions.

I haven't tested. I'd test after having answers. This is, anyway, going to be OK for me as long as auth is still working (and I do see that it was already tested).

israel-hdez · 2024-04-15T23:56:41Z

pkg/cluster/audiences.go

+		},
+	}
+
+	if err = cli.Create(context.Background(), tokenReview, &client.CreateOptions{}); err != nil {


This seems to use the ServiceAccount mounted by the cluster, as a pivot to find the audientes.

I wonder if it isn't enough to parse the ServiceAccount token?
Just by parsing the one from CRC, I see the audiences claim in the payload:

"aud": [ "https://kubernetes.default.svc" ],

I'm not sure how it would be for ROSA.

Correct, originally we were using opaque token for the bash "workaround" and this solution is based on this, so perhaps, using SA (jwt) token aud is simpler and more efficient. We could replace this impl and use https://github.com/golang-jwt/jwt instead to fetch standard claims without verification (so passing nil as the key func ). @israel-hdez @aslakknutsen do you think it's better to rely on the fact that SA token is JWT?

I just checked in ROSA and we can also parse JWT instead. For opaque (openshift oauth) tokens it's the same audience. We can get it by using the exact call to TokenReview API.

This behavior is only invoked when you do not specify anything in DSCI.ServiceMesh.Auth part. If you later do, it will update underlying config map which is now used by odh-model-controller

@israel-hdez @aslakknutsen do you think it's better to rely on the fact that SA token is JWT?

AFAIK, core Kubernetes only supports JWT (read 1st sentence of this). So, I'd expect any SA token to be JWT. I understand we have control about the mounted/projected token so, as long as the setup does not change, we should be safe to assume it is a JWT.

About the opaque tokens, my understanding is that in core Kubernetes these are only supported when a 3rd party issues the token and the API Server would validate the token via a Webhook to the 3rd party (i.e. Kubernetes does not validate by its own). OpenShift's OAuth server may fall in this category.

israel-hdez · 2024-04-15T23:59:27Z

pkg/cluster/audiences.go

+		return defaultAudiences
+	}
+
+	return tokenReview.Status.Audiences


Reading these docs: https://kubernetes.io/docs/reference/access-authn-authz/authentication/.

There is a note that the audiences field is optional and that, if not present, it may still mean the token is valid for the cluster. So, I'm not sure if this is safe.

Underlying AuthConfig will be defaulted if not configured specifically.

// If omitted, Authorino will review tokens expecting the host name of the requested protected service amongst the audiences.

Current implementation defaults it to "https://kubernetes.default.svc" if not specified.

In the docs that I linked, there is this text over status.audiences of the TokenReview:

# If this is omitted, the token is considered to be valid to authenticate to the Kubernetes API server.

Underlying AuthConfig will be defaulted if not configured specifically.
Current implementation defaults it to "https://kubernetes.default.svc" if not specified.

IIRC, this default is on odh-model-controller side. This is OK.

My concern is that my understanding of the kubernetes docs is that the cluster may choose to omit the status.audiences field as an indication that the token is valid for the API Server. Yet, in that case, I see no statement that the audience would be https://kubernetes.default.svc in case the audiences field is omitted. Thus, we may wrongly assume it is https://kubernetes.default.svc.

The writing is somewhat confusing. So, I may be understanding things badly.

openshift-merge-robot · 2024-04-18T22:35:39Z

PR needs rebase.

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/test-infra repository.

bartoszmajsak · 2024-04-19T14:09:25Z

Let's keep it on hold until we figure out the right path how reliably fetch audiences or fall back if they're not present.

openshift-ci · 2024-04-24T16:44:36Z

@cam-garrison: 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/ci-index	`65887e4`	link	true	`/test ci-index`
ci/prow/opendatahub-operator-pr-image-mirror	`65887e4`	link	true	`/test opendatahub-operator-pr-image-mirror`
ci/prow/opendatahub-operator-e2e	`65887e4`	link	true	`/test opendatahub-operator-e2e`
ci/prow/images	`65887e4`	link	true	`/test images`

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

israel-hdez · 2024-06-12T21:13:01Z

@cam-garrison @bartoszmajsak We saw an instance of this, and we'd prefer to have this PR merged soon, if possible.
cc @Jooho

bartoszmajsak · 2024-06-13T06:27:47Z

@cam-garrison @bartoszmajsak We saw an instance of this, and we'd prefer to have this PR merged soon, if possible. cc @Jooho

@israel-hdez can you elaborate more on what you mean by "instance" and what is the requirement that this PR can be part of?

israel-hdez · 2024-06-13T19:56:30Z

We saw in ROSA that the audience is not configured correctly. Thus, in the automated setup the inference requests do not go through for protected models.

We, anyway, need docs for the manual setup, but it is a weird experience that for the automated setup users need to be careful of setting the right audience.

zdtsw · 2024-06-14T14:15:13Z

@israel-hdez do we expect this PR for downstream 2.11?

israel-hdez · 2024-06-14T16:44:08Z

@israel-hdez do we expect this PR for downstream 2.11?

AFAIK, we don't have a target. Also, I'm not sure how much we support ROSA. If we do support it, we should try to have this in 2.11 if it fits... But I see this as best effort for 2.11.

BTW, @Jooho provided some feedback in the Jira.

bartoszmajsak · 2024-06-17T10:27:29Z

We saw in ROSA that the audience is not configured correctly. Thus, in the automated setup the inference requests do not go through for protected models.

We, anyway, need docs for the manual setup, but it is a weird experience that for the automated setup users need to be careful of setting the right audience.

Understood, but this is a known issue and the reason why audiences are configurable through DSCI. We will look at how to improve the experience but we need to prioritize this work first.

cam-garrison added 4 commits March 27, 2024 13:45

tmp init cmt

5547a5a

add audience switching logic

518f888

cleanup unnec comments

941e71d

restore get_all_mani

cea9305

cam-garrison requested review from bartoszmajsak and zdtsw April 1, 2024 17:17

Merge branch 'incubation' into automate-audiences

94d8626

openshift-ci bot requested review from atheo89 and tarilabs April 1, 2024 17:17

cam-garrison removed request for tarilabs and atheo89 April 1, 2024 19:48

zdtsw reviewed Apr 2, 2024

View reviewed changes

pkg/feature/servicemesh/resources.go Outdated Show resolved Hide resolved

controllers/dscinitialization/servicemesh_setup.go Outdated Show resolved Hide resolved

bartoszmajsak requested changes Apr 2, 2024

View reviewed changes

openshift-ci bot assigned bartoszmajsak Apr 2, 2024

Update controllers/dscinitialization/servicemesh_setup.go

a5b609c

Co-authored-by: Bartosz Majsak <[email protected]>

openshift-merge-robot added the needs-rebase label Apr 3, 2024

cam-garrison and others added 3 commits April 3, 2024 09:51

Update main.go

0b63d2d

Co-authored-by: Bartosz Majsak <[email protected]>

inline audience checking, check token status

1374f2a

Merge branch 'incubation' into automate-audiences

b156a34

openshift-merge-robot removed the needs-rebase label Apr 3, 2024

cam-garrison added 5 commits April 3, 2024 17:19

fetch audiences outside of main, move label

c99d6d7

move labels import

5fd931a

add basic unit tests

7365c32

lint

0e20b23

do not runSpec twice

cae6062

openshift-merge-robot added the needs-rebase label Apr 6, 2024

bartoszmajsak requested changes Apr 8, 2024

View reviewed changes

cam-garrison added 2 commits April 8, 2024 17:05

refactor from review

e38a8ad

Merge branch 'incubation' into automate-audiences

bb1289e

bartoszmajsak reviewed Apr 10, 2024

View reviewed changes

cam-garrison added 3 commits April 10, 2024 11:40

remove fakeClient, stub not mock

9bb75de

Merge branch 'incubation' into automate-audiences

adf2672

Merge branch 'incubation' into automate-audiences

ef204ab

bartoszmajsak reviewed Apr 10, 2024

View reviewed changes

pkg/cluster/audiences_test.go Outdated Show resolved Hide resolved

cam-garrison and others added 2 commits April 10, 2024 12:14

Update pkg/cluster/audiences_test.go

98c69c5

Co-authored-by: Bartosz Majsak <[email protected]>

fix typo, lint

ae69939

bartoszmajsak approved these changes Apr 10, 2024

View reviewed changes

openshift-ci bot added the lgtm label Apr 10, 2024

Merge branch 'incubation' into automate-audiences

fc544c5

openshift-ci bot removed the lgtm label Apr 15, 2024

cam-garrison and others added 2 commits April 15, 2024 09:51

lint post merge

a27dba9

Merge branch 'incubation' into automate-audiences

65887e4

zdtsw requested a review from israel-hdez April 15, 2024 15:09

israel-hdez reviewed Apr 16, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Apr 18, 2024

bartoszmajsak added the do-not-merge/hold label Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: simplify token audiences config #947

feat: simplify token audiences config #947

cam-garrison commented Apr 1, 2024

bartoszmajsak left a comment

bartoszmajsak Apr 2, 2024

cam-garrison Apr 3, 2024

cam-garrison commented Apr 4, 2024

bartoszmajsak Apr 10, 2024

openshift-ci bot commented Apr 10, 2024

openshift-ci bot commented Apr 15, 2024

zdtsw commented Apr 15, 2024

cam-garrison commented Apr 15, 2024

israel-hdez left a comment

israel-hdez Apr 15, 2024

bartoszmajsak Apr 16, 2024

israel-hdez Apr 16, 2024

israel-hdez Apr 15, 2024

bartoszmajsak Apr 16, 2024

israel-hdez Apr 16, 2024 •

edited

Loading

openshift-merge-robot commented Apr 18, 2024

bartoszmajsak commented Apr 19, 2024 •

edited

Loading

openshift-ci bot commented Apr 24, 2024

israel-hdez commented Jun 12, 2024

bartoszmajsak commented Jun 13, 2024 •

edited

Loading

israel-hdez commented Jun 13, 2024 •

edited

Loading

zdtsw commented Jun 14, 2024

israel-hdez commented Jun 14, 2024

bartoszmajsak commented Jun 17, 2024

feat: simplify token audiences config #947

Are you sure you want to change the base?

feat: simplify token audiences config #947

Conversation

cam-garrison commented Apr 1, 2024

Description

How Has This Been Tested?

Merge criteria:

bartoszmajsak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cam-garrison commented Apr 4, 2024

Choose a reason for hiding this comment

openshift-ci bot commented Apr 10, 2024

openshift-ci bot commented Apr 15, 2024

zdtsw commented Apr 15, 2024

cam-garrison commented Apr 15, 2024

israel-hdez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

israel-hdez Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

openshift-merge-robot commented Apr 18, 2024

bartoszmajsak commented Apr 19, 2024 • edited Loading

openshift-ci bot commented Apr 24, 2024

israel-hdez commented Jun 12, 2024

bartoszmajsak commented Jun 13, 2024 • edited Loading

israel-hdez commented Jun 13, 2024 • edited Loading

zdtsw commented Jun 14, 2024

israel-hdez commented Jun 14, 2024

bartoszmajsak commented Jun 17, 2024

israel-hdez Apr 16, 2024 •

edited

Loading

bartoszmajsak commented Apr 19, 2024 •

edited

Loading

bartoszmajsak commented Jun 13, 2024 •

edited

Loading

israel-hdez commented Jun 13, 2024 •

edited

Loading