-
Notifications
You must be signed in to change notification settings - Fork 208
WIP: Add containerRunOptions, add support for privielged, mounts and #82
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,6 @@ structure_test | |
| structure-test | ||
| structure_tests.test | ||
| out/ | ||
| **/bazel-* | ||
| **/bazel-* | ||
| .idea | ||
| container-structure-test | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package drivers | ||
|
|
||
| type ContainerRunOpts struct { | ||
| BindMounts []string `yaml:"bindMounts"` | ||
| Privileged bool `yaml:"privileged"` | ||
| Capabilities []string `yaml:"capabilities"` | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,14 @@ | ||
| schemaVersion: '2.0.0' # Make sure to test the latest schema version | ||
|
|
||
| driver: "docker" | ||
|
|
||
| containerRunOpts: | ||
| bindMounts: | ||
| - "/tmp/test:/tmp/test" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've actually been thinking about this overnight... I'm wondering whether we should support environment variable interpolation here. Because Docker requires absolute paths. So being able to interpolate a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yeah I see what you mean. In order to do that we'd need to first parse the config, then somehow retrieve the env from the image, and then substitute that into a new config before running the tests. This is probably doable, but might get tricky especially if we're not using docker. Feel free and give it a shot though, I'd like to see what it looks like |
||
| privileged: true | ||
| capabilities: | ||
| - "sys_admin" | ||
|
|
||
| commandTests: | ||
| - name: 'apt-get' | ||
| command: 'apt-get' | ||
|
|
@@ -13,6 +23,18 @@ commandTests: | |
| command: 'sh' | ||
| args: ['-c', 'echo $PATH'] | ||
| expectedOutput: ['/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'] | ||
| - name: "Test Capabilities ContainerRunOptions" | ||
| command: "capsh" | ||
| args: ["--print"] | ||
| expectedOutput: | ||
| - ".*cap_sys_admin.*" | ||
| - name: "Test bindMounts containerRunOptions" | ||
| command: "test" | ||
| args: | ||
| - "-d" | ||
| - "/tmp/test" | ||
| exitCode: 0 | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to test this using the command module, since it looks like the fileExistenceTests only work on the image content. Hopefully this is all fine.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I totally understand, can you explain a little more what you're actually testing here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had tested this using the fileExistenceTests, but the test was failing. I think that's because the fileExistenceTests don't run inside the running container, but on the underlying image data? (maybe i've misunderstood). So instead, I use the command module and run
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right, so the whole idea of this framework is that nothing happens inside the "running container": a new container is created from the base image on each test, as a way to isolate the testing environment. When you make changes through The idea here is that you'll want to treat your
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think i understand what you mean. I'm not sure this can work with docker though. Mounting volumes is something that happens at container creation time, and you can't commit their contents along with an image layer commit. The same goes for the privileged boolean and capabilities. The idea here is if i wanted to bootstrap an airflow test (for instance), I might want to mount in a test configuration in order to run the command tests. This was one example of a use case of ours. What shall we do about this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, I see what you mean. In that case, I think we'll need to just pass the One idea would be to make the |
||
|
|
||
| fileContentTests: | ||
| - name: 'Debian Sources' | ||
| excludedContents: ['.*gce_debian_mirror.*'] | ||
|
|
@@ -21,6 +43,7 @@ fileContentTests: | |
| - name: 'Retry Policy' | ||
| expectedContents: ['Acquire::Retries 3;'] | ||
| path: '/etc/apt/apt.conf.d/apt-retry' | ||
|
|
||
| fileExistenceTests: | ||
| - name: 'Root' | ||
| isDirectory: true | ||
|
|
@@ -34,6 +57,7 @@ fileExistenceTests: | |
| isDirectory: false | ||
| path: '/etc/machine-id' | ||
| shouldExist: true | ||
|
|
||
| licenseTests: | ||
| - debian: true | ||
| files: | ||
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've added this, but i'm not sure how we should handle this in code.
It feels to me like this should be the default, and overridable by the command line flag. Let me know your thoughts on how to proceed with this.
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.
Currently, the driver gets passed in as a CLI flag and then instantiated here: https://github.com/GoogleCloudPlatform/container-structure-test/blob/master/structure_test.go#L111
Instead, that will have to happen in the
Parse()method, similar to how theschemaVersiongets parsed out of the config file: https://github.com/GoogleCloudPlatform/container-structure-test/blob/master/structure_test.go#L80You should still be able to reuse the logic for
InitDriverImplhere.