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

*: use machine default host only for default value, 0.0.0.0 #7394

Merged
merged 2 commits into from
Mar 4, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Mar 1, 2017

Fix #7348.

#7027 broke this.
It overwriting the advertise-client-urls' host with 'localhost'.

Signed-off-by: Gyu-Ho Lee [email protected]

Contributing guidelines

Please read our contribution workflow before submitting a pull request.

@codecov-io
Copy link

codecov-io commented Mar 1, 2017

Codecov Report

Merging #7394 into master will decrease coverage by -0.29%.
The diff coverage is 65.51%.

@@            Coverage Diff             @@
##           master    #7394      +/-   ##
==========================================
- Coverage   71.12%   70.83%   -0.29%     
==========================================
  Files         236      236              
  Lines       21093    21093              
==========================================
- Hits        15003    14942      -61     
- Misses       4970     5026      +56     
- Partials     1120     1125       +5
Impacted Files Coverage Δ
etcdmain/etcd.go 45.64% <20%> (-2.51%)
embed/config.go 70.98% <75%> (-0.89%)
mvcc/watchable_store.go 72.72% <0%> (-15.59%)
pkg/fileutil/purge.go 73.68% <0%> (-7.9%)
clientv3/txn.go 96.36% <0%> (-3.64%)
clientv3/watch.go 89.81% <0%> (-3.14%)
rafthttp/msgappv2_codec.go 69.56% <0%> (-1.74%)
clientv3/lease.go 91.11% <0%> (-0.89%)
etcdserver/server.go 79.5% <0%> (+0.11%)
... and 5 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 2831b9d...4aa68e0. Read the comment docs.

embed/config.go Outdated
@@ -381,8 +381,8 @@ func (cfg *Config) UpdateDefaultClusterFromName(defaultInitialCluster string) {
// if client-listen-url is 0.0.0.0, just use detected default host
// otherwise, rewrite advertise-client-url with localhost
if ip != "0.0.0.0" {
_, acPort, _ := net.SplitHostPort(cfg.ACUrls[0].Host)
cfg.ACUrls[0] = url.URL{Scheme: cfg.ACUrls[0].Scheme, Host: fmt.Sprintf("localhost:%s", acPort)}
ahost, acPort, _ := net.SplitHostPort(cfg.ACUrls[0].Host)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong. Why do you parse the host and port and set the exactly same host and port back?

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR (https://github.com/coreos/etcd/pull/7027/files) does the wrong thing. We should not blindly rewrite the advertise url to localhost just because the ip is not 0.0.0.0. In another word, even if cfg.IsDefaultHost() returns true, it does not mean that we have overwritten both client and peer advertise urls. That is why we might overwrite the users configuration.

Please make sure you fully understand what happened before making the change.

@gyuho gyuho added the WIP label Mar 1, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from eb977f4 to a8b8cd9 Compare March 1, 2017 21:23
@gyuho gyuho changed the title embed: use correct host for advertise-client-urls *: not-update advertise client URL if user provided empty value Mar 1, 2017
@gyuho gyuho changed the title *: not-update advertise client URL if user provided empty value WIP *: not-update advertise client URL if user provided empty value Mar 1, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch 2 times, most recently from f681373 to 7ebb72b Compare March 1, 2017 21:50
@gyuho gyuho changed the title WIP *: not-update advertise client URL if user provided empty value *: not-update advertise client URL if user provided empty value Mar 1, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from 7ebb72b to cf5e537 Compare March 1, 2017 22:01
@gyuho gyuho removed the WIP label Mar 1, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Mar 2, 2017

@heyitsanthony @xiang90 PTAL. Now etcd binary does not overwrite advertise client URLs if user provides its own in the flag.

@heyitsanthony
Copy link
Contributor

This seems complicated/invasive for what it does and it doesn't fix the original issue since it doesn't affect the yaml path.

I think the fix is to keep DefaultAdvertise as localhost instead of patching it over in init(), then in UpdateDefaultClusterFromName (or somewhere more reasonable) check if the listen client url / listen peer url is 0.0.0.0 and if the advertise client url / advertise peer url is DefaultAdvertise, if so then set the advertise client / advertise peer to the default host (if available). This will still override if localhost is explicitly set, but it can be worked around by explicitly setting 127.0.0.1 instead.

@gyuho gyuho added the WIP label Mar 2, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from cf5e537 to 73f3e0a Compare March 2, 2017 22:46
@gyuho gyuho changed the title *: not-update advertise client URL if user provided empty value *: use machine default host only for default value, 0.0.0.0 Mar 2, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from 73f3e0a to c21f4c5 Compare March 2, 2017 22:50
@gyuho gyuho removed the WIP label Mar 2, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Mar 2, 2017

@heyitsanthony @xiang90 PTAL.

linux

$ ./bin/etcd

2017-03-02 22:53:10.971691 I | etcdmain: advertising using detected default host "10.240.0.19"
2017-03-02 22:53:10.971874 I | embed: listening for peers on http://localhost:2380
2017-03-02 22:53:10.971943 I | embed: listening for client requests on localhost:2379
2017-03-02 22:53:10.975235 I | etcdserver: advertise client URLs = http://10.240.0.19:2379
2017-03-02 22:53:10.975256 I | etcdserver: initial advertise peer URLs = http://10.240.0.19:2380
2017-03-02 22:53:10.975273 I | etcdserver: initial cluster = default=http://10.240.0.19:2380
$ ./bin/etcd --advertise-client-urls http://hello.world:2379

2017-03-02 22:54:31.774196 I | etcdmain: advertising using detected default host "10.240.0.19"
2017-03-02 22:54:31.774417 I | embed: listening for peers on http://localhost:2380
2017-03-02 22:54:31.774473 I | embed: listening for client requests on localhost:2379
2017-03-02 22:54:31.775641 I | etcdserver: advertise client URLs = http://hello.world:2379
2017-03-02 22:56:07.966405 I | etcdserver: initial advertise peer URLs = http://10.240.0.19:2380
2017-03-02 22:56:07.966417 I | etcdserver: initial cluster = default=http://10.240.0.19:2380

darwin

$ ./bin/etcd

2017-03-02 14:54:58.394632 N | etcdmain: failed to detect default host (default host not supported on darwin_amd64)
2017-03-02 14:54:58.395072 I | embed: listening for peers on http://localhost:2380
2017-03-02 14:54:58.395240 I | embed: listening for client requests on localhost:2379
2017-03-02 14:54:58.401788 I | etcdserver: advertise client URLs = http://localhost:2379
2017-03-02 14:56:35.437344 I | etcdserver: initial advertise peer URLs = http://localhost:2380
2017-03-02 14:56:35.437358 I | etcdserver: initial cluster = default=http://localhost:2380

embed/config.go Outdated

used := false
pip, pport, _ := net.SplitHostPort(cfg.LPUrls[0].Host)
if cfg.defaultPeerHost() || pip == "0.0.0.0" {
Copy link
Contributor

Choose a reason for hiding this comment

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

&&? no given advertise peer and the listen address is 0.0.0.0 => use default host, right?

embed/config.go Outdated
used = true
}
cip, cport, _ := net.SplitHostPort(cfg.LCUrls[0].Host)
if cfg.defaultClientHost() || cip == "0.0.0.0" {
Copy link
Contributor

Choose a reason for hiding this comment

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

&&?

embed/config.go Outdated
cfg.ACUrls[0] = url.URL{Scheme: cfg.ACUrls[0].Scheme, Host: fmt.Sprintf("localhost:%s", acPort)}
cfg.InitialCluster = cfg.InitialClusterFromName(cfg.Name)
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

no naked returns

embed/config.go Outdated
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

no naked returns

func (cfg *Config) UpdateDefaultClusterFromName(defaultInitialCluster string) {
defaultHost, defaultHostErr := cfg.IsDefaultHost()
defaultHostOverride := defaultHost == "" || defaultHostErr == nil
if (defaultHostOverride || cfg.Name != DefaultName) && cfg.InitialCluster == defaultInitialCluster {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened to cfg.Name != DefaultName) && cfg.InitialCluster == defaultInitialCluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misread cfg.InitialCluster as AdvertisePeerURL somehow. Now fixed. PTAL. Thanks!

@gyuho gyuho added the WIP label Mar 2, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch 6 times, most recently from e42a33e to 7f12c74 Compare March 3, 2017 00:45
@gyuho gyuho removed the WIP label Mar 3, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from 7f12c74 to 5cd442a Compare March 3, 2017 01:07
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from 5cd442a to fda9ad3 Compare March 3, 2017 13:22
@heyitsanthony
Copy link
Contributor

sgtm, defer to @xiang90 about behavior for cfg.Name != DefaultName && cfg.InitialCluster == defaultInitialCluster since I don't really understand what the original intention was (only copied it around when doing the embed refactor)

@heyitsanthony
Copy link
Contributor

will this need a backport?

@xiang90
Copy link
Contributor

xiang90 commented Mar 3, 2017

@gyuho Can we add a unit test to cover the bad case?

@xiang90
Copy link
Contributor

xiang90 commented Mar 3, 2017

since I don't really understand what the original intention was (only copied it around when doing the embed refactor)

If user only gives

./etcd --name=abc

, we still want etcd to start.

@xiang90
Copy link
Contributor

xiang90 commented Mar 3, 2017

@gyuho Probably we should document the use case I mentioned above?

@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from fda9ad3 to 6424443 Compare March 3, 2017 21:37
@gyuho gyuho added the WIP label Mar 3, 2017
@gyuho gyuho force-pushed the fix-advertise-client-url-host branch from 6424443 to 4aa68e0 Compare March 3, 2017 22:25
@gyuho gyuho removed the WIP label Mar 3, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Mar 3, 2017

@xiang90 @heyitsanthony Added test cases
and fix to cover ./etcd --name=abc use case.

PTAL.
Thanks!

@xiang90
Copy link
Contributor

xiang90 commented Mar 3, 2017

LGTM

@xiang90
Copy link
Contributor

xiang90 commented Mar 4, 2017

@gyuho merge this?

@gyuho gyuho merged commit b68416f into etcd-io:master Mar 4, 2017
@gyuho gyuho deleted the fix-advertise-client-url-host branch March 4, 2017 00:35
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