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

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jan 23, 2018

  • Better warning on mismatched --initial-cluster flag.
    • etcd compares --initial-advertise-peer-urls against corresponding --initial-cluster URLs with forward-lookup.
    • If resolved IP addresses of --initial-advertise-peer-urls and --initial-cluster do not match (e.g. due to DNS error), etcd will exit with errors.
      • v3.2 error: "--initial-cluster must include s1=https://s1.test:2380 given --initial-advertise-peer-urls=https://s1.test:2380".
      • v3.3 error: "--initial-advertise-peer-urls has https://s1.test:2380 but missing from --initial-cluster=" (DNS returns zero results).
      • v3.3 error should be: failed to resolve https://s1.test:2380 to match --initial-cluster=s1=https://s1.test:2380 (failed to resolve "https://s1.test:2380" (error x)).

For #9180.

ETCDCTL_API=3 etcdctl snapshot restore /var/etcd/latest.backup \
  --name s1 \
  --data-dir /var/etcd/data \
  --initial-cluster-token tkn \
  --initial-cluster s1=https://s1.test:2380 \
  --initial-advertise-peer-urls https://s1.test:2380

Error:  --initial-cluster must include s1=https://s1.test:2380 given --initial-advertise-peer-urls=https://s1.test:2380

@gyuho gyuho requested a review from xiang90 January 23, 2018 23:33
@gyuho
Copy link
Contributor Author

gyuho commented Jan 23, 2018

/cc @hongchaodeng

@codecov-io
Copy link

Codecov Report

Merging #9210 into master will decrease coverage by 0.14%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9210      +/-   ##
==========================================
- Coverage   76.06%   75.91%   -0.15%     
==========================================
  Files         363      363              
  Lines       30161    30165       +4     
==========================================
- Hits        22941    22900      -41     
- Misses       5625     5664      +39     
- Partials     1595     1601       +6
Impacted Files Coverage Δ
etcdserver/config.go 79.61% <66.66%> (-2.21%) ⬇️
proxy/grpcproxy/register.go 69.44% <0%> (-11.12%) ⬇️
clientv3/leasing/cache.go 87.77% <0%> (-3.89%) ⬇️
pkg/testutil/recorder.go 77.77% <0%> (-3.71%) ⬇️
proxy/grpcproxy/watch.go 90.06% <0%> (-3.11%) ⬇️
raft/storage.go 87.96% <0%> (-1.86%) ⬇️
rafthttp/msgappv2_codec.go 71.3% <0%> (-1.74%) ⬇️
etcdserver/api/v3rpc/watch.go 83.69% <0%> (-1.72%) ⬇️
rafthttp/peer.go 90.22% <0%> (-1.51%) ⬇️
etcdserver/cluster_util.go 76.64% <0%> (-1.46%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8025082...2f809e3. Read the comment docs.

@hongchaodeng
Copy link
Contributor

"failed to resolve https://s1.test:2380 to match --initial-cluster=s1=https://s1.test:2380"

It would be more helpful to show why it doesn't match. Because from the message they are the same https://s1.test:2380.
For this case, to show the resolved IPs.

@gyuho gyuho added the WIP label Jan 25, 2018
@gyuho gyuho changed the title etcdserver: clear error on DNS resolution fail on advertise URLs *: add detailed error message on URLs equal check Jan 25, 2018
@gyuho gyuho removed the WIP label Jan 25, 2018
@gyuho
Copy link
Contributor Author

gyuho commented Jan 25, 2018

@hongchaodeng @fanminshi PTAL. Now we provide detailed error messages (which URL failed to be resolved and how).

@@ -121,49 +122,62 @@ func resolveURL(ctx context.Context, u url.URL) (string, error) {
return "", ctx.Err()
}

func sprintURLs(us []url.URL) []string {
Copy link
Member

Choose a reason for hiding this comment

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

maybe change name to urlsToStrings? i think sprintURLs is specific to go's fmt naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -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?

@fanminshi
Copy link
Member

lgtm thanks!

@hongchaodeng
Copy link
Contributor

LGTM. Thanks!

@gyuho gyuho merged commit 0c692fb into etcd-io:master Jan 25, 2018
@gyuho gyuho deleted the dns-error branch January 25, 2018 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants