Cache docker.getDependencies and skip inspecting remote images with old manifest

docs/content/en/docs/references/api/grpc.md

@@ -812,6 +812,7 @@ For Cancelled Error code, use range 800 to 850.
 | DEVINIT_REGISTER_TEST_DEPS | 702 | Failed to configure watcher for test dependencies in dev loop |
 | DEVINIT_REGISTER_DEPLOY_DEPS | 703 | Failed to configure watcher for deploy dependencies in dev loop |
 | DEVINIT_REGISTER_CONFIG_DEP | 704 | Failed to configure watcher for Skaffold configuration file. |
+| DEVINIT_UNSUPPORTED_V1_MANIFEST | 705 | Failed to configure watcher for build dependencies for a base image with v1 manifest. |
 | STATUSCHECK_USER_CANCELLED | 800 | User cancelled the skaffold dev run |
 | STATUSCHECK_DEADLINE_EXCEEDED | 801 | Deadline for status check exceeded |
 | BUILD_CANCELLED | 802 | Build Cancelled |
@@ -860,6 +861,7 @@ Enum for Suggestion codes
 | CHECK_HOST_CONNECTION | 408 | Cluster Connectivity error |
 | START_MINIKUBE | 501 | Minikube is stopped: use `minikube start` |
 | UNPAUSE_MINIKUBE | 502 | Minikube is paused: use `minikube unpause` |
+| RUN_DOCKER_PULL | 551 | Run Docker pull for the image with v1 manifest and try again. |
 | OPEN_ISSUE | 900 | Open an issue so this situation can be diagnosed |
 
 

go.mod

@@ -39,6 +39,7 @@ require (
 	github.com/ghodss/yaml v1.0.0
 	github.com/go-git/go-git/v5 v5.0.0
 	github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b
+	github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e
 	github.com/golang/protobuf v1.4.2
 	github.com/google/go-cmp v0.4.1
 	github.com/google/go-containerregistry v0.1.1

pkg/skaffold/docker/dependencies.go

@@ -22,12 +22,24 @@ import (
 	"os"
 	"path/filepath"
 	"sort"
+	"sync"
 
 	"github.com/docker/docker/builder/dockerignore"
+	"github.com/golang/groupcache/singleflight"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
+var (
+	// sfGetDependencies ensures `GetDependencies` is called only once at any time
+	// for a given dockerfile.
+	// sfGetDependencies along with sync.Map will ensure no two concurrent processes read or
+	// write dependencies for a given dockerfile.
+	sfGetDependencies = singleflight.Group{}
+
+	dependencyCache = sync.Map{}
+)
+
 // NormalizeDockerfilePath returns the absolute path to the dockerfile.
 func NormalizeDockerfilePath(context, dockerfile string) (string, error) {
 	// Expected case: should be found relative to the context directory.
@@ -42,30 +54,48 @@ func NormalizeDockerfilePath(context, dockerfile string) (string, error) {
 	return filepath.Abs(rel)
 }
 
-// GetDependencies finds the sources dependencies for the given docker artifact.
+// GetDependencies finds the sources dependency for the given docker artifact.
 // All paths are relative to the workspace.
 func GetDependencies(ctx context.Context, workspace string, dockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) {
 	absDockerfilePath, err := NormalizeDockerfilePath(workspace, dockerfilePath)
 	if err != nil {
 		return nil, fmt.Errorf("normalizing dockerfile path: %w", err)
 	}
 
-	// If the Dockerfile doesn't exist, we can't compute the dependencies.
+	deps, _ := sfGetDependencies.Do(absDockerfilePath, func() (interface{}, error) {
+		if dep, ok := dependencyCache.Load(absDockerfilePath); ok {
+			return dep, nil
+		}
+		dep := getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg)
+		dependencyCache.Store(absDockerfilePath, dep)
+		return dep, nil
+	})
+
+	if paths, ok := deps.([]string); ok {
+		return paths, nil
+	} else if err, ok := deps.(error); ok {
+		return nil, err
+	}
+	return nil, fmt.Errorf("unexpected skaffold internal error encountered converting dependencies to []string")
+}
+
+func getDependencies(workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) interface{} {
+	// If the Dockerfile doesn't exist, we can't compute the dependency.
 	// But since we know the Dockerfile is a dependency, let's return a list
 	// with only that file. It makes errors down the line more actionable
 	// than returning an error now.
 	if _, err := os.Stat(absDockerfilePath); os.IsNotExist(err) {
-		return []string{dockerfilePath}, nil
+		return []string{dockerfilePath}
 	}
 
 	fts, err := readCopyCmdsFromDockerfile(false, absDockerfilePath, workspace, buildArgs, cfg)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	excludes, err := readDockerignore(workspace, absDockerfilePath)
 	if err != nil {
-		return nil, fmt.Errorf("reading .dockerignore: %w", err)
+		return fmt.Errorf("reading .dockerignore: %w", err)
 	}
 
 	deps := make([]string, 0, len(fts))
@@ -75,7 +105,7 @@ func GetDependencies(ctx context.Context, workspace string, dockerfilePath strin
 
 	files, err := WalkWorkspace(workspace, excludes, deps)
 	if err != nil {
-		return nil, fmt.Errorf("walking workspace: %w", err)
+		return fmt.Errorf("walking workspace: %w", err)
 	}
 
 	// Always add dockerfile even if it's .dockerignored. The daemon will need it anyways.
@@ -94,7 +124,7 @@ func GetDependencies(ctx context.Context, workspace string, dockerfilePath strin
 	}
 	sort.Strings(dependencies)
 
-	return dependencies, nil
+	return dependencies
 }
 
 // readDockerignore reads patterns to ignore

pkg/skaffold/docker/dependencies_test.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"sync"
 	"testing"
 
 	v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -221,6 +222,11 @@ FROM
 COPY . /
 `
 
+const fromV1Manifest = `
+FROM library/ruby:2.3.0
+ADD ./file /etc/file
+`
+
 type fakeImageFetcher struct{}
 
 func (f *fakeImageFetcher) fetch(image string, _ Config) (*v1.ConfigFile, error) {
@@ -235,6 +241,8 @@ func (f *fakeImageFetcher) fetch(image string, _ Config) (*v1.ConfigFile, error)
 				},
 			},
 		}, nil
+	case "library/ruby:2.3.0":
+		return nil, fmt.Errorf("retrieving image \"library/ruby:2.3.0\": unsupported MediaType: \"application/vnd.docker.distribution.manifest.v1+prettyjws\", see https://github.com/google/go-containerregistry/issues/377")
 	}
 
 	return nil, fmt.Errorf("no image found for %s", image)
@@ -541,6 +549,12 @@ func TestGetDependencies(t *testing.T) {
 			workspace:   ".",
 			shouldErr:   true,
 		},
+		{
+			description: "old manifest version - watch local file dependency.",
+			dockerfile:  fromV1Manifest,
+			workspace:   ".",
+			expected:    []string{"Dockerfile", "file"},
+		},
 	}
 
 	for _, test := range tests {
@@ -640,3 +654,61 @@ func checkSameFile(t *testutil.T, expected, result string) {
 		t.Errorf("returned wrong file\n   got: %s\nwanted: %s", result, expected)
 	}
 }
+
+func TestGetDependenciesCached(t *testing.T) {
+	imageFetcher := fakeImageFetcher{}
+	tests := []struct {
+		description     string
+		retrieveImgMock func(_ string, _ Config) (*v1.ConfigFile, error)
+		dependencyCache map[string]interface{}
+		expected        []string
+		shouldErr       bool
+	}{
+		{
+			description:     "with no cached results getDependencies will retrieve image",
+			retrieveImgMock: imageFetcher.fetch,
+			dependencyCache: map[string]interface{}{},
+			expected:        []string{"Dockerfile", "server.go"},
+		},
+		{
+			description: "with cached results getDependencies should read from cache",
+			retrieveImgMock: func(_ string, _ Config) (*v1.ConfigFile, error) {
+				return nil, fmt.Errorf("unexpected call")
+			},
+			dependencyCache: map[string]interface{}{"Dockerfile": []string{"random.go"}},
+			expected:        []string{"random.go"},
+		},
+		{
+			description: "with cached results is error getDependencies should read from cache",
+			retrieveImgMock: func(_ string, _ Config) (*v1.ConfigFile, error) {
+				return &v1.ConfigFile{}, nil
+			},
+			dependencyCache: map[string]interface{}{"Dockerfile": fmt.Errorf("remote manifest fetch")},
+			shouldErr:       true,
+		},
+		{
+			description:     "with cached results for dockerfile in another app",
+			retrieveImgMock: imageFetcher.fetch,
+			dependencyCache: map[string]interface{}{
+				filepath.Join("app", "Dockerfile"): []string{"random.go"}},
+			expected: []string{"Dockerfile", "server.go"},
+		},
+	}
+
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.Override(&RetrieveImage, test.retrieveImgMock)
+			t.Override(&util.OSEnviron, func() []string { return []string{} })
+
+			tmpDir := t.NewTempDir().Touch("server.go", "random.go")
+			tmpDir.Write("Dockerfile", copyServerGo)
+			// construct cache for abs dockerfile paths.
+			defer func() { dependencyCache = sync.Map{} }()
+			for k, v := range test.dependencyCache {
+				dependencyCache.Store(tmpDir.Path(k), v)
+			}
+			deps, err := GetDependencies(context.Background(), tmpDir.Root(), "Dockerfile", map[string]*string{}, nil)
+			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, deps)
+		})
+	}
+}

pkg/skaffold/docker/parse.go

@@ -32,6 +32,7 @@ import (
 	"github.com/moby/buildkit/frontend/dockerfile/shell"
 	"github.com/sirupsen/logrus"
 
+	sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
@@ -226,16 +227,15 @@ func extractCopyCommands(nodes []*parser.Node, onlyLastImage bool, cfg Config) (
 			// was already changed.
 			if !stages[strings.ToLower(from.image)] {
 				img, err := RetrieveImage(from.image, cfg)
-				if err != nil {
+				if err == nil {
+					workdir = img.Config.WorkingDir
+				} else if _, ok := sErrors.IsOldImageManifestProblem(err); !ok {
 					return nil, err
 				}
-
-				workdir = img.Config.WorkingDir
 				if workdir == "" {
 					workdir = "/"
 				}
 			}
-
 			if onlyLastImage {
 				copied = nil
 			}
@@ -334,7 +334,9 @@ func expandOnbuildInstructions(nodes []*parser.Node, cfg Config) ([]*parser.Node
 				onbuildNodes = ons
 			} else if ons, err := parseOnbuild(from.image, cfg); err == nil {
 				onbuildNodes = ons
-			} else {
+			} else if warnMsg, ok := sErrors.IsOldImageManifestProblem(err); ok && warnMsg != "" {
+				logrus.Warn(warnMsg)
+			} else if !ok {
 				return nil, fmt.Errorf("parsing ONBUILD instructions: %w", err)
 			}