-
Notifications
You must be signed in to change notification settings - Fork 119
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
Allow kafka-controller to list JobSinks #4155
Allow kafka-controller to list JobSinks #4155
Conversation
This is required otherwise KafkaBroker and KafkaSource can't forward events to JobSink. This type of error is thrown: failed to reconcile contract: failed to reconcile egress: failed to resolve subscriber: failed to get lister for sinks.knative.dev/v1alpha1, Resource=jobsinks: jobsinks.sinks.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:kafka-controller" cannot list resource "jobsinks" in API group "sinks.knative.dev" at the cluster scope
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4155 +/- ##
=======================================
Coverage 45.51% 45.51%
=======================================
Files 270 270
Lines 19925 19925
=======================================
Hits 9069 9069
Misses 10129 10129
Partials 727 727 ☔ View full report in Codecov by Sentry. |
/retest-required |
@mgencur I think the best fix would be to add jobsink to the addressable resolver aggregation in the core resources: https://github.com/knative/eventing/blob/main/config/core/roles/addressable-resolvers-clusterrole.yaml, that has the benefit that all controllers will get access to it for resolving the address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
I'll merge this one anyway, but I'd add the permissions there so that other controllers work too
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mgencur, pierDipi 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 |
1616ec3
into
knative-extensions:main
This is required otherwise KafkaBroker and KafkaSource can't forward events to JobSink. This type of error is thrown on Kafka Broker status:
I have also created a test for KafkaBroker forwarding events to JobSink to verify the fix. But it's probably not necessary to include it. Anyway, it's available here: mgencur@57dbb53
Fixes #
Proposed Changes
Release Note
Docs