Skip to content

Conversation

@Ace-Tang
Copy link
Contributor

@Ace-Tang Ace-Tang commented Dec 4, 2018

Signed-off-by: Ace-Tang [email protected]

Ⅰ. Describe what this PR did

support cli pouch exec --privileged

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

no need

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #2531 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2531      +/-   ##
==========================================
- Coverage    69.2%   69.05%   -0.15%     
==========================================
  Files         278      278              
  Lines       18494    18494              
==========================================
- Hits        12798    12771      -27     
- Misses       4243     4258      +15     
- Partials     1453     1465      +12
Flag Coverage Δ
#criv1alpha1test 31.37% <ø> (+0.04%) ⬆️
#criv1alpha2test 35.4% <ø> (+0.12%) ⬆️
#integrationtest 40.51% <ø> (-0.03%) ⬇️
#nodee2etest 32.59% <ø> (-0.2%) ⬇️
#unittest 26.91% <ø> (ø) ⬆️
Impacted Files Coverage Δ
cri/ocicni/cni_manager.go 58.82% <0%> (-11.77%) ⬇️
daemon/mgr/system.go 67.93% <0%> (-5.35%) ⬇️
daemon/mgr/events.go 96.29% <0%> (-3.71%) ⬇️
ctrd/client.go 66.92% <0%> (-2.31%) ⬇️
daemon/mgr/container_utils.go 84.81% <0%> (-1.9%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
pkg/meta/store.go 67.44% <0%> (-1.56%) ⬇️
ctrd/watch.go 80.28% <0%> (-1.41%) ⬇️
daemon/mgr/container.go 58.69% <0%> (-0.65%) ⬇️
cri/v1alpha2/cri.go 69.3% <0%> (-0.63%) ⬇️
... and 3 more

@rudyfly
Copy link
Collaborator

rudyfly commented Dec 5, 2018

How to add test case to check it works?
I think you should merge with this pr: #2533
@allencloud @HusterWan @fuweid WDYT

@Ace-Tang
Copy link
Contributor Author

Ace-Tang commented Dec 5, 2018

This is only a client part code, so I think you make it too complicated. We just add test in pr #2355 , this just two different type pr, no related @rudyfly

@Ace-Tang
Copy link
Contributor Author

Ace-Tang commented Dec 5, 2018

The test is failed, /cc @rudyfly

FAIL: /home/travis/gopath/src/github.com/alibaba/pouch/test/cli_run_network_test.go:28: PouchRunNetworkSuite.TestRunWithPing
/home/travis/gopath/src/github.com/alibaba/pouch/test/cli_run_network_test.go:34:
    res.Assert(c, icmd.Success)
/home/travis/gopath/src/github.com/alibaba/pouch/vendor/github.com/gotestyourself/gotestyourself/icmd/command.go:61:
    t.Fatalf("at %s:%d - %s\n", filepath.Base(file), line, err.Error())
... Error: at cli_run_network_test.go:34 - 
Command:  /usr/local/bin/pouch run --name TestRunWithPing registry.hub.docker.com/library/busybox:1.28 ping -c 3 www.taobao.com
ExitCode: 1
Error:    exit status 1
Stdout:   PING www.taobao.com (66.102.255.57): 56 data bytes
--- www.taobao.com ping statistics ---
3 packets transmitted, 0 packets received, 100% packet loss
Stderr:   
Failures:
ExitCode was 1 expected 0
Expected no error

AttachStdout: !e.Detach,
AttachStdin: !e.Detach && e.Interactive,
Privileged: false,
Privileged: e.Privileged,
Copy link
Contributor

Choose a reason for hiding this comment

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

there is only client side change. And I didn't see that the daemon can use the field.

I think you should add the NoNewPrivileges here.
https://github.com/alibaba/pouch/blob/8abf067257260a1570ed0a09bff2b16df9a1b899/daemon/mgr/container_exec.go#L88-L98

Copy link
Contributor Author

Choose a reason for hiding this comment

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

two different things, NoNewPrivileges https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt , Privileged here related with container level

Copy link
Contributor

Choose a reason for hiding this comment

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

anyway, LGTM. and wait for the CI.

@Ace-Tang
Copy link
Contributor Author

Ace-Tang commented Dec 5, 2018

closed by #2533

@Ace-Tang Ace-Tang closed this Dec 5, 2018
@Ace-Tang Ace-Tang deleted the add_priv_exec branch December 5, 2018 07:15
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.

4 participants