HTTP API support #4543

cretz · 2023-06-26T16:28:13Z

What changed?

Implements support for HTTP API as proposed at temporalio/proposals#79. Specifically:

Added httpPort ~~and httpDisabled~~ and httpAdditionalForwardedHeaders on config RPC
- Only applicable to frontend service
- ~~Default port is gRPC port + 10, default httpDisabled is false (meaning this is on by default)~~
- Default of port 0 now means HTTP disabled but all docker configs set to default to 7243
- There is a default set of headers as visible from code
- This config should be discussed to see if we're ok with it
Added HTTPAPIServer for frontend which runs HTTP API server using gRPC gateway with our custom modifications which include
- Ability to support ?pretty to pretty print JSON and/or ?noPayloadShorthand to disable shorthand payloads
- Uses direct gRPC server invocation via invocation of gRPC interceptors
- Implements well-defined graceful stop and adds basic common Go HTTP server protection measures
- Custom error handler due to gogo status vs Google gRPC status issues with Any details
- Properly forwards headers and TLS auth info so claim mapper and authorizer works
Added HTTPAPIServerProvider for frontend which creates HTTPAPIServer
Updated frontend Service to support HTTPAPIServer
- Make gRPC server and HTTP server start concurrently
- Make gRPC server and HTTP server shutdown concurrently
Added tests for HTTP API
Added basic mTLS integration tests for gRPC and HTTP API

This is ready for review. This will not pass CI until the other PRs are merged. Related PRs:

Depends on HTTP API annotations api#301
Depends on HTTP API helpers api-go#112

cretz · 2023-06-26T16:51:40Z

common/config/config.go

+		// HTTPPort is the port on which HTTP will listen. If unset/0 and HTTP is
+		// enabled (the default), this will be the gRPC port + 10. This setting only
+		// applies to the frontend service.
+		HTTPPort int `yaml:"httpPort"`
+		// HTTPDisabled can be set to true to disable HTTP API. This setting only
+		// applies to the frontend service.
+		HTTPDisabled bool `yaml:"httpDisabled"`


Alternatives to httpPort and httpDisabled in the frontend service config directly are to have a top-level HTTP configuration section. However this seemed cleanest.

EDIT: Note, httpDisabled is no longer present

service/frontend/http_api_server.go

tests/http_api_test.go

cretz · 2023-06-26T16:58:15Z

tests/http_api_test.go

+		Details []struct {
+			RunID string `json:"runId"`
+		} `json:"details"`


For this style of details to work, I had to implement a custom error handler because the default reusing our marshaler doesn't work with Google "Any" wkt, and we want to use our marshaler.

cretz · 2023-06-26T16:58:53Z

tests/http_api_test.go

+	_, respBody = s.httpPost(
+		http.StatusOK,
+		"/api/v1/namespaces/"+s.namespace+"/workflows/"+workflowID+"/query/some-query",
+		`{ "query": { "queryArgs": [{ "someField": "query-arg" }] } }`,


Notice the inconsistencies in our API come to the forefront here. What is query.queryArgs here is input for signal/workflow.

These are new APIs, though. Why not address this?

You cannot address this reasonably with gRPC gateway. Our HTTP API, as a rule and as a byproduct of our approach, exposes the RPCs we have made, it does not alter them. We chose to do this in our API, gRPC and HTTP users have to deal with it. This is covered a bit better in the proposal: https://github.com/temporalio/proposals/blob/master/api/http-api.md.

If we want to fix our RPCs to be more consistent, HTTP and gRPC will benefit. If not, HTTP and gRPC calls will be inconsistent across calls (but consistent across protocols).

cretz · 2023-06-26T17:00:55Z

tests/http_api_test.go

+		// Our version of gRPC gateway only supports integer enums in queries :-(
+		"/api/v1/namespaces/"+s.namespace+"/workflows/"+workflowID+"/history?historyEventFilterType=2",


Note how the enum in the query param has to be an integer. This is because our continued use of Gogo proto means we have to support an outdated gRPC gateway which doesn't support enum names in query params. I can customize this, but it's basically copying the entirety of https://github.com/grpc-ecosystem/grpc-gateway/blob/v1.16.0/runtime/query.go and making slight alterations for enum-name support. If I did make this change, it'd be in temporalio/api-go#112.

service/frontend/http_api_server.go

cretz · 2023-06-30T15:58:05Z

go.mod

@@ -148,3 +149,5 @@ require (
 	modernc.org/strutil v1.1.3 // indirect
 	modernc.org/token v1.1.0 // indirect
 )
+
+replace go.temporal.io/api => ../temporal-api-go


This is of course temporary pending merge of dependencies

MichaelSnowden · 2023-06-30T17:08:02Z

service/frontend/http_api_server.go

+	if tcpAddrRef == nil {
+		return nil, fmt.Errorf("must use TCP for gRPC listener to support HTTP API")
+	}


why not just use ok?

also, I'd include a %T somewhere

Don't need ok, why use it? I don't think we need Go types on this error message.

MichaelSnowden · 2023-06-30T17:11:02Z

service/frontend/http_api_server.go

+	tcpAddr := *tcpAddrRef
+	tcpAddr.Port = rpcConfig.HTTPPort
+	if tcpAddr.Port == 0 {
+		tcpAddr.Port = tcpAddrRef.Port + 10


Adding 10 here to the RPC port might work for our static configs, but it could conflict with others--it's also just too much implicit behavior IMO.

Are you suggesting I open on 7243 by default regardless of your gRPC port? I can do that, though would like others' opinions.

The +10 feels too magical and surprising to me also. I'd rather just require an explicit port here. All port selection should be done in static config (or by something wrapping the server and generating a static config in memory)

Feel free to add an explicit port to all the development configs and dev servers

Do we require an explicit port for gRPC today? What does the absence of a gRPC port in config mean today? Does requiring a port basically mean that we are not turning the HTTP server on by default? Or does it mean that we just bind it to any random port by default (arguably a security issue)?

We do always require an explicit port, afaik. (temporalite has code to pick a random port, but then it puts it in the static config.)

We can set a default port the same way we set ports for everything else: by putting them in all the static config files and docker template static config. (Or have it off by default but I think we decided to have it on by default?)

Great! That's information I didn't know. Yes, I will make this required and set the default in static configs. I assume this is only docker/config_template.yaml? Can you confirm for me what happens if frontend gRPC port config is left off? (I can try to dig as well, but I assume it picks a random port which makes it not very usable)

EDIT: I will just change our code to assume 0 port (i.e. unset) means HTTP disabled and set it in the docker config and remove explicit HTTP disabling.

This has been fixed to require an HTTP port since we require it for gRPC. The absence of an HTTP port (or 0) means disabled. Docker config templates have been updated to set to 7243 by default.

MichaelSnowden · 2023-06-30T17:11:30Z

service/frontend/http_api_server.go

+	}
+	tcpAddr := *tcpAddrRef
+	tcpAddr.Port = rpcConfig.HTTPPort
+	if tcpAddr.Port == 0 {


It should be ok to specify the zero port and get a random available port.

Do we support that for gRPC port today? How do you differentiate an unset YAML int from 0 with gRPC port today? Or do we actually default to a random port for gRPC? Are we ok w/ default as a random port for HTTP?

MichaelSnowden · 2023-06-30T17:19:24Z

service/frontend/http_api_server.go

+
+// GracefulStop stops the HTTP server. This will first attempt a graceful stop
+// with a drain time, then will hard-stop. This will not return until stopped.
+func (h *HTTPAPIServer) GracefulStop() {


This should accept a context because we have one at the top-level from fx (although you may have to plumb it down from there)

We don't use the context for bounding gRPC server graceful stop, why is it a new requirement for HTTP server graceful stop? I intentionally chose to match what gRPC server does.

service/frontend/http_api_server.go

MichaelSnowden · 2023-06-30T17:26:54Z

tests/http_api_test.go

+	_, respBody = s.httpPost(
+		http.StatusOK,
+		"/api/v1/namespaces/"+s.namespace+"/workflows/"+workflowID+"/query/some-query",
+		`{ "query": { "queryArgs": [{ "someField": "query-arg" }] } }`,


These are new APIs, though. Why not address this?

dnr · 2023-07-06T19:16:25Z

service/frontend/http_api_server.go

+	tcpAddr := *tcpAddrRef
+	tcpAddr.Port = rpcConfig.HTTPPort
+	if tcpAddr.Port == 0 {
+		tcpAddr.Port = tcpAddrRef.Port + 10


The +10 feels too magical and surprising to me also. I'd rather just require an explicit port here. All port selection should be done in static config (or by something wrapping the server and generating a static config in memory)

Feel free to add an explicit port to all the development configs and dev servers

dnr · 2023-07-06T19:19:33Z

service/frontend/http_api_server.go

+		return nil, fmt.Errorf("failed listening for HTTP API on %v: %w", &tcpAddr, err)
+	}
+	// Close the listener if anything else in this function fails
+	success := false


used named return values and check that instead of an extra local var?

I usually choose this approach because readers can get confused on when named return values are set and especially if you name it err it can often be swallowed in confusing ways. But I will change if preferred.

Agreed about potential confusion with err: elsewhere in the codebase we use retErr for that pattern. But no preference here

service/frontend/http_api_server.go

dnr · 2023-07-06T19:27:23Z

service/frontend/http_api_server.go

+		if conn, _ := r.Context().Value(httpRemoteAddrContextKey{}).(net.Conn); conn != nil {
+			addr = conn.RemoteAddr()
+		}
+		r = r.WithContext(peer.NewContext(r.Context(), &peer.Peer{


nice. this means we don't have to change the authorization interceptor at all?

Exactly. And I have tests proving it.

service/frontend/http_api_server.go

dnr · 2023-07-06T19:43:10Z

service/frontend/service.go

-	if err := s.server.Serve(listener); err != nil {
-		logger.Fatal("Failed to serve on frontend listener", tag.Error(err))
+
+	// Start the gRPC and HTTP server (if any) in the background


I'm changing this in #4584, so after that lands, you can just start two goroutines that check return value independently, and not have to do the channel here

I see. If that lands first I will change when I conflict.

that one landed

Ok, we now do the same for the HTTP server (start in background, don't block)

service/frontend/http_api_server.go

cretz · 2023-07-07T17:15:08Z

Added http_service_requests metric, made StartWorkflowExecutionRequest.request_id optional, and added general PR updates. I won't be constantly fixing conflicts from master until this is nearer approval.

dnr · 2023-07-26T05:37:08Z

service/frontend/fx.go

+	logger log.Logger,
+) (*HTTPAPIServer, error) {
+	// If HTTP API server not enabled, return nil
+	rpcConfig := config.Services[string(serviceName)].RPC


I think there's no reason internal-frontend should listen on http. Maybe we should exclude it here too? e.g.

if serviceName != primitives.FrontendService { return nil, nil }

👍 I was not aware there was an internal-frontend reusing this same package/code. Will only apply conditional to ensure only the public frontend gets the HTTP API.

dnr · 2023-07-26T05:44:36Z

service/frontend/http_api_server.go

+	tcpAddr := *tcpAddrRef
+	tcpAddr.Port = rpcConfig.HTTPPort
+	if tcpAddr.Port == 0 {
+		tcpAddr.Port = tcpAddrRef.Port + 10


We do always require an explicit port, afaik. (temporalite has code to pick a random port, but then it puts it in the static config.)

We can set a default port the same way we set ports for everything else: by putting them in all the static config files and docker template static config. (Or have it off by default but I think we decided to have it on by default?)

dnr · 2023-07-26T05:47:13Z

service/frontend/http_api_server.go

+		return nil, fmt.Errorf("failed listening for HTTP API on %v: %w", &tcpAddr, err)
+	}
+	// Close the listener if anything else in this function fails
+	success := false


Agreed about potential confusion with err: elsewhere in the codebase we use retErr for that pattern. But no preference here

dnr · 2023-07-26T05:55:10Z

service/frontend/http_api_server.go

+	// moment using gRPC's default at
+	// https://github.com/grpc/grpc-go/blob/0673105ebcb956e8bf50b96e28209ab7845a65ad/server.go#L58.
+	// Max header bytes is defaulted by Go to 1MB.
+	r.Body = http.MaxBytesReader(w, r.Body, 1024*1024*4)


There's been talk about increasing this limit for grpc. We already increase it for internode connections but not frontend: https://github.com/temporalio/temporal/blob/master/common/rpc/grpc.go#L58-L59

Can you put this is a constant so we can keep it in sync between grpc and http? Maybe next to that one?

Will do. Will call it maxHttpRequestBytes for now, but would expect it to be updated to share with the gRPC one if/when y'all get around to it.

dnr · 2023-07-26T06:00:06Z

service/frontend/service.go

-	if err := s.server.Serve(listener); err != nil {
-		logger.Fatal("Failed to serve on frontend listener", tag.Error(err))
+
+	// Start the gRPC and HTTP server (if any) in the background


that one landed

dnr · 2023-08-03T20:49:55Z

docker/config_template.yaml

@@ -304,6 +304,7 @@ services:
            grpcPort: {{ $temporalGrpcPort }}
            membershipPort: {{ default .Env.FRONTEND_MEMBERSHIP_PORT "6933" }}
            bindOnIP: {{ default .Env.BIND_ON_IP "127.0.0.1" }}
+            httpPort: {{ default .Env.FRONTEND_HTTP_PORT "7243" }}


You might want to check if there are any changes to be made here:
https://github.com/search?q=repo%3Atemporalio%2Fdocker-compose%207233&type=code
or here:
https://github.com/search?q=repo%3Atemporalio%2Fhelm-charts%207233&type=code
to expose the port

👍 Will open PRs to those upon merge here.

dnr · 2023-08-03T20:58:03Z

service/frontend/workflow_handler.go

-		return nil, errRequestIDNotSet
+		// For easy direct API use, we default the request ID here but expect all
+		// SDKs and other auto-retrying clients to set it
+		request.RequestId = uuid.New()


Do you think it's worth doing this only for http?

Doing any call-specific things for HTTP is not only a bit difficult, it violates the spirit of "generic HTTP interface over existing gRPC behavior". So far I have been able to avoid any call-specific HTTP behavior.

go.mod

# Conflicts: # go.mod # go.sum

HTTP API support

626c049

This was referenced Jun 26, 2023

HTTP API helpers temporalio/api-go#112

Merged

HTTP API annotations temporalio/api#301

Merged

cretz commented Jun 26, 2023

View reviewed changes

service/frontend/http_api_server.go Show resolved Hide resolved

cretz commented Jun 26, 2023

View reviewed changes

tests/http_api_test.go Outdated Show resolved Hide resolved

cretz commented Jun 26, 2023

View reviewed changes

dnr reviewed Jun 27, 2023

View reviewed changes

service/frontend/http_api_server.go Outdated Show resolved Hide resolved

cretz added 2 commits June 28, 2023 10:56

Inline gRPC invocation from HTTP API

e43d59e

HTTP API header support and mTLS test support

efe3978

cretz force-pushed the http-api branch from 9bf27d9 to efe3978 Compare June 30, 2023 15:13

cretz changed the title ~~HTTP API support (early preview)~~ HTTP API support Jun 30, 2023

cretz marked this pull request as ready for review June 30, 2023 15:49

cretz requested a review from a team as a code owner June 30, 2023 15:49

cretz commented Jun 30, 2023

View reviewed changes

MichaelSnowden reviewed Jun 30, 2023

View reviewed changes

dnr reviewed Jul 6, 2023

View reviewed changes

cretz added 3 commits July 7, 2023 11:32

Add HTTP request metric with test

6f59eaa

Default StartWorkflowExecutionRequest.request_id if not given

05edb25

PR suggested improvements

42e9e93

dnr reviewed Jul 26, 2023

View reviewed changes

feedmeapples force-pushed the http-api branch from 7f34578 to 42e9e93 Compare July 27, 2023 20:52

feedmeapples added 2 commits July 28, 2023 14:23

Merge branch 'master' into http-api

9cf6ab8

use built grpc gateway

7c878e4

feedmeapples force-pushed the http-api branch from 6988681 to 7c878e4 Compare July 28, 2023 21:03

Updates based on PR comments, lint, and master updates

094a641

cretz force-pushed the http-api branch from 40cf15c to 18aa7e4 Compare August 1, 2023 18:16

CI fix

5623630

cretz force-pushed the http-api branch from 18aa7e4 to 5623630 Compare August 1, 2023 22:01

Rework metric capturing for tests and other test races

b6b0f72

cretz force-pushed the http-api branch from 590e12e to b6b0f72 Compare August 2, 2023 17:21

dnr approved these changes Aug 3, 2023

View reviewed changes

feedmeapples suggested changes Aug 11, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

cretz added 3 commits August 14, 2023 15:59

Remove accept header validation

580c2bf

Update API dependency

dc44d50

Merge remote-tracking branch 'remotes/origin/master' into http-api

2ffde13

# Conflicts: # go.mod # go.sum

cretz merged commit c784796 into temporalio:main Aug 18, 2023

cretz deleted the http-api branch August 18, 2023 17:11

yiminc mentioned this pull request Oct 21, 2023

Does temporal support message/event queue? #4360

Closed

		// Our version of gRPC gateway only supports integer enums in queries :-(
		"/api/v1/namespaces/"+s.namespace+"/workflows/"+workflowID+"/history?historyEventFilterType=2",

HTTP API support #4543

HTTP API support #4543

Conversation

cretz commented Jun 26, 2023 • edited Loading

cretz Jun 26, 2023 • 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

cretz Jun 30, 2023 • 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

cretz Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jun 30, 2023 • 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

cretz Jul 6, 2023 • 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

cretz Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz commented Jul 7, 2023

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

cretz commented Jun 26, 2023 •

edited

Loading

cretz Jun 26, 2023 •

edited

Loading

cretz Jun 30, 2023 •

edited

Loading

cretz Jul 6, 2023 •

edited

Loading

cretz Jul 31, 2023 •

edited

Loading

cretz Jun 30, 2023 •

edited

Loading

cretz Jul 6, 2023 •

edited

Loading

cretz Jul 31, 2023 •

edited

Loading