dev-server: Properly deal with IPv6#611
Merged
mjameswh merged 2 commits intotemporalio:mainfrom Jul 3, 2024
Merged
Conversation
cretz
approved these changes
Jul 2, 2024
temporalcli/devserver/freeport.go
Outdated
| } | ||
|
|
||
| // Escapes an IPv6 address with square brackets, if it is an IPv6 address. | ||
| func MaybeEscapeIpv6(host string) string { |
Contributor
There was a problem hiding this comment.
Suggested change
| 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)) |
Contributor
There was a problem hiding this comment.
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.
Contributor
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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
--ip 0.0.0.0on 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 ::1or--ui-ip ::).--ui-ipand--ui-port. Fix [Bug] cannot bind ui to different IP using--ui-ip #602.ip:portis 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/ipv6exists), but the container image must not be configured for IPv6 (e.g.ip addr show dev lodoesn't show any IPv6 address, andping6 ::1fails). The VM used by Docker Desktop won't work, apparently because it configures container's IPv6 correctly.