Add additional filesystem checks for OCI devices #4074
Conversation
This adds checks for the default OCI devices to our conformance test for filesystem validation. This test also refactors where the file paths to check are located to reduce the number of transformations and simplify adding additional paths. Fixes knative#2973
knative-prow-robot
added
the
size/L
There was a problem hiding this comment.
@dgerd: 2 warnings.
In response to this:
This adds checks for the default OCI devices to our conformance test for
filesystem validation. This test also refactors where the file paths to
check are located to reduce the number of transformations and simplify
adding additional paths.Fixes #2973
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
knative-prow-robot
added
the
area/test-and-release
|
/assign @mattmoor |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgerd The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
There was a problem hiding this comment.
(sorry for the late review; this open window got lost in other tabs)
You might want to ensure that this container is run as a random non-root user and test actual side-effects rather than mode bits -- other items like filesystem extended attributes or apparmor profiles can prevent actual file creation while still having "allowed" filesystem permissions.
|
|
||
| // MustFiles specifies the file paths and expected permissions that MUST be set as specified in the runtime contract. | ||
| // See https://golang.org/pkg/os/#FileMode for "Mode" string meaning. '*' indicates no specification. | ||
| var MustFiles = map[string]FileInfo{ |
There was a problem hiding this comment.
Are some (many) of these specified by OCI?
I'm also wondering about the required permissions of /tmp and /var/log -- would it make more sense to test writing a file to those locations, rather than reading unix permissions (which may not be sufficient to grant access in some cases)?
There was a problem hiding this comment.
(Just meant here that a comment about where these are specified in OCI would help subsequent readers.)
There was a problem hiding this comment.
The first set of files are the "Default Devices" specified here.
The second set of files are the "Dev symbolic links" which we had as "As specified by OCI."
In my PR to update the runtime contract I pull in the "Dev symbolic links" inline and make the "Default Devices" requirement more clear.
There was a problem hiding this comment.
As for reading of files and running these tests with and without root user containers I think these are enhancements we definitely can make. I think those tests can be layers upon this rather than replacing this.
| var ShouldFiles = map[string]FileInfo{ | ||
| "/etc/resolv.conf": { | ||
| IsDir: ptr.Bool(false), | ||
| Mode: "*rw*r**r**", |
There was a problem hiding this comment.
Again, should this attempt reading the file, rather than checking permissions?
* Add additional filesystem checks for OCI devices This adds checks for the default OCI devices to our conformance test for filesystem validation. This test also refactors where the file paths to check are located to reduce the number of transformations and simplify adding additional paths. Fixes knative#2973 * Fix comments * Code review comments
This adds checks for the default OCI devices to our conformance test for
filesystem validation. This test also refactors where the file paths to
check are located to reduce the number of transformations and simplify
adding additional paths.
Fixes #2973