Refactor the AuthStatus Logic in Eventing OIDC Feature Track #7417

xiangpingjiang · 2023-10-29T12:30:16Z

Proposed Changes

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Refactor the AuthStatus Logic in Eventing OIDC Feature Track

Docs

Signed-off-by: pingjiang <[email protected]>

knative-prow · 2023-10-29T12:30:26Z

Hi @xiangpingjiang. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Signed-off-by: pingjiang <[email protected]>

codecov · 2023-10-31T02:31:13Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (67f382d) 76.79% compared to head (e9a9767) 76.70%.
Report is 9 commits behind head on main.

Files	Patch %	Lines
pkg/auth/serviceaccount.go	75.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7417      +/-   ##
==========================================
- Coverage   76.79%   76.70%   -0.09%     
==========================================
  Files         253      253              
  Lines       14098    14169      +71     
==========================================
+ Hits        10826    10868      +42     
- Misses       2732     2749      +17     
- Partials      540      552      +12

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

Leo6Leo · 2023-10-31T16:45:17Z

/ok-to-test

xiangpingjiang · 2023-11-02T10:28:01Z

/retest

Leo6Leo · 2023-11-13T14:35:37Z

Hey @xiangpingjiang, are you blocked on anything? Is this PR ready for review now?

xiangpingjiang · 2023-11-14T07:33:28Z

Hey @xiangpingjiang, are you blocked on anything? Is this PR ready for review now?

hello @Leo6Leo

I think it's ready for rewiew, just the upgrade-tests_eventing_main test sometimes fails, sometimes succeeds. Can I ignore it ?

Leo6Leo · 2023-11-14T17:37:04Z

@xiangpingjiang Sounds good, thanks for the PR! Will take a look at it!

creydr

Hello @xiangpingjiang,
thanks for working on this. I left a few comments (mostly about naming)

pkg/auth/serviceaccount.go

Signed-off-by: pingjiang <[email protected]>

creydr · 2023-11-16T15:27:54Z

/retest

Cali0707

I really like how this is looking so far!

Cali0707 · 2023-11-20T20:28:39Z

pkg/auth/serviceaccount.go

+	MarkOIDCIdentityCreatedFailed(reason, messageFormat string, messageA ...interface{})
+}
+
+func SetupOIDCServiceAccount(flags feature.Flags, ctx context.Context, serviceAccountLister corev1listers.ServiceAccountLister, kubeclient kubernetes.Interface, gvk schema.GroupVersionKind, objectMeta metav1.ObjectMeta, marker OIDCIdentityStatusMarker, setAuthStatus func(a *duckv1.AuthStatus)) pkgreconciler.Event {


@xiangpingjiang would you be able to add a unit test or two for this function? That way we can catch any breaking changes earlier if this every gets updated

@Cali0707 I will try to do that. SetupOIDCServiceAccount func rely on EnsureOIDCServiceAccountExistsForResource func, the EnsureOIDCServiceAccountExistsForResource didn't have unit test. Do I need add unit test for EnsureOIDCServiceAccountExistsForResource first ?

I think just SetupOIDCServiceAccount is totally fine, but if you wanted to add tests for both that would be great (just not necessary IMO)

Hello @Cali0707
I added some unit test, please give me a review when you have time

pkg/auth/serviceaccount.go

Signed-off-by: pingjiang <[email protected]>

…//developer20.com/go-context/ Signed-off-by: pingjiang <[email protected]>

Signed-off-by: pingjiang <[email protected]>

Cali0707

/lgtm
/cc @creydr

knative-prow · 2023-11-28T14:49:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

Refactor the AuthStatus Logic in Eventing OIDC Feature Track

3e0485f

Signed-off-by: pingjiang <[email protected]>

knative-prow bot requested review from lionelvillard and odacremolbap October 29, 2023 12:30

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2023

xiangpingjiang marked this pull request as draft October 30, 2023 01:37

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2023

fix ineffectual assignment to authStatus

15f886c

Signed-off-by: pingjiang <[email protected]>

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2023

revert apiserversource_test file

a7628df

Signed-off-by: pingjiang <[email protected]>

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 31, 2023

xiangpingjiang marked this pull request as ready for review October 31, 2023 02:50

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 Oct 31, 2023

knative-prow bot requested review from aliok and aslom October 31, 2023 02:50

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 31, 2023

xiangpingjiang force-pushed the RefactorAuthStatus branch from 13bd9a5 to a7628df Compare November 5, 2023 15:55

creydr reviewed Nov 15, 2023

View reviewed changes

pkg/auth/serviceaccount.go Outdated Show resolved Hide resolved

pkg/auth/serviceaccount.go Outdated Show resolved Hide resolved

pkg/auth/serviceaccount.go Outdated Show resolved Hide resolved

pkg/auth/serviceaccount.go Outdated Show resolved Hide resolved

xiangpingjiang added 2 commits November 16, 2023 17:58

fix after rewiews

504bca7

Signed-off-by: pingjiang <[email protected]>

OIDCAuthStatusUtility to SetupOIDCServiceAccount

f24b0df

Signed-off-by: pingjiang <[email protected]>

fix unit test

96e975c

Signed-off-by: pingjiang <[email protected]>

xiangpingjiang requested a review from creydr November 16, 2023 11:16

Cali0707 reviewed Nov 20, 2023

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2023

Merge branch 'main' into RefactorAuthStatus

9f04579

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2023

xiangpingjiang added 5 commits November 21, 2023 23:48

revert pkg/reconciler/sinkbinding/controller.go

df6e18a

Signed-off-by: pingjiang <[email protected]>

make context is the first argument to a function by convention https:…

d29077c

…//developer20.com/go-context/ Signed-off-by: pingjiang <[email protected]>

add TestEnsureOIDCServiceAccountExistsForResource

20df018

Signed-off-by: pingjiang <[email protected]>

add TestSetupOIDCServiceAccount

b3f6747

Signed-off-by: pingjiang <[email protected]>

apply to parallel and sequence

e9a9767

Signed-off-by: pingjiang <[email protected]>

xiangpingjiang requested a review from Cali0707 November 28, 2023 11:56

Cali0707 reviewed Nov 28, 2023

View reviewed changes

knative-prow bot assigned Cali0707 Nov 28, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2023

creydr approved these changes Nov 28, 2023

View reviewed changes

knative-prow bot assigned creydr Nov 28, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2023

knative-prow bot merged commit 023d618 into knative:main Nov 28, 2023
32 of 34 checks passed

xiangpingjiang deleted the RefactorAuthStatus branch February 3, 2024 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the AuthStatus Logic in Eventing OIDC Feature Track #7417

Refactor the AuthStatus Logic in Eventing OIDC Feature Track #7417

xiangpingjiang commented Oct 29, 2023

knative-prow bot commented Oct 29, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

Leo6Leo commented Oct 31, 2023

xiangpingjiang commented Nov 2, 2023

Leo6Leo commented Nov 13, 2023

xiangpingjiang commented Nov 14, 2023

Leo6Leo commented Nov 14, 2023

creydr left a comment

creydr commented Nov 16, 2023

Cali0707 left a comment

Cali0707 Nov 20, 2023

xiangpingjiang Nov 22, 2023 •

edited

Loading

Cali0707 Nov 22, 2023

xiangpingjiang Nov 28, 2023 •

edited

Loading

Cali0707 left a comment

knative-prow bot commented Nov 28, 2023

Refactor the AuthStatus Logic in Eventing OIDC Feature Track #7417

Refactor the AuthStatus Logic in Eventing OIDC Feature Track #7417

Conversation

xiangpingjiang commented Oct 29, 2023

Proposed Changes

Pre-review Checklist

knative-prow bot commented Oct 29, 2023

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

Leo6Leo commented Oct 31, 2023

xiangpingjiang commented Nov 2, 2023

Leo6Leo commented Nov 13, 2023

xiangpingjiang commented Nov 14, 2023

Leo6Leo commented Nov 14, 2023

creydr left a comment

Choose a reason for hiding this comment

creydr commented Nov 16, 2023

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 Nov 20, 2023

Choose a reason for hiding this comment

xiangpingjiang Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Cali0707 Nov 22, 2023

Choose a reason for hiding this comment

xiangpingjiang Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Nov 28, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

xiangpingjiang Nov 22, 2023 •

edited

Loading

xiangpingjiang Nov 28, 2023 •

edited

Loading