fetcher: fix dependency issue, new test TestImageDependencies + disco #1822

alban · 2015-12-01T19:17:58Z

fetch: allow duplicate dependencies in the dependency tree

Includes a new test TestImageDependencies that could reproduce the bug. The test run an image discovery on a local https server (port 443) with the appropriate insecure option.

I cannot run the test on http because httptest waits forever when using
httptest.Start() on a non-random port, see:
https://golang.org/src/net/http/httptest/server.go?s=2768:2792#L112

And I have to use the non-random port 443: I need standard https port because
of #375 (comment)

alban · 2015-12-02T11:33:09Z

I updated the scenario to the exact one in #1752 and I can reproduce it:

run: error setting up stage0: error setting up image
sha512-3dd2e63f7a07c5501f47d491cf749d9ca41572290c07fe4246bdf48258cfb851:
error rendering tree image: cannot calculate treestore id: cannot find
aci satisfying name: "localhost/image-e" and labels: ["version":"1"] in
the local store

alban · 2015-12-03T11:27:40Z

Rebased with the last changes from appc/spec#551

krnowak · 2015-12-03T12:05:16Z

tests/rkt_image_dependencies_test.go

+	// Actually, httptest.Start() is broken because it waits forever, so
+	// I have to use httptest.StartTLS().
+	// So, patching appc/spec to use InsecureSkipVerify: true
+	serverURL := flag.Lookup("httptest.serve")


This change probably won't be necessary, when the appriopriate change in appc/spec lands, right?

You're right. This one: "discovery: add port parameter" appc/spec#110

PR filed on Jan 13 😅

Ah, no. It probably can be dropped, when we have the discovery over custom port implemented in spec.

alban · 2015-12-03T15:56:45Z

@krnowak branch updated

krnowak · 2015-12-03T18:07:51Z

rkt/images.go

-		ep, err := discoverApp(app, f.headers, true)
+
+		var insecure discovery.InsecureOption
+		// FIXME: why do we always accept http?


Because we are secure by default! Oh, wait...

;)

Jokes aside, we should add an insecure flag for allowing http connections and use it here. There was even an issue about this, right?

I filed one: #1836

and added a comment referencing the issue

alban · 2015-12-04T10:59:32Z

Thanks for the review. Branch updated. I also fixed some comments.

alban · 2015-12-05T15:50:36Z

depends on:

a new release of appc/spec (v0.7.4)
a fix for rkt does not fully resolve dependencies in some cases #1752

alban · 2015-12-15T15:50:06Z

The new appc/spec has been vendored in #1861. Labels "do not merge" and "dependency/appc spec" removed.

Rebased and ready for review.

krnowak · 2015-12-15T16:06:43Z

tests/rkt_image_dependencies_test.go

+	// This means this test must:
+	// - use https only
+	// - ignore tls errors with --insecure-options=tls
+	serverURL := flag.Lookup("httptest.serve")


Maybe handle the case, when serverURL is nil. Like panic when it is nil, with some useful message instead of getting a nil dereference error.

I mean - the serverURL variable is accessed below without any prior nil checks.

Oops.

Done.

krnowak · 2015-12-15T16:22:12Z

Some nits, otherwise LFAD.

alban · 2015-12-15T16:56:01Z

Updated.

krnowak · 2015-12-15T17:14:46Z

tests/rkt_auth_test.go

-	go serverHandler(t, server)
-	return server
-}
-
 func serverHandler(t *testing.T, server *taas.Server) {


The serverHandler function needs to be moved too.

krnowak · 2015-12-15T17:17:45Z

More nits.

alban · 2015-12-15T20:24:53Z

Updated again

krnowak · 2015-12-16T10:56:19Z

tests/rkt_image_dependencies_test.go

+		if err := ioutil.WriteFile(tmpManifest.Name(), []byte(img.manifest), 0600); err != nil {
+			panic(fmt.Sprintf("Cannot write to temp manifest: %v", err))
+		}
+		defer os.Remove(tmpManifest.Name())


This defer should be placed before ioutil.WriteFile.

krnowak · 2015-12-16T10:57:36Z

One final nit. Sorry for not spotting it earlier. :)

Test inspired by rkt#1752

Fixes rkt#1752

krnowak · 2015-12-16T11:03:23Z

LFAD.

fetcher: fix dependency issue, new test TestImageDependencies + disco

This was referenced Dec 2, 2015

Make insecure flag in discovery granular appc/spec#545

Closed

discovery: add insecureTls flag to skip TLS checks appc/spec#551

Merged

jonboulle added do not merge area/testing labels Dec 2, 2015

alban force-pushed the alban/img-deps-disco branch from ada3898 to 262eb02 Compare December 3, 2015 11:23

krnowak reviewed Dec 3, 2015
View reviewed changes

alban force-pushed the alban/img-deps-disco branch from 262eb02 to 2a234b6 Compare December 3, 2015 15:55

krnowak reviewed Dec 3, 2015
View reviewed changes

alban mentioned this pull request Dec 4, 2015

discovery: adding option: --insecure-options=http #1836

Closed

alban force-pushed the alban/img-deps-disco branch from 2a234b6 to 1c7ebe8 Compare December 4, 2015 10:56

alban mentioned this pull request Dec 4, 2015

tests: new test TestImageDependencies #1820

Closed

alban force-pushed the alban/img-deps-disco branch from 1c7ebe8 to 44527d8 Compare December 5, 2015 15:47

alban added the depends-on/appc label Dec 5, 2015

alban changed the title ~~(WIP) tests: new test TestImageDependencies + disco~~ tests: new test TestImageDependencies + disco Dec 5, 2015

alban force-pushed the alban/img-deps-disco branch from 44527d8 to eca48b8 Compare December 8, 2015 18:07

alban added this to the v0.14.0 milestone Dec 8, 2015

alban self-assigned this Dec 8, 2015

This was referenced Dec 9, 2015

WIP: bump appc to v0.7.4 #1859

Closed

bump appc to v0.7.4 and fix rkt code #1861

Merged

alban force-pushed the alban/img-deps-disco branch from 5047afc to 01a0017 Compare December 15, 2015 15:46

alban removed do not merge depends-on/appc labels Dec 15, 2015

alban changed the title ~~tests: new test TestImageDependencies + disco~~ fetcher: fix dependency issue, new test TestImageDependencies + disco Dec 15, 2015

krnowak reviewed Dec 15, 2015
View reviewed changes

alban force-pushed the alban/img-deps-disco branch from 01a0017 to a6c113d Compare December 15, 2015 16:38

alban mentioned this pull request Dec 15, 2015

tests: taas: dropping support for prog.aci in favor of fileset #1878

Open

krnowak reviewed Dec 15, 2015
View reviewed changes

tests: move taas code from rkt_auth_test.go to rkt_tests.go

a04afdd

alban force-pushed the alban/img-deps-disco branch from a6c113d to 38604da Compare December 15, 2015 20:22

krnowak reviewed Dec 16, 2015
View reviewed changes

alban added 2 commits December 16, 2015 12:00

tests: new test TestImageDependencies

7b6fd23

Test inspired by rkt#1752

fetch: allow duplicate dependencies

cb99078

Fixes rkt#1752

alban force-pushed the alban/img-deps-disco branch from 38604da to cb99078 Compare December 16, 2015 11:01

krnowak added reviewed/lgtm and removed needs/review labels Dec 16, 2015

krnowak added a commit that referenced this pull request Dec 16, 2015

Merge pull request #1822 from kinvolk/alban/img-deps-disco

cb1b2a7

fetcher: fix dependency issue, new test TestImageDependencies + disco

krnowak merged commit cb1b2a7 into rkt:master Dec 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fetcher: fix dependency issue, new test TestImageDependencies + disco #1822

fetcher: fix dependency issue, new test TestImageDependencies + disco #1822

alban commented Dec 1, 2015

alban commented Dec 2, 2015

alban commented Dec 3, 2015

krnowak Dec 3, 2015

alban Dec 3, 2015

iaguis Dec 3, 2015

krnowak Dec 3, 2015

alban commented Dec 3, 2015

krnowak Dec 3, 2015

krnowak Dec 3, 2015

alban Dec 4, 2015

alban Dec 4, 2015

alban commented Dec 4, 2015

alban commented Dec 5, 2015

alban commented Dec 15, 2015

krnowak Dec 15, 2015

alban Dec 15, 2015

krnowak Dec 15, 2015

krnowak Dec 15, 2015

alban Dec 15, 2015

krnowak commented Dec 15, 2015

alban commented Dec 15, 2015

krnowak Dec 15, 2015

alban Dec 15, 2015

krnowak commented Dec 15, 2015

alban commented Dec 15, 2015

krnowak Dec 16, 2015

alban Dec 16, 2015

krnowak commented Dec 16, 2015

krnowak commented Dec 16, 2015

fetcher: fix dependency issue, new test TestImageDependencies + disco #1822

fetcher: fix dependency issue, new test TestImageDependencies + disco #1822

Conversation

alban commented Dec 1, 2015

alban commented Dec 2, 2015

alban commented Dec 3, 2015

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

alban commented Dec 3, 2015

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

alban commented Dec 4, 2015

alban commented Dec 5, 2015

alban commented Dec 15, 2015

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

krnowak commented Dec 15, 2015

alban commented Dec 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak commented Dec 15, 2015

alban commented Dec 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak commented Dec 16, 2015

krnowak commented Dec 16, 2015