remove container after extraction #10

pweil- · 2016-04-11T14:44:37Z

Fixes #8

After a second look I realized we can do this with a single defer if I extracted a method which is probably better anyway.

@simon3z @enoodle PTAL

Build and test:

[pweil@localhost image-inspector]$ rm image-inspector
[pweil@localhost image-inspector]$ go build cmd/image-inspector.go 
[pweil@localhost image-inspector]$ ./image-inspector --image=fedora:22 --serve 0.0.0.0:8080
2016/04/11 10:43:47 Image fedora:22 is available, skipping image pull
2016/04/11 10:43:47 Extracting image fedora:22 to /var/tmp/image-inspector-283317876
2016/04/11 10:43:49 !!!WARNING!!! It is insecure to serve the image content without changing
2016/04/11 10:43:49 root (--chroot). Absolute-path symlinks in the image can lead to disclose
2016/04/11 10:43:49 information of the hosting system.
2016/04/11 10:43:49 Serving image content /var/tmp/image-inspector-283317876 on webdav://0.0.0.0:8080/api/v1/content/
[pweil@localhost image-inspector]$ go test -v -cover ./...
?       github.com/simon3z/image-inspector/cmd  [no test files]
=== RUN   TestValidate
--- PASS: TestValidate (0.00s)
PASS
coverage: 75.0% of statements
ok      github.com/simon3z/image-inspector/pkg/cmd  0.001s  coverage: 75.0% of statements
?       github.com/simon3z/image-inspector/pkg/inspector    [no test files]
[pweil@localhost image-inspector]$

enoodle · 2016-04-11T16:08:52Z

pkg/inspector/image-inspector.go

+	}
+
+	reader, writer := io.Pipe()
+	go handleTarStream(reader, i.opts.DstPath)


@pweil-, @simon3z what if handleTarStream is taking more time for some reason but createAndExtractImage gets to its end? handleTarStream will keep working but maybe if it is slow we will get to the scanning with some of the files not written yet. Should we add some kind of channel here and wait for handleTarStream to finish? This existed before this patch and is not connected to it, but I noticed this now.

I was wondering that too. We could throw a channel in there like you suggest and wait on the channel or a timeout.

@enoodle @pweil- I'll probably need to see it in code. Can you re-submit with that change as well (in a different patch so it's easier to add/remove) thanks.

enoodle · 2016-04-11T16:10:35Z

LGTM 👍

pweil- · 2016-04-11T17:46:02Z

pkg/inspector/image-inspector.go

@@ -197,7 +198,8 @@ func (i *defaultImageInspector) createAndExtractImage(client *docker.Client, con
 	}

 	reader, writer := io.Pipe()
-	go handleTarStream(reader, i.opts.DstPath)
+	notificationChannel := make(chan int)
+	go handleTarStream(reader, i.opts.DstPath, notificationChannel)


@simon3z @enoodle adds a notification channel to the async method

simon3z · 2016-04-11T21:34:03Z

@pweil- we may need to queue this behind #3

simon3z · 2016-04-18T14:10:38Z

@pweil- let's try to merge this next. (It needs to be rebased).

pweil- · 2016-04-19T17:45:28Z

rebased. @enoodle can you double check the merge here to make sure I didn't miss anything? Thanks!

tests:

[pweil@localhost image-inspector]$ go test -cover ./...
?       github.com/simon3z/image-inspector/cmd  [no test files]
ok      github.com/simon3z/image-inspector/pkg/cmd  0.002s  coverage: 100.0% of statements
ok      github.com/simon3z/image-inspector/pkg/inspector    0.002s  coverage: 10.5% of statements
ok      github.com/simon3z/image-inspector/pkg/openscap 0.004s  coverage: 38.3% of statements

simon3z · 2016-04-19T22:12:01Z

@enoodle please re-review.

enoodle · 2016-04-20T07:50:11Z

LGTM 👍

simon3z · 2016-04-20T14:01:12Z

pkg/inspector/image-inspector.go

+	case <-notificationChannel:
+		break
+	case <-time.After(time.Minute * 2):
+		return imageMetadata, fmt.Errorf("timeout occured waiting for handleTarStream to finish")


@pweil- I am really worried about this one. IIUC if the extraction is longer than 2 minutes then we exit. It would be better if it was "2 minutes of inactivity"... but as it is now worries me.

Any chance that we can remove it? Maybe we can make it configurable... but I am worried anyway.

Why don't we just not use a separate go routine when calling handleTarStream? Then all this goes away.

never mind, that is required for the copy to start. Ok, let me look at it

@pweil- yeah I am all to remove the go routine if possible.

pweil- · 2016-04-20T15:26:31Z

Refactored the second commit so it will wait until both handleTarStream and the CopyFromContainer are finished without using a timeout. We can add a reasonable timeout later if necessary.

Testing:

# build
[pweil@localhost image-inspector]$ go build cmd/image-inspector.go 

# unit tests
[pweil@localhost image-inspector]$ go test -cover ./...
?       github.com/simon3z/image-inspector/cmd  [no test files]
ok      github.com/simon3z/image-inspector/pkg/cmd  0.002s  coverage: 100.0% of statements
ok      github.com/simon3z/image-inspector/pkg/inspector    0.002s  coverage: 10.6% of statements
ok      github.com/simon3z/image-inspector/pkg/openscap 0.003s  coverage: 38.3% of statements

# end-to-end
[pweil@localhost image-inspector]$ ./image-inspector --image=fedora:22 --serve 0.0.0.0:8080
2016/04/20 11:24:41 Image fedora:22 is available, skipping image pull
2016/04/20 11:24:41 Extracting image fedora:22 to /var/tmp/image-inspector-260853666
2016/04/20 11:24:43 !!!WARNING!!! It is insecure to serve the image content without changing
2016/04/20 11:24:43 root (--chroot). Absolute-path symlinks in the image can lead to disclose
2016/04/20 11:24:43 information of the hosting system.
2016/04/20 11:24:43 Serving image content /var/tmp/image-inspector-260853666 on webdav://0.0.0.0:8080/api/v1/content/
^C

# make sure it was extracted still
[pweil@localhost image-inspector]$ ll /var/tmp/image-inspector-260853666
total 80
drwxr-xr-x. 18 pweil pweil  4096 Apr 20 11:24 .
drwxrwxrwt. 47 root  root  12288 Apr 20 11:24 ..
lrwxrwxrwx.  1 pweil pweil     7 Apr 20 11:24 bin -> usr/bin
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 boot
drwxr-xr-x.  4 pweil pweil  4096 Apr 20 11:24 dev
-rwxr-xr-x.  1 pweil pweil     0 Apr 20 11:24 .dockerenv
-rwxr-xr-x.  1 pweil pweil     0 Apr 20 11:24 .dockerinit
drwxr-xr-x. 46 pweil pweil  4096 Apr 20 11:24 etc
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 home
lrwxrwxrwx.  1 pweil pweil     7 Apr 20 11:24 lib -> usr/lib
lrwxrwxrwx.  1 pweil pweil     9 Apr 20 11:24 lib64 -> usr/lib64
drwx------.  2 pweil pweil  4096 Apr 20 11:24 lost+found
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 media
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 mnt
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 opt
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 proc
drwxr-x---.  2 pweil pweil  4096 Apr 20 11:24 root
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 run
lrwxrwxrwx.  1 pweil pweil     8 Apr 20 11:24 sbin -> usr/sbin
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 srv
drwxr-xr-x.  2 pweil pweil  4096 Apr 20 11:24 sys
drwxrwxr-t.  7 pweil pweil  4096 Apr 20 11:24 tmp
drwxr-xr-x. 12 pweil pweil  4096 Apr 20 11:24 usr
drwxr-xr-x. 18 pweil pweil  4096 Apr 20 11:24 var

simon3z · 2016-04-20T15:34:54Z

@enoodle please review ASAP. Thanks!

enoodle · 2016-04-20T19:32:08Z

pkg/inspector/image-inspector.go

+
+	// capture any error from the copy, ensures both the handleTarStream and CopyFromContainer
+	// are done.
+	err = <- errorChannel


nit: gofmt is angry at this space after the arrow

enoodle · 2016-04-20T19:35:50Z

pkg/inspector/image-inspector.go

+
+	reader, writer := io.Pipe()
+	// handle closing the reader/writer in the method that creates them
+	defer writer.Close()


@pweil- we didn't have anything to close writer before?

no, didn't see anything

(I also checked if CopyFromContainer was doing it but it wasn't)

then we got evene better than we aimed for :) 🍰

enoodle · 2016-04-20T19:39:00Z

@simon3z @pweil- LGTM 👍

openscap: Warn instead of fail when loading configs

enoodle reviewed Apr 11, 2016
View reviewed changes

pweil- force-pushed the remove-container branch from 1614d58 to 1d60152 Compare April 11, 2016 17:45

pweil- reviewed Apr 11, 2016
View reviewed changes

pweil- force-pushed the remove-container branch from 1d60152 to 60e4f62 Compare April 19, 2016 17:36

remove container after extraction

d88a36f

pweil- force-pushed the remove-container branch from 60e4f62 to f4db9c1 Compare April 19, 2016 17:43

simon3z reviewed Apr 20, 2016
View reviewed changes

pweil- force-pushed the remove-container branch from f4db9c1 to b0266f7 Compare April 20, 2016 15:24

enoodle reviewed Apr 20, 2016
View reviewed changes

copy with error channel

f3a37e4

pweil- force-pushed the remove-container branch from b0266f7 to f3a37e4 Compare April 20, 2016 19:34

enoodle reviewed Apr 20, 2016
View reviewed changes

pweil- mentioned this pull request Apr 20, 2016

Update to use DownloadFromContainerOptions #14

Open

simon3z merged commit 8a0d8f9 into simon3z:master Apr 20, 2016

enoodle pushed a commit to enoodle/image-inspector that referenced this pull request May 18, 2016

Merge pull request simon3z#10 from enoodle/warn_on_bad_dockercfg

8d02275

openscap: Warn instead of fail when loading configs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove container after extraction #10

remove container after extraction #10

pweil- commented Apr 11, 2016

enoodle Apr 11, 2016

pweil- Apr 11, 2016

simon3z Apr 11, 2016

enoodle commented Apr 11, 2016

pweil- Apr 11, 2016

simon3z commented Apr 11, 2016

simon3z commented Apr 18, 2016

pweil- commented Apr 19, 2016

simon3z commented Apr 19, 2016

enoodle commented Apr 20, 2016

simon3z Apr 20, 2016

pweil- Apr 20, 2016

pweil- Apr 20, 2016

simon3z Apr 20, 2016

pweil- commented Apr 20, 2016

simon3z commented Apr 20, 2016

enoodle Apr 20, 2016

pweil- Apr 20, 2016

enoodle Apr 20, 2016

pweil- Apr 20, 2016

pweil- Apr 20, 2016

enoodle Apr 20, 2016

enoodle commented Apr 20, 2016

remove container after extraction #10

remove container after extraction #10

Conversation

pweil- commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Apr 11, 2016

Choose a reason for hiding this comment

simon3z commented Apr 11, 2016

simon3z commented Apr 18, 2016

pweil- commented Apr 19, 2016

simon3z commented Apr 19, 2016

enoodle commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Apr 20, 2016

simon3z commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Apr 20, 2016