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.

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

Go1.5 compatibility fix #166

Merged
merged 1 commit into from
Aug 3, 2015
Merged

Go1.5 compatibility fix #166

merged 1 commit into from
Aug 3, 2015

Conversation

codido
Copy link
Contributor

@codido codido commented Aug 1, 2015

This fixes an issue with Go 1.5, introduced by:
golang/go@f5c60ff

@@ -21,6 +24,15 @@ func (c *linuxContainer) addUidGidMappings(sys *syscall.SysProcAttr) error {
sys.GidMappings[i].HostID = gm.HostID
sys.GidMappings[i].Size = gm.Size
}

// Set sys.GidMappingsEnableSetgroups to true, so the process's
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be done by build tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree a build tag is cleaner than reflection, but wanted to avoid adding another file or even two (depending on whether we'd like to duplicate addUidGidMappings or add another function).

If you'd still like to do this with build tags, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to using build tags here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, updated this patch accordingly

@codido
Copy link
Contributor Author

codido commented Aug 2, 2015

Just noticed #70, which makes the two seccomp patches irrelevant. Should I remove these from this pull request, or keep them until the above is merged?

@mrunalp
Copy link
Contributor

mrunalp commented Aug 3, 2015

@GitIdo I think you could remove those fixes or create a separate PR for them so we can get the other fix merged.

A boolean field named GidMappingsEnableSetgroups was added to
SysProcAttr in Go1.5. This field determines the value of the process's
setgroups proc entry.

Since the default is to set the entry to 'deny', calling setgroups will
fail on systems running kernels 3.19+.

Set GidMappingsEnableSetgroups to true so setgroups wont be set to
'deny'.

Signed-off-by: Ido Yariv <ido@wizery.com>
@codido codido changed the title A few fixes Go1.5 compatibility fix Aug 3, 2015
@crosbymichael
Copy link
Member

LGTM

1 similar comment
@mrunalp
Copy link
Contributor

mrunalp commented Aug 3, 2015

LGTM

mrunalp pushed a commit that referenced this pull request Aug 3, 2015
Go1.5 compatibility fix
@mrunalp mrunalp merged commit ce0a339 into opencontainers:master Aug 3, 2015
@codido codido deleted the fixes branch August 8, 2015 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants