-
Notifications
You must be signed in to change notification settings - Fork 36
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
dev-server: Properly deal with IPv6 #611
Conversation
temporalcli/devserver/freeport.go
Outdated
if err != nil { | ||
return err | ||
} | ||
l.Close() | ||
return nil | ||
} | ||
|
||
// Escapes an IPv6 address with square brackets, if it is an IPv6 address. | ||
func MaybeEscapeIpv6(host string) string { |
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 MaybeEscapeIpv6(host string) string { | |
func MaybeEscapeIPv6(host string) string { |
(pedantic, I know)
// DialTCP(..., l.Addr()) might fail if machine has IPv6 support, but | ||
// isn't fully configured (e.g. doesn't have a loopback interface bound | ||
// to ::1). For safety, rebuild address form the original host instead. | ||
tcpAddr, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:%d", host, port)) |
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.
My concern here is that we are not choosing a specific IP version based on the input string when binding, but we are when dialing. I am concerned about an ipv6-only machine (assuming they exist) if we blindly say --ip 0.0.0.0
works for all IP versions. But it's not a real issue, we can just tell people to pass in --ip ::
or something.
Feel free to add those tests explicitly if it would help (though I only mean the tests for properly enabled IPv4/IPv6, obviously won't work for use cases where you want to test no-IPv6 loopback) |
What was changed
--ip 0.0.0.0
on machines that have IPv6 support but isn't fully configured (e.g. doesn't have a loopback interface bound to::1
). Fix [Bug] dev server panics when binding to 0.0.0.0 in docker container #595.--ip ::1
or--ui-ip ::
).--ui-ip
and--ui-port
. Fix [Bug] cannot bind ui to different IP using--ui-ip #602.ip:port
is already in use. Fix [Feature Request] Error when a port is taken #242.--ip 127.0.0.2
).How this was tested
I added a few integration tests.
However, most cases fixed in this PR relates to specific network configuration, which can't easily be reproduced in CI or on developer machines. These changes were therefore only validated by manual testing.
For future reference, this is the gist of what I did on my Mac.
To test in Docker, I had to use Rancher Desktop, with Kubernetes support enabled. The important thing for testing #595 is that the kernel of the virtual machine used by Docker must be compiled with IPv6 support (e.g.
/proc/sys/net/ipv6
exists), but the container image must not be configured for IPv6 (e.g.ip addr show dev lo
doesn't show any IPv6 address, andping6 ::1
fails). The VM used by Docker Desktop won't work, apparently because it configures container's IPv6 correctly.