Unable to escape colons in option lists

[gfiedler]

I want to create a hook like this

"hooks": {
  "prestart": [ {
    "path": "cmd",
    "args": [ "cmd", "foo:bar" ]
 } ] }

but I'm unable to do this, because colons have a special semantics and there is no posibility to escape colons in option lists:

oci-runtime-tool generate --hooks-prestart=cmd:foo:bar

results in

"args": [ "cmd", "foo", "bar" ]

The problem is in generate.go in func parseHook.

But maybe options with path arguments like --mount-bind are related to this effect too?

[wking]

+1 to extending the option syntax to allow for escaping. And for things the runtime-tools doesn't (yet) support, there's always jq ;).

[Mashimiao]

I‘m focusing on this problem recently.
Do you think comma works for you, as --hooks-prestart=cmd,foo:bar? @gfiedler @wking
It's really not easy to set separator, almost all characters can be used in path

[wking]

jq expects you to provide the value as JSON, which covers the general case. You could also cover the general case with a shell lexer or a custom escape syntax. But I think trading colons for commas just punts until we have someone who needs a literal comma.

[gfiedler]

I think we should leave the --hooks-prestart as it is and introduce a new option --hooks-prestart-args which works just like --args.

[wking]

I'm fine with --hooks-*-args too.

[Mashimiao]

Well, --hooks-*-args may can works for your case. But what if the name of hooks path contains colon?
I hope we can work out a general way to solve the problem.

[wking]

On Thu, Oct 19, 2017 at 04:58:59AM +0000, Ma Shimiao wrote: Well, --hooks-*-args may can works for your case. But what if the name of hooks path contains colon?

Ah, right. We need a way to add more than one hook entry. And even though path/args is confusing enough, folks may also want to set path, args, *and* timeout (etc.) for the same hook, which is currently not supported. And even shell lexing wouldn't cover timeout, etc. So I'm back to preferring jq-esqe: $ oci-runtime-tool generate --hooks-prestart '{"path": "cmd", "args": ["cmd", "foo:bar"], "timeout": 5}'

[Mashimiao]

I think we need generate, because we want to help users write json file. If users know how to wirte config json file. I think generate does not make any sense to him and he will no need it.

[wking]

I think we need generate, because we want to help users write json file.

That's fine, but we also need a sytax that allows setting each propery in an array of hooks. If you prefer a non-JSON syntax, that'a no problem, but I think leaving properties out will mean we cycle back around to this when, for example, someone wants to set both args and timeout. JSON is an easy choice for generic updates, but I'm sure there are others too.

[liangchenye]

• We need a separator to differentiate path, args and timeout.
  It is , in json format.
• We need a special char to escape the separator if it is used in path, args or timeout.
  It is " in json format.
• We need a separator to tell an array item.
  It is [] in json format.
• We need a separator to differentiate args in args.
  It is , in json format.
• We need a special char to escape the separator if it is used in on of args.
  It is " in json format.

So even we define our syntax, it seems that there will not be much differences comparing to json format.
Maybe we will have something like this:

oci-runtime-tool generate --hooks-prestart '"cmd", ["cmd", "foo:bar"], 5'

But the users need to learn a new syntax, so I think a raw json format here is a rational solution.

[Mashimiao]

If we decide to use json format, I think there is no base to keep generate.
If user can use and know what the json object should contain, they must can be easy to write config.json file.
From I can see, no matter json, xml or something else, they all select a symbol as a separator.
comma is nearly not used in path name, I think can select it as the separator in options and warn users it should not be used in path name when using runtime-tools.

[wking]

On Thu, Nov 23, 2017 at 07:04:23AM +0000, Ma Shimiao wrote: If we decide to use json format, I think there is no base to keep `generate`.

I agree that ‘generate’ feels weakly-motivated, and I'd drop its edit functionality if I was a runtime-tools maintainer. I'd suggest folks use jq (or whatever) followed by a call to ‘oci-runtime-tool validate’. The initial ‘generate’ code was just about a providing a default template (13430b8), and that's easier to motivate. Although see discussion in #379 and #477 about where folks see that going; with #379 and runtime-spec allowing very minimal configs, it becomes hard to motivate the “supply a default template” use-case too. With the #477 approach, you can continue to motivate it with “these values are useful for (Linux) containers doing the usual stuff”, although I'd like to nail down “the usual stuff” more clearly.

If user can use and know what the json object should contain, they must can be easy to write config.json file.

I don't think this is a strong argument. If a generate option for something doesn't already exist, I don't see how defining a new generate option with a custom separator is going to be easier for newcomers to learn about. Maybe they're already familiar with the runtime spec and its JSON structure. If not, I don't see why they couldn't learn the JSON structure for hooks by reading the runtime spec, just as you expect them to learn the generate command-line option structure for hooks by reading the generate man page. This doesn't have to be all or nothing though. Things like mounts and hooks are more complicated to handle via command-line arguments because they are arrays of objects. Using JSON for those still gives you lots of config properties where command-line callers won't need JSON.

From I can see, no matter json, xml or something else, they all select a symbol as a separator.

And they give you a way to escape that separator when you need to use it as part of a value.

comma is nearly not used in path name, I think can select it as the separator in options and warn users it should not be used in path name when using runtime-tools.

But I expect someone will eventually want commas in an argument, and with #524 they'll have no way to escape those. And also no way to add a hook which sets both args and timeout. And callers would have to remember that the hook options were using comma delimiters while --mount-bind was still using colon delimiters [1]. [1]: https://github.com/opencontainers/runtime-tools/blame/8e3ecf45856fdeafcba3aa3534a0fd3d1f6258b0/man/oci-runtime-tool-generate.1.md#L286

[Mashimiao]

Hmm, we can reform generate to replace current options with json supporting. That would be a huge project....

[wking]

On Fri, Nov 24, 2017 at 07:32:14AM +0000, Ma Shimiao wrote: Hmm, we can reform `generate` to replace current options with json supporting. That would be a huge project....

Why would you do that? I think the viable approaches are: a. Drop ‘generate’ entirely, extending #379. This is easy ;). b. Drop the modification arguments from ‘generate’ and have it only be about providing “reasonable” templates (as in #477). This is not quite as trivial as (a), but is still fairly straightforward. c. Keep the modification arguments mostly as they are, but use JSON values for hooks, using the usual --…-add, --…-remove, and --…-remove-all pattern we use for arrays. For example: --hooks-prestart-add '{"path": "cmd", "args": ["cmd", "foo:bar"], "timeout": 5}' d. Like (c), except with some non-JSON syntax. You can achieve (c) without getting into “huge project” territory, because the only affected option would be for hooks (and possibly mounts, if you want to handle that similar situation at the same time). There are a few more arrays of objects in the spec: $ git describe --tags v1.0.1 $ git grep 'array of objects' config-linux.md:**`uidMappings`** (array of objects, OPTIONAL) describes the user namespace uid mappings from the host to the container. config-linux.md:**`gidMappings`** (array of objects, OPTIONAL) describes the user namespace gid mappings from the host to the container. config-linux.md:**`devices`** (array of objects, OPTIONAL) lists devices that MUST be available in the container. config-linux.md:**`devices`** (array of objects, OPTIONAL) configures the [device whitelist][cgroup-v1-devices]. config-linux.md:* **`weightDevice`** *(array of objects, OPTIONAL)* - an array of per-device bandwidth weights. config-linux.md:* **`throttleReadBpsDevice`**, **`throttleWriteBpsDevice`** *(array of objects, OPTIONAL)* - an array of per-device bandwidth rate limits. config-linux.md:* **`throttleReadIOPSDevice`**, **`throttleWriteIOPSDevice`** *(array of objects, OPTIONAL)* - an array of per-device IO rate limits. config-linux.md:**`hugepageLimits`** (array of objects, OPTIONAL) represents the `hugetlb` controller which allows to limit the config-linux.md:* **`priorities`** *(array of objects, OPTIONAL)* - specifies a list of objects of the priorities assigned to traffic originating from processes in the group and egressing the system on various interfaces. config-linux.md:* **`syscalls`** *(array of objects, OPTIONAL)* - match a syscall in seccomp. config-linux.md: * **`args`** *(array of objects, OPTIONAL)* - the specific syscall in seccomp. config.md:**`mounts`** (array of objects, OPTIONAL) specifies additional mounts beyond [`root`](#root). config.md:* **`rlimits`** (array of objects, OPTIONAL) allows setting resource limits for the process. config.md: * **`prestart`** (array of objects, OPTIONAL) is an array of [pre-start hooks](#prestart). config.md: * **`poststart`** (array of objects, OPTIONAL) is an array of [post-start hooks](#poststart). config.md: * **`poststop`** (array of objects, OPTIONAL) is an array of [post-stop hooks](#poststop). But you know rlimits, *idMappings, weightDevice, throttleReadBpsDevice, and throttleReadIOPSDevice values won't contain colons or commas. linux.devices might contain them, but linux.resources.devices won't. I'm less sure about hugepageLimits and priorities, although I think colons and commas are unlikely there. syscalls (and its args) are probably already addressed by the seccomp options. What are you thinking of that would be lots of work?

[Mashimiao]

among the three options, I prefer c, but not very satisfied with that. Because it make args format not unified.
We make changes like that first, whether to make huge changes depends on whether we need it or not in the future.

[wking]

among the three options...

I'd listed four, but GitHub decided to hide a bunch of my comment behind an ellipsis :p. I've edited the comment to remove a blank line, and now GitHub shows the whole thing. My (d) isn't particularly exciting, but towards the end I have some notes on other arrays of objects which may be interesting.

[Mashimiao]

anyway, I will try to modify hooks first. If really works well, then consider other options

[liangchenye]

fixed by #525
close