-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update runtime-spec version #349
Conversation
cmd/oci-runtime-tool/generate.go
Outdated
@@ -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"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. In the subsequent PR can continue to improve.
cmd/oci-runtime-tool/generate.go
Outdated
|
||
// The required part and optional part are separated by ":" | ||
// The required part and optional part are seperated by ":" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“separated” was right.
cmd/runtimetest/main.go
Outdated
expectedCaps[ec] = true | ||
} | ||
for _, ec := range spec.Process.Capabilities.Effective { | ||
expectedCaps[ec] = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
generate/generate.go
Outdated
"CAP_AUDIT_WRITE", | ||
"CAP_KILL", | ||
"CAP_NET_BIND_SERVICE", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
generate/generate.go
Outdated
g.spec.Process.SelinuxLabel = "" | ||
g.spec.Process.ApparmorProfile = "" | ||
g.spec.Linux.Seccomp = nil | ||
} | ||
} | ||
|
||
// ClearProcessCapabilities clear g.spec.Process.Capabilities. | ||
func (g *Generator) ClearProcessCapabilities() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
644cef4
to
5ad3127
Compare
cmd/runtimetest/main.go
Outdated
actuallySet := processCaps.Get(capability.BOUNDING, cap) | ||
if expectedSet != actuallySet { | ||
if expectedSet { | ||
return fmt.Errorf("Expected Capability %v not set for process", cap.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the type of capability here?
return fmt.Errorf("Expected bounding capability %v not set for process", cap.String())`
cmd/runtimetest/main.go
Outdated
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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
cmd/runtimetest/main.go
Outdated
actuallySet = processCaps.Get(capability.EFFECTIVE, cap) | ||
if expectedSet != actuallySet { | ||
if expectedSet { | ||
return fmt.Errorf("Expected Capability %v not set for process", cap.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the type of capability here?
return fmt.Errorf("Expected effective capability %v not set for process", cap.String())`
cmd/runtimetest/main.go
Outdated
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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here and for the other capability types below.
generate/seccomp/seccomp_default.go
Outdated
{ | ||
Name: "accept", | ||
Names: []string{"accept"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the system calls can now be added to a single Names list here.
For e.g.
Names: []string{
"accept",
"accept4",
"access",
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, PTAL.
@grantseltzer Could you review the seccomp related changes? |
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. |
02d7d0d
to
51da485
Compare
Godeps/Godeps.json
Outdated
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to only update to v1.0.0-rc5
? Or like 237bca6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. Thank you very much for your guidance.
There is an issue with generate specifically in the syscall section. |
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 Are there any updates? |
I'll look into this by tomorrow if there are no more updates to the PR. |
yeah. the generated |
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.
|
{ | ||
Index: 0, | ||
Value: 0xffffffff, | ||
Op: rspec.OpEqualTo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@q384566678 could you include the diff from my comment above in the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… 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 oldName
s into the newNames
.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@q384566678 Did you test latest changes with latest runc? (I'll test it tomorrow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@q384566678 I cannot find dc38a7d. Did you forget to push it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
Much better now. runc start fine and tests also run. The one problem I see in the generated config is
|
}, | ||
} | ||
var sysCloneFlagsIndex uint | ||
|
||
capSysAdmin := false | ||
var cap string | ||
var caps []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 LinuxSyscall
s you only need to add the syscall if it's not already listed in Names
for any LinuxSyscall
.
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
|
@wking Yeah, we can fix that up in a follow on PR. |
LGTM |
👍 |
💯 |
Update the
runtime-spec
version, some corresponding change questionable, what questions please suggest.Signed-off-by: zhouhao zhouhao@cn.fujitsu.com