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

Auto create events in channels #7089

Merged
merged 7 commits into from
Jul 25, 2023

Conversation

Cali0707
Copy link
Member

Fixes #7044

Proposed Changes

  • Refactor eventtypes.go and eventtypes_test.go out of the broker package into their own package to facilitate sharing code with channels.
  • Use the auto create funcionality from the eventtypes package in the fanout message handler to autocreate event types.

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

Even Type auto-create feature:
- Based on CloudEvents processed in an inmemorychannel corresponding `EventType` resources are created in the namespace  

Docs

Tracking issue for docs: knative/docs#5612

@knative-prow 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 Jul 12, 2023
@knative-prow
Copy link

knative-prow bot commented Jul 12, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot requested review from matzew and pierDipi July 12, 2023 19:53
@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 32.05% and project coverage change: -0.30% ⚠️

Comparison is base (ed05a35) 78.29% compared to head (115461e) 77.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7089      +/-   ##
==========================================
- Coverage   78.29%   77.99%   -0.30%     
==========================================
  Files         250      250              
  Lines       13290    13367      +77     
==========================================
+ Hits        10405    10426      +21     
- Misses       2353     2405      +52     
- Partials      532      536       +4     
Files Changed Coverage Δ
pkg/broker/ingress/ingress_handler.go 60.09% <ø> (ø)
pkg/channel/fanout/fanout_message_handler.go 74.66% <21.42%> (-12.64%) ⬇️
...iler/inmemorychannel/dispatcher/inmemorychannel.go 65.19% <31.03%> (-6.52%) ⬇️
...nnelfanout/multi_channel_fanout_message_handler.go 86.88% <100.00%> (ø)
pkg/eventtype/eventtypes.go 72.22% <100.00%> (ø)
...econciler/inmemorychannel/dispatcher/controller.go 81.73% <100.00%> (+0.83%) ⬆️

... and 8 files with indirect coverage changes

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

@Cali0707
Copy link
Member Author

@creydr @pierDipi I'm getting the following error when running the InMemoryChannel after making my changes:

k8s.io/[email protected]/tools/cache/reflector.go:169: Failed to watch *v1beta2.EventType: failed to list *v1beta2.EventType: eventtypes.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:imc-dispatcher" cannot list resource "eventtypes" in API group "eventing.knative.dev" at the cluster scope

Any ideas what I should do?

@creydr
Copy link
Member

creydr commented Jul 13, 2023

@creydr @pierDipi I'm getting the following error when running the InMemoryChannel after making my changes:

k8s.io/[email protected]/tools/cache/reflector.go:169: Failed to watch *v1beta2.EventType: failed to list *v1beta2.EventType: eventtypes.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:imc-dispatcher" cannot list resource "eventtypes" in API group "eventing.knative.dev" at the cluster scope

Any ideas what I should do?

Hey @Cali0707,
it seems the imc-dispatcher user does not have permissions to list EventTypes. The config/channels/in-memory-channel/roles/dispatcher-clusterrole.yaml file defines the rules for this user. So you need to add the required permissions there too.

@Cali0707
Copy link
Member Author

/hold
This needs #7091 to be resolved before it will work

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2023
@Cali0707
Copy link
Member Author

This is now working for me:

kn-event-linux-amd64 send \                                                                                      
--to Channel:messaging.knative.dev/v1:example-channel \
--type org.test.message.error \
-f message="There is a problem"
kn-event-linux-amd64 send \                   
--to Channel:messaging.knative.dev/v1:example-channel \
--type com.company.test.warning \
-f message="There is a problem"
kubectl get eventtypes.eventing.knative.dev -A
NAMESPACE   NAME                            TYPE                       SOURCE             SCHEMA   REFERENCE NAME    REFERENCE KIND    DESCRIPTION                             READY   REASON
default     et-example-channel-b3jnlnrlc3   org.test.message.error     kn-event/v1.10.0            example-channel   InMemoryChannel   Event Type auto-created by controller   True    
default     et-example-channel-y29tlmnvbx   com.company.test.warning   kn-event/v1.10.0            example-channel   InMemoryChannel   Event Type auto-created by controller   True    

@Cali0707
Copy link
Member Author

Note: I had to change how the eventtype names are autogenerated because they were not unique enough. With the old function, I could only get one unique event name per channel/namespace pair. With the new function, I can get unique event names if the event type is different enough (e.g. org.test.message.error vs. com.company.test.warning), but if they are too similar then it still has issues. I will open a new issue to fix that as it isn't directly related to these changes

@Cali0707 Cali0707 changed the title [WIP]: Auto create events in channels Auto create events in channels Jul 18, 2023
@Cali0707 Cali0707 marked this pull request as ready for review July 18, 2023 18:33
@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 Jul 18, 2023
@knative-prow knative-prow bot requested a review from creydr July 18, 2023 18:33
@Cali0707
Copy link
Member Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2023
@@ -152,7 +153,7 @@ func main() {

// Init auto-create only if enabled, after ConfigMap watcher is started
if featureStore.IsEnabled(feature.EvenTypeAutoCreate) {
autoCreate := &broker.EventTypeAutoHandler{
autoCreate := &eventtype.EventTypeAutoHandler{
Copy link
Member

Choose a reason for hiding this comment

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

nice to move that out

Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2023
@knative-prow
Copy link

knative-prow bot commented Jul 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2023
@knative-prow knative-prow bot merged commit 855fbed into knative:main Jul 25, 2023
23 of 28 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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the auto-create feature flag for the InMemoryChannel
3 participants