-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Etcd DNS discovery #2521
Etcd DNS discovery #2521
Conversation
6c004c3
to
55f0e36
Compare
Pinging @xiang90 |
I will take a look over the weekend or early next week. |
physical/etcd.go
Outdated
@@ -13,6 +16,7 @@ import ( | |||
var ( | |||
EtcdSyncConfigError = errors.New("client setup failed: unable to parse etcd sync field in config") | |||
EtcdSyncClusterError = errors.New("client setup failed: unable to sync etcd cluster") | |||
EtcdMultipleDiscoveryError = errors.New("client setup failed: multiple discovery or bootstrap flags specified, use either \"address\" or \"discovery_srv\"") |
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.
MultipleBootstrapError?
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.
Will do
physical/etcd.go
Outdated
@@ -95,3 +99,44 @@ func getEtcdAPIVersion(c client.Client) (string, error) { | |||
|
|||
return "3", nil | |||
} | |||
|
|||
func getEtcdEndpoints(conf map[string]string) ([]string, error) { | |||
getOption := func(conf_key, env_var string) (string, bool) { |
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.
why this func is embedded?
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.
If it looks generally useful I can move it up to the module scope.
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.
Extracted.
physical/etcd.go
Outdated
|
||
func getEtcdEndpoints(conf map[string]string) ([]string, error) { | ||
getOption := func(conf_key, env_var string) (string, bool) { | ||
conf_val, in_conf := conf[conf_key] |
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.
confVal instead of conf_val. use camelcase instead of underscore
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.
Done
physical/etcd.go
Outdated
discoverer := client.NewSRVDiscover() | ||
endpoints, err := discoverer.Discover(domain) | ||
if err != nil { | ||
return nil, fmt.Errorf("etcd SRV discovery: %s", 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.
failed to discovery etcd endpoints through SRV discovery: %v
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.
Done
@@ -122,3 +138,4 @@ storage "etcd" { | |||
``` | |||
|
|||
[etcd]: https://coreos.com/etcd "Etcd by CoreOS" | |||
[dns discovery]: https://coreos.com/etcd/docs/latest/v2/clustering.html#dns-discovery "Etcd cluster DNS Discovery" |
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.
use this link instead: https://coreos.com/etcd/docs/latest/op-guide/clustering.html#dns-discovery
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.
Done
@jsok Have you gave this a try? I want to make sure it works. |
looks good in general. |
@xiang90 yes I've tested local with a 3-node etcd cluster with SSL. |
55f0e36
to
86078f5
Compare
86078f5
to
5a985b2
Compare
lgtm |
Merging with the 👍 from @xiang90 |
Adds a new etcd config option
discovery_srv
which contains the domain name which the etcd client will use to perform DNS SRV record discovery.Fixes #2519