Sequence and Parallel: announce correct OIDC identities in authstatus #7902

creydr · 2024-05-07T13:16:05Z

Currently the Sequence and Parallel use a dedicated OIDC service account in their .status.auth.serviceAccountName. Anyhow the underlying channel dispatchers use the SAs from the Subscription(s), so that the announced OIDC identity of the Sequence/Parallel is not correct.
In knative/pkg#3032 the AuthStatus was adjusted to support to announce multiple serviceAccountNames.
This PR addresses the above issue and populates the Sequences and Parallels .status.auth.serviceAccountNames (plural) with the OIDC identities from the underlying subscriptions.

Hint:

865edbc reverts the changes in the Sequence to create the OIDC SA (e5f2814)
a32d1da reverts the changes in the Parallel to create the OIDC SA (dc96522)

Release Note

fix: Expose OIDC identities of underlying Subscriptions in Sequence and Parallel

codecov · 2024-05-07T13:22:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.42%. Comparing base (7e1c082) to head (c7953fc).
Report is 43 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7902      +/-   ##
==========================================
+ Coverage   69.22%   69.42%   +0.20%     
==========================================
  Files         339      344       +5     
  Lines       19494    15897    -3597     
==========================================
- Hits        13494    11037    -2457     
+ Misses       5337     4184    -1153     
- Partials      663      676      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…[OIDC] (knative#7361)" This reverts commit e5f2814.

This reverts commit dc96522.

creydr · 2024-05-08T09:59:27Z

wait.go:200: test-mbcwiubs/sinkbinding-qcvlukqo condition is {"type":"Ready","status":"False","lastTransitionTime":"2024-05-08T07:37:57Z","reason":"TrustBundlePropagation","message":"failed to create ConfigMap test-mbcwiubs/knative-eventing-bundlekne-bundle: configmaps \"knative-eventing-bundlekne-bundle\" already exists"}

/retest-required

/cc @pierDipi

creydr · 2024-05-08T11:14:24Z

wait.go:200: test-xyfdvrfi/sinkbinding-fcpikzdn condition is {"type":"Ready","status":"False","lastTransitionTime":"2024-05-08T11:08:51Z","reason":"TrustBundlePropagation","message":"failed to create ConfigMap test-xyfdvrfi/knative-eventing-bundlekne-bundle: configmaps \"knative-eventing-bundlekne-bundle\" already exists"}

/retest-required

creydr · 2024-05-08T12:39:31Z

@pierDipi, we could also migrate the existing usages of serviceAccountName to serviceAccountNames for the other components too and then deprecate serviceAccountName. But we should do that in a separate PR.

creydr · 2024-05-10T06:08:19Z

@pierDipi, @Cali0707 could you PTAL?

Cali0707 · 2024-05-13T15:08:15Z

@creydr do we want to backport this to 1.14? If we do, maybe it would make sense to only have the CRD changes for the parallel and sequence in this PR, and then change the others in a separate PR?

creydr · 2024-05-13T15:31:52Z

@creydr do we want to backport this to 1.14? If we do, maybe it would make sense to only have the CRD changes for the parallel and sequence in this PR, and then change the others in a separate PR?

We would need to backport the knative/pkg changes too (to have the additional field). Therefor I tend to not backport it, as this is also mostly relevant for the upcoming authZ work (and in alpha state too) 🤷

Cali0707

Just have a small question about unit testing this (which we could also do as a follow up or maybe isn't needed)

Otherwise lgtm!

Cali0707 · 2024-05-13T16:35:13Z

pkg/apis/flows/v1/parallel_lifecycle_test.go

@@ -336,121 +327,89 @@ func TestParallelPropagateSubscriptionStatusUpdated(t *testing.T) {

 func TestParallelReady(t *testing.T) {


In this unit test (or another one?) could we add a test that verifies that the parallel correctly sets the audiences from the subscriptions?

Good point. For the audience, we have

eventing/pkg/apis/flows/v1/parallel_lifecycle_test.go

Lines 491 to 496 in 4951b74

}, {

name: "Audience",

address: &duckv1.Addressable{Audience: audience},

want: &duckv1.Addressable{Audience: audience},

wantStatus: corev1.ConditionTrue,

}}

. So I guess you mean the OIDC service account names (but we have this kind of in the e2e tests: 32f59d4). But I can add a unit test tomorrow, which might help on debugging

Cali0707 · 2024-05-13T16:35:51Z

pkg/apis/flows/v1/sequence_lifecycle_test.go

@@ -324,70 +315,52 @@ func TestSequencePropagateChannelStatuses(t *testing.T) {

 func TestSequenceReady(t *testing.T) {


In this unit tests (or another one?) could we add a test that verifies that the sequence correctly sets its audiences based off of the subscription audiences?

Similar to #7902 (comment)

Cali0707

/lgtm

knative-prow · 2024-05-14T12:42:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, creydr

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~~ [Cali0707,creydr]

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

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/test-and-release Test infrastructure, tests or release labels May 7, 2024

knative-prow bot requested review from aslom and mgencur May 7, 2024 13:16

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 7, 2024

creydr added 7 commits May 8, 2024 08:26

Update CRDs to include AuthStatus serviceAccountNames

e80b652

Revert "support auto generation of Sequence identity service account …

862a579

…[OIDC] (knative#7361)" This reverts commit e5f2814.

Update Sequence to expose OIDC identities of underlying Subscriptions

7ab72b8

Revert "Add serviceaccount in parallel (knative#7373)"

0ea5d8c

This reverts commit dc96522.

Update Parallel to expose OIDC identities of underlying Subscriptions

0599d49

Add e2e test for Parallel

32f59d4

Add e2e test for Sequence

b8c7756

creydr force-pushed the sequence-and-parallel-announce-correct-oidc-identities-in-authstatus branch from c79a69e to b8c7756 Compare May 8, 2024 06:29

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 8, 2024

creydr changed the title ~~[WIP] Sequence and Parallel: announce correct OIDC identities in authstatus~~ Sequence and Parallel: announce correct OIDC identities in authstatus May 8, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2024

knative-prow bot requested a review from pierDipi May 8, 2024 09:59

Cali0707 reviewed May 13, 2024

View reviewed changes

Add unit tests

c7953fc

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 14, 2024

creydr requested a review from Cali0707 May 14, 2024 07:20

Cali0707 approved these changes May 14, 2024

View reviewed changes

knative-prow bot assigned Cali0707 May 14, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2024

knative-prow bot merged commit 6b6f6d1 into knative:main May 14, 2024
35 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sequence and Parallel: announce correct OIDC identities in authstatus #7902

Sequence and Parallel: announce correct OIDC identities in authstatus #7902

creydr commented May 7, 2024 •

edited

codecov bot commented May 7, 2024 •

edited

creydr commented May 8, 2024

creydr commented May 8, 2024

creydr commented May 8, 2024

creydr commented May 10, 2024

Cali0707 commented May 13, 2024

creydr commented May 13, 2024

Cali0707 left a comment

Cali0707 May 13, 2024

creydr May 13, 2024

Cali0707 May 13, 2024

creydr May 13, 2024

Cali0707 left a comment

knative-prow bot commented May 14, 2024

		@@ -336,121 +327,89 @@ func TestParallelPropagateSubscriptionStatusUpdated(t *testing.T) {

		func TestParallelReady(t *testing.T) {

	}, {
	name: "Audience",
	address: &duckv1.Addressable{Audience: audience},
	want: &duckv1.Addressable{Audience: audience},
	wantStatus: corev1.ConditionTrue,
	}}

		@@ -324,70 +315,52 @@ func TestSequencePropagateChannelStatuses(t *testing.T) {

		func TestSequenceReady(t *testing.T) {

Sequence and Parallel: announce correct OIDC identities in authstatus #7902

Sequence and Parallel: announce correct OIDC identities in authstatus #7902

Conversation

creydr commented May 7, 2024 • edited

codecov bot commented May 7, 2024 • edited

Codecov Report

creydr commented May 8, 2024

creydr commented May 8, 2024

creydr commented May 8, 2024

creydr commented May 10, 2024

Cali0707 commented May 13, 2024

creydr commented May 13, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 May 13, 2024

Choose a reason for hiding this comment

creydr May 13, 2024

Choose a reason for hiding this comment

Cali0707 May 13, 2024

Choose a reason for hiding this comment

creydr May 13, 2024

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented May 14, 2024

creydr commented May 7, 2024 •

edited

codecov bot commented May 7, 2024 •

edited