Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable HTTP/2 Cleartext in HTTP Server #6601

Merged
merged 51 commits into from Aug 6, 2023

Conversation

ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle commented Jun 28, 2023

Fixes #6570
Fixes #6767

Enables support for HTTP/2 Cleartext in the HTTP server. This should allow apps to talk to Dapr using HTTP/2 with perf improvements.

Apps making a HTTP/1 connection can upgrade to HTTP/2 if the client supports that, transparently. E.g.:

curl http://localhost:3500/v1.0/healthz -v --http2               
*   Trying ::1:3500...
* Connected to localhost (::1) port 3500 (#0)
> GET /v1.0/healthz HTTP/1.1
> Host: localhost:3500
> User-Agent: curl/7.74.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c
* Received 101
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 204 
< content-type: text/plain; charset=utf-8
< date: Wed, 28 Jun 2023 15:48:04 GMT
< 
* Connection #0 to host localhost left intact

Clients that know that the server (daprd) supports HTTP/2 Cleartext can also make a H2C connection right away (this is called "with prior knowledge"):

curl http://localhost:3500/v1.0/healthz -v --http2-prior-knowledge 
*   Trying ::1:3500...
* Connected to localhost (::1) port 3500 (#0)
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x55778816fad0)
> GET /v1.0/healthz HTTP/2
> Host: localhost:3500
> user-agent: curl/7.74.0
> accept: */*
> 
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 204 
< content-type: text/plain; charset=utf-8
< date: Wed, 28 Jun 2023 15:49:05 GMT
< 
* Connection #0 to host localhost left intact

A few notes:

  • H2C support is enabled by default but can be disabled with the DAPR_HTTP_DISABLE_H2C=1 env var.
    • The main reason why we would want to disable this is testing. For example, to write a perf test that measures the perf with both HTTP/1 and HTTP/2, since perf testers like K6 automatically upgrade to HTTP/2 if that's offered.
    • I am not really aware of any other reason why we'd turn off H2C, so that's why it's an env var rather than a CLI flag.
  • We cannot get a perf test for this yet. I've opened an issue (Revamp perf tests for service invocation #6602) to track that, since it involves a lot more work than is in scope for this PR.
    • Our service invocation perf tests currently use fortio
    • We use a fork of fortio 1.38 where we added support for gRPC
    • Fortio added support for testing with HTTP/2 only at a later release
    • I was able to port our patches for gRPC to fortio 1.55 which does have HTTP/2 support, but...
    • When fortio runs with HTTP/1, it uses fasthttp as a HTTP client internally. As we know well, fasthttp doesn't support HTTP/2, so when running tests against HTTP/2, fortio uses a different HTTP client. That lead to results that were not comparable

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #6601 (23ad457) into master (41760a2) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6601      +/-   ##
==========================================
- Coverage   64.64%   64.56%   -0.09%     
==========================================
  Files         228      228              
  Lines       20260    20263       +3     
==========================================
- Hits        13098    13082      -16     
- Misses       6089     6104      +15     
- Partials     1073     1077       +4     
Files Changed Coverage Δ
pkg/http/server.go 60.00% <100.00%> (+0.71%) ⬆️

... and 4 files with indirect coverage changes

@ItalyPaleAle ItalyPaleAle added the autoupdate DaprBot will keep the Pull Request up to date with master branch label Jun 28, 2023
@daixiang0
Copy link
Member

/ok-to-test

@dapr-bot
Copy link
Collaborator

dapr-bot commented Jul 11, 2023

Dapr E2E test

🔗 Link to Action run

Commit ref: c032afc

✅ Build succeeded for linux/amd64

  • Image tag: dapre2e7fff0a0279l
  • Test image tag: dapre2e7fff0a0279l

✅ Infrastructure deployed

Cluster Resource group name Azure region
Linux Dapr-E2E-dapre2e7fff0a0279l westus3
Windows Dapr-E2E-dapre2e7fff0a0279w westus3
Linux/arm64 Dapr-E2E-dapre2e7fff0a0279la eastus

✅ Build succeeded for windows/amd64

  • Image tag: dapre2e7fff0a0279w
  • Test image tag: dapre2e7fff0a0279w

❌ Tests failed on linux/amd64

Please check the logs for details on the error.

✅ Tests succeeded on windows/amd64

  • Image tag: dapre2e7fff0a0279w
  • Test image tag: dapre2e7fff0a0279w

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

/ok-to-test

@dapr-bot
Copy link
Collaborator

dapr-bot commented Jul 19, 2023

Dapr E2E test

🔗 Link to Action run

Commit ref: 7d110e4

✅ Build succeeded for linux/amd64

  • Image tag: dapre2ed71b93176dl
  • Test image tag: dapre2ed71b93176dl

✅ Infrastructure deployed

Cluster Resource group name Azure region
Linux Dapr-E2E-dapre2ed71b93176dl westus3
Windows Dapr-E2E-dapre2ed71b93176dw westus3
Linux/arm64 Dapr-E2E-dapre2ed71b93176dla eastus

✅ Build succeeded for windows/amd64

  • Image tag: dapre2ed71b93176dw
  • Test image tag: dapre2ed71b93176dw

✅ Tests succeeded on linux/amd64

  • Image tag: dapre2ed71b93176dl
  • Test image tag: dapre2ed71b93176dl

✅ Tests succeeded on windows/amd64

  • Image tag: dapre2ed71b93176dw
  • Test image tag: dapre2ed71b93176dw

@mukundansundar
Copy link
Contributor

We cannot get a perf test for this yet. I've opened an issue (#6602) to track that, since it involves a lot more work than is in scope for this PR.
Our service invocation perf tests currently use fortio
We use a fork of fortio 1.38 where we added support for gRPC
Fortio added support for testing with HTTP/2 only at a later release
I was able to port our patches for gRPC to fortio 1.55 which does have HTTP/2 support, but...
When fortio runs with HTTP/1, it uses fasthttp as a HTTP client internally. As we know well, fasthttp doesn't support HTTP/2, so when running tests against HTTP/2, fortio uses a different HTTP client. That lead to results that were not comparable

I think we should move the service invocation perf tests to k6 if possible.

@ItalyPaleAle
Copy link
Contributor Author

I think we should move the service invocation perf tests to k6 if possible.

Tracking that in #6602 as it’s out of scope of this PR

@shivamkm07
Copy link
Contributor

@ItalyPaleAle @mukundansundar postgres setup fail :(

Yes, it hasn’t been working since #6230 has been merged.

CC: @shivamkm07

Yes. This is because of an issue in upstream bitnami charts for postgresql: bitnami/charts#17488

@yaron2 yaron2 dismissed stale reviews from mukundansundar and daixiang0 via 3ecdaab July 27, 2023 00:49
mukundansundar
mukundansundar previously approved these changes Jul 27, 2023
@mukundansundar mukundansundar merged commit 7c5c9c3 into dapr:master Aug 6, 2023
29 of 31 checks passed
@ItalyPaleAle ItalyPaleAle added this to the v1.12 milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoupdate DaprBot will keep the Pull Request up to date with master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

why Proto is HTTP/1.1 when dapr.io/app-protocol is h2c Enable HTTP/2 Cleartext support in Dapr HTTP Server
5 participants