Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

creydr
Copy link
Member

@creydr creydr commented May 7, 2024

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

@knative-prow 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 knative-prow bot requested review from aslom and mgencur May 7, 2024 13:16
@knative-prow 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
Copy link

codecov bot commented May 7, 2024

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.

@creydr 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 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 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 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
@creydr
Copy link
Member Author

creydr commented May 8, 2024

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

@knative-prow knative-prow bot requested a review from pierDipi May 8, 2024 09:59
@creydr
Copy link
Member Author

creydr commented May 8, 2024

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
Copy link
Member Author

creydr commented May 8, 2024

@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
Copy link
Member Author

creydr commented May 10, 2024

@pierDipi, @Cali0707 could you PTAL?

@Cali0707
Copy link
Member

@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
Copy link
Member Author

creydr commented May 13, 2024

@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) 🤷

Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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

func TestParallelReady(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. For the audience, we have

}, {
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

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

func TestSequenceReady(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to #7902 (comment)

@knative-prow 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 creydr requested a review from Cali0707 May 14, 2024 07:20
Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

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

knative-prow bot commented May 14, 2024

[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:

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

@knative-prow knative-prow bot merged commit 6b6f6d1 into knative:main May 14, 2024
35 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants