Handle user:group specification in Dockerfile#471
Handle user:group specification in Dockerfile#471openshift-bot merged 1 commit intoopenshift:masterfrom
Conversation
|
@bparees ptal |
| if strings.ToLower(parts[0]) != "user" { | ||
| continue | ||
| } | ||
| uname := extractUser(parts[1]) |
There was a problem hiding this comment.
is this also trimming the uname?
There was a problem hiding this comment.
it's not... it should. Will fix
00453b3 to
bc7a8d1
Compare
|
lgtm. |
| name: "AllowedUIDs is set, numeric user with group", | ||
| allowedUIDs: rangeList("1-"), | ||
| user: "5:5000", | ||
| expectErr: false, |
There was a problem hiding this comment.
Should add a test for:
5:users
That is, named group if want to ensure coverage of all cases.
|
LGTM, pending fixing @GrahamDumpleton check |
|
Hold off merging this. As per comments on original issue, this may be pointless as deployer may not use the group anyway. If that is the case, need to work out if deployer should/can honour the group in Docker image data. |
|
@GrahamDumpleton I believe we still need the fix for s2i not to reject images unnecessarily. We're simply not interpreting the user spec in a Dockerfile correctly right now. |
|
@csrwng +1, i think this fix is as good as it can get from an s2i perspective. lgtm. |
|
I have created openshift/origin#8618 for wider issue of changing Kubernetes/OpenShift to honour the group. This change in this patch is still useful in context of standalone S2I usage, but images produced still aren't going to necessarily run properly on OpenShift. |
|
@csrwng @GrahamDumpleton are we in agreement that this PR is as complete as it's going to get? |
|
yes |
|
[merge] |
|
Source To Image Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pr_sti/68/) |
|
Evaluated for source to image merge up to 5b3f5ca |
When checking whether a user is in the allowed range, a USER directive with uid:group is now allowed.