From 3f90394fbb45e1c5c2aa425c0d8bde3115a132ae Mon Sep 17 00:00:00 2001
From: Yicheng Qin <qycqycqycqycqyc@gmail.com>
Date: Sun, 26 Apr 2015 08:15:19 -0700
Subject: [PATCH] etcdmain: advertise-client-urls must be set if
 listen-client-urls is set

Before this PR, people can set listen-client-urls without setting
advertise-client-urls, and leaves advertise-client-urls as default
localhost value. The client libraries which sync the cluster info
fetch wrong advertise-client-urls and cannot connect to the cluster.
This PR avoids this case and provides better UX.

On the other hand, this change is safe because people always want to set
advertise-client-urls if listen-client-urls is set. The default localhost
advertise url cannot be accessed from the outside, and should always be
set except that etcd is bootstrapped with no flag.
---
 etcdmain/config.go      | 4 ++++
 etcdmain/config_test.go | 2 ++
 etcdmain/etcd.go        | 7 ++++++-
 etcdmain/help.go        | 3 ++-
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/etcdmain/config.go b/etcdmain/config.go
index 3b71aa926..047920e08 100644
--- a/etcdmain/config.go
+++ b/etcdmain/config.go
@@ -61,6 +61,7 @@ var (
 
 	ErrConflictBootstrapFlags = fmt.Errorf("multiple discovery or bootstrap flags are set" +
 		"Choose one of \"initial-cluster\", \"discovery\" or \"discovery-srv\"")
+	errUnsetAdvertiseClientURLsFlag = fmt.Errorf("-advertise-client-urls is required when -listen-client-urls is set explicitly")
 )
 
 type config struct {
@@ -264,6 +265,9 @@ func (cfg *config) Parse(arguments []string) error {
 	if err != nil {
 		return err
 	}
+	if flags.IsSet(cfg.FlagSet, "listen-client-urls") && !flags.IsSet(cfg.FlagSet, "advertise-client-urls") {
+		return errUnsetAdvertiseClientURLsFlag
+	}
 
 	if 5*cfg.TickMs > cfg.ElectionMs {
 		return fmt.Errorf("-election-timeout[%vms] should be at least as 5 times as -heartbeat-interval[%vms]", cfg.ElectionMs, cfg.TickMs)
diff --git a/etcdmain/config_test.go b/etcdmain/config_test.go
index b729b2de0..090f412da 100644
--- a/etcdmain/config_test.go
+++ b/etcdmain/config_test.go
@@ -29,6 +29,8 @@ func TestConfigParsingMemberFlags(t *testing.T) {
 		"-snapshot-count=10",
 		"-listen-peer-urls=http://localhost:8000,https://localhost:8001",
 		"-listen-client-urls=http://localhost:7000,https://localhost:7001",
+		// it should be set if -listen-client-urls is set
+		"-advertise-client-urls=http://localhost:7000,https://localhost:7001",
 	}
 	wcfg := &config{
 		dir:          "testdir",
diff --git a/etcdmain/etcd.go b/etcdmain/etcd.go
index 26fde6c8b..7e00006aa 100644
--- a/etcdmain/etcd.go
+++ b/etcdmain/etcd.go
@@ -60,7 +60,12 @@ func Main() {
 	cfg := NewConfig()
 	err := cfg.Parse(os.Args[1:])
 	if err != nil {
-		log.Fatalf("error verifying flags, %v. See 'etcd -help'.", err)
+		log.Printf("error verifying flags, %v. See 'etcd -help'.", err)
+		switch err {
+		case errUnsetAdvertiseClientURLsFlag:
+			log.Printf("When listening on specific address(es), this etcd process must advertise accessible url(s) to each connected client.")
+		}
+		os.Exit(1)
 	}
 	setupLogging(cfg)
 
diff --git a/etcdmain/help.go b/etcdmain/help.go
index 0676f2d49..5e0aa863d 100644
--- a/etcdmain/help.go
+++ b/etcdmain/help.go
@@ -56,7 +56,8 @@ clustering flags:
 	--initial-cluster-token 'etcd-cluster'
 		initial cluster token for the etcd cluster during bootstrap.
 	--advertise-client-urls 'http://localhost:2379,http://localhost:4001'
-		list of this member's client URLs to advertise to the rest of the cluster.
+		list of this member's client URLs to advertise to the public.
+		The client URLs advertised should be accessible to machines that talk to etcd cluster. etcd client libraries parse these URLs to connect to the cluster.
 	--discovery ''
 		discovery URL used to bootstrap the cluster.
 	--discovery-fallback 'proxy'