Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows process.username optional but POSIX process.uid required? #618

Closed
wking opened this issue Nov 10, 2016 · 2 comments
Closed

Windows process.username optional but POSIX process.uid required? #618

wking opened this issue Nov 10, 2016 · 2 comments

Comments

@wking
Copy link
Contributor

wking commented Nov 10, 2016

On POSIX (currently Linux and Solaris), uid and gid are required. My preferred approach here is to make those optional and use platform defaults:

If unset, the runtime will not attempt to manipulate the user ID (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly required properties.

The Windows username, on the other hand, is optional, although the default behavior is unclear. I see no discussion in #565 to suggest whether this was intentionally approved or not.

So the current spec seems to have different policies for whether user information is required, and it would be nice to make that consistent. I'd rather make it consistent by making the POSIX fields optional. Is the implicit support for an optional Windows username sufficient grounds to make the POSIX fields optional? Or was the optional username an accident? Or do maintainers approve of the current inconsistency?
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Nov 11, 2016
@wking
config: Make 'process.args' optional
35ee63b 
Since be59415 (Split create and start, 2016-04-01, opencontainers#384), it's
possible for a container process to never execute user-specified code
(e.g. you can call 'create', 'kill', 'delete' without calling
'start').  For folks who expect to do that, there's no reason to
define process.args.

The only other process property required for all platforms is 'cwd',
but the runtime's idler code isn't specified in sufficient detail for
the configuration author to have an opinion about what its working
directory should be.

On Linux and Solaris, 'user' is also required for 'uid' and 'gid'.  My
preferred approach here is to make those optional and define defaults
[1,2]:

  If unset, the runtime will not attempt to manipulate the user ID
  (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly
required properties [3,4,5].  With the current spec, one option could
be to make process optional (with the idler's working directory
unspecified) for OSes besides Linux and Solaris.  On Windows, username
is optional, but it's not clear how intentional that was [6].

[1]: opencontainers#417 (comment)
[2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#618

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Jan 11, 2017
@wking
config: Make 'process.args' optional
7071706 
Since be59415 (Split create and start, 2016-04-01, opencontainers#384), it's
possible for a container process to never execute user-specified code
(e.g. you can call 'create', 'kill', 'delete' without calling
'start').  For folks who expect to do that, there's no reason to
define process.args.

The only other process property required for all platforms is 'cwd',
but the runtime's idler code isn't specified in sufficient detail for
the configuration author to have an opinion about what its working
directory should be.

On Linux and Solaris, 'user' is also required for 'uid' and 'gid'.  My
preferred approach here is to make those optional and define defaults
[1,2]:

  If unset, the runtime will not attempt to manipulate the user ID
  (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly
required properties [3,4,5].  With the current spec, one option could
be to make process optional (with the idler's working directory
unspecified) for OSes besides Linux and Solaris.  On Windows, username
is optional, but it's not clear how intentional that was [6].

[1]: opencontainers#417 (comment)
[2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#618

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Jan 11, 2017
@wking
config: Make 'process.args' optional
e45ddba 
Since be59415 (Split create and start, 2016-04-01, opencontainers#384), it's
possible for a container process to never execute user-specified code
(e.g. you can call 'create', 'kill', 'delete' without calling
'start').  For folks who expect to do that, there's no reason to
define process.args.

The only other process property required for all platforms is 'cwd',
but the runtime's idler code isn't specified in sufficient detail for
the configuration author to have an opinion about what its working
directory should be.

On Linux and Solaris, 'user' is also required for 'uid' and 'gid'.  My
preferred approach here is to make those optional and define defaults
[1,2]:

  If unset, the runtime will not attempt to manipulate the user ID
  (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly
required properties [3,4,5].  With the current spec, one option could
be to make process optional (with the idler's working directory
unspecified) for OSes besides Linux and Solaris.  On Windows, username
is optional, but it's not clear how intentional that was [6].

[1]: opencontainers#417 (comment)
[2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#618

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Jan 11, 2017
@wking
config: Make 'process.args' optional
bebdd29 
Since be59415 (Split create and start, 2016-04-01, opencontainers#384), it's
possible for a container process to never execute user-specified code
(e.g. you can call 'create', 'kill', 'delete' without calling
'start').  For folks who expect to do that, there's no reason to
define process.args.

The only other process property required for all platforms is 'cwd',
but the runtime's idler code isn't specified in sufficient detail for
the configuration author to have an opinion about what its working
directory should be.

On Linux and Solaris, 'user' is also required for 'uid' and 'gid'.  My
preferred approach here is to make those optional and define defaults
[1,2]:

  If unset, the runtime will not attempt to manipulate the user ID
  (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly
required properties [3,4,5].  With the current spec, one option could
be to make process optional (with the idler's working directory
unspecified) for OSes besides Linux and Solaris.  On Windows, username
is optional, but it's not clear how intentional that was [6].

[1]: opencontainers#417 (comment)
[2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#618

Signed-off-by: W. Trevor King <wking@tremily.us>
@crosbymichael
Copy link
Member

No, both should be made explicit unless there is something on windows that prohibits this.

wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Feb 27, 2017
@wking
config: Make process optional
c41ea83 
Since be59415 (Split create and start, 2016-04-01, opencontainers#384), it's
possible for a container process to never execute user-specified code
(e.g. you can call 'create', 'kill', 'delete' without calling
'start').  For folks who expect to do that, there's no reason to
define process.args.

The only other process property required for all platforms is 'cwd',
but the runtime's idler code isn't specified in sufficient detail for
the configuration author to have an opinion about what its working
directory should be.

On Linux and Solaris, 'user' is also required for 'uid' and 'gid'.  My
preferred approach here is to make those optional and define defaults
[1,2]:

  If unset, the runtime will not attempt to manipulate the user ID
  (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be explicitly
required properties [3,4,5].  With the current spec, one option could
be to make process optional (with the idler's working directory
unspecified) for OSes besides Linux and Solaris.  On Windows, username
is optional, but that was likely accidental [6].

So an unspecified 'process' would leave process.cwd and process.user
unset.  What that means for the implementation-defined container
process between 'create' and 'start' is unclear, but clarifying how
that is handled is a separate issue [7] independent of whether
'process' is optional or not.

[1]: opencontainers#417 (comment)
[2]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#618 (comment)
[7]: opencontainers#700

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Apr 6, 2017
@wking
specs-go/round_trip_test: Add a minimal Windows config
bbd0abc 
process.user.username is not strictly required yet, but it is intended
to be [1], and it doesn't seem worth making Process.User a pointer in
the meantime.

[1]: opencontainers#618 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Apr 6, 2017
@wking
specs-go/round_trip_test: Add a minimal Windows config
2c17c87 
process.user.username is not strictly required yet, but it is intended
to be [1], and it doesn't seem worth making Process.User a pointer in
the meantime.

[1]: opencontainers#618 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Apr 6, 2017
@wking
specs-go/round_trip_test: Add a minimal Windows config
767e0ce 
process.user.username is not strictly required yet, but it is intended
to be [1], and it doesn't seem worth making Process.User a pointer in
the meantime.

[1]: opencontainers#618 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Apr 7, 2017
@wking
config: Require process.user.username on Windows
4e3945f 
On POSIX (currently Linux and Solaris), `uid` and `gid` are
required. My preferred approach here is to make those optional and use
platform defaults [1,2]:

    If unset, the runtime will not attempt to manipulate the user ID
    (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be
explicitly required properties [3,4,5].

The Windows `username`, on the other hand, was optional, although the
default behavior is unclear.  I see no discussion in f9e48e0
(Windows: User struct changes, 2016-09-14, opencontainers#565) or its pull-request
discussion to suggest whether this was intentionally approved or not.
When I asked whether the optional-ness was intentional, Michael said
[6]:

  No, both should be made explicit unless there is something on
  windows that prohibits this.

So this commit is making that happen.

[1]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
[2]: opencontainers#417 (comment)
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#618 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking mentioned this issue Apr 7, 2017
Closed
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue Apr 24, 2017
@wking
config: process.user.username is implementation-defined on Windows
a3ef036 
On POSIX (currently Linux and Solaris), `uid` and `gid` are
required. My preferred approach here is to make those optional and use
platform defaults [1,2]:

    If unset, the runtime will not attempt to manipulate the user ID
    (e.g. not calling setuid(2) or similar).

But the maintainer consensus is that they want those to be
explicitly required properties [3,4,5].

The Windows `username`, on the other hand, was optional, although the
default behavior is unclear.  I see no discussion in f9e48e0
(Windows: User struct changes, 2016-09-14, opencontainers#565) or its pull-request
discussion to suggest whether this was intentionally approved or not.
When I asked whether the optional-ness was intentional, Michael said
[6]:

  No, both should be made explicit unless there is something on
  windows that prohibits this.

However, when I filed a pull request to make the property required,
John pushed back [7] and prefered implementation-defined default
behavior.  I'm still not clear if that satisfies Michael's "prohibits"
condition, but having optional user values is closer to my personal
preference than requiring the property, and John seems to be fairly
strongly against requiring the property, so this commit documents the
default value to make the OPTIONAL-ness useful.

I've also added the property to the JSON Schema for validation.  The
empty-string bit follows wording from 'annotations', and avoids
ambiguity with the non-pointer Go property.  I doubt empty-string
usernames would work, and having the restriction in the spec allows
for us to validate this in runtime-tools (vs. passing validation and
then failing to launch a container when the runtime chokes on the
empty string).

[1]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/DWdystx5X3A
[2]: opencontainers#417 (comment)
     Subject: Exposing platform defaults
     Date: Thu, 14 Jan 2016 15:36:26 -0800
     Message-ID: <20160114233625.GN6362@odin.tremily.us>
[3]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-05-04-17.00.log.html#l-44
[4]: opencontainers#417 (comment)
[5]: opencontainers#417 (comment)
[6]: opencontainers#618 (comment)
[7]: opencontainers#760 (comment)
[8]: opencontainers#760 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this issue May 11, 2017
@wking
specs-go/round_trip_test: Add a minimal Windows config
bdae383 
process.user.username is not strictly required yet, but it is intended
to be [1], and it doesn't seem worth making Process.User a pointer in
the meantime.

[1]: opencontainers#618 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented May 31, 2017

Or do maintainers approve of the current inconsistency?

They do.

@wking wking closed this as completed May 31, 2017
@wking wking mentioned this issue Mar 12, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@wking @crosbymichael