Added digest cache, store digest instead of tags, added test

Fix PR comments

Fix PR comments 2

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

@@ -20,9 +20,9 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/name"
+	"github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/hashicorp/golang-lru"
 	corev1 "k8s.io/api/core/v1"
@@ -41,8 +41,8 @@ const (
 	InitContainerName = "place-tools"
 	ProcessLogFile    = "/tools/process-log.txt"
 	MarkerFile        = "/tools/marker-file.txt"
-
-	cacheSize = 1024
+	digestSeparator   = "@"
+	cacheSize         = 1024
 )
 
 var toolsMount = corev1.VolumeMount{
@@ -112,30 +112,47 @@ func getEnvVar(cmd, args []string) (string, error) {
 	return string(j), nil
 }
 
-// GetRemoteEntrypoint accepts a cache of image lookups, as well as the image
-// to look for. If the cache does not contain the image, it will lookup the
+// 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, image string) ([]string, error) {
-	if ep, ok := cache.get(image); ok {
+func GetRemoteEntrypoint(cache *Cache, digest string) ([]string, error) {
+	if ep, ok := cache.get(digest); ok {
 		return ep, nil
 	}
-	// verify the image name, then download the remote config file
-	ref, err := name.ParseReference(image, name.WeakValidation)
+	img, err := getRemoteImage(digest)
 	if err != nil {
-		return nil, fmt.Errorf("couldn't parse image %s: %v", image, err)
-	}
-	img, err := remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain))
-	if err != nil {
-		return nil, fmt.Errorf("couldn't get container image info from registry %s: %v", image, err)
+		return nil, fmt.Errorf("failed to fetch remote image %s: %v", digest, err)
 	}
 	cfg, err := img.ConfigFile()
 	if err != nil {
-		return nil, fmt.Errorf("couldn't get config for image %s: %v", image, err)
+		return nil, fmt.Errorf("couldn't get config for image %s: %v", digest, err)
 	}
-	cache.set(image, cfg.ContainerConfig.Entrypoint)
+	cache.set(digest, cfg.ContainerConfig.Entrypoint)
 	return cfg.ContainerConfig.Entrypoint, nil
 }
 
+// GetImageDigest tries to find and return image digest in cache, if
+// cache doesn't exists it will lookup the digest in remote image manifest
+// and then cache it
+func GetImageDigest(cache *Cache, image string) (string, error) {
+	if digestList, ok := cache.get(image); ok && (len(digestList) > 0) {
+		return digestList[0], nil
+	}
+	img, err := getRemoteImage(image)
+	if err != nil {
+		return "", fmt.Errorf("failed to fetch remote image %s: %v", image, err)
+	}
+	digestHash, err := img.Digest()
+	if err != nil {
+		return "", fmt.Errorf("couldn't get digest hash for image %s: %v", image, err)
+	}
+	// Parse Digest Hash struct into sha string
+	digest := fmt.Sprintf("%s%s%s", image, digestSeparator, digestHash.String())
+	cache.set(image, []string{digest})
+
+	return digest, nil
+}
+
 // RedirectSteps will modify each of the steps/containers such that
 // the binary being run is no longer the one specified by the Command
 // and the Args, but is instead the entrypoint binary, which will
@@ -158,3 +175,17 @@ func RedirectSteps(steps []corev1.Container) error {
 	}
 	return nil
 }
+
+func getRemoteImage(image string) (v1.Image, error) {
+	// verify the image name, then download the remote config file
+	ref, err := name.ParseReference(image, name.WeakValidation)
+	if err != nil {
+		return nil, fmt.Errorf("couldn't parse image %s: %v", image, err)
+	}
+	img, err := remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain))
+	if err != nil {
+		return nil, fmt.Errorf("couldn't get container image info from registry %s: %v", image, err)
+	}
+
+	return img, nil
+}

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

@@ -137,6 +137,11 @@ func mustConfigName(t *testing.T, img v1.Image) v1.Hash {
 	return h
 }
 
+func getDigestAsString(image v1.Image) string {
+	digestHash, _ := image.Digest()
+	return digestHash.String()
+}
+
 func TestGetRemoteEntrypoint(t *testing.T) {
 	expectedEntrypoint := []string{"/bin/expected", "entrypoint"}
 	img := getImage(t, &v1.ConfigFile{
@@ -145,8 +150,9 @@ func TestGetRemoteEntrypoint(t *testing.T) {
 		},
 	})
 	expectedRepo := "image"
+	digetsSha := getDigestAsString(img)
 	configPath := fmt.Sprintf("/v2/%s/blobs/%s", expectedRepo, mustConfigName(t, img))
-	manifestPath := fmt.Sprintf("/v2/%s/manifests/latest", expectedRepo)
+	manifestPath := fmt.Sprintf("/v2/%s/manifests/%s", expectedRepo, digetsSha)
 
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch r.URL.Path {
@@ -167,12 +173,14 @@ func TestGetRemoteEntrypoint(t *testing.T) {
 		}
 	}))
 	defer server.Close()
-	image := path.Join(strings.TrimPrefix(server.URL, "http://"), "image:latest")
+	image := path.Join(strings.TrimPrefix(server.URL, "http://"), expectedRepo)
+	finalDigest := image + "@" + digetsSha
+
 	entrypointCache, err := NewCache()
 	if err != nil {
 		t.Fatalf("couldn't create new entrypoint cache: %v", err)
 	}
-	ep, err := GetRemoteEntrypoint(entrypointCache, image)
+	ep, err := GetRemoteEntrypoint(entrypointCache, finalDigest)
 	if err != nil {
 		t.Errorf("couldn't get entrypoint remote: %v", err)
 	}
@@ -181,6 +189,44 @@ func TestGetRemoteEntrypoint(t *testing.T) {
 	}
 }
 
+func TestGetImageDigest(t *testing.T) {
+	img := getImage(t, &v1.ConfigFile{
+		ContainerConfig: v1.Config{},
+	})
+	digetsSha := getDigestAsString(img)
+	expectedRepo := "image"
+	manifestPath := fmt.Sprintf("/v2/%s/manifests/latest", expectedRepo)
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/v2/":
+			w.WriteHeader(http.StatusOK)
+		case manifestPath:
+			if r.Method != http.MethodGet {
+				t.Errorf("Method; got %v, want %v", r.Method, http.MethodGet)
+			}
+			w.Write(mustRawManifest(t, img))
+		default:
+			t.Fatalf("Unexpected path: %v", r.URL.Path)
+		}
+	}))
+	defer server.Close()
+	image := path.Join(strings.TrimPrefix(server.URL, "http://"), "image:latest")
+	expectedDigetsSha := image + "@" + digetsSha
+
+	digestCache, err := NewCache()
+	if err != nil {
+		t.Fatalf("couldn't create new digest cache: %v", err)
+	}
+	digest, err := GetImageDigest(digestCache, image)
+	if err != nil {
+		t.Errorf("couldn't get digest remote: %v", err)
+	}
+	if !reflect.DeepEqual(expectedDigetsSha, digest) {
+		t.Errorf("digest do not match: %s should be %s", expectedDigetsSha, digest)
+	}
+}
+
 func TestEntrypointCacheLRU(t *testing.T) {
 	entrypoint := []string{"/bin/expected", "entrypoint"}
 	entrypointCache, err := NewCache()

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -387,15 +387,23 @@ func (c *Reconciler) createBuildPod(ctx context.Context, tr *v1alpha1.TaskRun, t
 
 	// For each step with no entrypoint set, try to populate it with the info
 	// from the remote registry
-	cache, err := entrypoint.NewCache()
+	entrypointCache, err := entrypoint.NewCache()
 	if err != nil {
 		return nil, fmt.Errorf("couldn't create new entrypoint cache: %v", err)
 	}
+	digestCache, err := entrypoint.NewCache()
+	if err != nil {
+		return nil, fmt.Errorf("couldn't create new digest cache: %v", err)
+	}
 	bSpec := bs.DeepCopy()
 	for i := range bSpec.Steps {
 		step := &bSpec.Steps[i]
 		if len(step.Command) == 0 {
-			ep, err := entrypoint.GetRemoteEntrypoint(cache, step.Image)
+			digest, err := entrypoint.GetImageDigest(digestCache, step.Image)
+			if err != nil {
+				return nil, fmt.Errorf("could not get digest for %s: %v", step.Image, err)
+			}
+			ep, err := entrypoint.GetRemoteEntrypoint(entrypointCache, digest)
 			if err != nil {
 				return nil, fmt.Errorf("could not get entrypoint from registry for %s: %v", step.Image, err)
 			}