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 <bartosz.majsak@gmail.com>

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

controllers/dscinitialization/utils.go

controllers/dscinitialization/servicemesh_test.go

controllers/dscinitialization/utils.go

controllers/dscinitialization/servicemesh_setup.go

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

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 <bartosz.majsak@gmail.com>

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 hold it until I finally understand audiences.

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.

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 <bartosz.majsak@gmail.com>

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 <bartosz.majsak@gmail.com>

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

Merge branch 'incubation' into automate-audiences

287b303

zdtsw reviewed Apr 9, 2024

View reviewed changes

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

bartoszmajsak requested changes Apr 9, 2024

View reviewed changes

cam-garrison and others added 2 commits April 9, 2024 13:18

Update controllers/dscinitialization/servicemesh_test.go

0a32786

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

move tests + helpers to cluster pkg, rework tests

e349c3c

cam-garrison requested a review from bartoszmajsak April 9, 2024 21:05

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 <bartosz.majsak@gmail.com>

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

openshift-merge-robot commented Apr 18, 2024

bartoszmajsak commented Apr 19, 2024

openshift-ci bot commented Apr 24, 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

Choose a reason for hiding this comment

openshift-merge-robot commented Apr 18, 2024

bartoszmajsak commented Apr 19, 2024

openshift-ci bot commented Apr 24, 2024

israel-hdez Apr 16, 2024 •

edited