Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

rkt app add --mnt-volume: unable to mount directories with commas #3301

Open
euank opened this issue Oct 21, 2016 · 5 comments
Open

rkt app add --mnt-volume: unable to mount directories with commas #3301

euank opened this issue Oct 21, 2016 · 5 comments

Comments

@euank
Copy link
Member

euank commented Oct 21, 2016

Example:

$ mkdir -p "/tmp/bar,baz"
$ sudo rkt --debug app add $podUUID docker://busybox --name=name --mnt-volume=name=foo,kind=host,source="/tmp/bar,baz",target=/tmp
add: error parsing app image arguments
  └─invalid argument "name=foo,kind=host,source=/tmp/bar,baz,target=/tmp" for --mnt-volume=name=foo,kind=host,source=/tmp/bar,baz,target=/tmp: error parsing volume component of MountVolume

I don't think there's a way in cri-pods to mount a volume with a comma in it. I know that before you could get around these sorts of issues by just using the --pod-manifest arg, but for the app subcommands we don't have that option.

It seems possible that the best thing to do would be to allow passing in a raw json struct of a manifest for an individual app, but I'm open to other ideas/suggestions as to the best option.

cc @squeed @lucab

@lucab
Copy link
Member

lucab commented Oct 21, 2016

I didn't check the code, but I fear this is a more widespread issue than just --mnt-volume. If I recall correctly, most of current option parsing logic is just a split(,) + split(=). And the solution to that is probably agreeing on the separator, how to escape it (a custom \, or some broader "" quoting?) and then introducing a proper parser.

@squeed
Copy link
Contributor

squeed commented Oct 21, 2016

Ironically, a lot of the code actually parses these as CGI parameters, so you could theoretically use %-escaping. I don't think that's a very good idea, though.

@euank
Copy link
Member Author

euank commented Oct 21, 2016

Allowing escaping would be a little messy from a regular UX perspective because you'll have to double quote or double escape to get around the shell (--mnt-volume=..,source='"/foo/bar,baz"' type stuff).

Allowing an input of json for that field somehow would gain json escaping for free and would also be programmatically a bit easier to work with, but that's an even worse cli ux I think.

@lucab
Copy link
Member

lucab commented Oct 24, 2016

@euank I think I misread your initial suggestion, so my answer was a bit off-target, sorry for that.

I agree we should have some way of specifying a JSON runtime config, possibly allowing appc and oci formats in the future.

The problem with commas in CLI flags remains, though, and this is where I was suggesting escaping.

Braindumping mode: another option is a sed-like syntax, where the first character is a custom one and specify the delimiter. To be backward compatible we could switch to this mode only if the first character is in some specific range (eg. non-printable ascii, or non-alphanum). This would allow to keep CLI flags human-friendly while allowing programs to use something smarter (eg. 0x1e) as delimiter.

@euank
Copy link
Member Author

euank commented Oct 24, 2016

Yeah, I'm for fixing both.

My own braindump: another option would be to allow two ways to specify it, one with the commas, one with multiple flags. E.g.:

--mnt-volume-name=foo --mnt-volume-source "/foo/bar,baz" ...

However, that comes up with the problem of grouping flags. We already sorta have that with associating things to specific images, and we could do the same thing where the volume name must be first and other mnt flags after that associate with it..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants