-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
embed: warns about empty hosts in advertise urls #8384
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8384 +/- ##
=========================================
Coverage ? 76.97%
=========================================
Files ? 353
Lines ? 28127
Branches ? 0
=========================================
Hits ? 21652
Misses ? 4943
Partials ? 1532
Continue to review full report at Codecov.
|
I tried some empty host url abuse. ./etcd defaults: $ ETCDCTL_API=3 ./bin/etcdctl --endpoints=http://:2379 endpoint status
http://:2379, 8e9e05c52164694d, 3.2.0+git, 16 kB, true, 10, 20 fails because of IP check: $ ./bin/etcd --listen-client-urls=http://:2379 --advertise-client-urls=http://:2379
etcdmain: error verifying flags, expected IP in URL for binding (http://:2379). (fixed by this patch): $ ./bin/etcd --listen-client-urls=http://0.0.0.0:2379 --advertise-client-urls=http://:2379 Does anyone use the empty host => any address behavior? A workaround like this seems tempting: $ ETCDCTL_API=3 ./bin/etcdctl --endpoints=http://0.0.0.0:2379 endpoint status
http://0.0.0.0:2379, 8e9e05c52164694d, 3.2.0+git, 16 kB, true, 12, 24 but that only binds to any ipv4 address instead of any address. On the other hand, empty hosts fail the $ curl http://:2379/
curl: (6) Could not resolve host:
$ curl http://localhost:2379/health
{"health":true}
$ wget http://:2379/health
http://:2379/health: Invalid host name. If etcd is advertising a client address curl can't parse, that must be broken. Maybe someone somewhere somehow relies on this though. Can this be a warning in 3.3 then a full error/exit in 3.4? |
@heyitsanthony PTAL. Now just logs warnings. Thanks. |
embed/config.go
Outdated
@@ -299,6 +299,14 @@ func (cfg *Config) Validate() error { | |||
if err := checkBindURLs(cfg.ListenMetricsUrls); err != nil { | |||
return err | |||
} | |||
if err := checkHostURLs(cfg.APUrls); err != nil { | |||
// TODO: return err in v3.4 | |||
plog.Warning(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these say something like "advertise-peer-urls %q is deprecated (%v)" if the intent is to eventually exit out? warning with only the error doesn't seem clear enough
4a0fea8
to
6ad85d3
Compare
Signed-off-by: Gyu-Ho Lee <[email protected]>
@heyitsanthony PTAL. Now
Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks
Address #8379.