feature: add capabilities for exec process #2533
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2533 +/- ##
========================================
+ Coverage 69.2% 69.3% +0.1%
========================================
Files 278 278
Lines 18494 18513 +19
========================================
+ Hits 12798 12830 +32
+ Misses 4243 4230 -13
Partials 1453 1453
|
daemon/mgr/container_exec.go
Outdated
| Permitted: capList, | ||
| Inheritable: capList, | ||
| } | ||
| } else { |
There was a problem hiding this comment.
just combine else and if to be else if { in this case? @Ace-Tang
There was a problem hiding this comment.
But no else if here
There was a problem hiding this comment.
Since in the else, there is only one if. So I am wondering if we could make it
else if spec, err := mgr.spec(c); err == nil {
process.Capabilities = spec.Process.Capabilities
}
daemon/mgr/container_utils.go
Outdated
| @@ -125,6 +129,23 @@ func (mgr *ContainerManager) getRuntime(runtime string) (string, error) { | |||
| return rPath, nil | |||
| } | |||
|
|
|||
| // spec returns container runtime spec, unmarshal spec from config.json | |||
| // TODO: when runtime type can be specified, it need fix | |||
| func (mgr *ContainerManager) spec(c *Container) (*specs.Spec, error) { | |||
There was a problem hiding this comment.
Could we rename this function? Actually I am afraid that we could input meaningful details in the function name, for example GetContainerSpec? @Ace-Tang
There was a problem hiding this comment.
sure, but getContainerSpec, it won't export
| @@ -11,6 +11,7 @@ import ( | |||
| "github.com/alibaba/pouch/pkg/randomid" | |||
| "github.com/alibaba/pouch/pkg/streams" | |||
| "github.com/alibaba/pouch/pkg/user" | |||
| "github.com/docker/docker/daemon/caps" | |||
There was a problem hiding this comment.
Can we replace this package?
There was a problem hiding this comment.
The package is just there, in vendor , you can change it if you like
add current container capabilities for exec process Signed-off-by: Ace-Tang <aceapril@126.com>
Signed-off-by: Ace-Tang <aceapril@126.com>
Signed-off-by: Ace-Tang <aceapril@126.com>
| @@ -54,6 +55,7 @@ func (e *ExecCommand) addFlags() { | |||
| flagSet.BoolVarP(&e.Interactive, "interactive", "i", false, "Open container's STDIN") | |||
| flagSet.StringVarP(&e.User, "user", "u", "", "Username or UID (format: <name|uid>[:<group|gid>])") | |||
| flagSet.StringArrayVarP(&e.Envs, "env", "e", []string{}, "Set environment variables") | |||
| flagSet.BoolVar(&e.Privileged, "privileged", false, "Give extended privileges to the exec process") | |||
There was a problem hiding this comment.
Do we give extended or all capabilities to the exec process ?
There was a problem hiding this comment.
We give all capabilities, same with run container, the code in daemon show this
|
LGTM |
pouchrobot
added
the
LGTM
add current container capabilities for exec process
Signed-off-by: Ace-Tang aceapril@126.com
Ⅰ. Describe what this PR did
add current container capabilities for exec process
if container is created by docker and taken over by pouchd, no config.json can found under current path, runc exec is good even without these capabilities in exec process
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
add test for exec priviledged
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews