Adding Source Spec. by n3wscott · Pull Request #1856 · knative/eventing

n3wscott · 2019-09-10T17:37:51Z

Related to #1554

Proposed Changes

Add a first draft of a sources spec.

Release Note

Adding source-observer account and cluster roles to get and watch sources using aggregated RBAC roles.

n3wscott · 2019-09-10T17:38:02Z

/cc @spencer-p

knative-prow-robot · 2019-09-10T17:38:04Z

@n3wscott: GitHub didn't allow me to request PR reviews from the following users: spencer-p.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @spencer-p

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.

Harwayne

Wasn't there supposed to be something about the K_SINK env var?

Harwayne · 2019-09-10T17:47:36Z

+aggregationRule:
+  clusterRoleSelectors:
+    - matchLabels:
+        duck.knative.dev/source: "true"


Should this be duck.knative.dev/sourceObserver? In case we ever need a distinct ClusterRole in the future. Or just to differentiate from the label that is on sources themselves.

This is how I have been thinking about it: the duck is a source; the role is source observer.

n3wscott · 2019-09-10T17:59:51Z

RE:

K_SINK

This is only for Knative Source pod-specable runtimes. This will be a followup.

Co-Authored-By: Adam Harwayne <harwayne@google.com>

Justin2997

Good work :) I will try to move the RabbitMQ event source to this structure.

Justin2997 · 2019-09-10T18:26:49Z

+    duck.knative.dev/source: "true" # <-- required to be a source.
+```
+
+CRDs SHOULD be added to the `sources` category:


I suppose that the to yaml should make one file, this section is confusing to me.

I actutally do appreciate the breakdown of the different sections. This is a spec - not a tutorial.

Co-Authored-By: Adam Harwayne <harwayne@google.com>

matzew · 2019-09-11T07:31:17Z

+
+A Source:
+
+- Represent an off or on-cluster system, service or application.


Represents ?

I am not sure if the source itself does represent that system, service or app.

I usually tell people that with a source, you are able to connect to a 3rd party system, service or app - living on the cluster or somewhere outside.

you are right, it should be Represents the request for events from...

matzew · 2019-09-11T07:34:08Z

+In practice, sources are an abstract concept that allow us to create declarative
+configurations through the usage of Custom Resource Definitions (CRDs) extending
+Kubernetes. Those CRDs are instantiated by creating an instance of the resource.
+It is up to the implementation of the source author to understand the best way


Source Author: a persona ?

matzew · 2019-09-11T07:39:51Z

+      - example.com
+    resources:
+      - foos
+      - foos/status


- foos/finalizers

does an observer need to see the finalizers?

…ces_spec

vaikas

Thanks for doing this! Some comments...

matzew · 2019-09-12T13:56:42Z

+1 on V1

On Thu 12. Sep 2019 at 00:32, Ville Aikas ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks for doing this! Some comments... ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > +rules: + - apiGroups: + - example.com + resources: + - foos + - foos/status + verbs: + - get + - list + - watch +``` + +## Source Resource Shape + +The minimum definition of the Kubernetes resource shape is defined in the +[Source](https://godoc.org/github.com/knative/pkg/apis/duck/v1beta1#Source) Let's bump these to v1? ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > +```go +type SourceSpec struct { + // Sink is a reference to an object that will resolve to a domain name or a + // URI directly to use as the sink. + Sink apisv1alpha1.Destination `json:"sink,omitempty"` + + // CloudEventOverrides defines overrides to control the output format and + // modifications of the event sent to the sink. + // +optional + CloudEventOverrides *CloudEventOverrides `json:"ceOverrides,omitempty"` +} +``` + +For a full definition of `Sink` and `CloudEventsOverrides`, please see +[Destination](https://godoc.org/knative.dev/pkg/apis/v1alpha1#Destination), and +[CloudEventOverrides](https://godoc.org/github.com/knative/pkg/apis/duck/v1beta1#CloudEventOverrides). and here to v1? ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > + // inherits duck/v1beta1 Status, which currently provides: + // * ObservedGeneration - the 'Generation' of the Service that was last + // processed by the controller. + // * Conditions - the latest available observations of a resource's current + // state. + Status `json:",inline"` + + // SinkURI is the current active sink URI that has been configured for the + // Source. + // +optional + SinkURI *apis.URL `json:"sinkUri,omitempty"` +} +``` + +For a full definition of `Status` and `SinkURI`, please see +[Status](https://godoc.org/github.com/knative/pkg/apis/duck/v1beta1#Status), and aaaand here? ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > + + + +## Source RBAC + +Knative leverages an aggregated RBAC role to allow for controllers to check the +status of source type resources. Might be worth it to mention a use case or two as examples? Motivation section if you will :) ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > + +- Represents an off or on-cluster system, service or application that produces + events to be consumed by a sink. +- Produces or imports CloudEvents. +- Sends CloudEvents to the configured **sink**. + +In practice, sources are an abstract concept that allow us to create declarative +configurations through the usage of Custom Resource Definitions (CRDs) extending +Kubernetes. Those CRDs are instantiated by creating an instance of the resource. +It is up to the implementation of the source author to understand the best way +to realize the source application. This could be as 1:1 deployments inside of +Kubernetes per resource, as a single multi-tenant application, or even an +off-cluster implementation; or all combinations in-between. + +There are some guidelines on implementing sources to allow cluster operators and +tools to dynamically discover and understand source installations. This section sets up the following ones quite nicely. I'd expand on this just a little more only because source installation is only one part, the other part is instantiating a source (CRD vs. CR) in which case you want to convey additional information (in the Status of the resource as described below). So, we might try to expand on this explanation a bit. ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > +- Sends CloudEvents to the configured **sink**. + +In practice, sources are an abstract concept that allow us to create declarative +configurations through the usage of Custom Resource Definitions (CRDs) extending +Kubernetes. Those CRDs are instantiated by creating an instance of the resource. +It is up to the implementation of the source author to understand the best way +to realize the source application. This could be as 1:1 deployments inside of +Kubernetes per resource, as a single multi-tenant application, or even an +off-cluster implementation; or all combinations in-between. + +There are some guidelines on implementing sources to allow cluster operators and +tools to dynamically discover and understand source installations. + +## Source CRDs + +CRDs that are to be understood as a `source` MUST be labeled: Let's expand a bit on this. It would be maybe be useful to include the motivation here, for example, so support the Source in the listing (for example kubectl get ... command). So in addition to MUST be labeled, why would be helpful. Understood is just a bit of a vague term here. ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + labels: + duck.knative.dev/source: "true" # <-- required to be a source. +``` + +CRDs SHOULD be added to the `sources` category: + +```yaml +spec: + names: + categories: + - sources +``` + We should also talk about the validation aspects and encourage proper Description for the fields and so forth also? ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > +## Source CRDs + +CRDs that are to be understood as a `source` MUST be labeled: + +```yaml +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + labels: + duck.knative.dev/source: "true" # <-- required to be a source. +``` + +CRDs SHOULD be added to the `sources` category: + +```yaml +spec: We should mention consistent use of the Conditions and in particular the one Ready condition that we have defined in the apis.ConditionReady and probably add that as a recommended additional Printer Column for consistency? ------------------------------ In docs/spec/sources.md <#1856 (comment)>: > +For a full definition of `Sink` and `CloudEventsOverrides`, please see +[Destination](https://godoc.org/knative.dev/pkg/apis/v1alpha1#Destination), and +[CloudEventOverrides](https://godoc.org/github.com/knative/pkg/apis/duck/v1beta1#CloudEventOverrides). + +### duck.Status + +The `status` field is expected to have the following minimum shape: + +```go +type SourceStatus struct { + // inherits duck/v1beta1 Status, which currently provides: + // * ObservedGeneration - the 'Generation' of the Service that was last + // processed by the controller. + // * Conditions - the latest available observations of a resource's current + // state. + Status `json:",inline"` We should probably mention here about the apis.ConditionSet and each Source IMHO should have the apis.ConditionReady as the one way to look at the readiness of the Source. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1856?email_source=notifications&email_token=AABGPTTORLUJYJNCQBXLCVTQJFWX5A5CNFSM4IVKTF4KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEOIQPY#pullrequestreview-287082559>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABGPTTC4QE3AUVNZUGFIOLQJFWX5ANCNFSM4IVKTF4A> .

-- Sent from Gmail Mobile

matzew · 2019-09-24T14:03:34Z

perhaps this is relevant ?

#1956

I noticed that the eventing-contrib is even (gh) on a different notion :-)

vaikas · 2019-09-24T22:28:25Z

/approve

rhuss · 2019-10-01T07:50:42Z

While working on the Kn Eventing proposal, I remarked that this proposal supports a field .spec.eventTypes for all sources, which might be worth to consider to be included into this spec. This should be an array of event types for which an instance of this source CRD is configured to emit.

Does this make sense to add it as an (optional ?) field into this source specification ?

// @Harwayne @sixolet

matzew · 2019-10-01T10:28:17Z

On Tue 1. Oct 2019 at 09:50, Roland Huß ***@***.***> wrote: While working on the Kn Eventing proposal <https://docs.google.com/document/d/1B8VK-5nuxt3D-i1Up3onmUbzcztQXE0GRgf1lfx49P8/edit#>, I remarked that this proposal supports a field .spec.eventTypes <https://docs.google.com/document/d/1B8VK-5nuxt3D-i1Up3onmUbzcztQXE0GRgf1lfx49P8/edit#bookmark=id.lwk2rcdndj4> for all sources, which might be worth to consider to be included into this spec. This should be an array of event types for which an instance of this source CRD is configured to emit. Does this make sense to add it as an (optional ?) field into this source specification ?

yes, sounds reasonable to me! // @Harwayne <https://github.com/Harwayne> @sixolet

<https://github.com/sixolet> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1856?email_source=notifications&email_token=AABGPTVLO7JQTSQXKIXC4WDQML6NJA5CNFSM4IVKTF4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAAKXMI#issuecomment-536914865>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABGPTWOYI6Z4CZQM2QUBFDQML6NJANCNFSM4IVKTF4A> .

-- Sent from Gmail Mobile

evankanderson

A bunch of minor typographical suggestions; I don't think any make substantive design changes.

evankanderson · 2019-09-30T22:46:45Z

 - [Channel specification](channel.md) and
  [older channel (0.6.0) spec](channel_060.md)
-
-<!-- TODO(n3wscott): * [Error conditions and reporting](errors.md) -->


You might just reference https://knative.dev/docs/serving/spec/knative-api-specification-1.0/#error-signalling for the general error pattern, rather than needing to write your own.

vaikas · 2019-10-03T21:44:17Z

@n3wscott could we try to get this merged this week? It's blocking a few things and would be nice to unblock asap. Looks like most changes in the latest review are fairly cosmetic, so I'm hoping it will go quickly.

evankanderson · 2019-10-07T20:28:28Z

/lgtm
/approve

knative-prow-robot · 2019-10-07T20:28:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, n3wscott, 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~~ [evankanderson,n3wscott,vaikas-google]

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

* Patch the upstream config file to repair or reenable old behavior Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Adding check for (legacy) annotations Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Running 'make RELEASE=v1.6 generate-release' Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

Adding Source concept spec.

bb35aeb

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 10, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 10, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2019

knative-prow-robot requested review from Harwayne and evankanderson September 10, 2019 17:38

Harwayne reviewed Sep 10, 2019

View reviewed changes

n3wscott and others added 5 commits September 10, 2019 11:01

Update docs/spec/sources.md

557dec6

Co-Authored-By: Adam Harwayne <harwayne@google.com>

Update docs/spec/sources.md

b0ce5ec

Co-Authored-By: Adam Harwayne <harwayne@google.com>

Update docs/spec/sources.md

2084202

Co-Authored-By: Adam Harwayne <harwayne@google.com>

Update config/200-source-observer-clusterrole.yaml

0f19324

Co-Authored-By: Adam Harwayne <harwayne@google.com>

Update docs/spec/sources.md

8c898ce

Co-Authored-By: Adam Harwayne <harwayne@google.com>

Justin2997 reviewed Sep 10, 2019

View reviewed changes

Comment thread config/200-source-observer-clusterrole.yaml Outdated

rename observer, update text.

67f6df8

Justin2997 reviewed Sep 10, 2019

View reviewed changes

Update docs/spec/sources.md

5ea9a86

Co-Authored-By: Adam Harwayne <harwayne@google.com>

lionelvillard reviewed Sep 11, 2019

View reviewed changes

Comment thread docs/spec/sources.md Outdated

lionelvillard reviewed Sep 11, 2019

View reviewed changes

Comment thread docs/spec/sources.md

matzew reviewed Sep 11, 2019

View reviewed changes

Comment thread docs/spec/sources.md

matzew reviewed Sep 11, 2019

View reviewed changes

Harwayne mentioned this pull request Sep 11, 2019

Trigger status depends on importer status #1849

Merged

Scott Nichols added 2 commits September 11, 2019 09:20

typos.

8194bc9

Merge branch 'sources_spec' of github.com:n3wscott/eventing into sour…

684b438

…ces_spec

vaikas reviewed Sep 11, 2019

View reviewed changes

Scott Nichols added 2 commits September 18, 2019 07:52

no need to give read on /status.

851c857

comment out the registry section for now until we come to consensus.

b315642

mattmoor-sockpuppet reviewed Sep 18, 2019

View reviewed changes

bot nit.

5fe57df

This was referenced Sep 19, 2019

Add ClusterRole to get, list and watch sources in eventing-contrib knative/eventing-contrib#609

Merged

Add ClusterRole to get, list and watch pullsubscriptions and storages google/knative-gcp#292

Merged

vaikas reviewed Sep 24, 2019

View reviewed changes

Comment thread docs/spec/sources.md Outdated

evankanderson reviewed Oct 1, 2019

View reviewed changes

rgregg mentioned this pull request Oct 1, 2019

Define an importer specification #1554

Closed

vaikas mentioned this pull request Oct 3, 2019

Discussion: How to add CLI meta data to Source CRDs ? #1940

Closed

rhuss reviewed Oct 4, 2019

View reviewed changes

Comment thread docs/spec/sources.md Outdated

rhuss reviewed Oct 4, 2019

View reviewed changes

Comment thread docs/spec/sources.md Outdated

rhuss reviewed Oct 4, 2019

View reviewed changes

Comment thread docs/spec/sources.md Outdated

n3wscott mentioned this pull request Oct 7, 2019

Should sources have a spec.eventType always, start support for upstream filtering? #2003

Closed

updated wording from good feedback.

30b3726

knative-prow-robot assigned evankanderson Oct 7, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019

knative-prow-robot merged commit 0366517 into knative:master Oct 7, 2019

capri-xiyue mentioned this pull request Oct 9, 2019

Rename cluster role to get, list and watch sources in eventing-contrib so that it can be consistent with source spec knative/eventing-contrib#644

Closed

nachocano mentioned this pull request Oct 23, 2019

Add registry documentation to Source spec #2077

Closed

This was referenced Nov 27, 2019

kn source <type> - manage untyped sources knative/client#496

Closed

kn trigger create - create a source implicitely (based on #479) knative/client#498

Closed


		A Source:

		- Represent an off or on-cluster system, service or application.

Conversation

n3wscott commented Sep 10, 2019

Proposed Changes

Uh oh!

n3wscott commented Sep 10, 2019

Uh oh!

knative-prow-robot commented Sep 10, 2019

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

n3wscott commented Sep 10, 2019

Uh oh!

Uh oh!

Justin2997 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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matzew commented Sep 12, 2019 via email

Uh oh!

matzew commented Sep 24, 2019

Uh oh!

Uh oh!

vaikas commented Sep 24, 2019

Uh oh!

rhuss commented Oct 1, 2019

Uh oh!

matzew commented Oct 1, 2019 via email

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!