update runtime-spec version #349

zhouhao3 · 2017-03-21T02:00:24Z

Update the runtime-spec version, some corresponding change questionable, what questions please suggest.

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

zhouhao3 · 2017-03-21T02:04:49Z

cmd/oci-runtime-tool/generate.go

@@ -22,8 +22,6 @@ var generateFlags = []cli.Flag{
 	cli.StringFlag{Name: "arch", Value: runtime.GOARCH, Usage: "architecture the container is created for"},
 	cli.StringSliceFlag{Name: "args", Usage: "command to run in the container"},
 	cli.StringSliceFlag{Name: "bind", Usage: "bind mount directories src:dest[:options...]"},
-	cli.StringSliceFlag{Name: "cap-add", Usage: "add Linux capabilities"},
-	cli.StringSliceFlag{Name: "cap-drop", Usage: "drop Linux capabilities"},


Because the structure of capabilities changes, I do not know how to deal with these two functions, so temporarily delete, there are good ways to solve.

We can use these flags to add/drop from all the 5 sets. We can add other more specific flags like cap-bounding-add/cap-bounding-drop after this PR. Thanks!

updated. In the subsequent PR can continue to improve.

wking · 2017-03-21T06:19:17Z

cmd/oci-runtime-tool/generate.go


-	// The required part and optional part are separated by ":"
+	// The required part and optional part are seperated by ":"


“separated” was right.

wking · 2017-03-21T06:21:13Z

cmd/runtimetest/main.go

+		expectedCaps[ec] = true
+	}
+	for _, ec := range spec.Process.Capabilities.Effective {
+		expectedCaps[ec] = true


This isn't right. You'll need different maps for each spec.Process.Capabilities.* and then different checks for each map below. For example, the current actuallySet := processCaps.Get(capability.EFFECTIVE, cap) should be compared against an expected value from spec.Process.Capabilities.Effective.

wking · 2017-03-21T06:26:19Z

generate/generate.go

+					"CAP_AUDIT_WRITE",
+					"CAP_KILL",
+					"CAP_NET_BIND_SERVICE",
+				},


What you're moving back towards here is close to what @mrunalp landed way back in b462307. But in 3a2ba61, @mrunalp changed that default to more closely match values extracted from Docker. In the absence of arguments for or against a given value, I expect we want to leave the values alone (and keep the fuller set from Docker as we move into the bounding/permitted/… capabilities world).

wking · 2017-03-21T06:28:37Z

generate/generate.go

 		g.spec.Process.SelinuxLabel = ""
 		g.spec.Process.ApparmorProfile = ""
 		g.spec.Linux.Seccomp = nil
 	}
 }

-// ClearProcessCapabilities clear g.spec.Process.Capabilities.
-func (g *Generator) ClearProcessCapabilities() {


Clearing all of process.capabilities is still a well-defined operation. We may need new operations to clear individual process.capabilities.*, but there's no need to drop this one.

mrunalp · 2017-03-24T17:16:20Z

cmd/runtimetest/main.go

+		actuallySet := processCaps.Get(capability.BOUNDING, cap)
+		if expectedSet != actuallySet {
+			if expectedSet {
+				return fmt.Errorf("Expected Capability %v not set for process", cap.String())


Can we add the type of capability here?

return fmt.Errorf("Expected bounding capability %v not set for process", cap.String())`

mrunalp · 2017-03-24T17:16:26Z

cmd/runtimetest/main.go

+			if expectedSet {
+				return fmt.Errorf("Expected Capability %v not set for process", cap.String())
+			}
+			return fmt.Errorf("Unexpected Capability %v set for process", cap.String())


mrunalp · 2017-03-24T17:17:19Z

cmd/runtimetest/main.go

+		actuallySet = processCaps.Get(capability.EFFECTIVE, cap)
+		if expectedSet != actuallySet {
+			if expectedSet {
+				return fmt.Errorf("Expected Capability %v not set for process", cap.String())


Can we add the type of capability here?

return fmt.Errorf("Expected effective capability %v not set for process", cap.String())`

mrunalp · 2017-03-24T17:17:33Z

cmd/runtimetest/main.go

+			if expectedSet {
+				return fmt.Errorf("Expected Capability %v not set for process", cap.String())
+			}
+			return fmt.Errorf("Unexpected Capability %v set for process", cap.String())


Same here and for the other capability types below.

mrunalp · 2017-03-24T17:19:21Z

generate/seccomp/seccomp_default.go

 		{
-			Name:   "accept",
+			Names:  []string{"accept"},


All of the system calls can now be added to a single Names list here.
For e.g.

Names: []string{ "accept", "accept4", "access", }

updated, PTAL.

mrunalp · 2017-03-24T17:21:17Z

@grantseltzer Could you review the seccomp related changes?

grantseltzer · 2017-03-25T23:56:06Z

Everything appears to be working properly, but there should be capability added to place a comment inside the rules if we're updating to the new spec anyway.

wking · 2017-03-29T16:58:57Z

Godeps/Godeps.json

@@ -34,8 +33,8 @@
 		},
 		{
 			"ImportPath": "github.com/opencontainers/runtime-spec/specs-go",
-			"Comment": "v1.0.0-rc1-31-gbb6925e",
-			"Rev": "bb6925ea99f0e366a3f7d1c975f6577475ca25f0"
+			"Comment": "v1.0.0-rc5-30-g3adac26",


Is this intentionally bumping to a commit that is 30 commits after the tagged rc5? I don't think the LinuxSyscall.Comment removal or Linux.IntelRdt addition are so pressing that it's worth bringing in a non-release spec commit.

Do you mean to only update to v1.0.0-rc5? Or like 237bca6?

Do you mean to only update to v1.0.0-rc5?

That's what I mean. I'd rather avoid:

$ ./oci-runtime-tool validate 1 Errors detected: internal error: validate currently only handles version 1.0.0-rc5-dev, but the supplied configuration targets 1.0.0-rc5

I now use godep update to update the runtime-spec, after the update directly to the latest version (v1.0.0-rc5-30), what method can you only be updated to 1.0.0-rc5? Thanks.

… what method can you only be updated to 1.0.0-rc5?

In whichever GOPATH you use, go to src/github.com/opencontainers/runtime-spec. Then:

$ git checkout v1.0.0-rc5

Now, back in runtime-tools:

$ godep update github.com/opencontainers/runtime-spec/specs-go

and you should be good to go.

updated. Thank you very much for your guidance.

vbatts · 2017-04-03T15:30:15Z

LGTM

mrunalp · 2017-04-03T20:19:26Z

There is an issue with generate specifically in the syscall section.
runc run/ make localvalidation just hang.

cyphar · 2017-04-05T17:27:56Z

This is currently blocking a wide variety of things (like being able to use the newest version of runC with anything generated by this library). So umoci can't work with the latest runC at the moment, neither can cri-o or orca.

Are there any updates?

mrunalp · 2017-04-06T17:16:05Z

I'll look into this by tomorrow if there are no more updates to the PR.

vbatts · 2017-04-06T18:03:06Z

yeah. the generated config.json does not currently work with runc run
something in capabilities is not marshalling correctly as a []string, and the syscall names in seccomp are marshalling as empty strings.
I wish we could add a round-trip test for this (i realize that it's default to bootstrap that).

wking · 2017-04-06T20:43:23Z

On Thu, Apr 06, 2017 at 11:03:07AM -0700, Vincent Batts wrote: I wish we could add a round-trip test for this (i realize that it's default to bootstrap that).

It's one step down the stack, but I've started stubbing in round-trip tests in opencontainers/runtime-spec#759.

wking · 2017-04-06T20:50:42Z

generate/seccomp/seccomp_default.go

+				{
+					Index: 0,
+					Value: 0xffffffff,
+					Op:    rspec.OpEqualTo,


This looks buggy to me (and may be the source of @mrunalp's problem). In master, only personality has an 0xffffffff arg, but here you have a whole bunch of names with that arg. I think you over-flattened here, and need to split this entry into separate entries for each []LinuxSeccompArg value in master.

Adding this diff makes this PR work.

diff --git a/generate/seccomp/seccomp_default.go b/generate/seccomp/seccomp_default.go index e057ca7..e9561ae 100644 --- a/generate/seccomp/seccomp_default.go +++ b/generate/seccomp/seccomp_default.go @@ -344,23 +344,6 @@ func DefaultProfile(rs *specs.Spec) *rspec.LinuxSeccomp { "writev", }, Action: rspec.ActAllow, - Args: []rspec.LinuxSeccompArg{ - { - Index: 0, - Value: 0x0, - Op: rspec.OpEqualTo, - }, - { - Index: 0, - Value: 0x0008, - Op: rspec.OpEqualTo, - }, - { - Index: 0, - Value: 0xffffffff, - Op: rspec.OpEqualTo, - }, - }, }, } var sysCloneFlagsIndex uint

@q384566678 could you include the diff from my comment above in the PR?

… could you include the diff from my comment above in the PR?

I think that patch is too coarse. For example personality should probably keep its old entries. The algorithm for the change should be:

For all the old syscalls, group by (action, args) tuples, and create a new entry collecting all the old Names into the new Names.

It looks like:

Action: rspec.ActAllow, Args: []rspec.Arg{},

matches everything except personality (based on a very quick skim, may have missed something). And personality will still need three entries (one for each of its current []rspec.Arg values.

Or maybe personality can have a single entry:

{ Names: []string{"personality"}, Action: rspec.ActAllow, Args: []rspec.Arg{ { Index: 0, Value: 0x0, Op: rspec.OpEqualTo, }, { Index: 0, Value: 0x0008, Op: rspec.OpEqualTo, }, { Index: 0, Value: 0xffffffff, Op: rspec.OpEqualTo, }, }, },

I'm not sure what the semantics are supposed to be for multiple Args.

I think dc38a7d is better.
@mrunalp @wking PTAL

@q384566678 Did you test latest changes with latest runc? (I'll test it tomorrow).

@mrunalp I was doing it, but when I executed the sudo make RUNTIME=runc localvalidation command, the following error occurred：

go: GOPATH entry is relative; must be absolute path: "".

I have already set up GOPATH, I'm looking for the solution, may want to make trouble you first test, thanks a lot.

@q384566678 I cannot find dc38a7d. Did you forget to push it?

I cannot find dc38a7d. Did you forget to push it?

The current tip (afc8d35) includes the change I think we want, and was committed a few hours after @q384566678 mentioned dc38a7d. I expect he just fixed up some minor thing in the interim, and that afc8d35 is ready for testing/merging.

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

mrunalp · 2017-04-10T21:20:04Z

Much better now. runc start fine and tests also run. The one problem I see in the generated config is chroot being added to the seccomp section 5 times:

                               {
                                        "names": [
                                                "chroot"
                                        ],
                                        "action": "SCMP_ACT_ALLOW",
                                        "args": [],
                                        "comment": ""
                                },
                                {
                                        "names": [
                                                "chroot"
                                        ],
                                        "action": "SCMP_ACT_ALLOW",
                                        "args": [],
                                        "comment": ""
                                },
                                {
                                        "names": [
                                                "chroot"
                                        ],
                                        "action": "SCMP_ACT_ALLOW",
                                        "args": [],
                                        "comment": ""
                                },
                                {
                                        "names": [
                                                "chroot"
                                        ],
                                        "action": "SCMP_ACT_ALLOW",
                                        "args": [],
                                        "comment": ""
                                },
                                {
                                        "names": [
                                                "chroot"
                                        ],
                                        "action": "SCMP_ACT_ALLOW",
                                        "args": [],
                                        "comment": ""
                                },

mrunalp · 2017-04-10T21:31:11Z

generate/seccomp/seccomp_default.go

 		},
 	}
 	var sysCloneFlagsIndex uint

 	capSysAdmin := false
 	var cap string
+	var caps []string


The reason for repitition is because we are adding caps multiple times in this list. Practically the first 4 will be same for most containers and ambient could have extra capabilities. So, instead of adding all, can we just add the caps from permitted and ambient to a map and then iterate over the map to add the syscalls?

So, instead of adding all, can we just add the caps from permitted and ambient to a map…

This is probably the way to go (although I'm not sure it has to be part of this PR). However, the map is going to be complicated, because you want to key by (LinuxSeccompAction, LinuxSeccompArg) with values that are []*LinuxSyscall (because the same backing LinuxSyscall may show up as the value for multiple keys in the map, and there may be multiple entries for each key). And then once you find the appropriate group of LinuxSyscalls you only need to add the syscall if it's not already listed in Names for any LinuxSyscall.

wking · 2017-04-10T21:34:17Z

On Mon, Apr 10, 2017 at 02:20:04PM -0700, Mrunal Patel wrote: The one problem I see in the generated config is `chroot` being added to the seccomp section 5 times…

This is a pre-existing bug, because we're appending (e.g. [1]) instead of using append-if-missing logic. While it would be good to address this, I don't think it needs to block this PR (it will work, and just be less efficient than if we consolidated those duplicates). [1]: https://github.com/opencontainers/runtime-tools/blob/133b95395cd847ef951c237784b9863cdf1ed860/generate/seccomp/seccomp_default.go#L1685-L1692

mrunalp · 2017-04-10T23:28:13Z

@wking Yeah, we can fix that up in a follow on PR.

mrunalp · 2017-04-10T23:28:24Z

LGTM
cc: @Mashimiao @liangchenye

Mashimiao · 2017-04-11T01:26:08Z

LGTM

cyphar · 2017-04-11T08:57:45Z

👍

vbatts · 2017-04-11T12:03:25Z

💯

zhouhao3 commented Mar 21, 2017

View reviewed changes

wking reviewed Mar 21, 2017

View reviewed changes

zhouhao3 force-pushed the up-version branch 5 times, most recently from 644cef4 to 5ad3127 Compare March 24, 2017 02:17

mrunalp reviewed Mar 24, 2017

View reviewed changes

mrunalp mentioned this pull request Mar 24, 2017

Switch to using vendor directory #351

Closed

zhouhao3 force-pushed the up-version branch 4 times, most recently from 02d7d0d to 51da485 Compare March 28, 2017 03:30

wking reviewed Mar 29, 2017

View reviewed changes

wking mentioned this pull request Mar 29, 2017

FINAL v1.0 RELEASE BURN DOWN opencontainers/runtime-spec#726

Closed

4 tasks

zhouhao3 force-pushed the up-version branch from 51da485 to 237bca6 Compare March 30, 2017 01:45

wking mentioned this pull request Mar 30, 2017

add OCIError to differentiate 'MUST/SHOULD' and add reference support #354

Merged

zhouhao3 force-pushed the up-version branch from 237bca6 to 76add65 Compare March 31, 2017 05:26

wking reviewed Apr 6, 2017

View reviewed changes

cyphar mentioned this pull request Apr 6, 2017

Rootless building cyphar/orca-build#6

Closed

zhouhao3 force-pushed the up-version branch from 76add65 to dc38a7d Compare April 10, 2017 03:22

cyphar mentioned this pull request Apr 10, 2017

add proper integration tests for rootless opencontainers/umoci#111

Closed

update runtime-spec version

afc8d35

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 force-pushed the up-version branch from dc38a7d to afc8d35 Compare April 10, 2017 05:03

mrunalp reviewed Apr 10, 2017

View reviewed changes

Mashimiao merged commit 868323a into opencontainers:master Apr 11, 2017

zhouhao3 deleted the up-version branch April 11, 2017 01:34

Mashimiao mentioned this pull request Apr 11, 2017

update godeps and use by makefile #231

Merged

This was referenced Apr 11, 2017

update runtime generation to v1.0.0-rc5 opencontainers/umoci#113

Closed

update runtime-spec to v1.0.0-rc5 cri-o/cri-o#442

Closed


		// The required part and optional part are separated by ":"
		// The required part and optional part are seperated by ":"

update runtime-spec version #349

update runtime-spec version #349

Conversation

zhouhao3 commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Mar 24, 2017

grantseltzer commented Mar 25, 2017 • edited Loading

wking Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbatts commented Apr 3, 2017 • edited by caniszczyk Loading

mrunalp commented Apr 3, 2017

cyphar commented Apr 5, 2017

mrunalp commented Apr 6, 2017

vbatts commented Apr 6, 2017

wking commented Apr 6, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Apr 10, 2017 via email

mrunalp commented Apr 10, 2017

mrunalp commented Apr 10, 2017 • edited by caniszczyk Loading

Mashimiao commented Apr 11, 2017 • edited by caniszczyk Loading

cyphar commented Apr 11, 2017

vbatts commented Apr 11, 2017

mrunalp Mar 24, 2017 •

edited

Loading

grantseltzer commented Mar 25, 2017 •

edited

Loading

wking Mar 29, 2017 •

edited

Loading

vbatts commented Apr 3, 2017 •

edited by caniszczyk

Loading

wking Apr 7, 2017 •

edited

Loading

mrunalp commented Apr 10, 2017 •

edited by caniszczyk

Loading

Mashimiao commented Apr 11, 2017 •

edited by caniszczyk

Loading