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

Fix regression in handling flag-values starting with hyphen #1135

Closed
wants to merge 2 commits into from
Closed

Fix regression in handling flag-values starting with hyphen #1135

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
464c242
Fix regression in handling flag-values starting with hyphen
thaJeztah May 8, 2020
3358e6f
Fix handling of -- delimiter
thaJeztah May 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion command.go
View file
Open in desktop
Expand Up @@ -234,7 +234,7 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
break

// checks if this arg is a value that should be re-ordered next to its associated flag
} else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") {
Copy link

Choose a reason for hiding this comment

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

This is still not quite correct if you wanted to do something like:

% runc run --bundle --some-real-runc-flag ctr

Where it would still re-order things. The least-ugly way I can think of solving this is that we change nextIndexMayContainValue to make use of TakesValue() (with some interface casting) and removing this "skip reordering flags that start with -" logic if it doesn't break anything.

I also hasten to point out that urfave/cli has actually had this behaviour for a very long time and we (speaking collectively in the containers community) have misunderstood how urfave/cli handles this case. I discussed this in #1152 (which I've marked as a duplicate of this) and I've had to work around this in umoci (see opencontainers/umoci#328 for how I ended up hotfixing this -- you just disable argument reordering). Since we want to avoid breaking other users of urfave/cli we might have to migrate runc, umoci, and containerd away from urfave/cli (maybe to spf13/cobra which is what Docker uses)...

Copy link

Choose a reason for hiding this comment

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

A possible patch to do the above might look like the following. This change does pass all of the existing tests...

diff --git a/command.go b/command.go
index f02d3589ff3a..3c8e971a1c53 100644
--- a/command.go
+++ b/command.go
@@ -223,7 +223,7 @@ func (c *Command) useShortOptionHandling() bool {
 func reorderArgs(commandFlags []Flag, args []string) []string {
        var remainingArgs, reorderedArgs []string
 
-       nextIndexMayContainValue := false
+       nextIndexIsValue := false
        for i, arg := range args {
 
                // dont reorder any args after a --
@@ -233,17 +233,29 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
                        remainingArgs = append(remainingArgs, args[i:]...)
                        break
 
-                       // checks if this arg is a value that should be re-ordered next to its associated flag
-               } else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") {
-                       nextIndexMayContainValue = false
+                       // if this argument is a value then it isn't a flag and should be
+                       // kept with its associated flag (which was the last flag
+                       // reordered)
+               } else if nextIndexIsValue {
+                       nextIndexIsValue = false
                        reorderedArgs = append(reorderedArgs, arg)
 
                        // checks if this is an arg that should be re-ordered
-               } else if argIsFlag(commandFlags, arg) {
+               } else if flag := findFlagForArg(commandFlags, arg); flag != nil {
                        // we have determined that this is a flag that we should re-order
                        reorderedArgs = append(reorderedArgs, arg)
-                       // if this arg does not contain a "=", then the next index may contain the value for this flag
-                       nextIndexMayContainValue = !strings.Contains(arg, "=")
+
+                       // if the flag takes an argument and doesn't contain a "=" then the
+                       // next index contains a value for this flag -- unfortunately we
+                       // have to depend on the Flag implementing DocGenerationFlag for
+                       // TakesValue().
+                       if docFlag, ok := (*flag).(DocGenerationFlag); ok {
+                               nextIndexIsValue = docFlag.TakesValue() && !strings.Contains(arg, "=")
+                       } else {
+                               // assume that flags take arguments
+                               // XXX: This is quite ugly and I'm not sure what we should do here.
+                               nextIndexIsValue = !strings.Contains(arg, "=")
+                       }
 
                        // simply append any remaining args
                } else {
@@ -254,15 +266,16 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
        return append(reorderedArgs, remainingArgs...)
 }
 
-// argIsFlag checks if an arg is one of our command flags
-func argIsFlag(commandFlags []Flag, arg string) bool {
+// findFlagForArg checks if an arg is one of our command flags and returns the
+// Flag if the arg is a flag (nil otherwise).
+func findFlagForArg(commandFlags []Flag, arg string) *Flag {
        // checks if this is just a `-`, and so definitely not a flag
        if arg == "-" {
-               return false
+               return nil
        }
        // flags always start with a -
        if !strings.HasPrefix(arg, "-") {
-               return false
+               return nil
        }
        // this line turns `--flag` into `flag`
        if strings.HasPrefix(arg, "--") {
@@ -279,12 +292,12 @@ func argIsFlag(commandFlags []Flag, arg string) bool {
                for _, key := range strings.Split(flag.GetName(), ",") {
                        key := strings.TrimSpace(key)
                        if key == arg {
-                               return true
+                               return &flag
                        }
                }
        }
-       // return false if this arg was not one of our flags
-       return false
+       // couldn't find the flag
+       return nil
 }
 
 // Names returns the names including short names and aliases.

Copy link
Author

Choose a reason for hiding this comment

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

@cyphar I see you commented on the first commit; did you check with the second commit as well?

Copy link
Author

Choose a reason for hiding this comment

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

(I was looking at the requested changes to preserve some of the existing behaviour)

Copy link

@cyphar cyphar Jul 3, 2020 •
edited

Choose a reason for hiding this comment

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

No, this was just talking about the behaviour of the first change (and not the fun -- semantics that you quizzed everyone on in May). I /think/ that it should be fairly straight-forward (move the -- check after the "is this an option-value check" as in your current patch) but I agree with both of you that that particular change looks quite scary.

I think the --opt -a case is far more clear-cut.

Copy link
Author

Choose a reason for hiding this comment

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

So, in your example case (runc run --bundle --some-real-runc-flag) both --bundle and --some-real-runc-flag are "known" flags to runc, but the second one should be treated as value for --bundle (so --bundle="--some-real-runc-flag") or as flag? In the former case, wouldn't it have to be used as runc run --bundle -- --some-real-runc-flag? 🤔)

Copy link

@cyphar cyphar Jul 3, 2020 •
edited

Choose a reason for hiding this comment

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

The former (--some-real-runc-flag treated as a avalue for --bundle). The motivating use-case is (aside from POSIX-correctness and compatibility with getopt(3)) similar to --:

% runc run --bundle --foo ctr # where ./--foo is a directory containing an OCI bundle

(Well, for umoci it's more like umoci config --image foo:bar --config.cmd sh --config.cmd -c but that's a slightly more complicated example. But the root cause is the same as your original --memory-limit -1 issue.)

Should mean that --foo is a value for --bundle regardless of whether --foo is a known command-line flag. Currently (and with your patches), urfave/cli will not reorder --bundle --foo (where --foo is not a valid flag for runc) but it will reorder the flags if you do --bundle --systemd-cgroup (where --systemd-cgroup is a supported flag for runc). Even more strangely, --bundle=--systemd-cgroup works without issue because the arguments don't get reordered.

The patch I posted above should make it so that --bundle=--foo acts exactly like --bundle --foo regardless of whether --foo is a valid runc flag.

In the former case, wouldn't it have to be used as runc run --bundle -- --some-real-runc-flag?

I disagree (especially if we take the interpretation that -- should be an option-value in that case). If you use getopt:

% getopt -o '' -l bundle:,systemd-cgroup: -- --bundle --systemd-cgroup container-name
 --bundle '--systemd-cgroup' -- 'container-name'

But also it's the obvious behaviour if you just inject =s:

 getopt -o '' -l bundle:,systemd-cgroup: -- --bundle=--systemd-cgroup container-name
 --bundle '--systemd-cgroup' -- 'container-name'

Copy link
Contributor

Choose a reason for hiding this comment

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

@thaJeztah PTAL?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @thaJeztah 🙏

} else if nextIndexMayContainValue && !argIsFlag(commandFlags, arg) {
nextIndexMayContainValue = false
reorderedArgs = append(reorderedArgs, arg)

Expand Down
65 changes: 65 additions & 0 deletions command_test.go
View file
Open in desktop
Expand Up @@ -114,6 +114,71 @@ func TestParseAndRunShortOpts(t *testing.T) {
}
}

// test-case for https://github.com/urfave/cli/issues/1092
func TestParseAndRunHyphenValues(t *testing.T) {
cases := []struct {
testArgs []string
expectedArgs []string
expectedOpt string
}{
{[]string{"foo", "test", "argz"}, []string{"argz"}, ""},
{[]string{"foo", "test", "argz", "arga"}, []string{"argz", "arga"}, ""},
{[]string{"foo", "test", "--opt", "opt-value"}, []string{}, "opt-value"},
{[]string{"foo", "test", "--opt", "opt-value", "argz"}, []string{"argz"}, "opt-value"},
{[]string{"foo", "test", "--opt", "opt-value", "argz", "arga"}, []string{"argz", "arga"}, "opt-value"},
{[]string{"foo", "test", "argz", "--opt", "opt-value"}, []string{"argz"}, "opt-value"},
{[]string{"foo", "test", "argz", "--opt", "opt-value", "arga"}, []string{"argz", "arga"}, "opt-value"},

{[]string{"foo", "test", "--opt", "-opt-value"}, []string{}, "-opt-value"},
{[]string{"foo", "test", "--opt", "-opt-value", "argz"}, []string{"argz"}, "-opt-value"},
{[]string{"foo", "test", "--opt", "-opt-value", "argz", "arga"}, []string{"argz", "arga"}, "-opt-value"},
{[]string{"foo", "test", "argz", "--opt", "-opt-value"}, []string{"argz"}, "-opt-value"},
{[]string{"foo", "test", "argz", "--opt", "-opt-value", "arga"}, []string{"argz", "arga"}, "-opt-value"},

{[]string{"foo", "test", "--opt", "--opt-value"}, []string{}, "--opt-value"},
{[]string{"foo", "test", "--opt", "--opt-value", "argz"}, []string{"argz"}, "--opt-value"},
{[]string{"foo", "test", "--opt", "--opt-value", "argz", "arga"}, []string{"argz", "arga"}, "--opt-value"},
{[]string{"foo", "test", "argz", "--opt", "--opt-value"}, []string{"argz"}, "--opt-value"},
{[]string{"foo", "test", "argz", "--opt", "--opt-value", "arga"}, []string{"argz", "arga"}, "--opt-value"},
}

for _, tc := range cases {
tc := tc
t.Run(strings.Join(tc.testArgs, "_"), func(t *testing.T){
var (
args []string
opt string
)

cmd := Command{
Name: "test",
Usage: "this is for testing",
Description: "testing",
Action: func(c *Context) error {
args = c.Args()
opt = c.String("opt")
return nil
},
Flags: []Flag{
StringFlag{Name: "opt"},
},
}

app := NewApp()
app.Writer = ioutil.Discard
app.ErrWriter = ioutil.Discard
app.Commands = []Command{cmd}

err := app.Run(tc.testArgs)

expect(t, err, nil)
expect(t, args, tc.expectedArgs)
expect(t, opt, tc.expectedOpt)
})

}
}

func TestCommand_Run_DoesNotOverwriteErrorFromBefore(t *testing.T) {
app := NewApp()
app.Commands = []Command{
Expand Down