-
Notifications
You must be signed in to change notification settings - Fork 111
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
Add host header #185
Add host header #185
Conversation
protocols.go
Outdated
@@ -24,6 +24,7 @@ const ( | |||
P_UNIX = 400 | |||
P_P2P = 421 | |||
P_IPFS = P_P2P // alias for backwards compatibility | |||
P_HOST_HEADER = 476 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you choose 476? No strong opinion here, but maybe this should live after HTTP. I’m also wondering if it should be called P_HTTP_HOST_HEADER. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
476 is to be before both HTTP and WS (which works over HTTP and is 477).
I have no strong opinion about the name, http_host_header works as well. Once agreed with @MarcoPolo or other multiformats maintainers, I update this PR to the final name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also prefer 481 so that it's right after HTTP.
Slight preference for host-header
@marten-seemann because it looks nicer in text:
/ip4/1.2.3.4/tcp/80/http/host-header/example.com
vs
/ip4/1.2.3.4/tcp/80/http/http-host-header/example.com
I recognize this is a stylistic choice and this doesn't affect over the wire representation. So no strong opinion here. Do you feel strongly for http-host-header
@marten-seemann ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to 481. still holding on changing the name atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight preference for host-header @marten-seemann because it looks nicer in text
Good point! I think this changes my preference from being slightly in favor of HTTP_HOST_HEADER to being slightly in favor of HOST_HEADER.
|
||
var TranscoderHostHeader = NewTranscoderFromFunctions(hostHeaderStB, hostHeaderBtS, hostHeaderVal) | ||
|
||
func hostHeaderVal(b []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func hostHeaderVal(b []byte) error { | |
func hostHeaderValidate(b []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validation function for other protocols are all labeled <prococol>Val(
. I feel like using <protocol>Validate
is inconsistent.
transcoders.go
Outdated
var TranscoderHostHeader = NewTranscoderFromFunctions(hostHeaderStB, hostHeaderBtS, hostHeaderVal) | ||
|
||
func hostHeaderVal(b []byte) error { | ||
split := bytes.Split(b, []byte{':'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s probably easier to convert to string once in the beginning. I think there’s also a standard library function to split a hostname and a port. I don’t remember how it’s called though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the way you phrased it pointed me in the right direction. There is net.SplitHostPort. I have updated the function accordingly.
* Add host-header protocol * Add Host Header Transcoder, validating against HTTP [RFC 2616 § 14.23](https://www.rfc-editor.org/rfc/rfc2616#section-14.23)
Closing for now since we don't need this right now |
MultiAddr code to be merged in multiformats/multiaddr#144
Relates to multiformats/multicodec#296 and libp2p/go-libp2p#1829
cc @MarcoPolo