Isolate platform specific fields

[mlaventure]

The same way that platform specific fields are isolated into their own type at the top level (i.e. inside Spec). Other platform specific fields down the chain should also be isolated.

This would provide consistency throughout the type, make for a clearer reading/understanding of what's supported by a platform and avoid passing through/dealing with unneeded data.

[wking]

On Thu, Nov 10, 2016 at 07:27:04AM -0800, Kenfe-Mickaël Laventure wrote:

The same way that platform specific fields are isolated into their
own type at the top level (i.e. inside Spec). Other platform
specific fields down the chain should also be isolated.

I expect you're talking about process.capabilities and such. The
current policy on that (as far as I'm aware) is [1,2]:

If any platform cannot use your setting, namespace it, unless that
would seem weird for asthetic reasons we're punting on defining.

[mlaventure]

IMHO this has nothing to do with aesthetics.

Properly isolating platform specific fields for me:

• ensure consistency within the spec itself
• allow for a clean path to add new platform specific field without clutter
• prevent sending transmitting useless data around (by virtue that all those top level platform specific struct would be pointers)

[crosbymichael]

@mlaventure What would be the actual change to the spec? Are you suggesting having prefixes on all the Types or Types and Fields?

[mlaventure]

@crosbymichael no prefixes, more like regrouping them under a prefixed struct.

For instance:

type ProcessLinux struct {
    // Capabilities are Linux capabilities that are kept for the container.
    Capabilities []string `json:"capabilities,omitempty" platform:"linux"`
    // Rlimits specifies rlimit options to apply to the process.
    Rlimits []Rlimit `json:"rlimits,omitempty" platform:"linux"`
    // NoNewPrivileges controls whether additional privileges could be gained by processes in the container.
    NoNewPrivileges bool `json:"noNewPrivileges,omitempty" platform:"linux"`
    // ApparmorProfile specifies the apparmor profile for the container.
    ApparmorProfile string `json:"apparmorProfile,omitempty" platform:"linux"`
    // SelinuxLabel specifies the selinux context that the container process is run as.
    SelinuxLabel string `json:"selinuxLabel,omitempty" platform:"linux"`
}

// Process contains information to start a specific application inside the container.
type Process struct {
    // Terminal creates an interactive terminal for the container.
    Terminal bool `json:"terminal,omitempty"`
    // ConsoleSize specifies the size of the console.
    ConsoleSize Box `json:"consoleSize,omitempty"`
    // User specifies user information for the process.
    User User `json:"user"`
    // Args specifies the binary and arguments for the application to execute.
    Args []string `json:"args"`
    // Env populates the process environment for the process.
    Env []string `json:"env,omitempty"`
    // Cwd is the current working directory for the process and must be
    // relative to the container's root.
    Cwd string `json:"cwd"`

    Linux *ProcessLinux `json:"linux,omitempty" platform:"linux"`
}

Probably more controversial would be something like this for user:

type UserUnix struct {
    // UID is the user id.
    UID uint32 `json:"uid" platform:"linux,solaris"`
    // GID is the group id.
    GID uint32 `json:"gid" platform:"linux,solaris"`
    // AdditionalGids are additional group ids set for the container's process.
    AdditionalGids []uint32 `json:"additionalGids,omitempty" platform:"linux,solaris"`
}

type UserWindows struct {
    // Username is the user name.
    Username string `json:"username,omitempty" platform:"windows"`
}

// User specifies specific user (and group) information for the container process.
type User struct {
    Unix *UserUnix `json:"unix,omitempty" platform:"linux,solaris"`

    Windows *UserWindows `json:"windows,omitempty" platform:"windows"`
}

[cyphar]

My issue with this is that it is essentially equivalent to creating the entire spec be under "linux": { } or "windows": { } scopes. This means that generic tooling won't really be able to handle generating the generic user options and so on. Which defeats the whole point of us having these generic concepts like user and so on.

Now, for things like UID and GID it might actually be helpful to take user specifications (like user:group or uid:gid) but I recognise that's not a popular belief. But it would remove the need for having Username and UID. But the whole point of the "platform" tag is that you can do verification automatically with reflect (without needing to specify the platform in every structure member access).

Not to mention that Config.Process.User.Unix.UID is an eyesore and also will lead to duplication of information in different structures.

[mlaventure]

I personally prefer avoiding having to use reflect.

Generic options would stay outside the scope struct, and since those structs still have the platform tags, those wanting to use reflect can still do so.

To avoid the long path, the field can be named UserUnix and the struct embedded to get something like Config.Process.UserUnix.UID

[wking]

On Mon, Nov 14, 2016 at 08:05:46AM -0800, Kenfe-Mickaël Laventure wrote:

To avoid the long path, the field can be named UserUnix and the
struct embedded to get something like Config.Process.UserUnix.UID

With this approach, either the JSON field will have to be
processs.userUnix or the parsing Go will have to use interface{}-based
JSON unmarshalling. I'd rather avoid the former and don't think it
will be easy to talk folks into the latter.

[crosbymichael]

We discussed this on the OCI call today and are going to just use the platform tags for this type of issue. We already use the tags for json marshal and marshaling