Channel CRDs by scothis · Pull Request #66 · knative/eventing

scothis · 2018-05-31T04:15:07Z

Active work in progress based on the demo to the Eventing WG this morning. Code is spike/demo quality at the moment. Opening a PR to facilitate review and discussions.

Open questions:

~~names for CRDs~~
~~provisioning API for streams/subscriptions on a broker (push vs pull)~~
...

TODOs:

validate functionality inside a knative cluster
~~add tests~~
~~add docs~~
proper updating / removal of bus, channel and subscription resources
~~create a non-stub broker implementation~~
~~unify RouteRule creation under the Stream resource (influenced by Subscriptions for brokerless streams)~~
remove custom RouteRule struct once Add Istio RouteRule HTTPRewrite serving#1002 is merged

Refs knative/serving#940

// cc @vaikas-google @markfisher

- and Istio RouteRule CRD - generate go client for new CRDs

As an alternative to the subscription controller pushing new subscriptions to the broker. The monitor allows a broker to use the k8s api server to watch subscriptions.

scothis · 2018-06-01T15:29:48Z

Rebased on top of the knative rename. I generally don't like to force push to a branch, but a merge would have been ugly and harder to follow.

scothis · 2018-06-01T17:50:52Z

tl;dr proposed new CRD names:

Subscription (stays the same)
Stream -> Channel
Broker -> EventGateway (BrokerAgent?)

Based on comments on the WG call, slack and doc, there is a bit of confusion around the CRD names. We are open to further refining these names.

@takidau mentioned that the glossary defines a Stream as a sequence of events. The closest glossary term is Trigger, but that doesn't feel right in this model. Our proposal is to use Channel, as named destination event producers can publish to and event consumers can subscribe to. This aligns nicely with the EIP Message Channel

Broker didn't sit well with us because the resource doesn't provide a broker, but mediates one. For example, to use Google Pub/Sub, the event producer doesn't need to speak the Pub/Sub protocol or authenticate the request. This behavior is more inline with a Gateway, however, Istio already has a Gateway CRD. Our proposal is to call it an EventGateway.

One downside of using the Gateway term is that it implies connecting outside of the system. The stub broker in this PR is entirely in-memory. We've also discussed the term BrokerAgent.

Make k8s the source of truth for broker config

A Bus may specify a provisioner. The provisioner is expected to listen for new Channel resources for the Bus and provision them on the underlying broker. The container remains expected to handle and produce events for subscribers. TODO: - lock down the clusterrolebinding for the provisioner and event serviceaccounts for the role.

vaikas

just couple of nits.What's the status of the PR, still DNM?

vaikas · 2018-06-07T17:22:58Z

+	Status             *SubscriptionStatus `json:"status,omitempty"`
+}
+
+// Spec (what the user wants) for a subscription


Not a big deal for now, just jotting this down but these should be of the form:
// SubscriptionSpec specifies what the user wants for a subscription

As in, the first word should be SubscriptionSpec

here and elsewhere.

vaikas · 2018-06-07T17:26:17Z

+# Install
+
+```
+kubectl apply -f square.yaml


Don't you need to push this somewhere so the cluster will find it?

This was written for minikube, will update.

The square sample is now unified with the hello sample

vaikas · 2018-06-07T17:27:29Z

@@ -0,0 +1,50 @@
+apiVersion: extensions/v1beta1


this might be good to have in a form of Route/Configuration, but perhaps the idea is to demo that Serving stack is not necessary? Just curious.

I think it would be nice to have examples of both. We've been using this with the Primer sample also, and that does work (although it's not very interesting as you can only tail the log until we work out the next destination/reply behavior). We could include another configuration yaml for that.

The idea for Hello was the have the simplest possible base that could be implemented in raw k8s/istio and then the same functionality with channels.

I agree it would be good to have a sample that uses serving, perhaps square can do that.

The hello sample now uses a Knative service which is subscribed to a channel.

…ption

A bus can now define parameters that a channel provides as arguments, and a channel can define parameters that a subscription provides as arguments. This allows for custom configuration of channels and subscriptions according to the capabilities of the bus.

scothis · 2018-06-07T23:20:04Z

@vaikas-google this PR is now functionally complete for an mvp:

the bus is split into two deployments, a provisioner and dispatcher
the bus deployments run with a service account that only allows read-only access to channel.eventing.knative.dev CRDs (we will want to relax this for status updates in the future)
the bus-channel and channel-subscription relationships can specify parameters/arguments to configure themselves on the bus

I'll pickup changes based on your feedback later tonight or tomorrow morning.

scothis · 2018-06-08T18:16:00Z

+  name: aloha2hello
+spec:
+  channel: aloha
+  subscriber: hello-00001-service


I'd rather target the route rather than a specific revision, however, the route rule that forwards traffic from the route to revision has a host filter that drops the requests. We will need to either add more configuration to the subscription or remove the filter from the route rule.

or we could probably set the host header? Anyways, fine for now.

vaikas

One big question about the need to add in Istio. Did I miss where we use it?

vaikas · 2018-06-08T18:23:00Z

+rules:
+- apiGroups: ["channels.knative.dev"]
+  resources: ["buses", "channels", "subscriptions"]
+  verbs: ["get", "watch", "list"]


nit, missing newline here.

vaikas · 2018-06-08T18:24:05Z

+            value: https://github.com/projectriff/node-function-invoker.git#buildpack
+          - name: SKIP_DETECT
+            value: "true"
+        # - name: CACHE


delete? or add a comment?

vaikas · 2018-06-08T18:25:24Z

+          name: buildpack
+          arguments:
+          - name: IMAGE
+            value: &image DOCKER_REPO_OVERRIDE/hello


I think for these you can do:
value: github.com/knative/eventing/sample/hello

or something like that and then:
ko apply -f sample/hello/

and the right things happen.

vaikas · 2018-06-08T18:26:23Z

+  name: aloha2hello
+spec:
+  channel: aloha
+  subscriber: hello-00001-service


or we could probably set the host header? Anyways, fine for now.

vaikas · 2018-06-08T18:32:43Z

 ${CODEGEN_PKG}/generate-groups.sh "deepcopy,client,informer,lister" \
  github.com/knative/eventing/pkg/client github.com/knative/eventing/pkg/apis \
-  feeds:v1alpha1 \
+  "channels:v1alpha1 feeds:v1alpha1 istio:v1alpha2" \


I don't see Istio being used anywhere but perhaps I missed it? Rest of this seems fine, but just trying to understand why we need to import Istio in.

I hope to get rid of the custom Istio client in the near future. I would have used the client created by serving, except that it doesn't have the struct for rewriting the http host in a RouteRule.

I can drive the PR in serving to completion and then drop this client.

There it was ! :)

/cc @tcnghia who is working on importing the Istio 0.8 types into serving.

I filed a bug, with the hope that we only need to vendor/ this stuff in the future. istio/istio#6084

In the mean time, for istio v1alpha3 we will need to do the same (cloning _types.go from *.proto files and run codegen)

If you use the client in /serving you'll benefit from my istio/v1alpha3 PR (coming). I am translating everything, so you should have access to all features, unlike the version we hae for istio/v1alpha2

@tcnghia Thanks. I'll keep a look out for the PR in serving.

vaikas · 2018-06-08T19:02:08Z

/lgtm
/approve

google-prow-robot · 2018-06-08T19:02:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: scothis, vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas-google]

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

@evankanderson

@evankanderson

From [ROLES.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/ROLES.md#approver): > Reviewer of the codebase for at least 3 months or 50% of project lifetime, whichever is shorter - [First Issue](#80). Opened 6/11 - [First PR](#66). Opened 5/31 - [First Review](#79 (review)) 6/11 > Primary reviewer for at least 10 substantial PRs to the codebase - #422 (review) - #414 (review) - #325 (review) - #225 (review) - #189 (review) - #168 (review) - #165 (review) - #99 (review) - #79 (review) - #111 (review) > Reviewed or merged at least 30 PRs to the codebase - [Reviewed 23 PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+reviewed-by%3Ascothis) - [Authored 34 merged PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+author%3Ascothis+is%3Amerged) - [Authored 5 open PRs](https://github.com/knative/eventing/pulls/scothis) > Nominated by an area lead From [WORKING_GROUPS.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/WORKING-GROUPS.md#events) /assign @vaikas-google > With no objections from other leads 🤞 /cc @evankanderson @grantr @inlined @mattmoor

@evankanderson

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 31, 2018

scothis added 5 commits May 31, 2018 16:45

Import Broker, Stream and Subscriptions CRDs

5085643

- and Istio RouteRule CRD - generate go client for new CRDs

Add controllers for new CRDs

b2b526c

Stub broker implementation

c819788

Add hello and square samples

0cac79b

Add subscription monitor

9bf905c

As an alternative to the subscription controller pushing new subscriptions to the broker. The monitor allows a broker to use the k8s api server to watch subscriptions.

scothis and others added 13 commits June 1, 2018 17:54

Merge remote-tracking branch 'upstream/master' into stream-crd

d18efa6

Add Streams to the Subscription Monitor

e2d423a

Shift burden of tracking subscriptions and stream to broker

ad3970d

Merge pull request #1 from scothis/stream-crd-monitor

04da06a

Make k8s the source of truth for broker config

Cleanup lingering CRD group names

1069cda

Rename Stream to Channel

dcbab6a

Rename Broker to Bus

e39fd26

Drop 'Busless' support

efd068e

Merge remote-tracking branch 'upstream/master' into stream-crd

33866d6

Move channel CRDs into channels.eventing.knative.dev group

4b5eb8f

polish

c9118fa

Merge remote-tracking branch 'upstream/master' into stream-crd

20d1088

vaikas reviewed Jun 7, 2018

View reviewed changes

scothis added 3 commits June 7, 2018 15:33

Provisioner polish

de07ce8

Descope bus service account to read-only for bus, channel and subscri…

a45911d

…ption

scothis added 4 commits June 8, 2018 10:49

Add missing license headers

9262da3

Rename group channels.eventing.knative.dev to channels.knative.dev

ac6c8ff

Create a single sample that subscribes an Knative serivce to a channel

41887ee

polish

0695c25

scothis changed the title ~~[DNM] Stream CRD~~ Channel CRDs Jun 8, 2018

scothis commented Jun 8, 2018

View reviewed changes

vaikas reviewed Jun 8, 2018

View reviewed changes

google-prow-robot assigned vaikas Jun 8, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2018

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2018

google-prow-robot merged commit df9d5d9 into knative:master Jun 8, 2018

scothis deleted the stream-crd branch June 8, 2018 19:12

evankanderson mentioned this pull request Jun 21, 2018

Refine bus monitor #88

Merged

scothis mentioned this pull request Sep 11, 2018

Add scothis as an approver #426

Merged

Conversation

scothis commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scothis commented Jun 1, 2018

Uh oh!

scothis commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scothis commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaikas commented Jun 8, 2018

Uh oh!

google-prow-robot commented Jun 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scothis commented May 31, 2018 •

edited

Loading

scothis commented Jun 1, 2018 •

edited

Loading