Uh oh!
There was an error while loading. Please reload this page.
config: process.user.username is implementation-defined on Windows - #760
Closed
wking wants to merge 1 commit into
Closed
config: process.user.username is implementation-defined on Windows#760wking wants to merge 1 commit into
wking wants to merge 1 commit into
Conversation
lowenna
commented
Apr 7, 2017
Contributor
Does NOT LGTM. This absolutely IS an optional field on Windows. |
wking
commented
Apr 7, 2017
via email
ContributorAuthor
On Thu, Apr 06, 2017 at 10:43:58PM -0700, John Howard wrote:
This absolutely IS an optional field on Windows. uid/gid are technically optional on Linux too (just don't call setuid,
and you inherit the runtime caller uid/gid, modulo namespace mapping
[1]). The spec has a policy (which I don't agree with, but I'm not a
maintainer) that the uid and gid values are required anyway (on
Linux). See links in my original post for maintainers on this point.
If ‘username’ stays optional, then I think the spec should be updated
to clarify what happens when it is not specified. For example, is
there a default username (e.g. is it inherited from the runtime
caller)?
[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/config-linux.md#user-namespace-mappings |
Contributor
And with an explicitly added user Feel free to clarify. But this PR does not LGTM. And no, it's not inherited at all. |
wking
commented
Apr 7, 2017
ContributorAuthor
So which of the following would make more sense in the spec.
or
? |
lowenna
commented
Apr 24, 2017
Contributor
I would prefer |
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
commented
Apr 24, 2017
via email
ContributorAuthor
On Mon, Apr 24, 2017 at 09:31:32AM -0700, John Howard wrote:
I would prefer `The default username is implementation-defined.` |
crosbymichael
commented
May 9, 2017
Member
Closing based on feedback from the windows team |
wking
commented
May 9, 2017
ContributorAuthor
What feedback from the Windows team? The current commit (a3ef036) has been endorsed by @jhowardmsft, who's the only Windows member involved in this PR. Without this PR, the property remains:
And as I said earlier, I'm fine dropping the new “but not the empty string” requirement if anyone has issues with it. |
wking
commented
May 10, 2017
ContributorAuthor
Since the pivot from REQUIRED to OPTIONAL, the JSON Schema change in this PR mostly matches that which just landed in #703. So the only remaining features of this PR (which I think we still want) are:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On POSIX (currently Linux and Solaris),
uidandgidare required. My preferred approach here is to make those optional and use platform defaults:But the maintainer consensus is that they want those to be explicitlyrequiredproperties.
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. And in #618 (where I asked about the inconsistency), @crosbymichaelsaid:So this commit is making that happen.
Pinging some Windows folks for review of the “unless there is something on windows that prohibits this” condition: @jhowardmsft, @RobDolinMS, @jstarks.
Note that there is also a JSON Schema PR for
usernamein flight with #703, but that is orthogonal to this change (the JSON Schema cannot requireusernameeven if the field is REQUIRED on Windows, because it is not required, or even specified, on other OSes).Fixes#618.