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

Allow use of custom port value in Alt-Svc header. #3272

Merged
merged 5 commits into from
Oct 12, 2021

Conversation

aaronriekenberg
Copy link
Contributor

Fixes #3262

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@aaronriekenberg
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

http3/server.go Outdated
@@ -530,7 +548,8 @@ func ListenAndServe(addr, certFile, keyFile string, handler http.Handler) error
}

quicServer := &Server{
Server: httpServer,
Server: httpServer,
customAltSvcPort: customAltSvcPort,
Copy link
Contributor Author

@aaronriekenberg aaronriekenberg Sep 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possible way to do this would be to just set Server.port to customAltSvcPort here, and then SetQuicHeaders would find port is always having a non-zero value. Open to suggestions of what you think is the best way to do this. :)

http3/server.go Outdated
// customAltSvcPort is used to override the default port value in the Alt-Svc response header.
// This is useful when a Layer 4 firewall is redirecting UDP traffic and clients must use
// a port different from the port the QUIC server itself is listening on.
func ListenAndServeWithCustomAltSvcPort(addr, certFile, keyFile string, handler http.Handler, customAltSvcPort uint32) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need an extra function here. Users who need to change the port should just initialize an http3.Server..

example/main.go Outdated
err = http3.ListenAndServeWithCustomAltSvcPort(bCap, certFile, keyFile, handler, uint32(*customAltSvcPort))
} else {
err = http3.ListenAndServe(bCap, certFile, keyFile, handler)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is important enough to warrant complicating the example.

http3/server.go Outdated
@@ -92,7 +92,8 @@ type Server struct {
// See https://www.ietf.org/archive/id/draft-schinazi-masque-h3-datagram-02.html.
EnableDatagrams bool

port uint32 // used atomically
port uint32 // used atomically
customAltSvcPort uint32 // custom port used for Alt-Svc response header
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine just to export port here.

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good now. Can we have a test case please?

@codecov
Copy link

codecov bot commented Sep 18, 2021

Codecov Report

Merging #3272 (afbe993) into master (3b46d74) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3272      +/-   ##
==========================================
+ Coverage   85.48%   85.69%   +0.21%     
==========================================
  Files         132      132              
  Lines        9799     9775      -24     
==========================================
  Hits         8376     8376              
+ Misses       1048     1024      -24     
  Partials      375      375              
Impacted Files Coverage Δ
http3/server.go 69.40% <100.00%> (ø)
conn_oob.go 71.88% <0.00%> (+11.35%) ⬆️

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 3b46d74...afbe993. Read the comment docs.

@marten-seemann
Copy link
Member

I disabled the goconst linter in #3286, so the tests are expected to pass once this PR is merged.

@marten-seemann marten-seemann merged commit f0fab3a into quic-go:master Oct 12, 2021
@aaronriekenberg
Copy link
Contributor Author

Thank you @marten-seemann :)

@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting custom port in Alt-Svc header value
3 participants