draft for HTTP Target PR #14448

jinhoonbang · 2024-09-16T19:29:36Z

implements HTTP Target capability that sends HTTP request in a round robin manner with retries and timeouts
implements gateway handler that receives request from node and forwards the outgoing request to users
implements node handler that receives response back from gateway handler

TODO:

unit tests
rate limiting and allowlist checks

…ds HTTP request in a round robin manner with retries and timeouts. Implements handlers for gateways and the capability.

github-actions · 2024-09-16T19:31:57Z

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#added For any new functionality added.
#breaking_change For any functionality that requires manual action for the node to boot.
#bugfix For bug fixes.
#changed For any change to the existing functionality.
#db_update For any feature that introduces updates to database schema.
#deprecation_notice For any upcoming deprecation functionality.
#internal For changesets that need to be excluded from the final changelog.
#nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#removed For any functionality/config that is removed.
#updated For any functionality that is updated.
#wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

🎖️ No JIRA issue number found in: PR title, commit message, or branch name. Please include the issue ID in one of these.

cl-sonarqube-production · 2024-09-16T19:46:48Z

Quality Gate passed

Issues
12 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
1.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

DavidOrchard · 2024-09-16T22:47:37Z

Likely a question from ignorance, but why do you have 3 separate components, Launcher, Handler, Capability, all with start/stop etc overhead?

bolekk · 2024-09-16T23:56:27Z

core/services/gateway/handler_factory.go

 )

 const (
 	FunctionsHandlerType HandlerType = "functions"
 	DummyHandlerType     HandlerType = "dummy"
+	WorkflowHandlerType  HandlerType = "workflow"


I know I used "workflow" as an example in my skeleton PR but it's not a great name. The name proposed in the design doc is "web-capabilities" - let's use that please (same for the const name).

bolekk · 2024-09-16T23:59:38Z

core/services/gateway/delegate.go

@@ -54,7 +56,12 @@ func (d *Delegate) ServicesForSpec(ctx context.Context, spec job.Job) (services
 	if err2 != nil {
 		return nil, errors.Wrap(err2, "unmarshal gateway config")
 	}
-	handlerFactory := NewHandlerFactory(d.legacyChains, d.ds, d.lggr)
+	// TODO: pass in TLS config and default HTTP timeout
+	httpClient, err := network.NewHttpClient(nil, time.Duration(10)*time.Millisecond, d.lggr)


I'm not convinced that the HTTP Client should be created in the delegate and passed via HandlerFactory. Is there a good reason to do so?

It feels like it should be a library that capabilities could include whenever needed. That way, any config for HTTP client can become part of a handler config (and also differ across different handlers, if needed). OTOH a possible issue I see is some Proxy config that could be global to the node, rather than to handler. But maybe it's not a big deal. WDYT?

Some networking configs such as max response size and write timeout seem like global node settings. One other potential advantage is it can have its own state and allow additional logic in the future (e.g. keep track of endpoint health to determine wait time before retrying)

bolekk · 2024-09-17T00:01:34Z

core/services/gateway/network/httpclient.go

@@ -0,0 +1,105 @@
+package network


I'd put it in gateway/handlers/common and create inside handlers that need it.

bolekk · 2024-09-17T00:04:01Z

core/services/gateway/network/httpclient.go

+type HttpResponse struct {
+	StatusCode int               // HTTP status code
+	Headers    map[string]string // HTTP headers
+	Body       []byte            // Base64-encoded binary body


It's not rally base64-encoded here yet..? since it's a byte array not a string.

oops copy-pasta. fixed.

bolekk · 2024-09-17T00:07:21Z

core/services/gateway/network/httpclient.go

+
+func NewHttpClient(tlsConfig *tls.Config, defaultTimeout time.Duration, lggr logger.Logger) (HttpClient, error) {
+	transport := http.DefaultTransport
+	if tlsConfig != nil {


Likely won't be needed for an HTTPClient. This is useful for client-side certs, which we won't support. I think you can ignore this param for simplicity and just leave a comment saying it could be extended in the future.

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

bolekk · 2024-09-17T00:45:11Z

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

+		Timeout:    time.Duration(targetPayload.TimeoutMs) * time.Millisecond,
+		RetryCount: targetPayload.RetryCount,
+	}
+	resp, err := d.httpClient.Send(ctx, req)


Handlers have to be non-blocking so we can't do a Send() here directly. You should spawn a new goroutine (or have a pool of them and a queue). Handler should return success once the message is parsed and sent to HTTP client. If a real error appears later, it also needs to be sent back to the node.

bolekk · 2024-09-17T00:47:17Z

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

+	RetryCount uint8             `json:"retryCount,omitempty"` // Number of retries, defaults to 0.
+}
+
+type TargetResponsePayload struct {


This should also have a "higher-level" error code, if a Gateway is not even able to connect or parse or process the request. Check out ResponseBase in the Functions handler.

bolekk · 2024-09-17T00:48:26Z

core/capabilities/webapi/target/capability.go

+	if err := validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowExecutionID); err != nil {
+		return "", fmt.Errorf("workflow execution ID is invalid: %w", err)
+	}
+	messageID := []string{


cedric-cordenier · 2024-09-17T08:37:32Z

core/capabilities/webapi/target/capability.go

@@ -0,0 +1,172 @@
+package target


Does it make sense to put this in webapi instead and have the constructor be NewTarget? I think this will stutter less and also potentially lead to less conflicts: the unmodified name of the capability atm is target.NewCapability which doesn't say much

I'm open to doing that. I think that would make the code more clear overall. We should enforce the same convention in other capabilities though to be consistent. (i.e. avoid NewCapability and use New${CapabilityName}Capability)

cedric-cordenier · 2024-09-17T08:38:44Z

core/capabilities/webapi/target/capability.go

+	responseChs map[string]chan *api.Message, responseChsMu *sync.Mutex) (*Capability, error) {
+	return &Capability{
+		responseChs:   make(map[string]chan *api.Message),
+		responseChsMu: responseChsMu,


Note: I realise this is a draft but just fyi the registry is missing from here

cedric-cordenier · 2024-09-17T08:39:57Z

core/capabilities/webapi/target/capability.go

+func (c *Capability) Execute(ctx context.Context, req capabilities.CapabilityRequest) (capabilities.CapabilityResponse, error) {
+	c.lggr.Debugw("executing http target", "capabilityRequest", req)
+
+	var input WorkflowInput


Calling this WorkflowInput is slightly misleading IMO: the capability defines the input, not the workflow

makes sense. this can be webapi.TargetInput if we start enforcing the convention above.

cedric-cordenier · 2024-09-17T08:52:31Z

core/capabilities/webapi/target/types.go

+	URL     string            `json:"url"`               // URL to query, only http and https protocols are supported.
+	Method  string            `json:"method,omitempty"`  // HTTP verb, defaults to GET.
+	Headers map[string]string `json:"headers,omitempty"` // HTTP headers, defaults to empty.
+	Body    string            `json:"body,omitempty"`    // Base64-encoded binary body, defaults to empty.


Note: can we just use bytes here? Why are we expecting the user to encode the body ahead of time; that seems to leak implementation details.

I thought mapstructure used inside Map.UnwrapTo can't deserialize to []byte here. Will test it out.

DavidOrchard · 2024-09-17T19:15:09Z

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

+		Headers:    targetPayload.Headers,
+		Body:       targetPayload.Body,
+		Timeout:    time.Duration(targetPayload.TimeoutMs) * time.Millisecond,
+		RetryCount: targetPayload.RetryCount,


I'm not sure how this is generated, but this should be different than the retrycount from gateway. And it doesnt make sense to retry on many 4xx codes like 401, 403, 404

This will be the customer-provided retries. I agree that there are 2 level of retries needed:

user-defined retries - this is defined in the workflows

node-level retries - retries between gateway and node.

decided to leave a TODO for retry logic but yes, fully agreed we shouldn't retry on all errors

DavidOrchard · 2024-09-18T16:02:40Z

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

+	return nil
+}
+
+func (d *workflowHandler) HandleNodeMessage(ctx context.Context, msg *api.Message, nodeAddr string) error {


pardon my ignorance, but if the HandleNodeMessage fails via handleWebAPITargetMessage failure, how to we tell the difference between a not last retriable message and the final retried message?

draft for HTTP Trigger PR. Implements HTTP Target Capability that sen…

716f8d5

…ds HTTP request in a round robin manner with retries and timeouts. Implements handlers for gateways and the capability.

jinhoonbang added the do not merge label Sep 16, 2024

jinhoonbang requested review from DavidOrchard and bolekk September 16, 2024 19:29

jinhoonbang changed the title ~~draft for HTTP Trigger PR~~ draft for HTTP Target PR Sep 16, 2024

bolekk requested changes Sep 17, 2024

View reviewed changes

cedric-cordenier reviewed Sep 17, 2024

View reviewed changes

DavidOrchard reviewed Sep 17, 2024

View reviewed changes

jinhoonbang mentioned this pull request Sep 18, 2024

web api trigger connector handler #14459

Draft

DavidOrchard reviewed Sep 18, 2024

View reviewed changes

jinhoonbang mentioned this pull request Sep 25, 2024

add gateway handler and http client for outgoing messages #14536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

draft for HTTP Target PR #14448

draft for HTTP Target PR #14448

jinhoonbang commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024

cl-sonarqube-production bot commented Sep 16, 2024

DavidOrchard commented Sep 16, 2024

bolekk Sep 16, 2024

bolekk Sep 16, 2024 •

edited

Loading

jinhoonbang Sep 17, 2024

bolekk Sep 18, 2024

bolekk Sep 17, 2024

bolekk Sep 17, 2024 •

edited

Loading

jinhoonbang Sep 17, 2024

bolekk Sep 17, 2024

bolekk Sep 17, 2024

bolekk Sep 17, 2024

bolekk Sep 17, 2024

cedric-cordenier Sep 17, 2024

jinhoonbang Sep 17, 2024 •

edited

Loading

cedric-cordenier Sep 17, 2024

cedric-cordenier Sep 17, 2024

jinhoonbang Sep 17, 2024

cedric-cordenier Sep 17, 2024

jinhoonbang Sep 17, 2024

DavidOrchard Sep 17, 2024

jinhoonbang Sep 17, 2024

DavidOrchard Sep 18, 2024

draft for HTTP Target PR #14448

Are you sure you want to change the base?

draft for HTTP Target PR #14448

Conversation

jinhoonbang commented Sep 16, 2024 • edited Loading

github-actions bot commented Sep 16, 2024

cl-sonarqube-production bot commented Sep 16, 2024

Quality Gate passed

DavidOrchard commented Sep 16, 2024

Choose a reason for hiding this comment

bolekk Sep 16, 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

bolekk 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

jinhoonbang 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

jinhoonbang commented Sep 16, 2024 •

edited

Loading

bolekk Sep 16, 2024 •

edited

Loading

bolekk Sep 17, 2024 •

edited

Loading

jinhoonbang Sep 17, 2024 •

edited

Loading