Fix staleness issue in entrypoint resolver.

There was a bug in the entrypoint resolution logic that allowed for stale values of the
entrypoint to be returned. If an image is supplied by tag, and that tag changes to point
to an image with a new entrypoint, we returned the old one.

This change uses the image digest itself as our cache key, instead of the image name.

Author: dlorenc

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go

@@ -187,11 +187,34 @@ func getWaitFile(stepNum int) string {
 // GetRemoteEntrypoint accepts a cache of digest lookups, as well as the digest
 // to look for. If the cache does not contain the digest, it will lookup the
 // metadata from the images registry, and then commit that to the cache
-func GetRemoteEntrypoint(cache *Cache, digest string, kubeclient kubernetes.Interface, taskRun *v1alpha1.TaskRun) ([]string, error) {
+func GetRemoteEntrypoint(cache *Cache, image string, kubeclient kubernetes.Interface, taskRun *v1alpha1.TaskRun) ([]string, error) {
+	ref, err := name.ParseReference(image, name.WeakValidation)
+	if err != nil {
+		return nil, xerrors.Errorf("Failed to parse image %s: %w", image, err)
+	}
+
+	var digest string
+	// If the image is specified as a digest, we can just take the digest from the name and use that in our cache.
+	// Otherwise we first have to resolve the tag to a digest.
+	if d, ok := ref.(name.Digest); ok {
+		digest = d.String()
+	} else {
+		img, err := getRemoteImage(image, kubeclient, taskRun)
+		if err != nil {
+			return nil, xerrors.Errorf("Failed to fetch remote image %s: %w", digest, err)
+		}
+		d, err := img.Digest()
+		if err != nil {
+			return nil, xerrors.Errorf("Failed to get digest for image %s: %w", image, err)
+		}
+		digest = d.String()
+	}
+
 	if ep, ok := cache.get(digest); ok {
 		return ep, nil
 	}
-	img, err := getRemoteImage(digest, kubeclient, taskRun)
+
+	img, err := getRemoteImage(image, kubeclient, taskRun)
 	if err != nil {
 		return nil, xerrors.Errorf("Failed to fetch remote image %s: %w", digest, err)
 	}

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint_test.go

@@ -213,19 +213,14 @@ func getDigestAsString(image v1.Image) string {
 	return digestHash.String()
 }
 
-func TestGetRemoteEntrypoint(t *testing.T) {
-	expectedEntrypoint := []string{"/bin/expected", "entrypoint"}
-	img := getImage(t, &v1.ConfigFile{
-		Config: v1.Config{
-			Entrypoint: expectedEntrypoint,
-		},
-	})
+func getServer(t *testing.T, img v1.Image) *httptest.Server {
 	expectedRepo := "image"
-	digetsSha := getDigestAsString(img)
+
 	configPath := fmt.Sprintf("/v2/%s/blobs/%s", expectedRepo, mustConfigName(t, img))
-	manifestPath := fmt.Sprintf("/v2/%s/manifests/%s", expectedRepo, digetsSha)
+	manifestPath := fmt.Sprintf("/v2/%s/manifests/%s", expectedRepo, getDigestAsString(img))
+	latestPath := fmt.Sprintf("/v2/%s/manifests/latest", expectedRepo)
 
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch r.URL.Path {
 		case "/v2/":
 			w.WriteHeader(http.StatusOK)
@@ -236,7 +231,7 @@ func TestGetRemoteEntrypoint(t *testing.T) {
 			if _, err := w.Write(mustRawConfigFile(t, img)); err != nil {
 				t.Fatal(err)
 			}
-		case manifestPath:
+		case manifestPath, latestPath:
 			if r.Method != http.MethodGet {
 				t.Errorf("Method; got %v, want %v", r.Method, http.MethodGet)
 			}
@@ -247,6 +242,19 @@ func TestGetRemoteEntrypoint(t *testing.T) {
 			t.Fatalf("Unexpected path: %v", r.URL.Path)
 		}
 	}))
+}
+
+func TestGetRemoteEntrypoint(t *testing.T) {
+	expectedEntrypoint := []string{"/bin/expected", "entrypoint"}
+	img := getImage(t, &v1.ConfigFile{
+		Config: v1.Config{
+			Entrypoint: expectedEntrypoint,
+		},
+	})
+	expectedRepo := "image"
+	digetsSha := getDigestAsString(img)
+
+	server := getServer(t, img)
 	defer server.Close()
 	image := path.Join(strings.TrimPrefix(server.URL, "http://"), expectedRepo)
 	finalDigest := image + "@" + digetsSha
@@ -286,6 +294,68 @@ func TestGetRemoteEntrypoint(t *testing.T) {
 	}
 }
 
+func TestGetRemoteEntrypointStale(t *testing.T) {
+	initialEntrypoint := []string{"/bin/expected", "entrypoint"}
+	img := getImage(t, &v1.ConfigFile{
+		Config: v1.Config{
+			Entrypoint: initialEntrypoint,
+		},
+	})
+
+	server := getServer(t, img)
+	defer server.Close()
+	expectedRepo := "image"
+	image := path.Join(strings.TrimPrefix(server.URL, "http://"), expectedRepo) + ":latest"
+
+	entrypointCache, err := NewCache()
+	if err != nil {
+		t.Fatalf("couldn't create new entrypoint cache: %v", err)
+	}
+	taskRun := &v1alpha1.TaskRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo",
+			Name:      "taskRun",
+		},
+		Spec: v1alpha1.TaskRunSpec{
+			ServiceAccount: "default",
+		},
+	}
+	c := fakekubeclientset.NewSimpleClientset(&corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "default",
+			Namespace: "foo",
+		},
+	})
+	ep1, err := GetRemoteEntrypoint(entrypointCache, image, c, taskRun)
+	if err != nil {
+		t.Errorf("couldn't get entrypoint remote: %v", err)
+	}
+	server.Close()
+
+	// Now change the image
+	secondEntrypoint := []string{"/bin/expected", "entrypoint2"}
+	img = getImage(t, &v1.ConfigFile{
+		Config: v1.Config{
+			Entrypoint: secondEntrypoint,
+		},
+	})
+	server2 := getServer(t, img)
+	image = path.Join(strings.TrimPrefix(server2.URL, "http://"), expectedRepo) + ":latest"
+	defer server2.Close()
+	ep2, err := GetRemoteEntrypoint(entrypointCache, image, c, taskRun)
+	if err != nil {
+		t.Fatalf("couldn't get entrypoint remote: %v", err)
+	}
+
+	if !reflect.DeepEqual(ep1, initialEntrypoint) {
+		t.Errorf("entrypoints do not match: %s should be %s", ep1, initialEntrypoint)
+	}
+
+	if !reflect.DeepEqual(ep2, secondEntrypoint) {
+		t.Errorf("entrypoints do not match: %s should be %s", ep2, secondEntrypoint)
+	}
+}
+
 func TestGetRemoteEntrypointWithNonDefaultSA(t *testing.T) {
 	expectedEntrypoint := []string{"/bin/expected", "entrypoint"}
 	img := getImage(t, &v1.ConfigFile{