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.

Sign up for GitHub

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

Jump to bottom

embed: return error when advertise-client-urls are with empty hosts #8786

Merged
merged 1 commit into from
Jan 26, 2018
Merged

embed: return error when advertise-client-urls are with empty hosts #8786

merged 1 commit into from
Jan 26, 2018

Conversation

harryge00
Copy link
Contributor

@harryge00 harryge00 commented Oct 28, 2017 •
edited
Loading

solves #8379
Not sure if this is what wanted. 😅

@xiang90
Copy link
Contributor

xiang90 commented Dec 21, 2017

/cc @jpbetz @gyuho can you take a look?

gyuho
gyuho reviewed Dec 21, 2017
View reviewed changes
embed/config.go Outdated
@@ -343,6 +343,7 @@ func (cfg *Config) Validate() error {
addrs[i] = cfg.ACUrls[i].String()
}
plog.Warningf("advertise-client-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

@harryge00 Can we remove the line 340 as well?

Copy link
Contributor

@gyuho gyuho Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also make it as an error value

return fmt.Errorf("advertise-client-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)

It will be logged when etcd exits anyway

https://github.com/coreos/etcd/blob/3dd1c1b53c65182775098c3d005c7e28d02d3979/etcdmain/etcd.go#L63-L65

@xiang90
Copy link
Contributor

xiang90 commented Dec 23, 2017

not sure how this solves the empty host issue.

@xiang90
Copy link
Contributor

xiang90 commented Dec 23, 2017

oh. ok. i see. we are returning the error from checkhost now.

xiang90
xiang90 reviewed Dec 23, 2017
View reviewed changes
embed/config.go Outdated
@@ -343,6 +343,7 @@ func (cfg *Config) Validate() error {
addrs[i] = cfg.ACUrls[i].String()
}
plog.Warningf("advertise-client-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the warning? or change it to errorf?

Copy link
Contributor

Choose a reason for hiding this comment

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

same at line 337.

@xiang90
Copy link
Contributor

xiang90 commented Jan 3, 2018

@harryge00 can you help to resolve the comments? so that we can get this patch merged in?

@harryge00 harryge00 force-pushed the check-empty-hostname branch from 4a8def7 to fab17fc Compare January 25, 2018 11:34
harryge00
harryge00 commented Jan 25, 2018
View reviewed changes
embed/config.go Outdated
addrs := make([]string, len(cfg.APUrls))
for i := range cfg.APUrls {
addrs[i] = cfg.APUrls[i].String()
}
plog.Warningf("advertise-peer-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
return fmt.Errorf("advertise-peer-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now only --initial-advertise-peer-urls available ?

@harryge00
Copy link
Contributor Author

@xiang90 @gyuho I am very sorry for the late reply. Code udpated

@gyuho
Copy link
Contributor

gyuho commented Jan 25, 2018

Have you tried this branch with etcd --listen-client-urls=http://127.0.0.1:2379 --advertise-client-urls=http://:2379 command?

gyuho
gyuho reviewed Jan 25, 2018
View reviewed changes
embed/config.go Outdated
addrs := make([]string, len(cfg.APUrls))
for i := range cfg.APUrls {
addrs[i] = cfg.APUrls[i].String()
}
plog.Warningf("advertise-peer-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
return fmt.Errorf("advertise-peer-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
Copy link
Contributor

@gyuho gyuho Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

return fmt.Errorf(`--initial-advertise-peer-urls %q must be "host:port" (%v)`, strings.Join(addrs, ","), err)

gyuho
gyuho reviewed Jan 25, 2018
View reviewed changes
embed/config.go Outdated
addrs := make([]string, len(cfg.ACUrls))
for i := range cfg.ACUrls {
addrs[i] = cfg.ACUrls[i].String()
}
plog.Warningf("advertise-client-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
return fmt.Errorf("advertise-client-urls %q is deprecated (%v)", strings.Join(addrs, ","), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

return fmt.Errorf(`--advertise-client-urls %q must be "host:port" (%v)`, strings.Join(addrs, ","), err)

@harryge00 harryge00 force-pushed the check-empty-hostname branch from fab17fc to 6912a8e Compare January 26, 2018 03:10
@harryge00
Copy link
Contributor Author

harryge00 commented Jan 26, 2018 •
edited
Loading

Have you tried this branch with etcd --listen-client-urls=http://127.0.0.1:2379 --advertise-client-urls=http://:2379 command?

Yes @gyuho

# ./bin/etcd --listen-client-urls=http://127.0.0.1:2379 --advertise-client-urls=http://:2379
2018-01-26 11:10:44.995781 E | etcdmain: error verifying flags, --advertise-client-urls "http://:2379" must be "host:port" (unexpected empty host (http://:2379)). See 'etcd --help'.
# ./bin/etcd --listen-client-urls=http://localhost:2379 --initial-advertise-peer-urls=http://:2379
2018-01-26 11:11:27.591990 E | etcdmain: error verifying flags, --initial-advertise-peer-urls "http://:2379" must be "host:port" (unexpected empty host (http://:2379)). See 'etcd --help'.

# ./bin/etcd --listen-client-urls=http://127.0.0.1:2379 --advertise-client-urls="http://localhost:2379, http://:2379"
2018-01-26 11:13:09.070098 E | etcdmain: error verifying flags, --advertise-client-urls "http://:2379,http://localhost:2379" must be "host:port" (unexpected empty host (http://:2379)). See 'etcd --help'.
# ./bin/etcd --listen-client-urls=http://localhost:2379 --initial-advertise-peer-urls="http://localhost:2379,http://:2379"
2018-01-26 11:13:50.438739 E | etcdmain: error verifying flags, --initial-advertise-peer-urls "http://:2379,http://localhost:2379" must be "host:port" (unexpected empty host (http://:2379)). See 'etcd --help'.

gyuho
gyuho approved these changes Jan 26, 2018
View reviewed changes
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm. thanks!

@gyuho gyuho merged commit b26b858 into etcd-io:master Jan 26, 2018
@xiang90 xiang90 mentioned this pull request Jan 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiang90 xiang90 xiang90 left review comments

@gyuho gyuho gyuho approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@harryge00 @xiang90 @gyuho