TOOLS-2779: Add --config option for password values #62

edobranov · 2020-11-24T19:57:54Z

Jira: https://jira.mongodb.org/browse/TOOLS-2779

This adds support for the --config option along with some unit tests under TestParseConfigFile. Also vendors in the gopkg.in/yaml.v2 package.

edobranov · 2020-11-24T20:08:58Z

options/options.go

+			break
+		} else if strings.HasPrefix(arg, "--config") {
+			// No next argument or the next argument is an option.
+			if i+1 == len(args) || strings.HasPrefix(args[i+1], "-") {


One ambiguity I ran into was if the user does this:

$ mongodump --config -d myDB -c ...

Do we want to treat this as a missing config file, or try to open a file named -d instead? I see validity in both, though I'm maybe leaning a bit towards the latter option of just trying to open it and letting the user deduce what happened from the os.Open() error message.

Reason being that they could theoretically have a config file that starts with a hyphen:

$ mongodump --config -tools-config.yaml -d myDB -c ...

It's unlikely, but we'd either have to ensure the arg after --config isn't an option (lot of manual checking) or force the user to rename -tools-config.yaml to something else.

I can remove the || strings.HasPrefix(args[i+1], "-") check if others agree.

Yeah good question, go-flags has this behavior:

$ mongodump --sslPEMKeyFile -d 2020-11-25T18:58:47.875+0100 error parsing command line options: expected argument for flag `--sslPEMKeyFile', but got option `-d'

So I think disallowing files beginning with - is consistent. If people do have files beginning with a -, they can always use the --config="foo" format. But I also think trying to open -d is fine too. It would very quickly obvious what the problem is. If you prefer that approach, I think go for it.

Cool, I'm still leaning towards trying to open -d just because otherwise the error message would be kinda hard to get right. Like if a user says --config -tools-config.yaml the error would be --config option specified but no file given, which would be confusing since they obviously did provide a file and might not think to use --config=. We could separate out the conditions, but that's already more complexity than it's worth I feel.

Instead I'll add a bit of context to the ioutil.ReadFile() error, so now it'll look like:

$ mongodump --config -d myDB 2020-11-25T14:26:20.168-0500 error parsing command line options: error opening file with --config: open -d: no such file or directory

If --config is at the end, it'll stay the same:

$ mongodump -d myDB --config 2020-11-25T14:35:30.196-0500 error parsing command line options: --config option specified but no file given

tfogo

Good tests. I have a couple of concerns about parsing the config option.

tfogo · 2020-11-25T18:07:52Z

options/options.go

+			break
+		} else if strings.HasPrefix(arg, "--config") {
+			// No next argument or the next argument is an option.
+			if i+1 == len(args) || strings.HasPrefix(args[i+1], "-") {


Yeah good question, go-flags has this behavior:

$ mongodump --sslPEMKeyFile -d 2020-11-25T18:58:47.875+0100 error parsing command line options: expected argument for flag `--sslPEMKeyFile', but got option `-d'

So I think disallowing files beginning with - is consistent. If people do have files beginning with a -, they can always use the --config="foo" format. But I also think trying to open -d is fine too. It would very quickly obvious what the problem is. If you prefer that approach, I think go for it.

tfogo · 2020-11-25T18:09:01Z

options/options.go

+			}
+			configPath = arg[9:]
+			break
+		} else if strings.HasPrefix(arg, "--config") {


This would match --configFOO too. We need to do an exact match here.

tfogo · 2020-11-25T18:10:17Z

options/options.go

+	// Get config file path from the arguments, if specified.
+	configPath := ""
+	for i, arg := range args {
+		if strings.HasPrefix(arg, "--config=") {


Is there going to be a difference between --config=foo and --config="foo"? Will the latter make the config path be \"foo\"?

Hmm, interesting point. It seems the underlying call to os.Open() is smart enough to recognize that the surrounding quotation marks aren't intended unless they're explicitly escaped. So there should be no difference.

From trying it on the command line as-is:

$ echo "uri: mongodb://abc:@localhost:33333" > config.yaml $ mongodump --config=config.yaml Enter password: $ mongodump --config="config.yaml" Enter password: $ mongodump --config=\"config.yaml\" 2020-11-25T14:19:02.953-0500 error parsing command line options: open "config.yaml": no such file or directory $ mongodump --config="\"config.yaml\"" 2020-11-25T14:19:12.025-0500 error parsing command line options: open "config.yaml": no such file or directory

I don't think it's the call to os.Open -- my understanding is that the shell consumes the quotes, so they are never passed as args in this specific example.

I think there are other issues here though. For example, does go-flags allow /-style flags on windows? If so, that's a whole separate class of thing that we have to manually check for here.

As I have noted in another PR, I'm generally skeptical of trying to match the library's flag-parsing behavior by manually scanning arguments

tfogo · 2020-11-25T18:13:05Z

options/options_test.go

+			}
+		}
+
+		Convey(testCase.description, func() {


This can just be Convey(testCase.description, assertion)

tfogo · 2020-11-25T18:16:03Z

options/options_test.go

+		So(opts.ParseConfigFile(args), ShouldNotBeNil)
+	})
+
+	Convey("should error with non-existent config file specified", t, func() {


I think the different positions here are overkill to just test a non-existent file.

huan-Mongo

All seems good! Just a comment on the description of this option.

huan-Mongo · 2020-12-03T15:32:19Z

options/options.go

-	Version bool `long:"version" description:"print the tool version and exit"`
+	Help       bool   `long:"help" description:"print usage"`
+	Version    bool   `long:"version" description:"print the tool version and exit"`
+	ConfigPath string `long:"config" description:"path to a configuration file with password secrets"`


I am wondering do we need with password secrets since later we could introduce more fields inside?

Yea, I don't think we really need it either. We could also drop that phrase later on when we do support more/all fields, but I'm not too attached to it. I can go ahead and remove it if @rychipman agrees.

I think removing it makes sense!

rychipman · 2020-12-08T22:02:40Z

options/options.go

+	// Get config file path from the arguments, if specified.
+	configPath := ""
+	for i, arg := range args {
+		if strings.HasPrefix(arg, "--config=") {


I don't think it's the call to os.Open -- my understanding is that the shell consumes the quotes, so they are never passed as args in this specific example.

I think there are other issues here though. For example, does go-flags allow /-style flags on windows? If so, that's a whole separate class of thing that we have to manually check for here.

As I have noted in another PR, I'm generally skeptical of trying to match the library's flag-parsing behavior by manually scanning arguments

edobranov

Adjusted the --config help menu phrase and changed out the manual command line parsing for the opts.parser.ParseArgs() functionality. Should take care of a lot of what-ifs.

edobranov · 2020-12-09T22:10:58Z

options/options.go

+	// Assign each parsed value to its respective ToolOptions field.
+	opts.Auth.Password = config.Password
+	opts.URI.ConnectionString = config.ConnectionString
+	opts.SSL.SSLPEMKeyPassword = config.SSLPEMKeyPassword


To bring up a comment I made in the tech design a while ago -- before storing each field here, we could actually check to see if the fields were already set by the call to opts.parser.ParseArgs() at the beginning of this func. This would allow us to log a message that a config file's value will be overridden by the respective command line option.

Might offer a bit of transparency but I don't feel strongly either way.

I don't recommend doing that because the fields aren't necessarily directly set inside cmd parameters, for example, the password could be set inside connectionString. In that case, you won't be able to catch the overridden.
If you really wanna log the warning message, this function should return a separate ToolOptions object and later used as the comparison with the ParseArgs() result.

Right, it's true we wouldn't know if a password comes from --uri vs. a positional connection string, but would that matter a lot?

It might be enough to just say something like warning: connection string in config file will be overridden by connection string on command line.

Since we already decided this wasn't a requirement for this project, I would also recommend that we not do this

huan-Mongo

LGTM!

huan-Mongo · 2020-12-10T16:56:54Z

options/options.go

+	// Assign each parsed value to its respective ToolOptions field.
+	opts.Auth.Password = config.Password
+	opts.URI.ConnectionString = config.ConnectionString
+	opts.SSL.SSLPEMKeyPassword = config.SSLPEMKeyPassword


I don't recommend doing that because the fields aren't necessarily directly set inside cmd parameters, for example, the password could be set inside connectionString. In that case, you won't be able to catch the overridden.
If you really wanna log the warning message, this function should return a separate ToolOptions object and later used as the comparison with the ParseArgs() result.

rychipman

LGTM

rychipman · 2020-12-10T22:23:43Z

options/options.go

+	// Assign each parsed value to its respective ToolOptions field.
+	opts.Auth.Password = config.Password
+	opts.URI.ConnectionString = config.ConnectionString
+	opts.SSL.SSLPEMKeyPassword = config.SSLPEMKeyPassword


Since we already decided this wasn't a requirement for this project, I would also recommend that we not do this

TOOLS-2779: Add --config option for password values

9b1ecd8

edobranov requested review from tfogo, rychipman and huan-Mongo November 24, 2020 19:57

edobranov commented Nov 24, 2020

View reviewed changes

TOOLS-2779: Uncomment test type check

751694f

tfogo requested changes Nov 25, 2020

View reviewed changes

TOOLS-2779: Minor adjustments

6f7b550

edobranov requested a review from tfogo November 25, 2020 23:06

edobranov mentioned this pull request Dec 3, 2020

TOOLS-2780: Add warning when password value appears on command line #63

Merged

huan-Mongo reviewed Dec 3, 2020

View reviewed changes

edobranov removed the request for review from tfogo December 3, 2020 18:18

rychipman requested changes Dec 8, 2020

View reviewed changes

TOOLS-2779: Use opts.parser instead of manual parsing

8d06a63

edobranov commented Dec 9, 2020

View reviewed changes

edobranov requested review from rychipman and huan-Mongo December 9, 2020 22:15

huan-Mongo approved these changes Dec 10, 2020

View reviewed changes

rychipman approved these changes Dec 10, 2020

View reviewed changes

edobranov merged commit 56f379e into mongodb:master Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TOOLS-2779: Add --config option for password values #62

TOOLS-2779: Add --config option for password values #62

edobranov commented Nov 24, 2020

edobranov Nov 24, 2020 •

edited

tfogo Nov 25, 2020

edobranov Nov 25, 2020 •

edited

tfogo left a comment

tfogo Nov 25, 2020

tfogo Nov 25, 2020

tfogo Nov 25, 2020

edobranov Nov 25, 2020

rychipman Dec 8, 2020

tfogo Nov 25, 2020

tfogo Nov 25, 2020

huan-Mongo left a comment

huan-Mongo Dec 3, 2020

edobranov Dec 3, 2020 •

edited

rychipman Dec 8, 2020

rychipman Dec 8, 2020

edobranov left a comment

edobranov Dec 9, 2020

huan-Mongo Dec 10, 2020

edobranov Dec 10, 2020

rychipman Dec 10, 2020

huan-Mongo left a comment

huan-Mongo Dec 10, 2020

rychipman left a comment

rychipman Dec 10, 2020

TOOLS-2779: Add --config option for password values #62

TOOLS-2779: Add --config option for password values #62

Conversation

edobranov commented Nov 24, 2020

edobranov Nov 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edobranov Nov 25, 2020 • edited

Choose a reason for hiding this comment

tfogo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huan-Mongo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edobranov Dec 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edobranov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huan-Mongo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rychipman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edobranov Nov 24, 2020 •

edited

edobranov Nov 25, 2020 •

edited

edobranov Dec 3, 2020 •

edited