Make Actions dynamc and add sample. by inlined · Pull Request #50 · knative/eventing

inlined · 2018-04-25T21:12:18Z

Fixes Issue #44

Proposed Changes

1. Changed Bind.Spec.Action to match planned API
2. Reran codegen
3. Update code until existing GitHub demo worked
4. Factor out parts of the Bind controller so that we can
   cleanly handle Binds without triggers (will help with KubeCon demo)
5. Added dynamic action support
6. Added Service action
7. Added sample

Steps: 1. Changed Bind.Spec.Action to match planned API 2. Reran codegen 3. Update code until existing GitHub demo worked 4. Factor out parts of the Bind controller so that we can cleanly handle Binds without triggers (will help with KubeCon demo) 5. Added dynamic action support 6. Added Service action 7. Added sample

google-prow-robot · 2018-04-25T21:12:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inlined

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~~ [inlined]

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

inlined · 2018-04-25T21:13:20Z

/assign @eobrain

If you feel like this is something you'd like to work on today, that's great. Otherwise I can work with you or @vaikas-google to get this reviewed after KubeCon

eobrain · 2018-04-25T23:09:34Z

/unassign @eobrain
As discussed offline, sorry but I'm not going to have time to get to this before going away on leave.

vaikas

Thanks!!
I could be totally mistaken, but as it is, Elafros core has changed a bit and this will not work as is against it. Mainly figuring out the DNS of the route has changed.

vaikas · 2018-04-27T16:20:33Z

+// without an EventTrigger, it is assumed that the real event source is incompatible
+// with our event framework's control plane and will be set up manually through
+// side channels.
+func (t *EventTrigger) Zero() bool {


nit, I think I'd prefer something like isZero or isEmpty. Zero to me indicates you're zeroing something. Might be good to say something about why the go default zero value is not enough in the comments also.

vaikas · 2018-04-27T16:35:16Z

+		parts := strings.Split(name, "/")
+		if len(parts) == 2 {
+			namespace, name = parts[0], parts[1]
+		}


What happens if name does not contain namespace? Seems like we should accept one or the other (namespace, name OR name=namespace/name) but not both here?

vaikas · 2018-04-27T16:37:10Z

+
+// SendEvent will POST the event spec to the root URI of the elafros route.
+func (a *serviceAction) SendEvent(name string, data interface{}, context *event.Context) (interface{}, error) {
+	glog.Infof("Sending event %s to K8S service %s", context.EventID, name)


when logging things I prefer to use %q instead of %s so they get quoted.

vaikas · 2018-04-27T16:56:10Z

+	if err != nil {
+		return "", err
+	}
+	domainSuffix, ok := c.Data["domainSuffix"]


This has changed. There can be multiple of these. I have opened this issue to track this:
knative/serving#762

Aaaand digging further, you can get it from the route.Status:
https://github.com/elafros/elafros/blob/master/pkg/apis/ela/v1alpha1/route_types.go#L114

vaikas · 2018-04-27T17:05:59Z

+metadata:
+  name: ela-config
+  namespace: ela-system
+data:


this has changed. Just grab the one from elafros/elafros/elaconfig.yaml as a new starting point.

vaikas · 2018-04-27T17:09:37Z

+          - containerPort: 8080
+        env:
+          - name: KEY_SUFFIX
+            value: "InService"


nit, add newline here.

vaikas · 2018-04-27T17:09:53Z

+        elafros.dev/type: container
+    spec:
+      container:
+        image: action-demo:latest


nit, missing trailing newline.

vaikas · 2018-04-27T17:10:10Z

+    # Demos must currently publish directly to the endpoint for this Bind
+  action:
+    name: action-demo
+    processor: eventing.elafros.dev/EventLogger


nit: missing trailing newline.

vaikas · 2018-04-27T17:10:54Z

+sets an environment variable to change where in the Firebase Realtime Database events
+are published.
+
+To mkae the `action-demo` bind point to the Service vrsion of our demo, edit `bind.yaml` and change the spec.action.processor to `Service`.


is it just 'Service' or elafros.dev/Service?

or eventing.elafros.dev/Service?

vaikas · 2018-04-27T17:13:18Z

+
+If you were already running the elafros controllers, you will need to kill the ela-controller in the ela-system namespace for it to pick up the new domain suffix.
+
+4. Since we will be mimicing an internal service, we need access to the event-delivery service.


Just want to make sure I understand this correctly, is the plan here that every event coming in will be required to go through the EDS? I know in the past we've talked that there's value in being able to hit the Ela routes directly without always requiring an EDS. At least I understood from conversations with @markfisher that this could be desireable.

markfisher · 2018-04-30T14:36:37Z

 type BindAction struct {
-	// RouteName specifies Elafros route as a target.
-	RouteName string `json:"routeName,omitempty"`
+	// Processor dictates the kind of service that will handle the Event.


Just curious if you considered naming this "Handler". When it comes to naming, I tend to look for clues in the description, and in this case you wrote:

...will handle the Event

markfisher · 2018-04-30T18:20:15Z

Changed Bind.Spec.Action to match planned API

@inlined is that planned API documented somewhere that is publicly accessible?

markfisher · 2018-04-30T21:52:17Z

+// Bind implements EventSource.Bind
+func (s *GithubEventSource) Bind(bind *v1alpha1.Bind, parameters map[string]interface{}) (map[string]interface{}, error) {
+	glog.Infof("BINDING GITHUB EVENT")
+	// BUG(#43) The GitHub event source should be responsible for its own gateway rathan relying


Does this mean there will be a running process for the GitHub event source (separate from the Binding Controller)? If so will it be an Elafros Revision itself, subject to auto-scaling?

markfisher · 2018-05-04T21:22:43Z

-	RouteName string `json:"routeName,omitempty"`
+	// Processor dictates the kind of service that will handle the Event.
+	// For example "elafros.dev/Route"
+	Processor string `json:"processor"`


It seems like Action itself could simply be target where the value is a URL. Both elafrosAction and serviceAction are ultimately HTTP POSTs where the difference is just the way the URL is constructed. As for loggingAction, I would assume logging is an orthogonal concern that would be enabled regardless of the action type, not as an action in its own right.

The Processor field gives us some extra flexibility in configuring different types of actions. Even if all the actions expect HTTP POSTs, a same-cluster Elafros action might have different requirements than an external action (e.g. different auth headers).

Also conceivably we might want to allow non-HTTP actions.

So I think there is value in keeping the Processor field.

Wouldn't the internal vs. external header requirements best be handled via Istio/Envoy?

And is there an example of a non-HTTP action that's more than "conceivable" at this point? If not, it seems premature to add a strategy pattern. Also since the different action types in this PR are hardcoded, it's not yet designed in a way to be open for extension.

markfisher · 2018-05-05T14:35:45Z

+// Zero determines whether an EventTrigger is fully empty. If a Bind is set up
+// without an EventTrigger, it is assumed that the real event source is incompatible
+// with our event framework's control plane and will be set up manually through
+// side channels.


Is there any example or doc that describes the event framework control plane compatibility, and what this "side channels" concept means? Why would a bind be created if it's not establishing a connection to a trigger?

adrcunha · 2018-06-07T21:04:51Z

/retest

google-prow-robot · 2018-06-07T21:07:54Z

@inlined: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-eventing-unit-tests	`780320d`	link	`/test pull-knative-eventing-unit-tests`
pull-knative-eventing-build-tests	`780320d`	link	`/test pull-knative-eventing-build-tests`
pull-knative-eventing-integration-tests	`780320d`	link	`/test pull-knative-eventing-integration-tests`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

adrcunha · 2018-06-07T21:16:31Z

Note: the tests failed because the base commit is old (i.e., before the presubmit tests were implemented). Rebasing the PR should cause the tests to pass (e.g., #77).

inlined · 2018-07-09T18:37:29Z

This should have been closed ages ago. It took a long time for the WG to agree that we need certain abstractions and the code is out of date enough that it's probably as easy to rewrite if anything needs to be salvaged.

Includes the following changes 1e7c2e7 Update static_watcher.go d688349 Include note about embedding the ManualWatcher in the InformedWatcher 767d6a5 Few changes to the configmap package 0122abd The test logger will now log the correct caller (knative#63) e7a4b0d Dont call flag.parse in pkg/test (knative#62) b213523 Update test-infra dependency (knative#61) 382a2bf Make kube_checks generic so that it can be used in serving (knative#58) 760afb6 cleanup (knative#57) eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54) 6eff182 Remove docker repo from e2e flags (knative#53) 4be5c07 Vendor the test-infra scripts (knative#52) 8c687df Update WaitForEndpointState to return response (knative#51) 8f6a3be Update knative/pkg/test (knative#50) 3ca4270 Add a logkey for the reconcile key. (knative#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43) f4a77d7 Add a common test clients file (knative#46) 450739d Add common test logging module (knative#45)

Includes the following changes: 8fc80de Few changes to the configmap package (knative#59) 0122abd The test logger will now log the correct caller (knative#63) e7a4b0d Dont call flag.parse in pkg/test (knative#62) b213523 Update test-infra dependency (knative#61) 382a2bf Make kube_checks generic so that it can be used in serving (knative#58) 760afb6 cleanup (knative#57) eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54) 6eff182 Remove docker repo from e2e flags (knative#53) 4be5c07 Vendor the test-infra scripts (knative#52) 8c687df Update WaitForEndpointState to return response (knative#51) 8f6a3be Update knative/pkg/test (knative#50) 3ca4270 Add a logkey for the reconcile key. (knative#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43) f4a77d7 Add a common test clients file (knative#46) 450739d Add common test logging module (knative#45

* bump knative/pkg to 8fc80de Includes the following changes: 8fc80de Few changes to the configmap package (#59) 0122abd The test logger will now log the correct caller (#63) e7a4b0d Dont call flag.parse in pkg/test (#62) b213523 Update test-infra dependency (#61) 382a2bf Make kube_checks generic so that it can be used in serving (#58) 760afb6 cleanup (#57) eedc0a9 Make verify-codegen.sh compatible with OS X (#54) 6eff182 Remove docker repo from e2e flags (#53) 4be5c07 Vendor the test-infra scripts (#52) 8c687df Update WaitForEndpointState to return response (#51) 8f6a3be Update knative/pkg/test (#50) 3ca4270 Add a logkey for the reconcile key. (#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (#43) f4a77d7 Add a common test clients file (#46) 450739d Add common test logging module (#45 * Use updated methods that indicate that the configmap.Watcher is using an informer

…g_1.7 CI was here...:fire:

inlined added 2 commits April 25, 2018 14:05

Bazel auto-fixes

780320d

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 25, 2018

google-prow-robot assigned eobrain Apr 25, 2018

google-prow-robot unassigned eobrain Apr 25, 2018

vaikas reviewed Apr 27, 2018

View reviewed changes

markfisher reviewed Apr 30, 2018

View reviewed changes

evankanderson assigned evankanderson, markfisher and vaikas and unassigned evankanderson May 3, 2018

markfisher reviewed May 4, 2018

View reviewed changes

markfisher reviewed May 5, 2018

View reviewed changes

inlined closed this Jul 9, 2018

ReToCode pushed a commit to ReToCode/eventing that referenced this pull request Dec 21, 2022

Merge pull request knative#50 from matzew/trigger_image_build_eventin…

f2af46b

…g_1.7 CI was here...:fire:


		If you were already running the elafros controllers, you will need to kill the ela-controller in the ela-system namespace for it to pick up the new domain suffix.

		4. Since we will be mimicing an internal service, we need access to the event-delivery service.

Conversation

inlined commented Apr 25, 2018

Proposed Changes

Uh oh!

google-prow-robot commented Apr 25, 2018

Uh oh!

inlined commented Apr 25, 2018

Uh oh!

eobrain commented Apr 25, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markfisher commented Apr 30, 2018

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!

adrcunha commented Jun 7, 2018

Uh oh!

google-prow-robot commented Jun 7, 2018

Uh oh!

adrcunha commented Jun 7, 2018

Uh oh!

inlined commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants