refactor: refactor security spec #2271

Ace-Tang · 2018-09-21T09:57:47Z

Ⅰ. Describe what this PR did

Split #2115 PR into 3 independent PR， here is the first, still need 2 more PRs in the TODO List.

add opencontainers/runc/libcontainer/apparmor vendor
security parameter include seccomp, apparmor and no-new-privileges
are not implement good, refactor these part when create new spec.

TODO List

use pouchd default spec.
remove containerd default spec.

Ⅱ. Does this pull request fix one issue?

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

no.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov · 2018-09-21T09:57:48Z

Codecov Report

Merging #2271 into master will increase coverage by 4.37%.
The diff coverage is 58.18%.

@@            Coverage Diff             @@
##           master    #2271      +/-   ##
==========================================
+ Coverage   64.09%   68.47%   +4.37%     
==========================================
  Files         271      275       +4     
  Lines       18269    18278       +9     
==========================================
+ Hits        11710    12515     +805     
+ Misses       5285     4339     -946     
- Partials     1274     1424     +150

Flag	Coverage Δ
#criv1alpha1test	`31.86% <53.06%> (?)`
#criv1alpha2test	`35.71% <53.06%> (-0.08%)`	⬇️
#integrationtest	`39.79% <57.69%> (+0.01%)`	⬆️
#nodee2etest	`33.28% <50%> (+0.09%)`	⬆️
#unittest	`25.53% <0%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
daemon/mgr/container_utils.go	`86.7% <ø> (+2.65%)`	⬆️
daemon/mgr/spec_linux.go	`74.51% <ø> (-0.26%)`	⬇️
daemon/mgr/spec_process.go	`80% <ø> (+12.98%)`	⬆️
daemon/mgr/container_types.go	`83.33% <ø> (ø)`	⬆️
daemon/mgr/spec_apparmor_linux_unsupported.go	`100% <100%> (ø)`
daemon/mgr/spec_apparmor_linux.go	`20% <20%> (ø)`
daemon/mgr/spec_seccomp_unsupported.go	`50% <50%> (ø)`
pkg/system/system.go	`68.11% <54.54%> (-2.58%)`	⬇️
daemon/mgr/container_validation.go	`45% <55.55%> (+0.62%)`	⬆️
daemon/mgr/spec_seccomp_linux.go	`78.94% <78.94%> (ø)`
... and 19 more

allencloud · 2018-09-30T02:12:17Z

pkg/system/sysinfo.go

 )

+// SysInfo defines system info on current machine
+type SysInfo struct {


In the built-in package, Sysinfo refers to http://docs.studygolang.com/pkg/syscall/#Sysinfo_t :

type Sysinfo_t struct { Uptime int64 Loads [3]uint64 Totalram uint64 Freeram uint64 Sharedram uint64 Bufferram uint64 Totalswap uint64 Freeswap uint64 Procs uint16 Pad uint16 Pad_cgo_0 [4]byte Totalhigh uint64 Freehigh uint64 Unit uint32 X_f [0]byte Pad_cgo_1 [4]byte }

So, I am wondering if it is proper to change the definition of SysInfo. @Ace-Tang

This is not proper, also I recognize the file name is also not proper, it should be system.go, and change the Sysinfo to Info. That will make sense. @allencloud , WDYT.

Discussed offline. And @Ace-Tang would do some update.

fuweid

please rebase the commit and I will merge it

fuweid · 2018-11-01T01:19:03Z

vendor/vendor.json

+			"checksumSHA1": "gVVY8k2G3ws+V1czsfxfuRs8log=",
+			"path": "github.com/opencontainers/runc/libcontainer/apparmor",
+			"revision": "ab4a8191679806893207d9c1e507a77ff46dafaa",
+			"revisionTime": "2018-01-12T20:11:04Z"


could we align this package with other existing /runc/libcontainer package?

I think it add by govender, different directory will be a log.

add package opencontainers/runc/libcontainer/apparmor for apparmor used. Signed-off-by: Ace-Tang <aceapril@126.com>

Ace-Tang · 2018-11-01T05:52:15Z

ping @allencloud @fuweid , already updated.

fuweid · 2018-11-01T06:06:44Z

Makefile

@@ -74,7 +76,7 @@ build: build-daemon build-cli ## build PouchContainer both daemon and cli binari
 build-daemon: modules plugin ## build PouchContainer daemon binary
 	@echo "$@: bin/${DAEMON_BINARY_NAME}"
 	@mkdir -p bin
-	@GOOS=linux go build -ldflags ${DEFAULT_LDFLAGS} -o bin/${DAEMON_BINARY_NAME} -tags 'selinux'
+	@GOOS=linux go build -ldflags ${DEFAULT_LDFLAGS} ${GOBUILD_TAGS}-o bin/${DAEMON_BINARY_NAME}


add space before -o

I'm just curious about the CI. In my local, make job will fail.

$ BUILDTAGS="selinux seccomp apparmor" make build volume modules build plugin build-daemon: bin/pouchd GOOS=linux go build -ldflags "-X github.com/alibaba/pouch/version.GitCommit=1.0.0-230-gb292535-dirty -X github.com/alibaba/pouch/version.Version="1.0.0" -X github.com/alibaba/pouch/version.ApiVersion="1.24" -X github.com/alibaba/pouch/version.BuildTime=2018-11-01T14:05:11+08:00" -tags "selinux seccomp apparmor"-o bin/pouchd can't load package: package bin/pouchd: cannot find package "bin/pouchd" in any of: /usr/local/go/src/bin/pouchd (from $GOROOT) /home/vagrant/go/src/bin/pouchd (from $GOPATH) Makefile:77: recipe for target 'build-daemon' failed make: *** [build-daemon] Error 1

the CI passed because it doesn't use make build-daemon. So please update it @Ace-Tang

security parameter include seccomp, apparmor and no-new-privileges are not implement good, refactor these part when create new spec. Signed-off-by: Ace-Tang <aceapril@126.com>

Signed-off-by: Ace-Tang <aceapril@126.com>

Ace-Tang · 2018-11-01T09:35:16Z

• Failure [0.966 seconds]
[k8s.io] PodSandbox
/home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:72
  runtime should support sysctls
  /home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/pod.go:84
    should support safe sysctls [It]
    /home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/pod.go:95
    Expected
        <string>: 
    to equal
        <string>: 1
    /home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/pod.go:253

cri tests seems not stable ？

fuweid

LGTM

fuweid · 2018-11-01T12:21:13Z

• Failure [0.966 seconds]
[k8s.io] PodSandbox
/home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:72
  runtime should support sysctls
  /home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/pod.go:84
    should support safe sysctls [It]
    /home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/pod.go:95
    Expected
        <string>: 
    to equal
        <string>: 1
    /home/travis/gopath/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/pod.go:253
cri tests seems not stable ？

we will fix in the redesign IO.

starnop · 2018-11-01T15:55:17Z

@Ace-Tang Please revert it. Something is wrong. In fact, the CI test has failed except the IO problem.

allencloud · 2018-11-01T16:59:12Z

We will discuss this tomorrow. @alibaba/pouch

pouchrobot added kind/refactor size/XL labels Sep 21, 2018

allencloud reviewed Sep 30, 2018

View reviewed changes

fuweid added this to the v1.1 milestone Oct 11, 2018

Ace-Tang mentioned this pull request Oct 31, 2018

refactor: combine functions into one file #2390

Merged

fuweid reviewed Nov 1, 2018

View reviewed changes

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Nov 1, 2018

vendor: add opencontainers/runc/libcontainer/apparmor

b857645

add package opencontainers/runc/libcontainer/apparmor for apparmor used. Signed-off-by: Ace-Tang <aceapril@126.com>

fuweid reviewed Nov 1, 2018

View reviewed changes

Ace-Tang added 2 commits November 1, 2018 14:18

refactor: refactor security spec

f00767d

security parameter include seccomp, apparmor and no-new-privileges are not implement good, refactor these part when create new spec. Signed-off-by: Ace-Tang <aceapril@126.com>

doc: add build tags description

c920852

Signed-off-by: Ace-Tang <aceapril@126.com>

fuweid approved these changes Nov 1, 2018

View reviewed changes

fuweid merged commit 825128d into AliyunContainerService:master Nov 1, 2018

starnop mentioned this pull request Nov 1, 2018

vendor: update "github.com/contiv/executor" #2407

Merged

This was referenced Nov 2, 2018

[refactor] create runtime spec need refactor #2116

Closed

oci: add pouch default runtime spec #2411

Merged

pouchrobot mentioned this pull request Nov 2, 2018

WeeklyReport of PouchContainer from 2018-10-26 to 2018-11-02 #2412

Closed

Ace-Tang mentioned this pull request Nov 2, 2018

refactor: refactor security part in create spec #2115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: refactor security spec #2271

refactor: refactor security spec #2271

Ace-Tang commented Sep 21, 2018 •

edited

Loading

codecov bot commented Sep 21, 2018 •

edited

Loading

allencloud Sep 30, 2018

Ace-Tang Sep 30, 2018

allencloud Oct 15, 2018

fuweid left a comment

fuweid Nov 1, 2018

Ace-Tang Nov 1, 2018

Ace-Tang commented Nov 1, 2018

fuweid Nov 1, 2018

fuweid Nov 1, 2018

Ace-Tang commented Nov 1, 2018

fuweid left a comment

fuweid commented Nov 1, 2018

starnop commented Nov 1, 2018

allencloud commented Nov 1, 2018

refactor: refactor security spec #2271

refactor: refactor security spec #2271

Conversation

Ace-Tang commented Sep 21, 2018 • edited Loading

Ⅰ. Describe what this PR did

TODO List

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Sep 21, 2018 • edited Loading

Codecov Report

allencloud Sep 30, 2018

Choose a reason for hiding this comment

Ace-Tang Sep 30, 2018

Choose a reason for hiding this comment

allencloud Oct 15, 2018

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

fuweid Nov 1, 2018

Choose a reason for hiding this comment

Ace-Tang Nov 1, 2018

Choose a reason for hiding this comment

Ace-Tang commented Nov 1, 2018

fuweid Nov 1, 2018

Choose a reason for hiding this comment

fuweid Nov 1, 2018

Choose a reason for hiding this comment

Ace-Tang commented Nov 1, 2018

fuweid left a comment

Choose a reason for hiding this comment

fuweid commented Nov 1, 2018

starnop commented Nov 1, 2018

allencloud commented Nov 1, 2018

Ace-Tang commented Sep 21, 2018 •

edited

Loading

codecov bot commented Sep 21, 2018 •

edited

Loading