embed: only override default advertised client URL if the client listen URL is 0.0.0.0 #7027
Conversation
| return "", err | ||
| } | ||
| if ip == "0.0.0.0" { | ||
| cfg.APUrls[0] = url.URL{Scheme: cfg.APUrls[0].Scheme, Host: fmt.Sprintf("%s:%s", h, port)} |
There was a problem hiding this comment.
This function is misleading now; usually Is functions are idempotent. Here, cfg.IsDefaultHost() won't necessarily return the same value on subsequent calls. Probably should just have a separate function do to the advertise IP override.
| return defaultHostname, defaultHostStatus | ||
| func (cfg *Config) IsDefaultHost() (string, error) { | ||
| h, herr := netutil.GetDefaultHost() | ||
| if len(cfg.APUrls) == 1 { |
There was a problem hiding this comment.
The 0.0.0.0 check should be on the listen url. If 0.0.0.0, then the advertise urls need to be checked if they match the default url before overriding.
gyuho
changed the title
gyuho
force-pushed
the
default-host
branch
2 times, most recently
from
746c018
to
9cf5b31
Compare
| } | ||
| return "", defaultHostStatus | ||
| return false |
There was a problem hiding this comment.
this changes the meaning of the function though? This function was meant to say whether it's using the default hosts / user didn't specify, but now it's returning true if it's overriding the hosts.
There was a problem hiding this comment.
You are right. How about NeedDefaultHost(), which means it returns true if it needs to detect machine's default host?
There was a problem hiding this comment.
I don't think that will work-- IsDefaultHost means that it's using defaults (which may be overridden with the default route IP), not that it's using the default route IP. That will change the meaning of the InitialClusterFromName path, which needs to know whether the user set a host explicitly.
| cfg.InitialCluster = cfg.InitialClusterFromName(cfg.Name) | ||
| needDefaultHost := cfg.IsDefaultHost() | ||
| if (needDefaultHost || cfg.Name != embed.DefaultName) && cfg.InitialCluster == defaultInitialCluster { | ||
| defaultHost, dhErr := netutil.GetDefaultHost() |
There was a problem hiding this comment.
this overriding stuff should be a function in embed.Config so it can be reused elsewhere
gyuho
changed the title
gyuho
changed the title
gyuho
force-pushed
the
default-host
branch
2 times, most recently
from
c1cc6ad
to
65ca569
Compare
| @@ -89,6 +89,11 @@ func startEtcdOrProxyV2() { | |||
| defaultHostOverride := defaultHost == "" || defaultHostErr == nil | |||
| if (defaultHostOverride || cfg.Name != embed.DefaultName) && cfg.InitialCluster == defaultInitialCluster { | |||
There was a problem hiding this comment.
this path needs a lot of special support from Config and seems self-contained, can everything starting from IsDefaultHost() to the end of this if be func (cfg *Config) UpdateDefaultClusterFromName() instead?
There was a problem hiding this comment.
@heyitsanthony Just moved them all to embed.UpdateDefaultClusterFromName. PTAL. Thanks.
gyuho
force-pushed
the
default-host
branch
2 times, most recently
from
6cac0ca
to
3dbd23b
Compare
| @@ -356,6 +356,32 @@ func (cfg Config) IsDefaultHost() (string, error) { | |||
| return "", defaultHostStatus | |||
| } | |||
|
|
|||
| // UpdateDefaultClusterFromName updates cluster advertise URLs with default host. | |||
| // TODO: check whether fields are set instead of whether fields have default value | |||
| func UpdateDefaultClusterFromName(cfg *Config, defaultInitialCluster string) { | |||
There was a problem hiding this comment.
func (cfg *Config) UpdateDefaultClusterFromName(defaultCluster string)?
| } | ||
| } | ||
|
|
||
| func isDefaultRoute(host string) bool { |
There was a problem hiding this comment.
can probably inline this instead of a separate function?
| return ip == "0.0.0.0" | ||
| } | ||
|
|
||
| func (cfg *Config) updateACURLHost(host string) { |
|
lgtm after fixing minor nits |
…en URL is 0.0.0.0
Fix etcd-io#7348. etcd-io#7027 broke this. It overwriting the advertise-client-urls' host with 'localhost'. Signed-off-by: Gyu-Ho Lee <[email protected]>
Fix #7009.