Broker eventtype autocreate fixes

[Cali0707]

Fixes #7154

Proposed Changes

• Add in logic to handler broker missing typemeta
• Check the owner references on the channel to figure out if eventtypes should be created for the channel or for a broker

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

Event Types are now only created once when using a MTChannelBasedBroker.

[Cali0707]

/cc @matzew @dsimansk

[codecov]

Codecov Report

Patch coverage: 27.77% and project coverage change: -0.05% ⚠️

Comparison is base (7636997) 77.93% compared to head (3d462ac) 77.89%.
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7161      +/-   ##
==========================================
- Coverage   77.93%   77.89%   -0.05%     
==========================================
  Files         246      246              
  Lines       13200    13214      +14     
==========================================
+ Hits        10288    10293       +5     
- Misses       2390     2400      +10     
+ Partials      522      521       -1     

Files Changed	Coverage Δ
pkg/broker/ingress/ingress_handler.go	56.70% <0.00%> (-2.13%)	⬇️
...iler/inmemorychannel/dispatcher/inmemorychannel.go	64.13% <50.00%> (-0.68%)	⬇️

... and 2 files with indirect coverage changes

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

[dsimansk]

/lgtm

[Cali0707]

Btw @matzew I'm not 100% sure if the approach I've taken here is going to work for all cases. Because I'm using the owner reference on a channel and verifiying that it is a broker, if a broker creates a channel and then I send events directly to the channel (and not to the broker), the event type will still be registered with the broker as the owner. Do you think this behaviour is okay?

[matzew]

Do you think this behaviour is okay?

yes, that is OK.

However the event sent should be visible for the broker. Is that still the case?

[Cali0707]

@matzew this is the current behaviour:

kn-event-linux-amd64 send \
  --to Broker:eventing.knative.dev/v1:my-broker \
  --type com.corp.integration.warning \
  -f message="fooss"

kn-event-linux-amd64 send \
  --to InMemoryChannel:messaging.knative.dev/v1:my-broker-kne-trigger \
  --type org.somewhere.error \
  -f message="message"

kubectl get eventtypes.eventing.knative.dev -A
NAMESPACE   NAME                      TYPE                                   SOURCE             SCHEMA   REFERENCE NAME   REFERENCE KIND   DESCRIPTION                             READY   REASON
default     et-my-broker-b3jnlnnvbw   org.somewhere.error                    kn-event/v1.10.0            my-broker        Broker           Event Type auto-created by controller   True    
default     et-my-broker-y29tlmnvcn   com.corp.integration.warning           kn-event/v1.10.0            my-broker        Broker           Event Type auto-created by controller   True    

[matzew]

@Cali0707 Sorry for getting back late here.

kn-event-linux-amd64 send \
  --to InMemoryChannel:messaging.knative.dev/v1:my-broker-kne-trigger \
  --type org.somewhere.error \
  -f message="message"

when sending it directly to the channel, I'd not expect it to be "reference" for the broker. 🤔

[Cali0707]

when sending it directly to the channel, I'd not expect it to be "reference" for the broker. 🤔

@matzew the way I can currently see us implementing this is by adding a header or CE extension e.g. ce-type-owner-ref. When ce-type-owner-ref is set, we create the event type based on it, and if it is not present then we just use the default owner. So, on the broker ingress we could set this CE extension before passing the event to the channel, but events sent to the channel would not have the extension set.

Does this make sense, or do you have another idea?

[matzew]

@Cali0707
So, I think we should just completely ignore those, directly sent to the channel URL.

The channel is an IMP detail, and if we have a channel, created by the broker: only because that's how the broker is implemented.

I think an event also directly (as in incorrectly) sent to the channel (instead of the broker), is also not getting out to the sink of the trigger.

Hence, we should just correct the current behavior, described on the bug (which currently puts refernce to IMC, but really is broker).

So, "managed" channel (e.g. by broker) should not do the auto-create

[matzew]

/hold

[Cali0707]

@matzew this now behaves as follows:

kn-event-linux-amd64 send \                   
  --to Broker:eventing.knative.dev/v1:my-broker \      
  --type new.event.type \     
  -f message="message"

kn-event-linux-amd64 send \
  --to InMemoryChannel:messaging.knative.dev/v1:my-broker-kne-trigger \
  --type org.somewhere.error \
  -f message="message"

kn-event-linux-amd64 send \            
  --to Channel:messaging.knative.dev/v1:example-channel \
  --type org.somewhere.error \
  -f message="message"

kubectl get eventtypes.eventing.knative.dev
NAMESPACE   NAME                            TYPE                  SOURCE             SCHEMA   REFERENCE NAME    REFERENCE KIND    DESCRIPTION                             READY   REASON
default     et-example-channel-b3jnlnnvbw   org.somewhere.error   kn-event/v1.10.0            example-channel   InMemoryChannel   Event Type auto-created by controller   True    
default     et-my-broker-bmv3lmv2zw         new.event.type        kn-event/v1.10.0            my-broker         Broker            Event Type auto-created by controller   True    

Where example-channel and my-broker are user created, and the other channel was created by the broker under the hood (and hence did not have an event created).

[Cali0707]

/cc @matzew @Leo6Leo @dsimansk

[matzew]

Thanks to tackling this!

/lgtm
/approve

[knative-prow]

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

• OWNERS [matzew]

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

[matzew]

/unhold

[matzew]

/cherry-pick release-v1.11

[knative-prow-robot]

@matzew: cannot checkout release-v1.11: error checking out release-v1.11: exit status 1. output: error: pathspec 'release-v1.11' did not match any file(s) known to git

In response to this:

/cherry-pick release-v1.11

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.

[Cali0707]

/cherry-pick release-1.11

[knative-prow-robot]

@Cali0707: #7161 failed to apply on top of branch "release-1.11":

Applying: Fixed undefined typemeta on broker in eventtype autocreate
Applying: Fixed autocreate so that only one eventtype is created when events are sent to mt channel broker
Applying: Clean up
Using index info to reconstruct a base tree...
A	eventing-controller.env
A	imc-dispatcher.env
A	mt-broker-ingress.env
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Fixed unit tests
Applying: channel only creates eventtypes if not owned by broker
Using index info to reconstruct a base tree...
M	pkg/reconciler/inmemorychannel/dispatcher/controller.go
M	pkg/reconciler/inmemorychannel/dispatcher/inmemorychannel.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/inmemorychannel/dispatcher/inmemorychannel.go
CONFLICT (content): Merge conflict in pkg/reconciler/inmemorychannel/dispatcher/inmemorychannel.go
Auto-merging pkg/reconciler/inmemorychannel/dispatcher/controller.go
CONFLICT (content): Merge conflict in pkg/reconciler/inmemorychannel/dispatcher/controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 channel only creates eventtypes if not owned by broker
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.11

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.