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.

Sign up for GitHub

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

Jump to bottom

net/url: mention net.JoinHostPort for constructing url.URL.Host #61093

Closed
dsnet opened this issue Jun 30, 2023 · 10 comments
Closed

net/url: mention net.JoinHostPort for constructing url.URL.Host #61093

dsnet opened this issue Jun 30, 2023 · 10 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Jun 30, 2023 •
edited

In #16142, we add read support for extracting out the hostname and port.

However, that API is lacking the ability to write a host:port pair. Unfortunately, this is not as trivial as concatenating the host and port with a ":" in-between in the case of IPv6, which requires wrapping the entire host with "[" and "]" characters.

I propose the addition of:

// JoinHostnamePort joins a hostname and a port, separating them with a colon.
// If the hostname is an IPv6 address, then it is wrapped with brackets.
// If the port is empty, then the hostname alone is returned.
func JoinHostnamePort(hostname, port string) string

Example outputs:

JoinHostnamePort("carbonite.digitalstatic.ts.net", "")              // carbonite.digitalstatic.ts.net
JoinHostnamePort("carbonite.digitalstatic.ts.net", "9200")          // carbonite.digitalstatic.ts.net:9200
JoinHostnamePort("100.109.51.95", "")                               // 100.109.51.95
JoinHostnamePort("100.109.51.95", "9200")                           // 100.109.51.95:9200
JoinHostnamePort("fd7a:115c:a1e0:ab12:4843:cd96:626d:335f", "")     // fd7a:115c:a1e0:ab12:4843:cd96:626d:335f
JoinHostnamePort("fd7a:115c:a1e0:ab12:4843:cd96:626d:335f", "9200") // [fd7a:115c:a1e0:ab12:4843:cd96:626d:335f]:9200

Considerations:

  • It's called JoinHostnamePort instead of JoinHostPort to be consistent with the url.URL.Hostname method.
  • The port is a string to be consistent with url.URL.Port
  • A standalone function that returns a string is consistent with top-level string-only transformers like JoinPath, PathEscape, PathUnescape, QueryEscape, QueryUnescape.
  • What happens if the hostname contains colons but is not an IPv6 or the port is not a valid integer? We'll just concatenate the host and port with a colon in-between even if the result is ambiguous to parse. Alternatively, we could modify the API of JoinHostnamePort to return an error, but I personally like the simplicity of an error-less result.

\cc @bradfitz
@dsnet dsnet added the Proposal label Jun 30, 2023
@gopherbot gopherbot added this to the Proposal milestone Jun 30, 2023
@seankhliao
Copy link
Member

is the only difference with net.JoinHostPort the handling of an empty port string?

@dsnet
Copy link
Member Author

dsnet commented Jun 30, 2023 •
edited

is the only difference with net.JoinHostPort the handling of an empty port string?

No, it has proper handling for IPv6 addresses by wrapping them in a pair of "[" and "]".

See RFC 2732, section 3:

It defines a syntax for IPv6 addresses and allows the use of "[" and "]" within a URI explicitly for this reserved purpose.

@seankhliao
Copy link
Member

The net version also does it: https://go.dev/play/p/l0Wu3N4SJWS
outputs from your example

carbonite.digitalstatic.ts.net:
carbonite.digitalstatic.ts.net:9200
100.109.51.95:
100.109.51.95:9200
[fd7a:115c:a1e0:ab12:4843:cd96:626d:335f]:
[fd7a:115c:a1e0:ab12:4843:cd96:626d:335f]:9200

@dsnet
Copy link
Member Author

dsnet commented Jun 30, 2023 •
edited

I had no idea the "net" package has this. I'd be okay downgrading this issue to just documentation in the "url" package to point towards net.JoinHostPort.

I personally, find it odd that JoinHostPort is in "net", rather than "url", since the whole handling of brackets for IPv6 seems to only exist for URLs.

Edit: I guess, it makes sense to be in "net" since Dial relies on host:port pairs.

@dsnet dsnet changed the title proposal: net/url: add JoinHostnamePort net/url: mention net.JoinHostPort for constructing url.URL.Host Jun 30, 2023
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 30, 2023
@bcmills bcmills modified the milestones: Proposal, Backlog Jun 30, 2023
@mateenbagheri
Copy link

Hello. I came across this documentation issue and I would like to contribute by addressing it. I would be happy to work on this issue and provide a PR. Could you please assign it to me?
Thank you

@mateenbagheri
Copy link

mateenbagheri commented Jul 10, 2023 •
edited

Hi @dsnet! I was looking into issue #61093 regarding the addition detail regarding how JoinHostPort needs doc change for net package. While reading through the discussion, I noticed your comment mentioning that something in the net package needs to be changed and there is no need to mention this in url package when you said:

"I guess, it makes sense to be in "net" since Dial relies on host:port pairs."

I would like to understand better what specific changes you think are necessary in the net package. Could you please provide more details or point me to the relevant code/documentation that needs attention?
Thank you in advance for your help!

@jjlin
Copy link

jjlin commented Jul 10, 2023

The net version also does it: https://go.dev/play/p/l0Wu3N4SJWS outputs from your example

carbonite.digitalstatic.ts.net:
carbonite.digitalstatic.ts.net:9200
100.109.51.95:
100.109.51.95:9200
[fd7a:115c:a1e0:ab12:4843:cd96:626d:335f]:
[fd7a:115c:a1e0:ab12:4843:cd96:626d:335f]:9200

net.JoinHostPort leaves a trailing colon when the port is empty, though. Is that technically allowed? Even if it is, I expect it would break some code out there that doesn't know how to handle that.

@jjlin jjlin mentioned this issue Jul 10, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/508976 mentions this issue: net/url: document requirements for IPv6 addresses in URL.Host

@dsnet
Copy link
Member Author

dsnet commented Jul 11, 2023

net.JoinHostPort leaves a trailing colon when the port is empty, though. Is that technically allowed?

Apparently yes.

According to RFC 3986, section 3.2:

authority   = [ userinfo "@" ] host [ ":" port ]

which implies that a port must be present.
BUT, the grammar for port according to section 3.2.3 is:

port        = *DIGIT

where a port can be zero or more digits. Syntactically, it can even be greater than 65535.

@jjlin
Copy link

jjlin commented Jul 11, 2023

Good to know. But section 3.2.3 does also say

URI producers and normalizers should omit the port component and its ":" delimiter if port is empty or if its value would be the same as that of the scheme's default.

So it would be nice if net.JoinHostPort would do that when the port argument is empty, and it seems to me that should be backward compatible behavior anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@jjlin @bcmills @dsnet @gopherbot @seankhliao @mateenbagheri