knative / eventing-contrib

Event Sources
Apache License 2.0
224 stars 225 forks source link

GitHubSource shouldn't require Knative Serving #510

Closed imjasonh closed 3 years ago

imjasonh commented 4 years ago

Expected Behavior

The GitHubSource controller creates a Knative Service (example) in response to newly created GitHubSource resources. This Service is responsible for receiving webhooks from GitHub.

If Knative Serving is not installed on the cluster, installing Eventing and GitHubSource fails when it attempts to create the Kn Service.

Actual Behavior

It should be the default/only behavior of the controller to create vanilla K8s resources to listen for webhook messages, so that this source doesn't have a hard dependency on Knative Serving.

Or, if Serving is expected to be a hard requirement for this event source, it should be documented upfront. If Serving is expected to be a hard requirement for all or even most event sources, that should be documented in Eventing's documentation.

In general, the relationship and interdependency of Eventing and Serving should be spelled out more clearly. Are they "building blocks" that work well together, or is Eventing an optional addition to Serving, which requires Serving? Even if Eventing doesn't require Serving, if all or most of the most popular well-supported Eventing sources do require Serving, then Eventing effectively does require it.

mattmoor commented 4 years ago

tl;dr We don't want the core of Eventing to have a hard dependency on Serving, but it's perfectly fine for sources to depend on it.

For webhook sources like this, I believe a serving dependency makes a lot of sense, and brings a lot of value, off the top of my head:

Doing this in terms of K8s abstractions, you'd end up with every webhook reinventing a fair amount of stuff.

This is just one category of source though, and I think a reasonable sub-division is to think of them in terms of K8s "apps" abstractions: Deployment (see: ContainerSource), Job, CronJob, StatefulSet, ...

@n3wscott and I put together a doc after discussing this generalization of ContainerSource a few weeks back in Germany.

duglin commented 4 years ago

+1 In my talks/demos people seem to really like that the github eventsource creates a KnService and they automatically get all of the cool benefits like auto-scaling. When the authors of a project don't "eat their own dog food", it tends to send a bad message.

If part of the concern that KnServing is too big for someone who just wants KnEventing, then we may want to consider ways to slim KnServing down.

imjasonh commented 4 years ago

So for users who want to use Eventing without Serving, and want to ingest GitHub events specifically, would you recommend writing a new Source CRD+controller using ContainerSource? Is there a sample I can crib from to try this out myself? Maybe I don't know what I'm missing. :smile:

Perhaps my issue title is too strong: it's probably completely fine that the default/preferred GitHubSource requires Knative Serving, since there are clear benefits there. But for users that don't want to install Serving and its dependencies (e.g., Istio, or some other service mesh) and just want to ingest GitHub events, the path forward is not very clear.

Scaling to zero is definitely useful, but if the user can expect to receive sufficient traffic 24/7, scaling to zero isn't ever going to happen. A regular HPA or VPA can scale normal Pods, in that case.

In a sense, not making the non-Serving path well-lit is not "eating your own dogfood", it's prescribing a solution for people. That's what I mean by delineating the separation of concerns between Serving and Eventing. They're "two great tastes that taste great together", but they should also be separately usable and well-documented. Dogfooding that process is as important as dogfooding the Serving-requiring path, IMO.

Or, if the two projects want to be "two great tastes that taste great together" but one only tastes okay without the other, that's fine too, but then the documentation should make that clearer. As a specific request, docs should denote which sources require Serving and which don't. (Perhaps this is just a docs bug, in this case)

duglin commented 4 years ago

But for users that don't want to install Serving and its dependencies (e.g., Istio, or some other service mesh) and just want to ingest GitHub events, the path forward is not very clear.

Perhaps some config on it would help here:

  - some-flag-that-implies-setup-importer: false (default is true)

and when false it'll use the sink as the address for the github webhook, and NOT create a KnService to get the events.

vincent-pli commented 4 years ago

@ImJasonH @duglin Agree with you both. Several months ago, I have the some requirement with @ImJasonH , so I write this:https://github.com/vincent-pli/gitlabsource. use the k8s svc and deployment to expose the event received adapter.

So I guess this requirement is exist, we need consider it.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.