knative / eventing

Event-driven application platform for Kubernetes
https://knative.dev/docs/eventing
Apache License 2.0
1.4k stars 590 forks source link

EventTypes should be registered by the EventType controller only #2484

Closed lionelvillard closed 4 years ago

lionelvillard commented 4 years ago

Problem Today each source implementation creates EventType objects. This is an unnecessary burden for the Source implementors.

An EventType consists of:

Persona: Which persona is this feature for? Source developer

Exit Criteria A measurable (binary) test that would indicate that the problem has been resolved.

Time Estimate (optional): How many developer-days do you think this may take to resolve? 1-2

Additional context (optional) Add any other context about the feature request here.

https://github.com/knative/eventing-contrib/issues/653 https://github.com/knative/eventing-contrib/issues/388

/cc @nachocano @n3wscott @vaikas

n3wscott commented 4 years ago

We should revisit EventType. It should not be linked to the broker. We likely should remove it and align with the CloudSubscriptions work being done in CloudEvents.

Thanks for bringing this up, we have until 0.13 cuts to prevent it from going to v1beta1 and I think we should no promote it in the current state.

mikehelmick commented 4 years ago

Strong objection to removing EventType altogether.

I agree that they should not be linked to a Broker and would like to see that change.

I would like to see EventType extended to (optionally) store the actual data of the event schema. This would allow developers to have an easy mechanism for creation and management of custom event types, keeping the entire experience on cluster.

mikehelmick commented 4 years ago

Added a PR that shows what I'm thinking for schema storage inline

vaikas commented 4 years ago

I agree with the original motivation here, namely:

  1. make it easy for the Source writer to "do the right thing". A source writer should already provide the list of eventtypes that it produces, and there are already proposals to reduce the work of the source writer as linked in the issues above. As far as removing the eventtypes, I do not personally think they should be removed as they serve as an important piece of discoverability. The original motivation for them was to provide a user with a way to discover what events are available within a broker. There was also some discussion about the Broker auto generating the types it has seen on the fly, and while that would solve some of the use cases, it would not work for the cases where events have not been seen yet by the broker, while also solving the case where events are emitted into a broker as response to other events (in which case there's really no Source emitting the events).

If we are thinking about changing the behaviour of the user facing experience, we should also probably have a conversation with the Client WG to ensure they are not surprised by any of these changes.

However, +1 to reducing the friction of the Event Source writers, which the original issue was about.

nachocano commented 4 years ago

/assign so that I don't miss this... I got us into this mess long time back, I will fix it :)

mattmoor commented 4 years ago

So I think there's an interesting use case that we should be sensitive to: Can I write a source without any Knative bits pre-installed in my cluster?

We started in a good place, and the current scheme regresses that in the sense the Source writers are expected to create EventType resources, which means they need the EventType CRD pre-installed 😬

Our goal state should be a model for source authors where they have zero install-time dependency on Knative (maybe they use a library, but that's linked into their controller). Our "pitch" for authoring sources be:

No downside, tons of upside.

This leaves a big open question of how we get the EventTypes created, and I think there's a bunch of ways we can get there, but I wanted to see what folks thought about the goal state?

markfisher commented 4 years ago

+1 @mattmoor

I would think any workload that emits CloudEvents should be a valid source. Likewise, any workload should be able to access a registry directly to look up CloudEvent schemas. Neither of those should necessarily require anything to be installed in the cluster.

CRDs and the reconcilers that watch/manage them can then provide support for higher-level experiences, such as injecting configuration metadata (e.g. the event type schema) into the workload's deployment manifest based on a simple resource-name lookup.

vaikas commented 4 years ago

curl is a valid source, you post to something :) I totally agree with the sentiment however, and as we have talked about in the past, I think exposing the configured event types in the status and decouple who injects them for other consumption would alleviate that concern.

nachocano commented 4 years ago

Just as FYI, created #2750 to track API improvements to EventType (which might take longer), and let this one to just focus on improving the current implementation.

nachocano commented 4 years ago

eventing-contrib counterpart merged: https://github.com/knative/eventing-contrib/pull/1036 Need to add more UTs in eventing, will do in the upcoming days. Overall, these changes cleaned up a bunch of things, and put us in a much better position to address changes to EventType https://github.com/knative/eventing/issues/2750