Cache results of getDependencies

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 |
@@ -857,6 +858,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 |
 
 

examples/microservices/leeroy-app/Dockerfile

@@ -2,6 +2,7 @@ FROM golang:1.12.9-alpine3.10 as builder
 COPY app.go .
 RUN go build -o /app .
 
+FROM library/ruby:2.3.0
 FROM alpine:3.10
 # Define GOTRACEBACK to mark this container as using the Go language runtime
 # for `skaffold debug` (https://skaffold.dev/docs/workflows/debug/).

pkg/skaffold/docker/dependencies.go

@@ -28,16 +28,15 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
-
-// dockerDependencies describes
-type dockerfileDependencies struct {
-	 files []string
-	 err error
+// dependency represents computed dependency for a dockerfile.
+type dependency struct {
+	files []string
+	err   error
 }
 
-// dependencyCache is a cache for dependencies for each dockerfile.
+// dependencyCache caches the results for `GetDependencies` for individual dockerfile.
 var (
-	dependencyCache = map[string]dockerfileDependencies{}
+	dependencyCache = map[string]dependency{}
 )
 
 // NormalizeDockerfilePath returns the absolute path to the dockerfile.
@@ -54,7 +53,7 @@ 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)
@@ -64,16 +63,16 @@ func GetDependencies(ctx context.Context, workspace string, dockerfilePath strin
 
 	if _, ok := dependencyCache[absDockerfilePath]; !ok {
 		paths, err := getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg)
-		dependencyCache[absDockerfilePath] = dockerfileDependencies{
+		dependencyCache[absDockerfilePath] = dependency{
 			files: paths,
-			err: err,
+			err:   err,
 		}
 	}
 	return dependencyCache[absDockerfilePath].files, dependencyCache[absDockerfilePath].err
 }
 
-func getDependencies(workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error){
-	// If the Dockerfile doesn't exist, we can't compute the dependencies.
+func getDependencies(workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) {
+	// 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.
@@ -116,6 +115,7 @@ func getDependencies(workspace string, dockerfilePath string, absDockerfilePath
 		dependencies = append(dependencies, file)
 	}
 	sort.Strings(dependencies)
+
 	return dependencies, nil
 }
 

pkg/skaffold/docker/dependencies_test.go

@@ -216,6 +216,11 @@ FROM stage
 ADD ./file /etc/file
 `
 
+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) {
@@ -230,6 +235,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)
@@ -530,6 +537,12 @@ func TestGetDependencies(t *testing.T) {
 			ignoreFilename: "Dockerfile.dockerignore",
 			expected:       []string{".dot", "Dockerfile", "Dockerfile.dockerignore", "file", "server.go", "test.conf", "worker.go"},
 		},
+		{
+			description: "old manifest version - watch local file dependency.",
+			dockerfile:  fromV1Manifest,
+			workspace:   ".",
+			expected:    []string{"Dockerfile", "file"},
+		},
 	}
 
 	for _, test := range tests {
@@ -629,3 +642,57 @@ 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]dependency
+		expected        []string
+		shouldErr       bool
+	}{
+		{
+			description:     "with no cached results, getDependencies will retrieve image",
+			retrieveImgMock: imageFetcher.fetch,
+			dependencyCache: map[string]dependency{},
+			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]dependency{"Dockerfile": {[]string{"random.go"}, nil}},
+			expected:        []string{"random.go"},
+		},
+		{
+			description:     "with cached results for dockerfile in another app",
+			retrieveImgMock: imageFetcher.fetch,
+			dependencyCache: map[string]dependency{
+				filepath.Join("app", "Dockerfile"): {[]string{"random.go"}, nil}},
+			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.
+			cache := map[string]dependency{}
+			for k, v := range test.dependencyCache {
+				cache[tmpDir.Path(k)] = v
+			}
+			t.Override(&dependencyCache, cache)
+			deps, err := GetDependencies(context.Background(), tmpDir.Root(), "Dockerfile", map[string]*string{}, nil)
+			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, deps)
+			if _, ok := dependencyCache[tmpDir.Path("Dockerfile")]; !ok {
+				t.Fatal("expected a cache entry for Dockerfile, did not found")
+			}
+		})
+	}
+}

pkg/skaffold/docker/parse.go

@@ -18,14 +18,14 @@ package docker
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"os"
 	"path"
 	"path/filepath"
 	"strings"
 
+	sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/moby/buildkit/frontend/dockerfile/command"
 	"github.com/moby/buildkit/frontend/dockerfile/parser"
@@ -62,10 +62,8 @@ type fromTo struct {
 var (
 	// RetrieveImage is overridden for unit testing
 	RetrieveImage = retrieveImage
-	unsupportedMediaTypeError   = errors.New("unsupported MediaType error")
 )
 
-
 func readCopyCmdsFromDockerfile(onlyLastImage bool, absDockerfilePath, workspace string, buildArgs map[string]*string, cfg Config) ([]fromTo, error) {
 	f, err := os.Open(absDockerfilePath)
 	if err != nil {
@@ -218,17 +216,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
+				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
 			}
@@ -321,12 +317,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 {
-				if errors.Is(err, notSupportedManifestError) {
-					// TODO: [4895] collect warning codes for warnings seen during a dev iteration.
-					logrus.Warnf("could not retrieve ONBUILD image %s. Will ignore files dependencies for all ONBUILD triggers", from.image)
-					return []*parser.Node{}, nil
-				}
+			} else if warnMsg, ok := sErrors.IsOldImageManifestProblem(err); ok && warnMsg != "" {
+				logrus.Warn(warnMsg)
+			} else if !ok {
 				return nil, fmt.Errorf("parsing ONBUILD instructions: %w", err)
 			}
 
@@ -348,7 +341,7 @@ func parseOnbuild(image string, cfg Config) ([]*parser.Node, error) {
 	// Image names are case SENSITIVE
 	img, err := RetrieveImage(image, cfg)
 	if err != nil {
-		return nil,
+		return nil, fmt.Errorf("retrieving image %q: %w", image, err)
 	}
 
 	if len(img.Config.OnBuild) == 0 {

pkg/skaffold/errors/err_map.go

@@ -24,6 +24,12 @@ import (
 	"github.com/GoogleContainerTools/skaffold/proto"
 )
 
+const (
+	// Skaffold commands
+	dev   = "dev"
+	debug = "debug"
+)
+
 // re is a shortcut around regexp.MustCompile
 func re(s string) *regexp.Regexp {
 	return regexp.MustCompile(s)
@@ -36,54 +42,83 @@ type problem struct {
 	suggestion  func(opts config.SkaffoldOptions) []*proto.Suggestion
 }
 
-// Build Problems are Errors in build phase
-var knownBuildProblems = []problem{
-	{
-		regexp:  re(fmt.Sprintf(".*%s.* denied: .*", PushImageErr)),
-		errCode: proto.StatusCode_BUILD_PUSH_ACCESS_DENIED,
-		description: func(error) string {
-			return "Build Failed. No push access to specified image repository"
-		},
-		suggestion: suggestBuildPushAccessDeniedAction,
-	},
-	{
-		regexp:  re(BuildCancelled),
-		errCode: proto.StatusCode_BUILD_CANCELLED,
-		description: func(error) string {
-			return "Build Cancelled."
+var (
+
+	// regex for detecting old manifest images
+	retrieveFailedOldManifest = `(.*retrieving image.*\"(.*)\")?.*unsupported MediaType.*manifest\.v1\+prettyjws.*`
+
+	// Build Problems are Errors in build phase
+	knownBuildProblems = []problem{
+		{
+			regexp:  re(fmt.Sprintf(".*%s.* denied: .*", PushImageErr)),
+			errCode: proto.StatusCode_BUILD_PUSH_ACCESS_DENIED,
+			description: func(error) string {
+				return "Build Failed. No push access to specified image repository"
+			},
+			suggestion: suggestBuildPushAccessDeniedAction,
 		},
-		suggestion: func(config.SkaffoldOptions) []*proto.Suggestion {
-			return nil
+		{
+			regexp:  re(BuildCancelled),
+			errCode: proto.StatusCode_BUILD_CANCELLED,
+			description: func(error) string {
+				return "Build Cancelled."
+			},
+			suggestion: func(config.SkaffoldOptions) []*proto.Suggestion {
+				return nil
+			},
 		},
-	},
-	{
-		regexp: re(fmt.Sprintf(".*%s.* unknown: Project", PushImageErr)),
-		description: func(error) string {
-			return "Build Failed"
+		{
+			regexp: re(fmt.Sprintf(".*%s.* unknown: Project", PushImageErr)),
+			description: func(error) string {
+				return "Build Failed"
+			},
+			errCode: proto.StatusCode_BUILD_PROJECT_NOT_FOUND,
+			suggestion: func(config.SkaffoldOptions) []*proto.Suggestion {
+				return []*proto.Suggestion{{
+					SuggestionCode: proto.SuggestionCode_CHECK_GCLOUD_PROJECT,
+					Action:         "Check your GCR project",
+				}}
+			},
 		},
-		errCode: proto.StatusCode_BUILD_PROJECT_NOT_FOUND,
-		suggestion: func(config.SkaffoldOptions) []*proto.Suggestion {
-			return []*proto.Suggestion{{
-				SuggestionCode: proto.SuggestionCode_CHECK_GCLOUD_PROJECT,
-				Action:         "Check your GCR project",
-			}}
+		{
+			regexp:  re(DockerConnectionFailed),
+			errCode: proto.StatusCode_BUILD_DOCKER_DAEMON_NOT_RUNNING,
+			description: func(err error) string {
+				matchExp := re(DockerConnectionFailed)
+				if match := matchExp.FindStringSubmatch(fmt.Sprintf("%s", err)); len(match) >= 2 {
+					return fmt.Sprintf("Build Failed. %s", match[1])
+				}
+				return "Build Failed. Could not connect to Docker daemon"
+			},
+			suggestion: func(config.SkaffoldOptions) []*proto.Suggestion {
+				return []*proto.Suggestion{{
+					SuggestionCode: proto.SuggestionCode_CHECK_DOCKER_RUNNING,
+					Action:         "Check if docker is running",
+				}}
+			},
 		},
-	},
-	{
-		regexp:  re(DockerConnectionFailed),
-		errCode: proto.StatusCode_BUILD_DOCKER_DAEMON_NOT_RUNNING,
+	}
+
+	// error retrieving image with old manifest
+	oldImageManifest = problem{
+		regexp: re(retrieveFailedOldManifest),
 		description: func(err error) string {
-			matchExp := re(DockerConnectionFailed)
+			matchExp := re(retrieveFailedOldManifest)
+			imageName := "specified image"
 			if match := matchExp.FindStringSubmatch(fmt.Sprintf("%s", err)); len(match) >= 2 {
-				return fmt.Sprintf("Build Failed. %s", match[1])
+				imageName = fmt.Sprintf("image %s", match[1])
 			}
-			return "Build Failed. Could not connect to Docker daemon"
+			return fmt.Sprintf("Could not retrieve %s pushed with the deprecated manifest v1. Ignoring files dependencies for all ONBUILD triggers", imageName)
 		},
-		suggestion: func(config.SkaffoldOptions) []*proto.Suggestion {
-			return []*proto.Suggestion{{
-				SuggestionCode: proto.SuggestionCode_CHECK_DOCKER_RUNNING,
-				Action:         "Check if docker is running",
-			}}
+		errCode: proto.StatusCode_DEVINIT_UNSUPPORTED_V1_MANIFEST,
+		suggestion: func(opts config.SkaffoldOptions) []*proto.Suggestion {
+			if opts.Command == dev || opts.Command == debug {
+				return []*proto.Suggestion{{
+					SuggestionCode: proto.SuggestionCode_RUN_DOCKER_PULL,
+					Action:         "To avoid, hit Cntrl-C and run `docker pull` to fetch the specified image and retry",
+				}}
+			}
+			return nil
 		},
-	},
-}
+	}
+)