web api trigger connector handler #14459

DavidOrchard · 2024-09-17T15:03:16Z

web api trigger connector handler cappl-22

For high level review not final code yet.
To review

create an implementation of the capability API and add it to the Registry
create a handler and register it with Gateway Connector
manage trigger subscriptions

TODO

decode config
process incoming trigger events and related metadata
responses, including signing

justinkaseman · 2024-09-17T19:38:26Z

core/services/gateway/handlers/workflow/handler.go

+	// NOTE: more methods will go here: CRUD for workflow specs; HTTP trigger/action/target; etc.
+)
+
+type workflowHandler struct {


Would be nice to have a little description here of what a workflow handler is

justinkaseman · 2024-09-17T19:39:23Z

core/services/gateway/handlers/workflow/handler.go

+var _ handlers.Handler = (*workflowHandler)(nil)
+
+func NewWorkflowHandler(donConfig *config.DONConfig, don handlers.DON, lggr logger.Logger) (*workflowHandler, error) {
+	lggr.Debugw("-------HNewWorkflowHandler")


Stray debug log

We can't have debug logs until final review?

You can, I'm just calling it out in case it had gotten forgotten. Seemed like you removed some others.

justinkaseman · 2024-09-17T19:39:32Z

core/services/gateway/handlers/workflow/handler.go

+}
+
+func (d *workflowHandler) HandleUserMessage(ctx context.Context, msg *api.Message, callbackCh chan<- handlers.UserCallbackPayload) error {
+	d.lggr.Debugw("-------HandleUserMessage", "method", msg.Body.Method)


Stray debug log

justinkaseman · 2024-09-17T19:47:04Z

core/capabilities/webapi/trigger.go

+		return nil
+	})
+}
+func (h *workflowConnectorHandler) Close() error {


Since you are using services.StateMachine StartOnce, you should use StopOnce here

justinkaseman · 2024-09-17T19:47:12Z

core/capabilities/webapi/trigger.go

+	return nil
+}
+
+func (h *workflowConnectorHandler) Ready() error {


Let services.StateMachine handle this

justinkaseman · 2024-09-17T19:47:41Z

core/capabilities/webapi/trigger.go

+}
+
+func (h *workflowConnectorHandler) HealthReport() map[string]error {
+	return map[string]error{h.Name(): nil}


Suggested change

return map[string]error{h.Name(): nil}

return map[string]error{h.Name(): h.Healthy()}

justinkaseman · 2024-09-17T19:48:17Z

core/capabilities/webapi/trigger.go

+}
+
+func (h *workflowConnectorHandler) sendResponse(ctx context.Context, gatewayID string, requestBody *api.MessageBody, payload any) error {
+	payloadJson, err := json.Marshal(payload)


justinkaseman · 2024-09-17T19:52:07Z

core/capabilities/webapi/trigger.go

+		lggr:      lggr.Named("WorkflowConnectorHandler"),
+	}
+
+	// is this the right way to register with gateway connector?  Cron trigger doesn't do this.


Cron trigger doesn't have connectors/handlers.

What I'd consider here is should this be in .Start instead? Trace through where constructor gets called versus where .Start gets called - is the connector ready for handlers at that point?

I agree, better to add in Start().

justinkaseman · 2024-09-17T19:53:54Z

core/capabilities/webapi/trigger.go

+}
+
+func (h *workflowConnectorHandler) Start(ctx context.Context) error {
+	// how does the


Stray comment

justinkaseman · 2024-09-17T19:57:44Z

core/scripts/gateway/workflow/upload_workflow.go

+		fmt.Println("error parsing private key", err)
+		return
+	}
+	//address := crypto.PubkeyToAddress(key.PublicKey)


Stray commented out code

This was a copy from the template so I left as is.

justinkaseman · 2024-09-17T19:58:06Z

core/services/gateway/handlers/workflow/handler.go

+
+const (
+	MethodAddWorkflow = "add_workflow"
+	// NOTE: more methods will go here: CRUD for workflow specs; HTTP trigger/action/target; etc.


More of a TODO than a NOTE probably

This TODO/NOTE is not correct anyway (I left that comment on Jin's PR too). We will not reuse this handler for workflow spec CRUDs.

justinkaseman · 2024-09-17T20:01:23Z

core/capabilities/webapi/trigger.go

+	ErrorMessage string `json:"error_message,omitempty"`
+}
+
+type workflowConnectorHandler struct {


A short description of what a connector handler is would be nice to have here

Maybe call it ConnectorHandler or TriggerConnectorHandler. Not sure if workflow adds useful meaning here. Similar changes were recommended in target PR: #14448

Yes, let's not use "workflow" in web capability names. I used it in my example of a handler for spec uploads, which is not relevant here.

justinkaseman · 2024-09-17T20:03:15Z

core/capabilities/webapi/trigger.go

+		},
+	}
+
+	// How do we get the signerKey from the connector?


It's not from the connector, it comes from the node's keystore.

https://github.com/smartcontractkit/chainlink/pull/14171/files#diff-c13fc3163a5a160db5c5fd6b9b6f99c2696915f6b07f0ebca64ff45429e4ff2fR44

jinhoonbang

Still going through it. Left some comments for the trigger.go

jinhoonbang · 2024-09-17T23:30:30Z

core/capabilities/webapi/trigger.go

+	ErrorMessage string `json:"error_message,omitempty"`
+}
+
+type workflowConnectorHandler struct {


Maybe call it ConnectorHandler or TriggerConnectorHandler. Not sure if workflow adds useful meaning here. Similar changes were recommended in target PR: #14448

jinhoonbang · 2024-09-17T23:34:18Z

core/capabilities/webapi/trigger.go

+	}
+
+	// is this the right way to register with gateway connector?  Cron trigger doesn't do this.
+	err := connector.AddHandler([]string{"add_workflow"}, handler)


I believe "web_api_trigger" is the suggested method name in the design doc. "add_workflow` was just an example for an unrelated feature in the skeleton

jinhoonbang · 2024-09-17T23:35:38Z

core/capabilities/webapi/trigger.go

+	body := &msg.Body
+	fromAddr := ethCommon.HexToAddress(body.Sender)
+	// TODO: apply allowlist and rate-limiting
+	h.lggr.Debugw("handling gateway request", "id", gatewayID, "method", body.Method, "address", fromAddr)


Suggested change

h.lggr.Debugw("handling gateway request", "id", gatewayID, "method", body.Method, "address", fromAddr)

h.lggr.Debugw("handling gateway request", "id", gatewayID, "method", body.Method, "sender", fromAddr)

jinhoonbang · 2024-09-17T23:42:41Z

core/capabilities/webapi/trigger.go

+	fromAddr := ethCommon.HexToAddress(body.Sender)
+	// TODO: apply allowlist and rate-limiting
+	h.lggr.Debugw("handling gateway request", "id", gatewayID, "method", body.Method, "address", fromAddr)
+	// ERR: payload is error, how to unmarshall?


You can define a struct

type TriggerRequestPayload struct { // add fields here }

then, pass a pointer to that struct as argument

var payload TriggerRequestPayload err := json.Unmarshal(body.Payload, &payload) if err != nil { return err }

jinhoonbang · 2024-09-17T23:44:22Z

core/capabilities/webapi/trigger.go

+	triggerID := payload.trigger_id
+
+	switch body.Method {
+	case workflow.MethodAddWorkflow:


same as above.

jinhoonbang · 2024-09-18T00:07:37Z

core/capabilities/webapi/trigger.go

+		// we need access to Job ORM or whatever CLO uses to fully launch a new spec
+		h.lggr.Debugw("added workflow spec", "payload", string(body.Payload))
+
+		// Question: should this call the registerTrigger and then handleNodeResponse call unregister?


IIUC registerTrigger and unregisterTrigger should be called during Start() or Close(). I think HandleGatewayMessage() needs to emit an event to workflow engine

Yes exactly - here you only to send a TriggerEvent on appropriate channel(s) (potentially many), like you do below.

From meeting with Bolek & JK, registerTrigger and unregisterTrigger is called by engine, not by any code that I am writing.

bolekk · 2024-09-18T00:47:06Z

core/capabilities/webapi/trigger.go

+	ErrorMessage string `json:"error_message,omitempty"`
+}
+
+type workflowConnectorHandler struct {


Yes, let's not use "workflow" in web capability names. I used it in my example of a handler for spec uploads, which is not relevant here.

bolekk · 2024-09-18T00:48:15Z

core/capabilities/webapi/trigger.go

+		lggr:      lggr.Named("WorkflowConnectorHandler"),
+	}
+
+	// is this the right way to register with gateway connector?  Cron trigger doesn't do this.


I agree, better to add in Start().

bolekk · 2024-09-18T00:50:19Z

core/capabilities/webapi/trigger.go

+
+	switch body.Method {
+	case workflow.MethodAddWorkflow:
+		// TODO: add a new workflow spec and return success/failure


These are remnants of my example PR - please remove.

bolekk · 2024-09-18T00:52:04Z

core/capabilities/webapi/trigger.go

+func (h *workflowConnectorHandler) UnregisterTrigger(ctx context.Context, req capabilities.TriggerRegistrationRequest) error {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	trigger := h.triggers[req.TriggerID]


Also check if it exists here.

bolekk · 2024-09-18T00:53:37Z

core/capabilities/webapi/trigger.go

+		},
+	}
+
+	if err = msg.Sign(h.signerKey); err != nil {


Jin is going to modify Connector to sign automatically. You can remove this (and also don't pass the private key at all).

my changes, as of now, only introduces a new method that sends to first available gateway. In this case, the caller already knows the sender. I will modify my changes to account for this use case.

bolekk · 2024-09-18T01:10:55Z

core/services/gateway/handler_factory.go

@@ -33,6 +35,8 @@ func (hf *handlerFactory) NewHandler(handlerType HandlerType, handlerConfig json
 	switch handlerType {
 	case FunctionsHandlerType:
 		return functions.NewFunctionsHandlerFromConfig(handlerConfig, donConfig, don, hf.legacyChains, hf.ds, hf.lggr)
+	case workflowHandlerType:


@jinhoonbang is already adding it in his PR, maybe wait for that to be merged and then rebase.

bolekk · 2024-09-18T01:12:10Z

core/scripts/gateway/workflow/upload_workflow.go

@@ -0,0 +1,106 @@
+package main


We don't need a script to upload workflow specs as this is not something we're implementing right now. If you want to have a test script then modify it to send valid Web Trigger messages. Otherwise you can remove it from this PR.

Agreed, again my thinking was that if the trigger needed updating then that would affect the script.

bolekk · 2024-09-18T01:12:38Z

core/services/gateway/handlers/workflow/handler.go

+
+const (
+	MethodAddWorkflow = "add_workflow"
+	// NOTE: more methods will go here: CRUD for workflow specs; HTTP trigger/action/target; etc.


This TODO/NOTE is not correct anyway (I left that comment on Jin's PR too). We will not reuse this handler for workflow spec CRUDs.

bolekk · 2024-09-18T01:14:18Z

core/services/gateway/handlers/workflow/handler.go

@@ -0,0 +1,91 @@
+package workflow


@jinhoonbang is already adding this handler in his PR. Best to wait on that and then only extend with the Trigger method name (can be in a separate PR since it's technically part of CAPPL-21).

I'll proceed with this in mine so that I'm not blocked if that's ok.

bolekk · 2024-09-18T01:14:32Z

core/services/standardcapabilities/delegate.go

@@ -80,7 +80,8 @@ func (d *Delegate) ServicesForSpec(ctx context.Context, spec job.Job) ([]job.Ser
 			return nil, errors.New("gateway connector is required for web API Trigger capability")
 		}
 		connector := d.gatewayConnectorWrapper.GetGatewayConnector()
-		triggerSrvc, err := webapi.NewTrigger(spec.StandardCapabilitiesSpec.Config, d.registry, connector, log)
+		signer := d.gatewayConnectorWrapper.GetSignerKey()
+		triggerSrvc, err := webapi.NewTrigger(spec.StandardCapabilitiesSpec.Config, d.registry, connector, signer, log)


signer key not needed

cl-sonarqube-production · 2024-09-20T18:57:50Z

Quality Gate failed

Failed conditions
6 New Major Issues (required ≤ 5)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

jinhoonbang · 2024-09-20T19:46:06Z

core/capabilities/webapi/trigger.go

+type Response struct {
+	Success      bool   `json:"success"`
+	ErrorMessage string `json:"error_message,omitempty"`
+	Status       string `json:"ACCEPTED"`


do we need both Success and Status? do they both convey request status to gateway?

jinhoonbang · 2024-09-20T20:03:10Z

core/capabilities/webapi/trigger.go

+
+		// TODO: ACCEPTED, PENDING, COMPLETED
+		response := Response{Success: true, Status: "ACCEPTED"}
+		h.sendResponse(ctx, gatewayID, body, response)


do we need to make this non-blocking? @bolekk had similar comment here: #14448 (comment)

jinhoonbang · 2024-09-20T20:21:36Z

core/capabilities/webapi/trigger.go

+	if err != nil {
+		return nil, err
+	}
+	allowedSendersMap := map[string]bool{}


I believe senders should be defined at workflow level. Should we create a map from triggerID to list of senders and populate that map using TriggerRegistrationRequest.Config in RegisterTrigger()?

jinhoonbang · 2024-09-20T20:24:20Z

core/capabilities/webapi/trigger.go

 	// TODO (CAPPL-22, CAPPL-24):
 	//   - decode config
 	//   - create an implementation of the capability API and add it to the Registry
 	//   - create a handler and register it with Gateway Connector
 	//   - manage trigger subscriptions
 	//   - process incoming trigger events and related metadata
-	return nil, nil
+
+	rateLimiter, err := common.NewRateLimiter(config.RateLimiter)


similar to below. rateLimit is also a workflow config https://docs.google.com/document/d/1mCTAo-ix-P923eUlh4SloZfBN9PCvgf90oHWbmykjsc/edit#heading=h.qqkbzbl5lbmv

jinhoonbang · 2024-09-20T20:55:02Z

core/capabilities/webapi/trigger.go

+
+type TriggerConfig struct {
+	AllowedSenders []ethCommon.Address      `toml:"allowedSenders"`
+	Allowedtopics  []string                 `toml:"allowedTopics"`


should this become a map also? map from triggerID to topics

jinhoonbang · 2024-09-20T20:58:25Z

core/capabilities/webapi/trigger.go

+
+		// is this right?
+		// Sri did wrappedPayload, err := values.WrapMap(log.Data), does that work in this case?
+		wrappedPayload, _ := values.WrapMap(payload)


IIUC, we would need to populate payloads with topics and params

DavidOrchard force-pushed the feature/CAPPL-22-web-api-trigger-connector-handler branch 3 times, most recently from 7c7a231 to f4c79c3 Compare September 17, 2024 15:36

web api trigger connector handler

9a0ab6a

DavidOrchard force-pushed the feature/CAPPL-22-web-api-trigger-connector-handler branch from f4c79c3 to 9a0ab6a Compare September 17, 2024 15:50

justinkaseman reviewed Sep 17, 2024

View reviewed changes

DavidOrchard added 2 commits September 17, 2024 13:54

figure out signer and progress on handling message

28b5a25

Some PR comments and add payload samples

de300e0

jinhoonbang previously approved these changes Sep 18, 2024

View reviewed changes

jinhoonbang self-requested a review September 18, 2024 00:12

bolekk requested changes Sep 18, 2024

View reviewed changes

DavidOrchard dismissed jinhoonbang’s stale review via 9ea2b54 September 18, 2024 16:30

DavidOrchard force-pushed the feature/CAPPL-22-web-api-trigger-connector-handler branch 4 times, most recently from 62d4dbc to fdc4510 Compare September 18, 2024 23:49

some PR comments, updated register unregister trigger

f6d4543

DavidOrchard force-pushed the feature/CAPPL-22-web-api-trigger-connector-handler branch from fdc4510 to f6d4543 Compare September 19, 2024 00:25

DavidOrchard added 4 commits September 18, 2024 19:53

updates based on design docs updates today sep 18

c9f8c38

Small fixes including response, payload conversion to target event

8218273

add config parsing and rateLimit checking

3573fd8

add unit tests

2231012

DavidOrchard force-pushed the feature/CAPPL-22-web-api-trigger-connector-handler branch from 7464429 to 2231012 Compare September 20, 2024 18:36

jinhoonbang reviewed Sep 20, 2024

View reviewed changes

	return map[string]error{h.Name(): nil}
	return map[string]error{h.Name(): h.Healthy()}

	h.lggr.Debugw("handling gateway request", "id", gatewayID, "method", body.Method, "address", fromAddr)
	h.lggr.Debugw("handling gateway request", "id", gatewayID, "method", body.Method, "sender", fromAddr)

web api trigger connector handler #14459

Are you sure you want to change the base?

web api trigger connector handler #14459

Conversation

DavidOrchard commented Sep 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

justinkaseman Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

jinhoonbang 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

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

cl-sonarqube-production bot commented Sep 20, 2024

Quality Gate failed

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

DavidOrchard commented Sep 17, 2024 •

edited

Loading

justinkaseman Sep 17, 2024 •

edited

Loading