awsudo CLI has changed

Question

awsudo CLI has changed

Closed this issue 3 years ago · 9 comments

jaytmiller commented 3 years ago

Describe the bug

Thanks for awsudo!

I'm not sure I'd call this a bug but maybe an unintended CLI change for Linux users in particular.

The CLI of awsudo seems to have changed from roughly:

awsudo <role> <cmd word1> <cmd word2> ...

to:

awsudo <role> <cmd>

where <cmd> must be a single shell "word" and the multi-word form no longer works.

In the problem cases I've seen, <cmd word2> is now being interpreted as an awsudo option/switch rather than as part of our command as it used to be.

Answer 1 · 2021-11-09T23:40:01.000Z

Thank you for the bug report! We're working through this issue as well as several others. For now reverting to version 1.5.0 will be your best bet.

We will update this issue once we have a fix in place.

We may choose to deprecate versions 1.6.0 and 1.7.0, but we haven't yet come to a conclusion on that.

Thank you for using awsudo!

Answer 2 · 2021-11-12T23:07:55.000Z

Thank you very much for opening this issue @jaytmiller !

If you're curious, this happened in the course of the recent work to make using a role ARN optional when using profiles. Once there wasn't a guarantee of an ARN in the argument list, the parsing that handled the post-ARN arguments (i.e. the command) got lost.

Fortunately this was fairly easy to replicate based on your description:

awsudo ARN ls -l

Pull request #59 is intended to resolve this and we'll merge it as soon as possible.

Answer 3 · 2021-11-12T23:12:57.000Z

Fellow maintainers, I think this experience suggests it's time for us to document some test cases to verify new features. If we're in agreement on that, should I enable the wiki?

Answer 4 · 2021-11-15T03:37:14.000Z

I'll work to follow up on this with some test cases to prevent future regressions.

I'll attempt to make it something executable to ensure repeatability and lower demands on time -- however if that becomes infeasible, I will pursue the pure documentation approach I originally suggested.

Answer 5 · 2021-11-16T14:23:58.000Z

Thanks again for awsudo and for such a rapid fix!

Answer 6 · 2021-11-16T19:47:36.000Z

We have also run into this issue. I thought I would share a bit about our use case. While most people are probably using awsudo to run aws CLI commands, we are using it to run Serverless framework sls commands while assuming a role in AWS. Our command line looks like this:

npx awsudo --session-name serverless-template-nonprod --duration 3600 arn:aws:iam::REDACTED:role/REDACTED npx sls deploy --verbose --stage nonprod --region us-east-2

This worked fine in version 1.5.0, but when used in 1.7.0, it interprets --verbose --stage nonprod --region us-east-2 as arguments of awsudo rather than sls. As a result, we saw more verbose output from awsudo and less verbose output from sls, and sls used the default stage and region values rather than the ones we were trying to pass. (It appears from the code that the issue was introduced in 1.6.0, but we never used that version to my knowledge.) A related but separate issue here is that awsudo accepted the --stage and --region parameters even though it does not support such parameters. It would have been helpful if it indicated an error. Instead, it took some clever deduction to realize what was happening.

So in short, this version of awsudo takes any arguments that start with -- to apply to awsudo rather than the command, because these arguments are not positional. It will take any positional arguments, however, and include them in the command.

I am a little concerned with the way the fix was implemented in PR #59, however. It looks for the first argument that doesn't start with - and isn't a role ARN, and takes that to be the first part of the command. However, as you can see in our example command line above, some non-positional arguments take an additional argument that doesn't start with -. For example, serverless-template-nonprod does not begin with a -, but it's the value of the non-positional --session-name argument. So I'm afraid that this implementation is a bit too naive. It needs to be smart enough to take values of named arguments. I wonder if there is some feature of yargs that could be used to help with this?

An alternative that some command line tools use is to support a special parameter -- to separate arguments that are meant for the tool from arguments that are meant for the command that the tool will run. It would require some change to the way the tool is used, however. For example, our command line would become:

npx awsudo --session-name serverless-template-nonprod --duration 3600 arn:aws:iam::REDACTED:role/REDACTED -- npx sls deploy --verbose --stage nonprod --region us-east-2

This would be simpler to implement, but it would still be a breaking change compared to the way 1.5.0 worked.

Answer 7 · 2021-11-16T20:43:59.000Z

Thank you @meyertime ! That's a very good catch and you're absolutely right that implementation was too naive.

I had considered the -- parameter, but hoped I could restore the original behavior without it. I think there's still some investigation into 1.5.0 compatible solutions, but that option is definitely on the table.

In light of this, I'm reopening the issue.

Answer 8 · 2021-11-16T23:18:41.000Z

I've opened the above PR to leverage a feature in yargs to allow awsudo to parse the command arguments more intelligently.

Any feedback is welcome!

I'd like to get that (and the associated test cases PR) merged for everyone who has been impacted by this regression as soon as possible.

Answer 9 · 2021-11-17T16:44:47.000Z

Version 1.7.1 is released and should resolve these issues.

Thank you again everyone for the feedback!