feat: support auto generation of ContainerSource identity service account and expose in AuthStatus #7461

prakrit55 · 2023-11-21T03:22:40Z

Fixes #7228

Proposed Changes

Added feature for autogeneration of serviceaccount in ContainerSource
Added tests

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

Docs

Signed-off-by: Griffin <prakritimandal611@gmail.com>

knative-prow · 2023-11-21T03:22:50Z

Hi @prakrit55. 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: Griffin <prakritimandal611@gmail.com>

creydr

Hey @prakrit55,
thanks a lot for your PR. I left a few comments.

/ok-to-test

pkg/apis/sources/v1/container_lifecycle_test.go

pkg/reconciler/containersource/containersource_test.go

creydr · 2023-11-21T07:00:56Z

pkg/reconciler/containersource/controller.go

+	serviceaccountInformer := serviceaccountinformer.Get(ctx)
+
+	var globalResync func(obj interface{})
+	featureStore := feature.NewStore(logging.FromContext(ctx).Named("featur-config-store"),


Suggested change

featureStore := feature.NewStore(logging.FromContext(ctx).Named("featur-config-store"),

featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"),

creydr · 2023-11-21T07:02:18Z

pkg/reconciler/containersource/containersource_test.go

@@ -361,3 +435,22 @@ func makeSinkBindingStatus(ready *corev1.ConditionStatus) *sourcesv1.SinkBinding
 		},
 	}
 }
+
+func makeContainerSourceOIDCServiceAccount() *corev1.ServiceAccount {
+	return auth.GetOIDCServiceAccountForResource(sourcesv1.SchemeGroupVersion.WithKind("Source"), metav1.ObjectMeta{


Suggested change

return auth.GetOIDCServiceAccountForResource(sourcesv1.SchemeGroupVersion.WithKind("Source"), metav1.ObjectMeta{

return auth.GetOIDCServiceAccountForResource(sourcesv1.SchemeGroupVersion.WithKind("ContainerSource"), metav1.ObjectMeta{

Signed-off-by: Griffin <prakritimandal611@gmail.com>

pkg/apis/sources/v1/container_lifecycle_test.go

creydr · 2023-11-22T07:46:46Z

pkg/apis/sources/v1/container_lifecycle_test.go

@@ -179,6 +195,7 @@ func TestContainerSourceStatusIsReady(t *testing.T) {
 			s.InitializeConditions()
 			s.PropagateSinkBindingStatus(&notReadySinkBinding.Status)
 			s.PropagateReceiveAdapterStatus(availableDeployment)
+			s.MarkOIDCIdentityCreatedFailed("unknown", "")


Suggested change

s.MarkOIDCIdentityCreatedFailed("unknown", "")

s.MarkOIDCIdentityCreatedSucceeded()

Maybe we should put some cases where it gets failed, to make the tests more obedient.

That would be great if you could add such a test

pkg/apis/sources/v1/container_lifecycle_test.go

creydr

two more comments regarding the unit test failures.
BTW: you can always run all unit test locally via go test ./...

pkg/reconciler/containersource/controller_test.go

pkg/reconciler/containersource/containersource_test.go

Signed-off-by: Griffin <prakritimandal611@gmail.com>

codecov · 2023-11-22T12:04:40Z

Codecov Report

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

Comparison is base (c8f4624) 76.74% compared to head (eae182c) 76.81%.
Report is 27 commits behind head on main.

Files	Patch %	Lines
pkg/reconciler/containersource/controller.go	68.75%	3 Missing and 2 partials ⚠️
pkg/reconciler/containersource/containersource.go	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7461      +/-   ##
==========================================
+ Coverage   76.74%   76.81%   +0.06%     
==========================================
  Files         253      253              
  Lines       13916    14136     +220     
==========================================
+ Hits        10680    10858     +178     
- Misses       2702     2736      +34     
- Partials      534      542       +8

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

Signed-off-by: Griffin <prakritimandal611@gmail.com>

creydr

Hello @prakrit55,
thanks for adding the tests.
I left a few comments. As mentioned, you can run the tests locally too (via go test ./...) - this also saves review time 🙃

pkg/apis/sources/v1/container_lifecycle_test.go

Signed-off-by: Griffin <prakritimandal611@gmail.com>

creydr

Thanks a lot for your contribution @prakrit55!

/lgtm

knative-prow · 2023-11-23T09:49:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

creydr · 2023-11-23T10:31:16Z

/test reconciler-tests

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

* Revert "feat: support auto generation of ContainerSource identity service account and expose in AuthStatus (#7461)" This reverts commit 116abe2. * Containersource use OIDC identity of corresponding SinkBinding * Update dependencies * Add e2e test * Run gofmt and goimport

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

…nkBinding (#603) * Revert "feat: support auto generation of ContainerSource identity service account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2. * Containersource use OIDC identity of corresponding SinkBinding

…kBinding (#7892) * Revert "feat: support auto generation of ContainerSource identity service account and expose in AuthStatus (#7461)" This reverts commit 116abe2. * Containersource use OIDC identity of corresponding SinkBinding * Run gofmt and goimport

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

…kBinding (#7898) * Revert "feat: support auto generation of ContainerSource identity service account and expose in AuthStatus (#7461)" This reverts commit 116abe2. * Containersource use OIDC identity of corresponding SinkBinding * Run gofmt and goimports * Fix build issue

…nkBinding (#604) * Revert "feat: support auto generation of ContainerSource identity service account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2. * Containersource use OIDC identity of corresponding SinkBinding

prakrit55 added 2 commits November 5, 2023 07:31

added serviceaccount in containersource

9091ddc

Signed-off-by: Griffin <prakritimandal611@gmail.com>

added tests for containersource

3641487

Signed-off-by: Griffin <prakritimandal611@gmail.com>

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

knative-prow bot requested review from creydr and lionelvillard November 21, 2023 03:22

fixed lint

c8ca99c

Signed-off-by: Griffin <prakritimandal611@gmail.com>

creydr requested changes Nov 21, 2023

View reviewed changes

knative-prow bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 21, 2023

knative-prow bot assigned creydr Nov 21, 2023

knative-prow bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 21, 2023

modified tests

26df162

Signed-off-by: Griffin <prakritimandal611@gmail.com>

creydr requested changes Nov 22, 2023

View reviewed changes

pkg/reconciler/containersource/controller_test.go Show resolved Hide resolved

pkg/reconciler/containersource/containersource_test.go Outdated Show resolved Hide resolved

configured tests

ac3010a

Signed-off-by: Griffin <prakritimandal611@gmail.com>

prakrit55 added 2 commits November 22, 2023 18:24

fixed lint

6f42858

Signed-off-by: Griffin <prakritimandal611@gmail.com>

added tests

e00cfed

Signed-off-by: Griffin <prakritimandal611@gmail.com>

creydr requested changes Nov 23, 2023

View reviewed changes

fixed tests

eae182c

Signed-off-by: Griffin <prakritimandal611@gmail.com>

creydr approved these changes Nov 23, 2023

View reviewed changes

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

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

knative-prow bot merged commit 116abe2 into knative:main Nov 23, 2023
38 of 41 checks passed

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

356a273

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

8d0a68c

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

8ce890f

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

cb9da4a

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

c8062c7

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

2af74ec

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

58a6bfd

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

30bb944

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 3, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

b8188f4

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

creydr added a commit to creydr/knative-eventing that referenced this pull request May 7, 2024

Revert "feat: support auto generation of ContainerSource identity ser…

7ce6517

…vice account and expose in AuthStatus (knative#7461)" This reverts commit 116abe2.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support auto generation of ContainerSource identity service account and expose in AuthStatus #7461

feat: support auto generation of ContainerSource identity service account and expose in AuthStatus #7461

prakrit55 commented Nov 21, 2023

knative-prow bot commented Nov 21, 2023

creydr left a comment

creydr Nov 21, 2023

creydr Nov 21, 2023

creydr Nov 22, 2023

prakrit55 Nov 22, 2023

creydr Nov 22, 2023

creydr left a comment

codecov bot commented Nov 22, 2023 •

edited

Loading

creydr left a comment

creydr left a comment

knative-prow bot commented Nov 23, 2023

creydr commented Nov 23, 2023

	featureStore := feature.NewStore(logging.FromContext(ctx).Named("featur-config-store"),
	featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"),

	return auth.GetOIDCServiceAccountForResource(sourcesv1.SchemeGroupVersion.WithKind("Source"), metav1.ObjectMeta{
	return auth.GetOIDCServiceAccountForResource(sourcesv1.SchemeGroupVersion.WithKind("ContainerSource"), metav1.ObjectMeta{

	s.MarkOIDCIdentityCreatedFailed("unknown", "")
	s.MarkOIDCIdentityCreatedSucceeded()

feat: support auto generation of ContainerSource identity service account and expose in AuthStatus #7461

feat: support auto generation of ContainerSource identity service account and expose in AuthStatus #7461

Conversation

prakrit55 commented Nov 21, 2023

Proposed Changes

knative-prow bot commented Nov 21, 2023

creydr left a comment

Choose a reason for hiding this comment

creydr Nov 21, 2023

Choose a reason for hiding this comment

creydr Nov 21, 2023

Choose a reason for hiding this comment

creydr Nov 22, 2023

Choose a reason for hiding this comment

prakrit55 Nov 22, 2023

Choose a reason for hiding this comment

creydr Nov 22, 2023

Choose a reason for hiding this comment

creydr left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2023 • edited Loading

Codecov Report

creydr left a comment

Choose a reason for hiding this comment

creydr left a comment

Choose a reason for hiding this comment

knative-prow bot commented Nov 23, 2023

creydr commented Nov 23, 2023

codecov bot commented Nov 22, 2023 •

edited

Loading