rkt · krnowak · Dec 16, 2015 · Dec 15, 2015 · Dec 1, 2015 · Dec 15, 2015
diff --git a/rkt/image/fetcher.go b/rkt/image/fetcher.go
@@ -90,10 +90,10 @@ func (f *Fetcher) addImageDeps(hash string, imgsl *list.List, seen map[string]st
 			return fmt.Errorf("one of image ID's %q dependencies (image %q) is invalid: %v", hash, imgName, err)
 		}
 		appStr := app.String()
-		imgsl.PushBack(appStr)
 		if _, ok := seen[appStr]; ok {
-			return fmt.Errorf("dependency %q specified multiple times in the dependency tree for image ID %q", appStr, hash)
+			continue
 		}
+		imgsl.PushBack(app.String())
 		seen[appStr] = struct{}{}
 	}
 	return nil

diff --git a/tests/rkt_auth_test.go b/tests/rkt_auth_test.go
@@ -143,30 +143,6 @@ func testAuthIgnoreSubdirectories(t *testing.T, server *taas.Server) {
 	expectedRunRkt(ctx, t, server.URL, "oauth-subdirectories", authFailedDownload)
 }
 
-func runServer(t *testing.T, auth taas.Type) *taas.Server {
-	actool := testutils.GetValueFromEnvOrPanic("ACTOOL")
-	gotool := testutils.GetValueFromEnvOrPanic("GO")
-	server, err := taas.NewServerWithPaths(auth, 20, actool, gotool)
-	if err != nil {
-		t.Fatalf("Could not start server: %v", err)
-	}
-	go serverHandler(t, server)
-	return server
-}
-
-func serverHandler(t *testing.T, server *taas.Server) {
-	for {
-		select {
-		case msg, ok := <-server.Msg:
-			if ok {
-				t.Logf("server: %v", msg)
-			}
-		case <-server.Stop:
-			return
-		}
-	}
-}
-
 // expectedRunRkt tries to fetch and run a prog.aci from host within
 // given directory on host. Note that directory can be anything - it's
 // useful for ensuring that image name is unique and for descriptive

diff --git a/tests/rkt_image_dependencies_test.go b/tests/rkt_image_dependencies_test.go
@@ -0,0 +1,226 @@
+// Copyright 2015 The rkt Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"flag"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"strings"
+	"testing"
+
+	taas "github.com/coreos/rkt/tests/test-auth-server/aci"
+	"github.com/coreos/rkt/tests/testutils"
+)
+
+const (
+	manifestDepsTemplate = `
+{
+   "acKind" : "ImageManifest",
+   "acVersion" : "0.7.3",
+   "dependencies" : [
+      DEPENDENCIES
+   ],
+   "labels" : [
+      {
+         "name" : "version",
+         "value" : "VERSION"
+      },
+      {
+         "name" : "arch",
+         "value" : "amd64"
+      },
+      {
+         "value" : "linux",
+         "name" : "os"
+      }
+   ],
+   "app" : {
+      "user" : "0",
+      "exec" : [
+         "/inspect", "--print-msg=HelloDependencies"
+      ],
+      "workingDirectory" : "/",
+      "group" : "0",
+      "environment" : [
+      ]
+   },
+   "name" : "IMG_NAME"
+}
+`
+)
+
+// TestImageDependencies generates ACIs with a complex dependency tree and
+// fetches them via the discovery mechanism. Some dependencies are already
+// cached in the CAS, and some dependencies are fetched via the discovery
+// mechanism. This is to reproduce the scenario in explained in:
+// https://github.com/coreos/rkt/issues/1752#issue-117121841
+func TestImageDependencies(t *testing.T) {
+	tmpDir := createTempDirOrPanic("rkt-TestImageDeps-")
+	defer os.RemoveAll(tmpDir)
+
+	ctx := testutils.NewRktRunCtx()
+	defer ctx.Cleanup()
+
+	// TODO: we can avoid setting the port manually when appc/spec gains
+	// the ability to specify ports for discovery.
+	// See https://github.com/appc/spec/pull/110
+	//
+	// httptest by default uses random ports. We override this via the
+	// "httptest.serve" flag.
+	//
+	// As long as we set the port via the "httptest.serve" flag, we have
+	// to use https rather than http because httptest.Start() would wait
+	// forever in "select {}", see
+	// https://golang.org/src/net/http/httptest/server.go?s=2768:2792#L92
+	//
+	// This means this test must:
+	// - use https only
+	// - ignore tls errors with --insecure-options=tls
+	serverURL := flag.Lookup("httptest.serve")
+	if serverURL == nil {
+		panic("could not find the httptest.serve flag")
+	}
+	serverURL.Value.Set("127.0.0.1:443")
+
+	server := runServer(t, taas.None)
+	if server == nil {
+		panic("could not start the https test server")
+	}
+	defer server.Close()
+
+	// reset httptest.serve to "" so we don't influence other tests
+	serverURL.Value.Set("")
+
+	baseImage := getInspectImagePath()
+	_ = importImageAndFetchHash(t, ctx, baseImage)
+	emptyImage := getEmptyImagePath()
+	fileSet := make(map[string]string)
+
+	// Scenario from https://github.com/coreos/rkt/issues/1752#issue-117121841
+	//
+	// A->B
+	// A->C
+	// A->D
+	//
+	// B: prefetched
+	//
+	// C->B
+	// C->E
+	//
+	// D->B
+	// D->E
+
+	topImage := "localhost/image-a"
+	imageList := []struct {
+		shortName string
+		imageName string
+		deps      string
+		version   string
+		prefetch  bool
+
+		manifest string
+		fileName string
+	}{
+		{
+			shortName: "a",
+			imageName: topImage,
+			deps:      `{"imageName":"localhost/image-b"}, {"imageName":"localhost/image-c"}, {"imageName":"localhost/image-d"}`,
+			version:   "1",
+		},
+		{
+			shortName: "b",
+			imageName: "localhost/image-b",
+			deps:      ``,
+			version:   "1",
+			prefetch:  true,
+		},
+		{
+			shortName: "c",
+			imageName: "localhost/image-c",
+			deps:      `{"imageName":"localhost/image-b"}, {"imageName":"localhost/image-e", "labels": [{"name": "version", "value": "1"}]}`,
+			version:   "1",
+		},
+		{
+			shortName: "d",
+			imageName: "localhost/image-d",
+			deps:      `{"imageName":"localhost/image-b"}, {"imageName":"localhost/image-e", "labels": [{"name": "version", "value": "1"}]}`,
+			version:   "1",
+		},
+		{
+			shortName: "e",
+			imageName: "localhost/image-e",
+			deps:      `{"imageName":"coreos.com/rkt-inspect"}`,
+			version:   "1",
+		},
+	}
+
+	for i, _ := range imageList {
+		// We need a reference rather than a new copy from "range"
+		// because we modify the content
+		img := &imageList[i]
+
+		img.manifest = manifestDepsTemplate
+		img.manifest = strings.Replace(img.manifest, "IMG_NAME", img.imageName, -1)
+		img.manifest = strings.Replace(img.manifest, "DEPENDENCIES", img.deps, -1)
+		img.manifest = strings.Replace(img.manifest, "VERSION", img.version, -1)
+
+		tmpManifest, err := ioutil.TempFile(tmpDir, "manifest-"+img.shortName+"-")
+		if err != nil {
+			panic(fmt.Sprintf("Cannot create temp manifest: %v", err))
+		}
+		defer os.Remove(tmpManifest.Name())
+		if err := ioutil.WriteFile(tmpManifest.Name(), []byte(img.manifest), 0600); err != nil {
+			panic(fmt.Sprintf("Cannot write to temp manifest: %v", err))
+		}
+
+		baseName := "image-" + img.shortName + ".aci"
+		img.fileName = patchACI(emptyImage, baseName, "--manifest", tmpManifest.Name())
+		defer os.Remove(img.fileName)
+		fileSet[baseName] = img.fileName
+	}
+
+	server.UpdateFileSet(fileSet)
+
+	for i := len(imageList) - 1; i >= 0; i-- {
+		img := imageList[i]
+		if img.prefetch {
+			t.Logf("Importing image %q: %q", img.imageName, img.fileName)
+			testImageShortHash := importImageAndFetchHash(t, ctx, img.fileName)
+			t.Logf("Imported image %q: %s", img.imageName, testImageShortHash)
+		}
+	}
+
+	runCmd := fmt.Sprintf("%s --debug --insecure-options=image,tls run %s", ctx.Cmd(), topImage)
+	child := spawnOrFail(t, runCmd)
+
+	expectedList := []string{
+		"rkt: fetching image from https://localhost/localhost/image-a.aci",
+		"rkt: using image from local store for image name localhost/image-b",
+		"rkt: fetching image from https://localhost/localhost/image-c.aci",
+		"rkt: fetching image from https://localhost/localhost/image-d.aci",
+		"rkt: using image from local store for image name coreos.com/rkt-inspect",
+		"HelloDependencies",
+	}
+
+	for _, expected := range expectedList {
+		if err := expectWithOutput(child, expected); err != nil {
+			t.Fatalf("Expected %q but not found: %v", expected, err)
+		}
+	}
+
+	waitOrFail(t, child, true)
+}
diff --git a/tests/rkt_tests.go b/tests/rkt_tests.go
@@ -38,6 +38,7 @@ import (
 	"github.com/coreos/rkt/Godeps/_workspace/src/github.com/coreos/gexpect"
 	"github.com/coreos/rkt/Godeps/_workspace/src/google.golang.org/grpc"
 	"github.com/coreos/rkt/api/v1alpha"
+	taas "github.com/coreos/rkt/tests/test-auth-server/aci"
 	"github.com/coreos/rkt/tests/testutils"
 )
 
@@ -175,13 +176,13 @@ func createTempDirOrPanic(dirName string) string {
 
 func importImageAndFetchHashAsGid(t *testing.T, ctx *testutils.RktRunCtx, img string, gid int) string {
 	// Import the test image into store manually.
-	cmd := fmt.Sprintf("%s --insecure-options=image fetch %s", ctx.Cmd(), img)
+	cmd := fmt.Sprintf("%s --insecure-options=image,tls fetch %s", ctx.Cmd(), img)
 
 	// TODO(jonboulle): non-root user breaks trying to read root-written
 	// config directories. Should be a better way to approach this. Should
 	// config directories be readable by the rkt group too?
 	if gid != 0 {
-
10000
		cmd = fmt.Sprintf("%s --insecure-options=image fetch %s", ctx.CmdNoConfig(), img)
+		cmd = fmt.Sprintf("%s --insecure-options=image,tls fetch %s", ctx.CmdNoConfig(), img)
 	}
 	child, err := gexpect.Command(cmd)
 	if err != nil {
@@ -556,3 +557,27 @@ func newAPIClientOrFail(t *testing.T, address string) (v1alpha.PublicAPIClient,
 	c := v1alpha.NewPublicAPIClient(conn)
 	return c, conn
 }
+
+func runServer(t *testing.T, auth taas.Type) *taas.Server {
+	actool := testutils.GetValueFromEnvOrPanic("ACTOOL")
+	gotool := testutils.GetValueFromEnvOrPanic("GO")
+	server, err := taas.NewServerWithPaths(auth, 20, actool, gotool)
+	if err != nil {
+		t.Fatalf("Could not start server: %v", err)
+	}
+	go serverHandler(t, server)
+	return server
+}
+
+func serverHandler(t *testing.T, server *taas.Server) {
+	for {
+		select {
+		case msg, ok := <-server.Msg:
+			if ok {
+				t.Logf("server: %v", msg)
+			}
+		case <-server.Stop:
+			return
+		}
+	}
+}
diff --git a/tests/test-auth-server/aci/server.go b/tests/test-auth-server/aci/server.go
@@ -18,6 +18,7 @@ import (
 	"crypto/tls"
 	"encoding/base64"
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"os/exec"
@@ -43,10 +44,11 @@ func (e *httpError) Error() string {
 }
 
 type serverHandler struct {
-	auth  Type
-	stop  chan<- struct{}
-	msg   chan<- string
-	tools *aciToolkit
+	auth    Type
+	stop    chan<- struct{}
+	msg     chan<- string
+	tools   *aciToolkit
+	fileSet map[string]string
 }
 
 func (h *serverHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
@@ -107,6 +109,10 @@ func (h *serverHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 	h.sendMsg(fmt.Sprintf("Trying to serve %q", r.URL.String()))
 	switch filepath.Base(r.URL.Path) {
+	case "/":
+		indexHTML := `<meta name="ac-discovery" content="localhost https://localhost/{name}.{ext}">`
+		w.Write([]byte(indexHTML))
+		h.sendMsg(fmt.Sprintf("    done."))
 	case "prog.aci":
 		h.sendMsg(fmt.Sprintf("  serving"))
 		if data, err := h.tools.prepareACI(); err != nil {
@@ -117,8 +123,20 @@ func (h *serverHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			h.sendMsg(fmt.Sprintf("    done."))
 		}
 	default:
-		h.sendMsg(fmt.Sprintf("  not found."))
-		w.WriteHeader(http.StatusNotFound)
+		path, ok := h.fileSet[filepath.Base(r.URL.Path)]
+		if ok {
+			contents, err := ioutil.ReadFile(path)
+			if err == nil {
+				w.Write(contents)
+				h.sendMsg(fmt.Sprintf("    done."))
+			} else {
+				h.sendMsg(fmt.Sprintf("  not found."))
+				w.WriteHeader(http.StatusNotFound)
+			}
+		} else {
+			h.sendMsg(fmt.Sprintf("  not found."))
+			w.WriteHeader(http.StatusNotFound)
+		}
 	}
 }
 
@@ -171,6 +189,10 @@ func (s *Server) Close() {
 	close(s.handler.stop)
 }
 
+func (s *Server) UpdateFileSet(fileSet map[string]string) {
+	s.handler.fileSet = fileSet
+}
+
 func NewServer(auth Type, msgCapacity int) (*Server, error) {
 	return NewServerWithPaths(auth, msgCapacity, "actool", "go")
 }
@@ -203,6 +225,7 @@ func NewServerWithPaths(auth Type, msgCapacity int, acTool, goTool string) (*Ser
 				acTool: acTool,
 				goTool: goTool,
 			},
+			fileSet: make(map[string]string),
 		},
 	}
 	server.http = httptest.NewUnstartedServer(server.handler)