From b922a286220f2348ca8662dbf8dc7203cde57f66 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 --- pkg/netutil/netutil.go | 42 ++++++++++++++++++++++++------------- pkg/netutil/netutil_test.go | 33 ++++++++++++++++++++++++----- 2 files changed, 55 insertions(+), 20 deletions(-) diff --git a/pkg/netutil/netutil.go b/pkg/netutil/netutil.go index bf737a4d942..cda09003499 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 } if lg == nil { lg, _ = zap.NewProduction() @@ -196,7 +188,15 @@ func URLStringsEqual(ctx context.Context, lg *zap.Logger, a []string, b []string lg = zap.NewExample() } } - return urlsEqual(ctx, lg, urlsA, urlsB) + sort.Sort(types.URLs(urlsA)) + sort.Sort(types.URLs(urlsB)) + for i := range urlsA { + if !reflect.DeepEqual(urlsA[i], urlsB[i]) { + // If urls are not equal, try to resolve it and compare again. + return urlsEqual(ctx, lg, urlsA, urlsB) + } + } + return true, nil } func urlsToStrings(us []url.URL) []string { @@ -207,6 +207,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 42b05ca295a..7d1d17aa269 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) + } + 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}, } - if err != nil { - t.Errorf("unexpected error %v", err) + 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 eac75c28ae2aad5df898058d1d7bb0ceb1944e3d 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 --- pkg/netutil/netutil.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/netutil/netutil.go b/pkg/netutil/netutil.go index cda09003499..5b1e179667f 100644 --- a/pkg/netutil/netutil.go +++ b/pkg/netutil/netutil.go @@ -192,7 +192,7 @@ func URLStringsEqual(ctx context.Context, lg *zap.Logger, a []string, b []string sort.Sort(types.URLs(urlsB)) for i := range urlsA { if !reflect.DeepEqual(urlsA[i], urlsB[i]) { - // If urls are not equal, try to resolve it and compare again. + // If URLs are not equal, try to resolve it and compare again. return urlsEqual(ctx, lg, urlsA, urlsB) } } @@ -212,7 +212,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 b272b98b79a392ac269d5c1577e15b655844ce1a 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 --- pkg/netutil/netutil.go | 31 ++++++++++++++------------ pkg/netutil/netutil_test.go | 44 +++++++++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/pkg/netutil/netutil.go b/pkg/netutil/netutil.go index 5b1e179667f..43c93ba3cc4 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 @@ -188,15 +199,7 @@ func URLStringsEqual(ctx context.Context, lg *zap.Logger, a []string, b []string lg = zap.NewExample() } } - sort.Sort(types.URLs(urlsA)) - sort.Sort(types.URLs(urlsB)) - for i := range urlsA { - if !reflect.DeepEqual(urlsA[i], urlsB[i]) { - // If URLs are not equal, try to resolve it and compare again. - return urlsEqual(ctx, lg, urlsA, urlsB) - } - } - return true, nil + return urlsEqual(ctx, lg, urlsA, urlsB) } func urlsToStrings(us []url.URL) []string { diff --git a/pkg/netutil/netutil_test.go b/pkg/netutil/netutil_test.go index 7d1d17aa269..22db427e0a0 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) } } }