Conversation
…r the client config
Also switches to yaml.UnmarshalStrict so we error out on unexpected fields in the config file
| func writeClientConfig(conf config.Config) error { | ||
| filename := filepath.Join("/keybase/team/", conf.GetTeamName(), shared.ConfigFilename) | ||
| // We only write the client config into the first team since that is enough for kssh to find it. This means | ||
| // kssh will talk to the bot in the first team that is listed in the config file |
There was a problem hiding this comment.
it's good this is documented, but it might be unexpected behavior. are you sure it's not better to write it into all of the teams?
There was a problem hiding this comment.
Imagine I'm running a CA bot with two listed teams: teamname.a and teamname.b. If the config file is written into both directories, then kssh will believe there are two bots running and will prompt the user to ask which team they wish to use. This is unnecessary (since there really is only one bot) and negatively impacts the user experience. Does that make sense as a reason to only write it into one team?
There was a problem hiding this comment.
@ddworken but the name of the bot is in the config, and the client can easily see that. it's the number of bots that are running that matters, not the number of configs.
There was a problem hiding this comment.
Hmm, but that means that kssh now needs to go through and read/parse all the configs and then deduplicate them based off of the name of the bot in the config. That is certainly doable. Is there an advantage to that though?
There was a problem hiding this comment.
the advantage is is more expected behavior. i would assume that if i set this up with three teams and then removed one of them, that i wouldn't then need to set it up again (it's automated, but still). similarly (and perhaps more importantly), that i could work backwards: i.e. if there is a file in this team, then the bot is doing it's thing for anyone in this team. this is definitely how i would expect it to work.
i'd say it should error and request a team name if it sees more than one botname that looks legit. and this means we can swallow any weird errors in the parsing as long as at least one of them parses correctly (or whatever is easier).
There was a problem hiding this comment.
So I think from your perspective, you don't need to set this up again. Each time keybaseca starts (either for the generate command or the service command) it writes a client config file at the correct location. And keybaseca doesn't do any hot reloading of config files, so doesn't this already work correctly? Any time you update the config file to change the teams, it works correctly.
And I'm not sure we want to set the standard of users looking into KBFS to determine whether or not a team is being used for SSH access. That seems especially tricky because a modified kssh client could delete the client config file and still connect fine by caching it.
| return signedKeyLocation + defaultTeam, nil | ||
| } | ||
|
|
||
| // handleArgs parses os.Args for use with kssh. This is handwritten rather than using go's flag library (or |
There was a problem hiding this comment.
since this is hand-rolled with assumptions like the placement of the --team flag, this is going to be a pain to add functionality to. even if we expect nothing, something will come along. i'd definitely suggest leaning on some existing code to parse anything coming in if possible. i realize a lot of what we want to do here is just pass along arguments straight through to ssh, but yeah. also leaning on existing code would allow a user to put --team at the end for instance.
There was a problem hiding this comment.
Of the three big golang CLI parsing libraries I found (flags, urfave/cli and cobra) none of them support this functionality without adding a -- prior to any arguments that we would want to pass to ssh. This means the standard user experience would be kssh -- root@server which seems significantly worse than kssh root@server. I agree it would be nice if we could use an existing library to get some better handling here, but I don't see any way of doing that without sacrificing the kssh user experience.
There was a problem hiding this comment.
huh. i spent a few minutes playing around with flags, and yeah. that's a little weird. it's definitely doable (see https://play.golang.org/p/7YX8icPhBRd) but meh. i suppose i wouldn't go to the mat over it.
There was a problem hiding this comment.
without doing this, you should assume no one will ever add any more command line args. which, the more i think about it, seems like a bad assumption. :/
There was a problem hiding this comment.
Yeah, I tested a demo similar to that one and came to the exact same behavior of dropping the first argument after the parsed arguments. One additional demo to note is this one:
https://play.golang.org/p/Hkl1ggZX4or
So it appears that what the flags library does with ContinueOnError is it will correctly process the arguments until it hits the first unknown argument. Once it finds an unknown argument, it eats that one and exits.
So if we wanted to we could use the flags library and then manually add back in the first argument after the last parsed one. But that seems like a pretty messy workaround that could be very brittle (just as brittle as my hand done parser I would say). And that doesn't even give us the ability to do kssh root@server --team teamname.ssh as you suggested in your first comment.
There was a problem hiding this comment.
Yeah. Just makes me appreciate the python stdlib where this is super easy:
https://docs.python.org/3/library/argparse.html#partial-parsing
xgess
left a comment
xgess
left a comment
There was a problem hiding this comment.
LGTM. i think it's worth using an existing cli arg parser assuming it's lightweight and allows pretty naive passthrough of arbitrary args that we don't care about.
2 participants
Changes:
--teamand--set-default-team) that the user is prompted to use if the user is in multiple teams.config: simplev/sconfig: advanceddirective. This change is really just setting up for the advanced mode support but I included it since I was already changing the config file format.