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

Handle IPv6 in isMovedError #2981

Merged
merged 3 commits into from
Apr 28, 2024

Conversation

daviddzxy
Copy link
Contributor

If the address in the error is an IPv6 address it adds squared brackets around the host part of the address.

@daviddzxy daviddzxy force-pushed the handle-ipv6-in-isMovedError branch 2 times, most recently from 3d4267b to cbb06f2 Compare April 25, 2024 18:24
@monkey92t
Copy link
Collaborator

monkey92t commented Apr 26, 2024

should:

func getAddr(addr string) string {
	ind := strings.LastIndex(addr, ":")
	if ind == -1 {
		return ""
	}

	if string(addr[ind-1]) == "]" {
		return ""
	}

	return net.JoinHostPort(addr[:ind], addr[ind+1:])
}

func TestAddr(t *testing.T) {
	if v := getAddr("127.0.0.1:1234"); v != "127.0.0.1:1234" {
		t.Fatalf("getAddr failed: %v", v)
	}
	if v := getAddr("[::1]:1234"); v != "[::1]:1234" {
		t.Fatalf("getAddr failed: %v", v)
	}
	if v := getAddr("::1:1234"); v != "[::1]:1234" {
		t.Fatalf("getAddr failed: %v", v)
	}
	if v := getAddr("fd01:abcd::7d03:6379"); v != "[fd01:abcd::7d03]:6379" {
		t.Fatalf("getAddr failed: %v", v)
	}
	if v := getAddr("[fd01:abcd::7d03]:6379"); v != "[fd01:abcd::7d03]:6379" {
		t.Fatalf("getAddr failed: %v", v)
	}
	if v := getAddr("127.0.0.1"); v != "" {
		t.Fatalf("getAddr failed: %v", v)
	}
	if v := getAddr("127"); v != "" {
		t.Fatalf("getAddr failed: %v", v)
	}
}

@monkey92t
Copy link
Collaborator

There were some errors in the unit tests, but I have already fixed them.

@daviddzxy
Copy link
Contributor Author

daviddzxy commented Apr 26, 2024

I moved the GetAddr function into internal/util.go and unit tests were moved to internal/util_test.go. What do you think?

Copy link
Collaborator

@monkey92t monkey92t left a comment

Choose a reason for hiding this comment

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

It seems that the functionality is implemented correctly, but there are too many unnecessary queries and checks. IPv4 is used in most cases, so we should prioritize IPv4 checks. Additionally, we should use the more efficient method IndexByte() instead of Index().

@monkey92t
Copy link
Collaborator

In the current scenario, we should trust the validity of the data returned by the redis-server. It is known that the redis-server may return three types of data: 127.0.0.1:6379 (IPv4), ::1:6379 (incorrect IPv6), [::1]:6379 (correct IPv6).

@monkey92t
Copy link
Collaborator

func getAddr(addr string) string {
	idx := strings.LastIndexByte(addr, ':')
	if idx == -1 {
		return ""
	}

	// ipv4
	if strings.IndexByte(addr, '.') != -1 {
		return addr
	}

	// ipv6
	if addr[0] == '[' {
		return addr
	}
	return net.JoinHostPort(addr[:idx], addr[idx+1:])
}

@monkey92t
Copy link
Collaborator

Both LastIndex() and Index() are complex functions that search for one string within another string, rather than searching for a single character. This is a completely different approach. In the current situation, we only need to search for a single character, not a string.

@monkey92t monkey92t merged commit b64d9de into redis:master Apr 28, 2024
10 checks passed
@callmeadi
Copy link

callmeadi commented May 9, 2024

When are we releasing a tagged version for this? @daviddzxy @monkey92t

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.

3 participants