Uh oh!
There was an error while loading. Please reload this page.
config-linux: Convert linux.namespaces from an array to an object - #598
config-linux: Convert linux.namespaces from an array to an object#598wking wants to merge 1 commit into
Conversation
| "ipc": {}, | ||
| "uts": {}, | ||
| "user": {}, | ||
| "cgroup": {} |
There was a problem hiding this comment.
Then empty and absence have different meanings, I think that's some confusing we are trying to avoid.
There was a problem hiding this comment.
I'm ok making that distinction (like I'm ok distinguishing unset and null). If the consensus is that we shouldn't make that distinction, alternatives include:
a. Using anyOf to allow boolean or namespace-object values and requiring path in namespace-objects. E.g. "pid": true or "pid": {"path": "..."}. This is clear in JSON Schema, but may be awkward to unmarshal in Go.
b. Replace namespace objects with the path string, and distinguish between unset, empty, and non-empty strings. This makes namespace payload extension more difficult, but I don't hear anyone calling for namespace extension at the moment.
c. Keep the namespace object, require path, and distinguish between empty and non-empty strings. This is just exchanging empty-string checking (one level deeper) for the empy-object check that currently concerns you.
There was a problem hiding this comment.
d. Adding a new boolean to the namespace object and erroring if new is true and path is set/non-empty.
There was a problem hiding this comment.
I'm leaning towards b, as you said, as long as we don't have the need for namespace extension, it should be enough.
For better future extension, c looks better though.
a will be really disaster for json unmarshaling, and d looks redundant.
There was a problem hiding this comment.
Both (b) and (c) are really close to the PR's current approach. Do folks really see a significant difference between switching on empty strings vs. empty objects?
| CgroupsPath *string `json:"cgroupsPath,omitempty"` | ||
| // Namespaces contains the namespaces that are created and/or joined by the container | ||
| Namespaces []LinuxNamespace `json:"namespaces,omitempty"` | ||
| Namespaces map[string]LinuxNamespace `json:"namespaces,omitempty"` |
There was a problem hiding this comment.
It should be map[LinuxNamespaceType]LinuxNamespace, what's more, maybe we need to rename LinuxNamespace to LinuxNamespaceProperty
Namespaces do not need repeated entries and the ordering is handled by the runtime regardless of the spec ordering (e.g. in runC [1]). Using an object leans on the new wording from eeaccfa (glossary: Make objects explicitly unordered and forbid duplicate names, 2016-09-27, opencontainers#584) to make both of those points explicit. [1]: opencontainers/runc#977 Subject: nsenter: guarantee correct user namespace ordering Signed-off-by: W. Trevor King <wking@tremily.us>
cyphar
commented
Nov 1, 2016
This looks fairly unwieldy to me (given the path comments), why can't we just say in the spec that duplicate entries are forbidden and call it a day? |
Namespaces do not need repeated entries and the ordering is handled by the runtime regardless of the spec ordering (e.g. in opencontainers/runc#977). Using an object leans on the new wording from #584 to make both of those points explicit.
An alternative to #597.
I feel like we've talked this idea over before, but the only reference I could dig up was my passing mention of “probably convert the array to an object keyed by name” here.