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

dev-server: Properly deal with IPv6 #611

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

mjameswh
Copy link
Contributor

@mjameswh mjameswh commented Jul 2, 2024

What was changed

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.

# Add two more loopback IPs
sudo ifconfig lo0 alias 127.0.0.2 up
sudo ifconfig lo0 alias 127.0.0.3 up

# Make sure that loopback has IPv6
ifconfig lo0 | grep ::1

# Test various cases
./temporal server start-dev --port 7235 --ui-port 7236 --http-port 7237 --metrics-port 7238 --ip :: --ui-ip ::1
./temporal server start-dev --port 7235 --ui-port 7236 --http-port 7237 --metrics-port 7238 --ip 127.0.0.2 --ui-ip 127.0.0.3
./temporal server start-dev --port 7235 --ui-port 7236 --http-port 7237 --metrics-port 7238 --ip 0.0.0.0 --ui-ip 127.0.0.1

# For each of the preceeding commands, assert:

# 1. That ports were bounded correctly
lsof -n -p $( ps | grep "temporal[ ]server" | cut -f 1 -d " " ) | grep LISTEN

# 2. That UI interface opens without issue
# 3. The prometheus metrics page opens  without issue
# 4. No error in server logs

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, and ping6 ::1 fails). The VM used by Docker Desktop won't work, apparently because it configures container's IPv6 correctly.

@mjameswh mjameswh requested a review from cretz July 2, 2024 15:31
temporalcli/commands.server.go Show resolved Hide resolved
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Member

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.

@cretz
Copy link
Member

cretz commented Jul 2, 2024

How this was tested

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)

@mjameswh mjameswh merged commit ec94f96 into temporalio:main Jul 3, 2024
7 checks passed
@mjameswh mjameswh deleted the get-free-port-ipv6 branch July 3, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants