Migrate HTTP server to net/http #6248

ItalyPaleAle · 2023-04-16T21:58:12Z

Fixes Migrate HTTP server from fasthttp to net/http #4979 (although more work will be needed to completely remove fasthttp, see below)
Necessary (but not quite sufficient) to fix also:

This PR changes the HTTP server's listeners from fasthttp to net/http and fixes (or enables fixing) a lot of the scenarios that are blocked by #4979. Immediately, this should enable the web server to use HTTP/2 when TLS is enabled.

Included and not included

What this PR includes:

Changes the listeners for the 3 HTTP servers (app, healthz, pprof) from using fasthttp to using the standard library (net/http)
Changes all built-in middlewares used by the servers above to be based on net/http

What this PR does NOT include:

No handler has been updated from fasthttp to net/http, and handlers continue to work thanks to the "adapters"
- This is not something new as in Dapr 1.10, all requests go through adapters twice to be converted fasthttp(listener + built-in middlewares)->net/http (middleware components)->fasthttp (handlers). Now there's only one conversion: net/http(everything but handlers)->fasthttp(handlers)
Does not change the router, which is still the fasthttp one. This is blocking updating handlers from fasthttp to net/http, and needs to be done in a separate PR.
Does not enable H2C. Should be done in a separate PR.
Some unit tests, including many (not all) in pkg/http still use fasthttp to create the test server. This isn't a concern at least until the handlers are migrated, but it should be changed in the future.

All features continue to work as expected, as testified by the fact that tests have not changed. Only one exception to this rule is that when performing HTTP->gRPC invocations, the "dapr-host" metadata is not included anymore (this is probably ok also because HTTP->gRPC service invocation is being deprecated)

Next steps

Update the router to one based on net/http, and apply the adapters only on individual handlers rather than on the router itself.
Migrate the Universal API adapter from fasthttp to net/http
Migrate APIs that can use streaming (such as service invocation) to be based on net/http and leverage streaming for input and output
Migrate all other APIs from fasthttp to either net/http handlers or Universal APIs.

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

See dapr#6246 Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle · 2023-04-16T22:00:18Z

pkg/http/api.go


 	statusCode := int(resp.Status().Code)

-	// TODO @ItalyPaleAle: Make this the only path once streaming is finalized
-	if a.isStreamingEnabled {


Reviewers: this was removed for now since it's not effective (see #6246 )

ItalyPaleAle · 2023-04-16T22:01:52Z

pkg/http/server.go

 }

-func (s *server) useCors(next fasthttp.RequestHandler) fasthttp.RequestHandler {
+func (s *server) useCors(next http.Handler) http.Handler {
+	// TODO: Technically, if "AllowedOrigins" is "*", all origins should be allowed


Reviewers: Dapr is likely behaving incorrectly here. Setting * in CORS should mean "allow all origins", but Dapr <=1.10 was actually omitting the CORS headers entirely. Since this is a backwards-incompatible change, I am not touching it now.

codecov · 2023-04-16T22:20:25Z

Codecov Report

Merging #6248 (0cd9e3c) into master (054df8c) will increase coverage by 0.04%.
The diff coverage is 76.43%.

@@            Coverage Diff             @@
##           master    #6248      +/-   ##
==========================================
+ Coverage   65.80%   65.84%   +0.04%     
==========================================
  Files         194      195       +1     
  Lines       18995    19021      +26     
==========================================
+ Hits        12499    12524      +25     
+ Misses       5487     5480       -7     
- Partials     1009     1017       +8

Impacted Files	Coverage Δ
pkg/http/responses.go	`88.23% <ø> (+33.68%)`	⬆️
pkg/messaging/direct_messaging.go	`52.39% <0.00%> (ø)`
pkg/messaging/v1/invoke_method_request.go	`83.83% <0.00%> (-1.73%)`	⬇️
utils/nethttpadaptor/nethttpadaptor.go	`72.34% <33.33%> (-2.66%)`	⬇️
pkg/diagnostics/utils/trace_utils.go	`8.77% <45.45%> (-3.96%)`	⬇️
pkg/grpc/api_daprinternal.go	`38.96% <50.00%> (+0.14%)`	⬆️
pkg/http/server.go	`61.67% <59.25%> (-2.30%)`	⬇️
utils/responsewriter/response_writer.go	`71.11% <71.11%> (ø)`
pkg/messaging/v1/util.go	`51.09% <71.42%> (-1.09%)`	⬇️
utils/streams/limitreadcloser.go	`84.61% <77.77%> (+84.61%)`	⬆️
... and 7 more

... and 2 files with indirect coverage changes

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

utils/responsewriter/README.md

daixiang0 · 2023-04-17T01:53:30Z

/ok-to-test

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle · 2023-04-17T14:57:47Z

/ok-to-perf

dapr-bot · 2023-04-17T14:58:18Z

Dapr perf test

🔗 Link to Action run

Commit ref: 947ec61

✅ Build succeeded

Image tag: daprprfac9c242881
Test image tag: daprprfac9c242881

✅ Infrastructure deployed

Resource group name: Dapr-Perf-daprprfac9c242881
Azure region: westus3

✅ Perf tests succeeded

Image tag: daprprfac9c242881
Test image tag: daprprfac9c242881

yaron2 · 2023-04-17T15:06:25Z

/ok-to-perf

Long have we waited for this change... Very curious about the perf results

ItalyPaleAle · 2023-04-17T16:37:07Z

@yaron2 all perf tests are passing, and looks like perf may actually be improving

Looking at the service invocation perf test (which is particularly useful here as it is among those more sensitive to the web server's own perf):

This PR: 1 2

P50: 1.38ms / 1.42ms
P90: 1.98ms / 2.13ms

Three recent perf tests from master (1 2 3)

Added latency avg: 1.76ms / 1.50ms / 1.47ms
P90: 2.86ms / 2.13ms /2.09ms

yaron2 · 2023-05-30T22:52:02Z

/ok-to-perf

dapr-bot · 2023-05-30T22:52:26Z

Dapr perf test

🔗 Link to Action run

Commit ref: e6e4cbd

✅ Build succeeded

Image tag: daprprf3811325376
Test image tag: daprprf3811325376

✅ Infrastructure deployed

Resource group name: Dapr-Perf-daprprf3811325376
Azure region: westus3

✅ Perf tests succeeded

Image tag: daprprf3811325376
Test image tag: daprprf3811325376

dapr-bot · 2023-05-30T22:52:28Z

Dapr perf test

🔗 Link to Action run

Commit ref: e6e4cbd

✅ Build succeeded

Image tag: daprprfb757de21a9
Test image tag: daprprfb757de21a9

✅ Infrastructure deployed

Resource group name: Dapr-Perf-daprprfb757de21a9
Azure region: westus3

✅ Perf tests succeeded

Image tag: daprprfb757de21a9
Test image tag: daprprfb757de21a9

dapr-bot · 2023-05-30T22:52:30Z

Dapr perf test

🔗 Link to Action run

Commit ref: e6e4cbd

✅ Build succeeded

Image tag: daprprf9520bd677c
Test image tag: daprprf9520bd677c

✅ Infrastructure deployed

Resource group name: Dapr-Perf-daprprf9520bd677c
Azure region: westus3

✅ Perf tests succeeded

Image tag: daprprf9520bd677c
Test image tag: daprprf9520bd677c

artursouza · 2023-05-30T23:50:48Z

/test-sdk-all

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle · 2023-05-31T04:11:31Z

/test-sdk-all

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

…b-server

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle · 2023-05-31T20:45:56Z

/ok-to-test

ItalyPaleAle · 2023-05-31T20:46:01Z

/test-sdk-all

dapr-bot · 2023-05-31T20:46:22Z

Dapr SDK Python test

🔗 Link to Action run

Commit ref: d150ce1

✅ Python SDK tests passed

dapr-bot · 2023-05-31T20:46:25Z

Dapr SDK Java test

🔗 Link to Action run

Commit ref: d150ce1

✅ Java SDK tests passed

dapr-bot · 2023-05-31T20:46:27Z

Dapr E2E test

🔗 Link to Action run

Commit ref: d150ce1

✅ Build succeeded for linux/arm64

Image tag: dapre2ed7d8620e8bla
Test image tag: dapre2ed7d8620e8bla

✅ Infrastructure deployed

Cluster	Resource group name	Azure region
Linux	`Dapr-E2E-dapre2ed7d8620e8bl`	westus3
Windows	`Dapr-E2E-dapre2ed7d8620e8bw`	westus3
Linux/arm64	`Dapr-E2E-dapre2ed7d8620e8bla`	eastus

✅ Build succeeded for linux/amd64

Image tag: dapre2ed7d8620e8bl
Test image tag: dapre2ed7d8620e8bl

✅ Build succeeded for windows/amd64

Image tag: dapre2ed7d8620e8bw
Test image tag: dapre2ed7d8620e8bw

⚠️ Tests skipped on linux/arm64

Image tag: dapre2ed7d8620e8bla
Test image tag: dapre2ed7d8620e8bla

✅ Tests succeeded on linux/amd64

Image tag: dapre2ed7d8620e8bl
Test image tag: dapre2ed7d8620e8bl

✅ Tests succeeded on windows/amd64

Image tag: dapre2ed7d8620e8bw
Test image tag: dapre2ed7d8620e8bw

dapr-bot · 2023-05-31T20:46:48Z

Dapr SDK JS test

🔗 Link to Action run

Commit ref: d150ce1

✅ JS SDK tests passed

ItalyPaleAle · 2023-05-31T20:59:41Z

@artursouza

Fixed all the issues reported by the SDK tests:

Java: issue was with the shutdown sequence. Stopping a net/http server returns an error always, which normally is ErrServerClosed, and had to be ignored.
Python: issue was related to trace headers not being converted between net/http and fasthttp. The routes are still implemented as fasthttp, but the middleware that sets the tracing span now uses net/http. I added a temporary compatibility layer to make it work (until the routes are converted to net/http)

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle added 15 commits April 15, 2023 23:15

Convert pprof server to net/http

356dc0f

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

🧹

128127d

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Converted some middlewares to net/http

315fa1e

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Fixed tests

062012a

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Migrated metrics middleware

781eb38

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Working on tracing middleware

df19806

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Completed converting middlewares

cd4f649

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

💄

12a3109

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Changed the web server

ea19507

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Removed streaming from HTTP API since it is not working at this time

f8af99c

See dapr#6246 Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Fixed useMaxBodySize middleware

3dbce23

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Fixed tracing headers

69561d3

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Should fix tests

f0486f5

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Should actually fix tests

18e027b

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Should fix limiting request body

379caea

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle requested review from a team as code owners April 16, 2023 21:58

Merge branch 'master' into migrate-web-server

b3521eb

ItalyPaleAle commented Apr 16, 2023

View reviewed changes

Add unit tests for limitreadcloser

ee6af0d

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

daixiang0 reviewed Apr 17, 2023

View reviewed changes

utils/responsewriter/README.md Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

ItalyPaleAle added 2 commits April 17, 2023 02:47

Document changes in responsewriter

8731861

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Merge branch 'master' into migrate-web-server

947ec61

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

Handle ErrServerClosed better

02728a9

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

ItalyPaleAle added 5 commits May 31, 2023 04:31

Tweak SDK pipeline

8439ab2

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Added InvokeMethodRequest.WithHTTPHeaders

e810c39

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Merge branch 'master' of https://github.com/dapr/dapr into migrate-we…

ca5cf54

…b-server

Fix propagation of trace context between net/http and fasthttp

76fc6f6

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Fix handling of error responses from app in CallLocalStream

d150ce1

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

💄

0cd9e3c

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

yaron2 approved these changes May 31, 2023

View reviewed changes

yaron2 merged commit 5aba3c9 into dapr:master May 31, 2023

ItalyPaleAle mentioned this pull request Jun 7, 2023

Migrate HTTP router to Chi #6461

Merged

Migrate HTTP server to net/http #6248

Migrate HTTP server to net/http #6248

Uh oh!

Conversation

ItalyPaleAle commented Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Included and not included

Next steps

Uh oh!

ItalyPaleAle Apr 16, 2023

Choose a reason for hiding this comment

Uh oh!

ItalyPaleAle Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

daixiang0 commented Apr 17, 2023

Uh oh!

This comment was marked as outdated.

ItalyPaleAle commented Apr 17, 2023

Uh oh!

dapr-bot commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dapr perf test

✅ Build succeeded

✅ Infrastructure deployed

✅ Perf tests succeeded

Uh oh!

yaron2 commented Apr 17, 2023

Uh oh!

ItalyPaleAle commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaron2 commented May 30, 2023

Uh oh!

dapr-bot commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dapr perf test

✅ Build succeeded

✅ Infrastructure deployed

✅ Perf tests succeeded

Uh oh!

dapr-bot commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dapr perf test

✅ Build succeeded

✅ Infrastructure deployed

✅ Perf tests succeeded

Uh oh!

dapr-bot commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dapr perf test

✅ Build succeeded

✅ Infrastructure deployed

✅ Perf tests succeeded

Uh oh!

artursouza commented May 30, 2023

Uh oh!

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

ItalyPaleAle commented May 31, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

ItalyPaleAle commented May 31, 2023

Uh oh!

ItalyPaleAle commented May 31, 2023

Uh oh!

dapr-bot commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dapr SDK Python test

✅ Python SDK tests passed

ItalyPaleAle commented Apr 16, 2023 •

edited

Loading

ItalyPaleAle Apr 16, 2023 •

edited

Loading

codecov bot commented Apr 16, 2023 •

edited

Loading

dapr-bot commented Apr 17, 2023 •

edited

Loading

ItalyPaleAle commented Apr 17, 2023 •

edited

Loading

dapr-bot commented May 30, 2023 •

edited

Loading

dapr-bot commented May 30, 2023 •

edited

Loading

dapr-bot commented May 30, 2023 •

edited

Loading

dapr-bot commented May 31, 2023 •

edited

Loading

dapr-bot commented May 31, 2023 •

edited

Loading

dapr-bot commented May 31, 2023 •

edited

Loading

dapr-bot commented May 31, 2023 •

edited

Loading