WIP: Add containerRunOptions, add support for privielged, mounts and by mogthesprog · Pull Request #82 · GoogleContainerTools/container-structure-test

mogthesprog · 2018-03-11T21:07:06Z

covering some of the changes discussed in #70

I'm still working on this, but it's coming together a bit now. Apologies for the delay, had some issue getting the tests to run on OS X.

If you've got feedback on what i've got so far that'd be great. I'll add some comments where i've got questions too.

mogthesprog · 2018-03-11T21:09:28Z

@@ -1,4 +1,14 @@
 schemaVersion: '2.0.0' # Make sure to test the latest schema version
+
+driver: "docker"


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.

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 the schemaVersion gets parsed out of the config file: https://github.com/GoogleCloudPlatform/container-structure-test/blob/master/structure_test.go#L80

You should still be able to reuse the logic for InitDriverImpl here.

mogthesprog · 2018-03-11T21:10:04Z

+  args:
+    - "-d"
+    - "/tmp/test"
+  exitCode: 0


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.

I'm not sure I totally understand, can you explain a little more what you're actually testing here?

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 test -d /tmp/test which i've added as a bindMount. This tests that a file exists at path /tmp/test and that it's of type directory.

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 Setup command for example, those get made on a running container and then committed to a new image which is used as the base image for all subsequent tests.

The idea here is that you'll want to treat your runOpts as a sort of setup, that gets run before any of the tests get run, and then committed to a new image. Then you should be able to use the fileExistence tests to test this as normal. Does that make sense?

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?

Ah ok, I see what you mean.

In that case, I think we'll need to just pass the runOpts anytime we create a container in the docker driver (e.g. here). We'll need to just be careful to pass it everywhere since we can't commit this as the "base image".

One idea would be to make the runOpts a subset of the Config struct that gets passed around in the client library we're using: then you could just have a method that creates a Config with the values in the runOpts, and use that where ever we're called CreateContainer().

mogthesprog · 2018-03-11T21:10:38Z

-	FileContentTests   []FileContentTest    `yaml:"fileContentTests"`
-	MetadataTest       MetadataTest         `yaml:"metadataTest"`
-	LicenseTests       []LicenseTest        `yaml:"licenseTests"`
+	Driver             string                   `yaml:"driver"`


Again, as above. While i've added this. I'm not sure what to do with it now that it's there. Let me know, and i'll make the required changes :)

You actually probably don't want to put this here. Check out the example I linked above for the schemaVersion, it should look very similar to that 👍

mogthesprog · 2018-03-12T08:35:43Z

+
+containerRunOpts:
+  bindMounts:
+    - "/tmp/test:/tmp/test"


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 ~ or $PWD would be really useful.

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

nkubala

This is definitely looking like the right shape. Check out the inline comments I made, hopefully those should point you in the right direction. Let me know if you have other questions!

mogthesprog · 2018-03-26T20:05:47Z

@nkubala

Hey man. I've rebased against the cobra changes (much better after that :)) Any chance you could take another look?

mogthesprog · 2018-03-27T21:27:58Z

Just noticed this is failing, any ideas why?

When i run locally by just compiling and running the code against the tests i get no failures. However if i run the bazel then i get the following, which i think is related to me running on a Mac.

standard_init_linux.go:195: exec user process caused "exec format error"

I figured i wouldn't get this in the CI. Any ideas on how i can debug this?

nkubala

@mogthesprog getting real close, thanks for the work on this. Currently Bazel's cross compilation is broken, so if you're trying to run the normal bazel build on a Mac it won't work. I think you can change your platform with the --cpu flag and it should work, but I wouldn't worry too much about it. You can build with make locally and test that way.

CI is failing with

FATA[0000] Unsupported driver type:

though, looks like you need to provide the default driver somewhere.

nkubala · 2018-03-29T17:41:51Z

 		return nil, errors.New("Please provide JSON schema version")
 	}

+	// We


something missing here? :)

nkubala · 2018-03-29T17:42:02Z

+	}
+	logrus.Infof("Using driver %s\n", driver)
+	return driverImpl
+


nit: remove newline

capabilities

nkubala · 2018-05-21T17:59:41Z

@mogthesprog any updates here? This is looking pretty close, could you rebase?

nkubala · 2018-12-04T18:44:46Z

hey @mogthesprog, this PR has been inactive for 6 months now, so I'm going to close it. Please reopen if and when you start working on this again!

vishalsangave · 2019-08-02T05:04:53Z

This will be a good feature to be have as some configuration files are needed to be mapped with the container to work correctly. Can we check on this request.

fentonfentonfenton · 2019-09-05T13:19:29Z

+1

* add containerRunOptions * largely picked up from #82, add privileged, capabilities and host volume mounts. these are passed into HostConfig if set * added "user" field which corresponds -u/--user arg in docker wasn't tested for other drivers and more tests likely needed * rename 16.04 tests, pull test image * additional options, update docs * update deps * github.com/containerd/containerd - resolve CVE-2022-23471, CVE-2023-25153 & CVE-2023-25173 * golang.org/x/net - resolve CVE-2022-41717

mogthesprog requested review from dlorenc and nkubala as code owners March 11, 2018 21:07

mogthesprog commented Mar 11, 2018

View reviewed changes

mogthesprog commented Mar 12, 2018

View reviewed changes

nkubala reviewed Mar 16, 2018

View reviewed changes

mogthesprog force-pushed the mogthesprog/container-run-opts branch 3 times, most recently from a835611 to 44581ec Compare March 26, 2018 20:03

nkubala reviewed Mar 29, 2018

View reviewed changes

Add containerRunOptions, add support for privielged, mounts and

b24f34b

capabilities

mogthesprog force-pushed the mogthesprog/container-run-opts branch from 44581ec to b24f34b Compare April 1, 2018 19:26

nkubala mentioned this pull request Apr 20, 2018

Metadata Test should support all fields from ContainerConfig #80

Open

dlorenc mentioned this pull request May 21, 2018

Feature request: support copyfile from local to container #73

Open

nkubala closed this Dec 4, 2018

coopernetes mentioned this pull request Dec 9, 2022

add containerRunOptions #327

Merged

		@@ -1,4 +1,14 @@
		schemaVersion: '2.0.0' # Make sure to test the latest schema version

		driver: "docker"

Uh oh!

Conversation

mogthesprog commented Mar 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

mogthesprog commented Mar 26, 2018

Uh oh!

mogthesprog commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkubala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkubala commented May 21, 2018

Uh oh!

nkubala commented Dec 4, 2018

Uh oh!

vishalsangave commented Aug 2, 2019

Uh oh!

fentonfentonfenton commented Sep 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mogthesprog commented Mar 27, 2018 •

edited

Loading