From 25ef9b6f46cfe4a989a2c4060ebdc6fe42f92c89 Mon Sep 17 00:00:00 2001 From: Sergey Kacheev Date: Mon, 19 Jul 2021 01:31:21 +0700 Subject: [PATCH 1/3] netutil: add url comparison without resolver to URLStringsEqual If one of the nodes in the cluster has lost a dns record, restarting the second node will break it. This PR makes an attempt to add a comparison without using a resolver, which allows to protect cluster from dns errors and does not break the current logic of comparing urls in the URLStringsEqual function. You can read more in the issue #7798 Fixes #7798 Signed-off-by: Prasad Chandrasekaran --- pkg/netutil/netutil.go | 32 ++++++++++++++++++-------------- pkg/netutil/netutil_test.go | 33 ++++++++++++++++++++++++++++----- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/pkg/netutil/netutil.go b/pkg/netutil/netutil.go index bd8c1fc93..0e8f6c666 100644 --- a/pkg/netutil/netutil.go +++ b/pkg/netutil/netutil.go @@ -174,21 +174,13 @@ func URLStringsEqual(ctx context.Context, lg *zap.Logger, a []string, b []string if len(a) != len(b) { 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, fmt.Errorf("failed to parse %q", str) - } - urlsA = append(urlsA, *u) + urlsA, err := stringsToURLs(a) + if err != nil { + return false, err } - urlsB := make([]url.URL, 0) - for _, str := range b { - u, err := url.Parse(str) - if err != nil { - return false, fmt.Errorf("failed to parse %q", str) - } - urlsB = append(urlsB, *u) + urlsB, err := stringsToURLs(b) + if err != nil { + return false, err } return urlsEqual(ctx, lg, urlsA, urlsB) } @@ -201,6 +193,18 @@ func urlsToStrings(us []url.URL) []string { return rs } +func stringsToURLs(us []string) ([]url.URL, error) { + urls := make([]url.URL, 0, len(us)) + for _, str := range us { + u, err := url.Parse(str) + if err != nil { + return nil, fmt.Errorf("failed to parse %q", str) + } + urls = append(urls, *u) + } + return urls, nil +} + func IsNetworkTimeoutError(err error) bool { nerr, ok := err.(net.Error) return ok && nerr.Timeout() diff --git a/pkg/netutil/netutil_test.go b/pkg/netutil/netutil_test.go index 42b05ca29..7d1d17aa2 100644 --- a/pkg/netutil/netutil_test.go +++ b/pkg/netutil/netutil_test.go @@ -17,6 +17,7 @@ package netutil import ( "context" "errors" + "fmt" "net" "net/url" "reflect" @@ -292,11 +293,33 @@ func TestURLsEqual(t *testing.T) { } } func TestURLStringsEqual(t *testing.T) { - result, err := URLStringsEqual(context.TODO(), zap.NewExample(), []string{"http://127.0.0.1:8080"}, []string{"http://127.0.0.1:8080"}) - if !result { - t.Errorf("unexpected result %v", result) + defer func() { resolveTCPAddr = resolveTCPAddrDefault }() + errOnResolve := func(ctx context.Context, addr string) (*net.TCPAddr, error) { + return nil, fmt.Errorf("unexpected attempt to resolve: %q", addr) } - if err != nil { - t.Errorf("unexpected error %v", err) + cases := []struct { + urlsA []string + urlsB []string + resolver func(ctx context.Context, addr string) (*net.TCPAddr, error) + }{ + {[]string{"http://127.0.0.1:8080"}, []string{"http://127.0.0.1:8080"}, resolveTCPAddrDefault}, + {[]string{ + "http://host1:8080", + "http://host2:8080", + }, []string{ + "http://host1:8080", + "http://host2:8080", + }, errOnResolve}, + } + for idx, c := range cases { + t.Logf("TestURLStringsEqual, case #%d", idx) + resolveTCPAddr = c.resolver + result, err := URLStringsEqual(context.TODO(), zap.NewExample(), c.urlsA, c.urlsB) + if !result { + t.Errorf("unexpected result %v", result) + } + if err != nil { + t.Errorf("unexpected error %v", err) + } } } From 3e195ba4731360d26a3ddef9b9d7b8b0e045c46e Mon Sep 17 00:00:00 2001 From: Sergey Kacheev Date: Sun, 1 Aug 2021 00:20:50 +0700 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Lili Cosic Signed-off-by: Prasad Chandrasekaran --- pkg/netutil/netutil.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/netutil/netutil.go b/pkg/netutil/netutil.go index 0e8f6c666..2e7c578db 100644 --- a/pkg/netutil/netutil.go +++ b/pkg/netutil/netutil.go @@ -198,7 +198,7 @@ func stringsToURLs(us []string) ([]url.URL, error) { for _, str := range us { u, err := url.Parse(str) if err != nil { - return nil, fmt.Errorf("failed to parse %q", str) + return nil, fmt.Errorf("failed to parse string to URL: %q", str) } urls = append(urls, *u) } From e712234a1a4b3896c31e444be7b03867634e646c Mon Sep 17 00:00:00 2001 From: Sergey Kacheev Date: Sun, 26 Sep 2021 13:50:10 +0700 Subject: [PATCH 3/3] netutil: make a `raw` URL comparison part of the urlsEqual function Signed-off-by: Prasad Chandrasekaran --- pkg/netutil/netutil.go | 21 +++++++++++++----- pkg/netutil/netutil_test.go | 44 +++++++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/pkg/netutil/netutil.go b/pkg/netutil/netutil.go index 2e7c578db..2f3558995 100644 --- a/pkg/netutil/netutil.go +++ b/pkg/netutil/netutil.go @@ -148,20 +148,31 @@ func urlsEqual(ctx context.Context, lg *zap.Logger, a []url.URL, b []url.URL) (b if len(a) != len(b) { return false, fmt.Errorf("len(%q) != len(%q)", urlsToStrings(a), urlsToStrings(b)) } + + sort.Sort(types.URLs(a)) + sort.Sort(types.URLs(b)) + var needResolve bool + for i := range a { + if !reflect.DeepEqual(a[i], b[i]) { + needResolve = true + break + } + } + if !needResolve { + return true, nil + } + + // If URLs are not equal, try to resolve it and compare again. urls, err := resolveTCPAddrs(ctx, lg, [][]url.URL{a, b}) if err != nil { 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, fmt.Errorf("%q(resolved from %q) != %q(resolved from %q)", - a[i].String(), preva[i].String(), - b[i].String(), prevb[i].String(), - ) + return false, fmt.Errorf("resolved urls: %q != %q", a[i].String(), b[i].String()) } } return true, nil diff --git a/pkg/netutil/netutil_test.go b/pkg/netutil/netutil_test.go index 7d1d17aa2..22db427e0 100644 --- a/pkg/netutil/netutil_test.go +++ b/pkg/netutil/netutil_test.go @@ -167,113 +167,133 @@ func TestURLsEqual(t *testing.T) { } tests := []struct { + n int a []url.URL b []url.URL expect bool err error }{ { + n: 0, a: []url.URL{{Scheme: "http", Host: "127.0.0.1:2379"}}, b: []url.URL{{Scheme: "http", Host: "127.0.0.1:2379"}}, expect: true, }, { + n: 1, a: []url.URL{{Scheme: "http", Host: "example.com:2379"}}, b: []url.URL{{Scheme: "http", Host: "10.0.10.1:2379"}}, expect: true, }, { + n: 2, 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")`), + err: errors.New(`resolved urls: "http://10.0.10.1:2379" != "https://10.0.10.1:2379"`), }, { + n: 3, 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")`), + err: errors.New(`resolved urls: "https://10.0.10.1:2379" != "http://10.0.10.1:2379"`), }, { + n: 4, a: []url.URL{{Scheme: "unix", Host: "abc:2379"}}, b: []url.URL{{Scheme: "unix", Host: "abc:2379"}}, expect: true, }, { + n: 5, 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:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}}, expect: true, }, { + n: 6, a: []url.URL{{Scheme: "http", Host: "example.com:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}}, b: []url.URL{{Scheme: "http", Host: "example.com:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}}, expect: true, }, { + n: 7, a: []url.URL{{Scheme: "http", Host: "10.0.10.1:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}}, b: []url.URL{{Scheme: "http", Host: "example.com:2379"}, {Scheme: "http", Host: "127.0.0.1:2380"}}, expect: true, }, { + n: 8, 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")`), + err: errors.New(`resolved urls: "http://127.0.0.1:2379" != "http://127.0.0.1:2380"`), }, { + n: 9, 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")`), + err: errors.New(`resolved urls: "http://10.0.10.1:2380" != "http://10.0.10.1:2379"`), }, { + n: 10, 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")`), + err: errors.New(`resolved urls: "http://127.0.0.1:2379" != "http://10.0.0.1:2379"`), }, { + n: 11, 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")`), + err: errors.New(`resolved urls: "http://10.0.10.1:2379" != "http://10.0.0.1:2379"`), }, { + n: 12, 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")`), + err: errors.New(`resolved urls: "http://127.0.0.1:2379" != "http://127.0.0.1:2380"`), }, { + n: 13, 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")`), + err: errors.New(`resolved urls: "http://10.0.10.1:2379" != "http://127.0.0.1:2380"`), }, { + n: 14, 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")`), + err: errors.New(`resolved urls: "http://127.0.0.1:2379" != "http://10.0.0.1:2379"`), }, { + n: 15, 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")`), + err: errors.New(`resolved urls: "http://10.0.10.1:2379" != "http://10.0.0.1:2379"`), }, { + n: 16, 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"])`), }, { + n: 17, a: []url.URL{{Scheme: "http", Host: "first.com:2379"}, {Scheme: "http", Host: "second.com:2380"}}, b: []url.URL{{Scheme: "http", Host: "10.0.11.1:2379"}, {Scheme: "http", Host: "10.0.11.2:2380"}}, expect: true, }, { + n: 18, a: []url.URL{{Scheme: "http", Host: "second.com:2380"}, {Scheme: "http", Host: "first.com:2379"}}, b: []url.URL{{Scheme: "http", Host: "10.0.11.1:2379"}, {Scheme: "http", Host: "10.0.11.2:2380"}}, expect: true, @@ -283,11 +303,11 @@ func TestURLsEqual(t *testing.T) { for i, test := range tests { result, err := urlsEqual(context.TODO(), zap.NewExample(), test.a, test.b) if result != test.expect { - t.Errorf("#%d: a:%v b:%v, expected %v but %v", i, test.a, test.b, test.expect, result) + t.Errorf("idx=%d #%d: a:%v b:%v, expected %v but %v", i, test.n, 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) + t.Errorf("idx=%d #%d: err expected %v but %v", i, test.n, test.err, err) } } }