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.

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

etcdctl: add --password flag to the subcommand user add #9730

Merged
merged 1 commit into from
May 29, 2018

Conversation

mitake
Copy link
Contributor

@mitake mitake commented May 17, 2018

Fix #9691

/cc @cristifalcas you can create a user whose name includes : now

@gyuho gyuho requested a review from joelegasse May 17, 2018 14:10
Copy link
Contributor

@joelegasse joelegasse left a comment

Choose a reason for hiding this comment

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

With this PR, we also need to update places that parse user:password and do not expect : to be a part of the user name. For example, in https://github.com/coreos/etcd/blob/master/etcdctl/ctlv3/command/global.go#L351 :

	splitted := strings.SplitN(userFlag, ":", 2)
	if len(splitted) < 2 {
		cfg.username = userFlag
		cfg.password, err = speakeasy.Ask("Password: ")
		if err != nil {
			ExitWithError(ExitError, err)
		}
	} else {
		cfg.username = splitted[0]
		cfg.password = splitted[1]
	}

Also, the documentation around using a colon strictly as a password separator needs to be updated, as well as adding in new notes about the separate --password flag that mention colons in the username will no longer be treated specially when the password flag is given.

The example I posted above should not be taken as an exhaustive list of places where colons aren't expected to be a part of the username, but just the first example I found. My main point is that allowing a username with a colon in it is just the first step in adding full support for colons in usernames. I'd rather have a single PR for full support rather than merging in code that could leave the system in a weird state: the user exists, but can't login.

@xiang90
Copy link
Contributor

xiang90 commented May 17, 2018

lgtm after addressing joelegasse's concerns.

@mitake
Copy link
Contributor Author

mitake commented May 21, 2018

@joelegasse thanks for your comment, I'll address it

This commit adds two flags to etcdctl:
1. `--password` flag to etcdctl as a global option. It can be used for
specifying password for authentication required for the command
execution.
2. `--new-user-password` flag to `etcdctl user add`. It can be used
for specifying password of newly created user by the command.

The main motivation of the flags is allowing user to have : in its
name.

Fix etcd-io#9691
@mitake
Copy link
Contributor Author

mitake commented May 21, 2018

@joelegasse @xiang90 updated based on the comment, could you take a look?

@gyuho gyuho merged commit dbb37f9 into etcd-io:master May 29, 2018
@mitake mitake deleted the user-w-colon branch May 30, 2018 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants