Skip updates on parent Devices cgroup #958
Conversation
| @@ -66,7 +66,8 @@ func TestDevicesSetDeny(t *testing.T) { | |||
| "devices.allow": "a", | |||
| }) | |||
|
|
|||
| helper.CgroupData.config.Resources.AllowAllDevices = true | |||
| allowAllDevices := true | |||
| helper.CgroupData.config.Resources.AllowAllDevices = &allowAllDevices | |||
| helper.CgroupData.config.Resources.DeniedDevices = deniedDevices | |||
| devices := &DevicesGroup{} | |||
| if err := devices.Set(helper.CgroupPath, helper.CgroupData.config); err != nil { | |||
There was a problem hiding this comment.
Can you add a test that AllowAllDevices is nil, and make sure it won't change the old data?
|
@hqhq I have added a test. PTAL. |
| @@ -36,7 +36,7 @@ type Cgroup struct { | |||
| type Resources struct { | |||
| // If this is true allow access to any kind of device within the container. If false, allow access only to devices explicitly listed in the allowed_devices list. | |||
| // Deprecated | |||
| AllowAllDevices bool `json:"allow_all_devices,omitempty"` | |||
There was a problem hiding this comment.
Why did you drop the omitempty tag? That is meant to qualify if a field is optional or not.
There was a problem hiding this comment.
I thought we would want to omit the field from the object if its Nil ??
There was a problem hiding this comment.
To omit a field you need to specify omitempty.
There was a problem hiding this comment.
Oh no. BrainFart. I will fix that in a sec.
Signed-off-by: Buddha Prakash <[email protected]>
|
LGTM, thanks. |
| t.Fatalf("Failed to parse devices.allow - %s", err) | ||
| } | ||
| if value != allowedList { | ||
| t.Fatal("Got the wrong value, set devices.allow failed.") |
There was a problem hiding this comment.
Please make this error message descriptive of the actual error. Something like AllowAllDevices = nil changed devices policy. Otherwise grepping for the test failure message is a pain.
There was a problem hiding this comment.
Makes sense! will make the change.
Signed-off-by: Buddha Prakash <[email protected]>
|
@cyphar Updated the error message. PTAL. |
|
LGTM /cc @opencontainers/runc-maintainers |
|
LGTM |
temporary fix for #932
I am currently working on introducing a cgroup hierarchy in the node in Kubernetes. See (kubernetes/kubernetes#27204)
Allowing or denying all devices by writing 'a' to devices.allow or devices.deny is
not possible once the device cgroups has children. As a libcontainer user I should atleast have the option of skipping updates on devices cgroup.
cc @cyphar @vishh @derekwaynecarr @mrunalp
Signed-off-by: Buddha Prakash [email protected]