config-linux: Convert linux.namespaces from an array to an object #598
Conversation
wking
force-pushed
the
namespaces-object
branch
from
1ded350
to
44c0117
Compare
| "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?
| @@ -141,7 +141,7 @@ type Linux struct { | |||
| // If resources are specified, the cgroups at CgroupsPath will be updated based on resources. | |||
| 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>
wking
force-pushed
the
namespaces-object
branch
from
44c0117
to
2d83782
Compare
|
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.