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

Serve HTTP2 health check #1442

Merged
merged 3 commits into from Jan 28, 2020
Merged

Serve HTTP2 health check #1442

merged 3 commits into from Jan 28, 2020

Conversation

@gdbelvin
Copy link
Collaborator

gdbelvin commented Jan 27, 2020

Prioritize HTTP2 over HTTP1

Revert #1435 and use http.ServeHTTP rather than cmux.

  • cmux prevents http.Serve from serving HTTP2
  • http.Serve will only serve HTTP2 over TLS
    • http.Serve uses a type assertion to validate this
    • cmux interrupts this because it wraps the connection.
    • even if the connection did come from a TLS listener

The GCE ingress load ballancer requires services to serve
an HTTP 200 OK at / on the same protocol that the server uses.
To support GRPC, this means using HTTP2 throuought.
HTTP2 also requires TLS, both by golang and GCE ingress.

@gdbelvin gdbelvin requested a review from google/keytransparency as a code owner Jan 27, 2020
@gdbelvin gdbelvin requested a review from RJPercival Jan 27, 2020
@googlebot googlebot added the cla: yes label Jan 27, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #1442 into master will decrease coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1442      +/-   ##
==========================================
- Coverage   66.56%   66.31%   -0.25%     
==========================================
  Files          54       54              
  Lines        4026     4026              
==========================================
- Hits         2680     2670      -10     
- Misses        955      962       +7     
- Partials      391      394       +3
Impacted Files Coverage Δ
core/client/mutations.go 83.78% <0%> (-5.41%) ⬇️
core/keyserver/revisions.go 62.58% <0%> (-2.05%) ⬇️
impl/sql/directory/storage.go 67.66% <0%> (-1.51%) ⬇️
core/sequencer/server.go 72.96% <0%> (-0.98%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8831a2...23df57a. Read the comment docs.

@gdbelvin gdbelvin force-pushed the gdbelvin:http2 branch from e866854 to ebdfdaa Jan 27, 2020
gdbelvin added 2 commits Jan 27, 2020
This reverts commit ef79a0a.

cmux cannot serve HTTP2 with Golang's http server.
@gdbelvin gdbelvin force-pushed the gdbelvin:http2 branch from ebdfdaa to 1dbf657 Jan 28, 2020
@gdbelvin gdbelvin requested a review from Mercurrent Jan 28, 2020
cmd/serverutil/serverutil.go Outdated Show resolved Hide resolved
cmd/serverutil/serverutil.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@google google deleted a comment from RJPercival Jan 28, 2020
@gdbelvin gdbelvin merged commit 69dad1c into google:master Jan 28, 2020
5 checks passed
5 checks passed
GolangCI No issues found!
Details
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
codecov/patch Coverage not affected when comparing d8831a2...23df57a
Details
codecov/project Absolute coverage decreased by -0.25, only covered lines were removed
Details
@gdbelvin gdbelvin deleted the gdbelvin:http2 branch Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.