Windows: User struct changes #565
Merged
Conversation
This file contains 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
wking
reviewed
Sep 14, 2016
| type User struct { | ||
| // UID is the user id. (this field is platform dependent) | ||
| UID uint32 `json:"uid" platform:"linux,solaris"` | ||
| // GID is the group id. (this field is platform dependent) | ||
| GID uint32 `json:"gid" platform:"linux,solaris"` | ||
| // AdditionalGids are additional group ids set for the container's process. (this field is platform dependent) | ||
| AdditionalGids []uint32 `json:"additionalGids,omitempty" platform:"linux,solaris"` | ||
| // User is the user name. (this field is platform dependent) | ||
| User string `json:"user,omitempty" platform:"windows"` |
There was a problem hiding this comment.
To avoid User.User, can we call this Username or something? And in that case it could be either Process.User.Username or Process.Username instead of Process.User.User. I like Process.Username best, as long as Windows doesn't plan on needing a more complicated structure in the future (in which case it's probably better to stay under Process.User).
There was a problem hiding this comment.
Good idea. I prefer Username. Updated
Unfortunately though, I can't predict the future as well 😸
lowenna
force-pushed
the
jjh/user
branch
2 times, most recently
from
510e5de to
08cf7d4
Compare
wking
reviewed
Sep 14, 2016
| "cwd": "c:\\foo", | ||
| "args": [ | ||
| "someapp.exe"], | ||
| ], |
There was a problem hiding this comment.
@jhowardmsft I think you may have duplicate instances of "]," on lines 212 and 213.
There was a problem hiding this comment.
Good catch. That's what I get for copy/pasting from the Solaris example immediately above it. Fixed both 😄
There was a problem hiding this comment.
@RobDolinMS Yes, fixed.
Signed-off-by: John Howard <[email protected]>
|
LGTM |
1 similar comment
|
LGTM |
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Sep 15, 2016
We dropped these in 4774080 (specs-go/config: Drop "this field is platform dependent", 2016-09-14, opencontainers#568) but f9e48e0 (Windows: User struct changes, 2016-09-14, opencontainers#565) was developed in parallel and brought in a new one. Signed-off-by: W. Trevor King <[email protected]>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Apr 7, 2017
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: <[email protected]>
[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 <[email protected]>
wking
added a commit
to wking/opencontainer-runtime-spec
that referenced
this pull request
Apr 24, 2017
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: <[email protected]>
[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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Signed-off-by: John Howard [email protected]
Extracting pieces from the proof of concept PR for Windows OCI support at #504. This PR modified the
Userstruct by fixing the description, adding a Windows-specificuserfield, and updating the documentation to include the changes, plus provide a sample JSON in the context of theProcessstructure.