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

*: add detailed error message on URLs equal check #9210

Merged
merged 3 commits into from
Jan 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions etcdserver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ func (c *ServerConfig) advertiseMatchesCluster() error {
sort.Strings(apurls)
ctx, cancel := context.WithTimeout(context.TODO(), 30*time.Second)
defer cancel()
if netutil.URLStringsEqual(ctx, apurls, urls.StringSlice()) {
ok, err := netutil.URLStringsEqual(ctx, apurls, urls.StringSlice())
if ok {
return nil
}

Expand All @@ -148,17 +149,24 @@ func (c *ServerConfig) advertiseMatchesCluster() error {
}
mstr := strings.Join(missing, ",")
apStr := strings.Join(apurls, ",")
return fmt.Errorf("--initial-cluster has %s but missing from --initial-advertise-peer-urls=%s ", mstr, apStr)
return fmt.Errorf("--initial-cluster has %s but missing from --initial-advertise-peer-urls=%s (%v)", mstr, apStr, err)
}

for url := range apMap {
if _, ok := initMap[url]; !ok {
missing = append(missing, url)
}
}
mstr := strings.Join(missing, ",")
if len(missing) > 0 {
mstr := strings.Join(missing, ",")
umap := types.URLsMap(map[string]types.URLs{c.Name: c.PeerURLs})
return fmt.Errorf("--initial-advertise-peer-urls has %s but missing from --initial-cluster=%s", mstr, umap.String())
}

// resolved URLs from "--initial-advertise-peer-urls" and "--initial-cluster" did not match or failed
apStr := strings.Join(apurls, ",")
umap := types.URLsMap(map[string]types.URLs{c.Name: c.PeerURLs})
return fmt.Errorf("--initial-advertise-peer-urls has %s but missing from --initial-cluster=%s", mstr, umap.String())
return fmt.Errorf("failed to resolve %s to match --initial-cluster=%s (%v)", apStr, umap.String(), err)
}

func (c *ServerConfig) MemberDir() string { return filepath.Join(c.DataDir, "member") }
Expand Down
4 changes: 2 additions & 2 deletions etcdserver/membership/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,8 @@ func ValidateClusterAndAssignIDs(local *RaftCluster, existing *RaftCluster) erro
ctx, cancel := context.WithTimeout(context.TODO(), 30*time.Second)
defer cancel()
for i := range ems {
if !netutil.URLStringsEqual(ctx, ems[i].PeerURLs, lms[i].PeerURLs) {
return fmt.Errorf("unmatched member while checking PeerURLs")
if ok, err := netutil.URLStringsEqual(ctx, ems[i].PeerURLs, lms[i].PeerURLs); !ok {
return fmt.Errorf("unmatched member while checking PeerURLs (%v)", err)
}
lms[i].ID = ems[i].ID
}
Expand Down
40 changes: 27 additions & 13 deletions pkg/netutil/netutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package netutil

import (
"context"
"fmt"
"net"
"net/url"
"reflect"
Expand Down Expand Up @@ -73,14 +74,14 @@ func resolveTCPAddrs(ctx context.Context, urls [][]url.URL) ([][]url.URL, error)
for i, u := range us {
nu, err := url.Parse(u.String())
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to parse %q (%v)", u.String(), err)
}
nus[i] = *nu
}
for i, u := range nus {
h, err := resolveURL(ctx, u)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to resolve %q (%v)", u.String(), err)
}
if h != "" {
nus[i].Host = h
Expand Down Expand Up @@ -123,50 +124,63 @@ func resolveURL(ctx context.Context, u url.URL) (string, error) {

// urlsEqual checks equality of url.URLS between two arrays.
// This check pass even if an URL is in hostname and opposite is in IP address.
func urlsEqual(ctx context.Context, a []url.URL, b []url.URL) bool {
func urlsEqual(ctx context.Context, a []url.URL, b []url.URL) (bool, error) {
if len(a) != len(b) {
return false
return false, fmt.Errorf("len(%q) != len(%q)", urlsToStrings(a), urlsToStrings(b))
}
urls, err := resolveTCPAddrs(ctx, [][]url.URL{a, b})
if err != nil {
return false
return false, err
}
preva, prevb := a, b
a, b = urls[0], urls[1]
sort.Sort(types.URLs(a))
sort.Sort(types.URLs(b))
for i := range a {
if !reflect.DeepEqual(a[i], b[i]) {
return false
return false, fmt.Errorf("%q(resolved from %q) != %q(resolved from %q)",
a[i].String(), preva[i].String(),
b[i].String(), prevb[i].String(),
)
}
}

return true
return true, nil
}

func URLStringsEqual(ctx context.Context, a []string, b []string) bool {
// URLStringsEqual returns "true" if given URLs are valid
// and resolved to same IP addresses. Otherwise, return "false"
// and error, if any.
func URLStringsEqual(ctx context.Context, a []string, b []string) (bool, error) {
if len(a) != len(b) {
return false
return false, fmt.Errorf("len(%q) != len(%q)", a, b)
}
urlsA := make([]url.URL, 0)
for _, str := range a {
u, err := url.Parse(str)
if err != nil {
return false
return false, fmt.Errorf("failed to parse %q", str)
}
urlsA = append(urlsA, *u)
}
urlsB := make([]url.URL, 0)
for _, str := range b {
u, err := url.Parse(str)
if err != nil {
return false
return false, fmt.Errorf("failed to parse %q", str)
}
urlsB = append(urlsB, *u)
}

return urlsEqual(ctx, urlsA, urlsB)
}

func urlsToStrings(us []url.URL) []string {
rs := make([]string, len(us))
for i := range us {
rs[i] = us[i].String()
}
return rs
}

func IsNetworkTimeoutError(err error) bool {
nerr, ok := err.(net.Error)
return ok && nerr.Timeout()
Expand Down
28 changes: 24 additions & 4 deletions pkg/netutil/netutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ func TestURLsEqual(t *testing.T) {
a []url.URL
b []url.URL
expect bool
err error
}{
{
a: []url.URL{{Scheme: "http", Host: "127.0.0.1:2379"}},
Expand All @@ -182,11 +183,13 @@ func TestURLsEqual(t *testing.T) {
a: []url.URL{{Scheme: "http", Host: "example.com:2379"}},
b: []url.URL{{Scheme: "https", Host: "10.0.10.1:2379"}},
expect: false,
err: errors.New(`"http://10.0.10.1:2379"(resolved from "http://example.com:2379") != "https://10.0.10.1:2379"(resolved from "https://10.0.10.1:2379")`),
Copy link
Member

Choose a reason for hiding this comment

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

hmm, http://10.0.10.1:2379 from http://example.com:2379 do == to https://10.0.10.1:2379 (resolved from "https://10.0.10.1:2379") ?

I am not sure why this test fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scheme is different :)

Copy link
Member

Choose a reason for hiding this comment

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

oh, i see now. sorry i didn't notice the scheme change.

},
{
a: []url.URL{{Scheme: "https", Host: "example.com:2379"}},
b: []url.URL{{Scheme: "http", Host: "10.0.10.1:2379"}},
expect: false,
err: errors.New(`"https://10.0.10.1:2379"(resolved from "https://example.com:2379") != "http://10.0.10.1:2379"(resolved from "http://10.0.10.1:2379")`),
Copy link
Member

Choose a reason for hiding this comment

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

same here https://10.0.10.1:2379 == http://10.0.10.1:2379?

},
{
a: []url.URL{{Scheme: "unix", Host: "abc:2379"}},
Expand All @@ -212,46 +215,55 @@ func TestURLsEqual(t *testing.T) {
a: []url.URL{{Scheme: "http", Host: "127.0.0.1:2379"}},
b: []url.URL{{Scheme: "http", Host: "127.0.0.1:2380"}},
expect: false,
err: errors.New(`"http://127.0.0.1:2379"(resolved from "http://127.0.0.1:2379") != "http://127.0.0.1:2380"(resolved from "http://127.0.0.1:2380")`),
},
{
a: []url.URL{{Scheme: "http", Host: "example.com:2380"}},
b: []url.URL{{Scheme: "http", Host: "10.0.10.1:2379"}},
expect: false,
err: errors.New(`"http://10.0.10.1:2380"(resolved from "http://example.com:2380") != "http://10.0.10.1:2379"(resolved from "http://10.0.10.1:2379")`),
},
{
a: []url.URL{{Scheme: "http", Host: "127.0.0.1:2379"}},
b: []url.URL{{Scheme: "http", Host: "10.0.0.1:2379"}},
expect: false,
err: errors.New(`"http://127.0.0.1:2379"(resolved from "http://127.0.0.1:2379") != "http://10.0.0.1:2379"(resolved from "http://10.0.0.1:2379")`),
},
{
a: []url.URL{{Scheme: "http", Host: "example.com:2379"}},
b: []url.URL{{Scheme: "http", Host: "10.0.0.1:2379"}},
expect: false,
err: errors.New(`"http://10.0.10.1:2379"(resolved from "http://example.com:2379") != "http://10.0.0.1:2379"(resolved from "http://10.0.0.1:2379")`),
},
{
a: []url.URL{{Scheme: "http", Host: "127.0.0.1:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
b: []url.URL{{Scheme: "http", Host: "127.0.0.1:2380"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
expect: false,
err: errors.New(`"http://127.0.0.1:2379"(resolved from "http://127.0.0.1:2379") != "http://127.0.0.1:2380"(resolved from "http://127.0.0.1:2380")`),
},
{
a: []url.URL{{Scheme: "http", Host: "example.com:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
b: []url.URL{{Scheme: "http", Host: "127.0.0.1:2380"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
expect: false,
err: errors.New(`"http://10.0.10.1:2379"(resolved from "http://example.com:2379") != "http://127.0.0.1:2380"(resolved from "http://127.0.0.1:2380")`),
},
{
a: []url.URL{{Scheme: "http", Host: "127.0.0.1:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
b: []url.URL{{Scheme: "http", Host: "10.0.0.1:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
expect: false,
err: errors.New(`"http://127.0.0.1:2379"(resolved from "http://127.0.0.1:2379") != "http://10.0.0.1:2379"(resolved from "http://10.0.0.1:2379")`),
},
{
a: []url.URL{{Scheme: "http", Host: "example.com:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
b: []url.URL{{Scheme: "http", Host: "10.0.0.1:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
expect: false,
err: errors.New(`"http://10.0.10.1:2379"(resolved from "http://example.com:2379") != "http://10.0.0.1:2379"(resolved from "http://10.0.0.1:2379")`),
},
{
a: []url.URL{{Scheme: "http", Host: "10.0.0.1:2379"}},
b: []url.URL{{Scheme: "http", Host: "10.0.0.1:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}},
expect: false,
err: errors.New(`len(["http://10.0.0.1:2379"]) != len(["http://10.0.0.1:2379" "http://127.0.0.1:2380"])`),
},
{
a: []url.URL{{Scheme: "http", Host: "first.com:2379"}, {Scheme: "http", Host: "second.com:2380"}},
Expand All @@ -265,16 +277,24 @@ func TestURLsEqual(t *testing.T) {
},
}

for _, test := range tests {
result := urlsEqual(context.TODO(), test.a, test.b)
for i, test := range tests {
result, err := urlsEqual(context.TODO(), test.a, test.b)
if result != test.expect {
t.Errorf("a:%v b:%v, expected %v but %v", test.a, test.b, test.expect, result)
t.Errorf("#%d: a:%v b:%v, expected %v but %v", i, test.a, test.b, test.expect, result)
}
if test.err != nil {
if err.Error() != test.err.Error() {
t.Errorf("#%d: err expected %v but %v", i, test.err, err)
}
}
}
}
func TestURLStringsEqual(t *testing.T) {
result := URLStringsEqual(context.TODO(), []string{"http://127.0.0.1:8080"}, []string{"http://127.0.0.1:8080"})
result, err := URLStringsEqual(context.TODO(), []string{"http://127.0.0.1:8080"}, []string{"http://127.0.0.1:8080"})
if !result {
t.Errorf("unexpected result %v", result)
}
if err != nil {
t.Errorf("unexpected error %v", err)
}
}