stage1/fly: make run/enter honour uid/gid/suppGids #3717
Conversation
|
Can one of the admins verify this patch? |
|
@lucab PTAL |
|
ok to test |
|
It looks like this introduced a new file which is missing the license boilerplate, thus failing travis (I didn't perform a review pass yet). |
|
@lucab Oops, sorry, should be OK now. |
stage1_fly/enter/main.go
Outdated
|
|
||
| func getRuntimeApp(pm *schema.PodManifest) (*schema.RuntimeApp, error) { | ||
| if len(pm.Apps) != 1 { | ||
| return nil, fmt.Errorf("flavor %q only supports 1 application per Pod for now", flavor) |
There was a problem hiding this comment.
I don't think this needs to be generic on flavor name, as this directory/subtree is specific to fly.
stage1_fly/enter/main.go
Outdated
| @@ -47,6 +55,33 @@ func getRootDir(pid string) (string, error) { | |||
| return os.Readlink(rootLink) | |||
| } | |||
|
|
|||
| func getPodManifest() (*schema.PodManifest, error) { | |||
There was a problem hiding this comment.
I think this has quite a lot in common with common/types.LoadPod() and can probably just use that, or refactor/expose/move some logic from there.
stage1_fly/enter/main.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| flavor = "fly" |
There was a problem hiding this comment.
See comment below about removing this.
stage1_fly/process-credentials.go
Outdated
| // lock the current goroutine to its current OS thread. | ||
| // This will force the subsequent syscalls to be executed in the same OS thread as Setresuid, and Setresgid, | ||
| // see https://github.com/golang/go/issues/1435#issuecomment-66054163. | ||
| runtime.LockOSThread() |
There was a problem hiding this comment.
I think this should be moved as one of the first steps in "enter" and "run" and here just mention that it affects only the current thread and may misbehave with free goroutines.
stage1_fly/process-credentials.go
Outdated
| supplementaryGIDs []int | ||
| } | ||
|
|
||
| func LookupProcessCredentials(ra *schema.RuntimeApp, rfs string) (*ProcessCredentials, error) { |
There was a problem hiding this comment.
Is it possible to re-use parseUserGroup here (after exporting it)?
|
@lucab I'm pretty sure I addressed all your comments now. Thanks for the review. |
stage1_fly/process-credentials.go
Outdated
| } | ||
|
|
||
| func SetProcessCredentials(c *ProcessCredentials, diag *rktlog.Logger) error { | ||
| diag.Printf("setting credentials: uid=%d, gid=%d, suppGids=%v", c.uid, c.gid, c.supplementaryGIDs) |
There was a problem hiding this comment.
I think you can move this debug logging to the caller and completely remove the logger from this file.
stage1_fly/process-credentials.go
Outdated
| var err error | ||
|
|
||
| // mock up a pod so we can call ParseUserGroup | ||
| pod := &stage1commontypes.Pod{ |
There was a problem hiding this comment.
Why do you want to mock a pod here instead of using the real one you got in main? This is generally dangerous because of parameters implicitely initialized to their default value.
There was a problem hiding this comment.
I realized I only needed to mock the pod in enter (where we don't have one). For run, I'm now using the real one.
|
@lucab If you're happy with the mocking of a pod just for enter (where we don't have one available), this should be good to go. |
Refactored common functionality out of run.
Fixes #3392, fixes #3716.