adding webhook sensor #15

VaibhavPage · 2018-06-06T22:14:01Z

No description provided.

magaldima

finished review. let's go over these changes

magaldima · 2018-06-07T21:19:58Z

controller/sensorjob.go

+	// If signal is of type Webhook, create a service backed by the job
+	for _, signal := range soc.s.Spec.Signals {
+		if signal.GetType() == v1alpha1.SignalTypeWebhook {
+			var targetPort int


i think the most idiomatic go would be to initialize the targetPort as follows:

targetPort := signal.Webhook.Port if targetPort == 0 { targetPort = common.DefaultWebhookServiceTargetPort }

magaldima · 2018-06-07T21:23:20Z

controller/sensorjob.go

+					Name: common.CreateServiceSuffix(soc.s.Name),
+					Namespace: soc.s.Namespace,
+					Labels: map[string]string{
+						"sensor": soc.s.Name,


are these labels used anywhere else? according to the k8s docs:
"Labels are intended to be used to specify identifying attributes of objects that are meaningful and relevant to users"

Its for the user who wants to list the services for webhook sensors or list a particular service for job by using kubectl get with labels

okay, since its 1-1-1 for webhook sensors to jobs and services.. it doesn't really tell you anything more useful than what you expect.

magaldima · 2018-06-07T21:27:10Z

controller/sensorjob.go

+					Labels: map[string]string{
+						"sensor": soc.s.Name,
+						"job": createdJob.Name,
+					},


seems like we are creating a service for every sensor... so there will be a one-to-one mapping here. If that's the case, we should attach an ownerReference to the ObjectMeta to this particular sensor. However, let's discuss the feasability of my above comment of creating a separate service for every sensor...

magaldima · 2018-06-07T21:29:53Z

controller/sensorjob.go

-	soc.log.Infof("created signal executor job '%s'", created.Name)
+	soc.log.Infof("created signal executor job '%s'", createdJob.Name)
+
+	// If signal is of type Webhook, create a service backed by the job


should we really be creating a difference service for every sensor? i understand we would like to be flexible with ports and all that, but it seems a large tradeoff to spin up a service for every sensor instead of forcing all webhook sensors on a particular port. Also a service allows u to expose an internal port to any external one. It seems very unlikely that we would be conflicting on an internal port in the executor...

additionally, we could still have all the flexibility we want on specifying any route endpoint

Let's think about a better way to implement the service

magaldima · 2018-06-07T21:33:15Z

job/event.go

@@ -77,3 +77,7 @@ func (ae *AbstractEvent) SetError(err error) {
 func (ae *AbstractEvent) GetError() error {
 	return ae.err
 }
+
+func (ae *AbstractEvent) GetTimestamp() time.Time {


this was purposely not implemented because it forces all the non abstract events to implement this method. let's remove this.

magaldima · 2018-06-07T21:33:44Z

job/webhook/event.go

+
+func (e *event) GetSignal() job.Signal {
+	return e.webhook
+}


let's add the GetTimestamp() implementation here...

magaldima · 2018-06-07T21:37:00Z

job/webhook/signal.go

+// Hanlder for the http rest endpoint
+func (w *webhook) handler(writer http.ResponseWriter, request *http.Request) {
+	w.Log.Info("Received a request from", zap.String("host", request.Host))
+	if request.Method == common.WebhookHTTPMethod {


i think it's OKAY that we only allow POST methods right now, but ideally this should be able to be specified in the specification. It's not that much more to add, very simple, and standard.

magaldima · 2018-06-07T21:44:18Z

job/webhook/signal.go

+		}
+		w.events <- event
+		// Do we want to keep the server running?
+		go func() {


this is a very hacky way to shutdown the webserver.. and it also only works if this method has been fired. we probably need to run a separate goroutine outside of this handler to monitor a channel for a stop signal. please look at other signal implementations for some ideas

magaldima · 2018-06-07T21:46:13Z

job/webhook/signal_test.go

+
+	wg.Add(1)
+
+	go func() {


why do we need to run this in a separate goroutine? we can probably just run this in the main thread.

not actually.

magaldima · 2018-06-07T21:47:37Z

pkg/apis/sensor/v1alpha1/types.go

@@ -271,6 +275,14 @@ type Kafka struct {
 	Partition int32 `json:"partition" protobuf:"bytes,3,opt,name=partition"`
 }

+// WebhookSignal is a general purpose REST API


this description is a little misleading. its not a general purpose REST API since it only supports POST methods.. if we add the method to the spec, then it would be more true..

makes sense.

magaldima

please address the following comments

magaldima · 2018-06-08T12:57:40Z

controller/sensorjob.go

@@ -146,6 +144,14 @@ func (soc *sOperationCtx) createSensorExecutorJob() error {
 				return err
 			}
 			soc.log.Infof("Created executor service %s", createdSvc.Name)
+			// Assign service as owner of sensor
+			soc.s.OwnerReferences = []metav1.OwnerReference {


this owner reference should be for the sensor. in this way, when we delete the sensor, it will remove this service. look at the job implementation for the code and update the comment above

magaldima · 2018-06-08T13:02:30Z

job/webhook/event.go

@@ -40,3 +39,7 @@ func (e *event) GetSource() string {
 func (e *event) GetSignal() job.Signal {
 	return e.webhook
 }
+
+func (e *event) GetTimestamp() time.Time {
+	return time.Now().UTC()


this will return a different timestamp every time this method is called. we need to snapshot the time at which the handler receives the request. i would put the timestamp field back into the event and just use ...timestamp: time.Now().UTC()... in the handler's init of event

magaldima · 2018-06-08T13:06:44Z

job/webhook/signal.go

@@ -68,15 +60,14 @@ func (w *webhook) Start(events chan job.Event) error {
 	// Start http server
 	go func() {
 		if err := w.server.ListenAndServe(); err != nil {
-			panic(err)
+			w.Log.Warn("HTTP server stopped. Cause %v", zap.Error(err))


question: does this get logged when we call w.server.Shutdown()? according to the docs, the method ListenAndServe always returns a non nil error, but we don't want to log an warning if we actually want to stop the signal. Maybe we can use a boolean flag to indicate if we're actually intending to stop the server.. if the stop flag is not true, we would want to return the error here.

actually we can check if err was http.ErrServerClosed which will occur when we call ShutDown and then print the shutdown message, else for all other errors invoke panic

magaldima · 2018-06-08T13:16:51Z

job/webhook/signal_test.go

-			assert.NotNil(t, resp, nil)
-			wg.Done()
-			break
+	for {


couple things here.

we don't need the for loop.

remove the err != nil check. in fact, this test is not properly testing the signal with this. we should replace this an `assert.Nil(t, err)

you never actually check for the event on the testEventChan, let's add that check after the POST.

you assert the response is not nil which is true, but the response is empty.. you never write anything with the writer in the handler method. should we be responding to these methods? maybe a simple boolean ACK or OK?

let's add more tests for all the different REST methods and perform the same strict assertions as i mentioned above for the POST

we do need the for loop as the server is started in separate go routine . So you never know when it will start accepting requests.

maybe a better way is to check for response status.

okay i ran this test myself without the loop and it worked. we just need to check when the server is ready. a loop is not the right answer here.

we should probably just do a defer writer.WriteHeader(http.StatusOK)

it worked because the server did start before you made request. a simpler way would be to wait for a few seconds before making request as there is no other way server let you know when it started. Other way is to check if port is bound and accepts connections using "net"
and then make the request

magaldima · 2018-06-08T13:18:23Z

pkg/apis/sensor/v1alpha1/types.go

@@ -281,6 +281,8 @@ type WebhookSignal struct {
 	Endpoint string `json:"endpoint" protobuf:"bytes,1,opt,name=endpoint"`
 	// Port to listen on
 	Port int `json:"port" protobuf:"bytes,2,opt,name=port"`
+	// Method is HTTP method


we can make this documentation much better. refer to this page: http://www.restapitutorial.com/lessons/httpmethods.html

But I don't think people need a primer on what HTTP methods are.
I guess a good description would be something like, Method is HTTP request method that indicates the desired action to be performed for a given resource

i wouldn't assume anything about our users. we don't need to copy the description, but maybe just reference a link to the REST API standards.

* adding webhook as new type of signal

VaibhavPage requested a review from magaldima June 6, 2018 22:14

VaibhavPage force-pushed the webhook-sensor branch 3 times, most recently from 59a80f7 to 457f1bd Compare June 6, 2018 22:37

magaldima suggested changes Jun 7, 2018

View reviewed changes

magaldima suggested changes Jun 8, 2018

View reviewed changes

adding webhook as new type of signal

98a0e23

VaibhavPage force-pushed the webhook-sensor branch from 51983b4 to 98a0e23 Compare June 8, 2018 18:33

update webhook event

1f22719

magaldima approved these changes Jun 8, 2018

View reviewed changes

magaldima merged commit 47c64a4 into master Jun 8, 2018

magaldima deleted the webhook-sensor branch June 8, 2018 19:36

magaldima mentioned this pull request Jun 11, 2018

Add webhook sensor #11

Closed

juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022

adding webhook sensor (argoproj#15)

97d6104

* adding webhook as new type of signal

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding webhook sensor #15

adding webhook sensor #15

VaibhavPage commented Jun 6, 2018

magaldima left a comment

magaldima Jun 7, 2018

VaibhavPage Jun 7, 2018

magaldima Jun 7, 2018

VaibhavPage Jun 7, 2018

magaldima Jun 7, 2018

magaldima Jun 7, 2018

magaldima Jun 7, 2018

VaibhavPage Jun 7, 2018

magaldima Jun 7, 2018

VaibhavPage Jun 7, 2018

magaldima Jun 7, 2018

magaldima Jun 7, 2018

magaldima Jun 7, 2018

magaldima Jun 7, 2018

VaibhavPage Jun 7, 2018

magaldima Jun 7, 2018

VaibhavPage Jun 7, 2018

magaldima left a comment

magaldima Jun 8, 2018

VaibhavPage Jun 8, 2018

magaldima Jun 8, 2018

magaldima Jun 8, 2018

VaibhavPage Jun 8, 2018

magaldima Jun 8, 2018

VaibhavPage Jun 8, 2018

VaibhavPage Jun 8, 2018

magaldima Jun 8, 2018

magaldima Jun 8, 2018

VaibhavPage Jun 8, 2018

magaldima Jun 8, 2018

VaibhavPage Jun 8, 2018

magaldima Jun 8, 2018

adding webhook sensor #15

adding webhook sensor #15

Conversation

VaibhavPage commented Jun 6, 2018

magaldima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magaldima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment