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

Use uri.Path instead of uri.Host for UNIX domain socket URLs #540

Merged
merged 2 commits into from Sep 21, 2021

Conversation

webconn
Copy link
Contributor

@webconn webconn commented Sep 20, 2021

This patch allows to use proper UNIX socket URIs such as unix:///var/run/socket.sock. This format is OK with RFC3986 and used in Docker, for example.

Current implementation makes possible only relative paths (like unix://socket.sock) and it looks like it does so by accident.

Example of how it is parsed by url.Parse(): https://play.golang.org/p/OajK5IJX2F9

package main

import (
	"fmt"
	"net/url"
)

func main() {
	val, err := url.Parse("unix:///var/run/socket.sock")
	fmt.Printf("%#v %v", val, err)
}
&url.URL{Scheme:"unix", Opaque:"", User:(*url.Userinfo)(nil), Host:"", Path:"/var/run/socket.sock", RawPath:"", ForceQuery:false, RawQuery:"", Fragment:"", RawFragment:""} <nil>

@MattBrittan
Copy link
Contributor

Unfortunately it will not be possible to merge this until you sign the ECA (see the contributing section of the readme).

This is not a feature I have used (and I don't really know all that much about the UNIX socket interface) but I am concerned that this may break existing uses (e.g. the example you gave unix://socket.sock). I may be wrong but assume whoever added this functionality had a use for it the way its written; would something like the following work (without breaking any existing usage):

var conn net.Conn
var err error
if len(uri.Host) > 0 {
    conn, err = net.DialTimeout("unix", uri.Host, timeout)
else {
    conn, err = net.DialTimeout("unix", uri.Path, timeout)
}

@webconn
Copy link
Contributor Author

webconn commented Sep 21, 2021

Good point, added uri.Host check for compatibility. Also I signed the ECA just now, may require triggering checks.

@MattBrittan
Copy link
Contributor

Looks good to me; thanks for your contribution.

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.

None yet

2 participants