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

etcdctl: add discovery-srv global flag for v3 #8462

Merged
merged 1 commit into from
Sep 5, 2017
Merged

etcdctl: add discovery-srv global flag for v3 #8462

merged 1 commit into from
Sep 5, 2017

Conversation

jiaxuanzhou
Copy link
Contributor

@jiaxuanzhou jiaxuanzhou commented Aug 29, 2017 •
edited
Loading

fix #8440

heyitsanthony
heyitsanthony suggested changes Aug 29, 2017
View reviewed changes
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

the SRV records should probably be fetched as part of mustClientFromCmd

etcdctl/ctlv3/command/global.go Outdated
cert string
key string
cacert string
serverName string
Copy link
Contributor

Choose a reason for hiding this comment

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

SRV isn't part of TLS, it doesn't belong here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i place the value in a new struct discoveryCfg, please help review again

etcdctl/ctlv3/command/global.go Outdated
@@ -219,7 +220,7 @@ func insecureSkipVerifyFromCmd(cmd *cobra.Command) bool {
return skipVerify
}

func keyAndCertFromCmd(cmd *cobra.Command) (cert, key, cacert string) {
func keyAndCertFromCmd(cmd *cobra.Command) (cert, key, cacert, serverName string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't change this; it's for TLS configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok， i will check and modify it soon

heyitsanthony
heyitsanthony suggested changes Aug 30, 2017
View reviewed changes
etcdctl/ctlv3/command/make_mirror_command.go Outdated
@@ -71,8 +71,9 @@ func makeMirrorCommandFunc(cmd *cobra.Command, args []string) {
cacert: mmcacert,
insecureTransport: mminsecureTr,
}
discovery := discoveryCfgFromCmd(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

the destination is already given on the command line, I don't think discovery makes sense here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

etcdctl/ctlv3/command/ep_command.go Outdated
cfgs := []*v3.Config{}
for _, ep := range endpointsFromCluster(cmd) {
cfg, err := newClientCfg([]string{ep}, dt, sec, auth)
cfg, err := newClientCfg([]string{ep}, dt, sec, auth, discovery)
Copy link
Contributor

Choose a reason for hiding this comment

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

the endpoints were already derived from the cluster, there's nothing for discovery to do here; leave unchanged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

etcdctl/ctlv3/command/global.go Outdated

initDisplayFromCmd(cmd)

return mustClient(endpoints, dialTimeout, sec, auth)
return mustClient(endpoints, dialTimeout, sec, auth, discovery)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think mustClient needs to change at all; mustClientFromCmd can get the endpoints from discovery and then pass that into mustClient. secureCfg will need a serverName field.

Copy link
Contributor Author

@jiaxuanzhou jiaxuanzhou Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

in the last commit , you mentioned that the serverName does not belong to struct secureCfg ,i am confused that you have opposite opinion on the location of serverName in secureCfg; but it is a better way to add the serverName to secureCfg, as i just want to pass the domain & insecure to func newClientCfg and give value to tlsinfo.ServerName.

Copy link
Contributor

@heyitsanthony heyitsanthony Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

sorry, the context wasn't clear from what was there before; I thought the intent was to pass the srv domain info into secureCfg, instead of using it to lock down tls for srv records; having serverName in secureCfg looks OK now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, i will modify it soon

etcdctl/ctlv3/command/global.go Outdated
@@ -64,6 +67,11 @@ type authCfg struct {
password string
}

type discoveryCfg struct {
domainStr string
Copy link
Contributor

Choose a reason for hiding this comment

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

domain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will change to domain

etcdctl/ctlv3/command/global.go Outdated
return []string{}, nil
}

discoverer := client.NewSRVDiscover()
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid using the v2 client in v3 code:

import "github.com/coreos/etcd/pkg/srv"

srv.GetClient("etcd-client", domain)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok ,will modify it

@jiaxuanzhou jiaxuanzhou changed the title [WIP]etcdctl: add discovery-srv global flag for v3 etcdctl: add discovery-srv global flag for v3 Sep 1, 2017
heyitsanthony
heyitsanthony suggested changes Sep 1, 2017
View reviewed changes
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

A few nits, but otherwise looks OK. Thanks!

etcdctl/ctlv3/command/global.go Outdated
return []string{}, nil
}

eps, err := discoverer(discoveryCfg.domain)
Copy link
Contributor

Choose a reason for hiding this comment

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

just inline discoverer and get rid of the function?

srvs, err := srv.getClient("etcd-client", domain)
if err != nil {
    return nil, err
}
eps := srvs.Endpoints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified

etcdctl/ctlv3/command/global.go Outdated
func discoveryCfgFromCmd(cmd *cobra.Command) *discoveryCfg {
var err error
var domainStr string
if domainStr, err = cmd.Flags().GetString("discovery-srv"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

simpler to treat empty string as no discovery option:

domainStr, err := cmd.Flags().GetString("discovery-srv")
if err != nil {
    ExitWithError(ExitBadArgs, err)
}
return &discoveryCfg{domain: domainStr, insecure: insecureDiscoveryFromCmd(cmd)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified

heyitsanthony
heyitsanthony approved these changes Sep 5, 2017
View reviewed changes
Copy link
Contributor

@heyitsanthony heyitsanthony 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 approved these changes Sep 5, 2017
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!

@heyitsanthony heyitsanthony merged commit 9021b85 into etcd-io:master Sep 5, 2017
@seh
Copy link
Contributor

seh commented May 11, 2018

In which version of etcd is this change first available?

I'm running version 3.2.15 here, released on 22 January 2018, but it doesn't yet know the "discovery-srv" flag introduced here on 5 September 2017.

@seh
Copy link
Contributor

seh commented May 11, 2018

Ah, it looks like it came in with 3.3.0-rc.0, and is not available in the 3.2.x era.

@gyuho
Copy link
Contributor

gyuho commented May 11, 2018

@seh Yeah, it's in 3.3 https://github.com/coreos/etcd/blob/master/CHANGELOG-3.3.md#etcdctl-v3-1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heyitsanthony heyitsanthony heyitsanthony approved these changes

@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.

etcdctl v3 has no --discovery-srv flag
4 participants
@jiaxuanzhou @seh @gyuho @heyitsanthony