Add '.status.imageDigest' to Revision (knative#2197)

* Add '.status.imageDigest' to Revision

This change moves image digest resolution from Deployment creation
to Revision reconciliation, and introduces a new field on Revision
status. This change preseves the option to list registries to skip
in which case the imageDigest field will not be populated. When the
imageDigest field is populated, it is used for Deployment creation.

Fixes knative#2064

* Resolve comments

* Update spec documentation and change Resolve interface

* Update deploy test and add conformance test

* Fix rebase test error and typo

Author: Dan Gerdesmeier

docs/spec/spec.md

@@ -304,6 +304,10 @@ status:
   #   revision. Typically, the name will be the same as the name of the
   #   revision.
   serviceName: myservice-a1e34
+
+  # imageDigest: The imageDigest is the spec.container.image field resolved
+  #   to a particular digest at revision creation.
+  imageDigest: gcr.io/my-project/...@sha256:60ab5...
 ```
 
 

pkg/apis/serving/v1alpha1/revision_types.go

@@ -224,6 +224,14 @@ type RevisionStatus struct {
 	// based on the revision url template specified in the controller's config.
 	// +optional
 	LogURL string `json:"logUrl,omitempty"`
+
+	// ImageDigest holds the resolved digest for the image specified
+	// within .Spec.Container.Image. The digest is resolved during the creation
+	// of Revision. This field holds the digest value regardless of whether
+	// a tag or digest was originally specified in the Container object. It
+	// may be empty if the image comes from a registry listed to skip resolution.
+	// +optional
+	ImageDigest string `json:"imageDigest,omitempty"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

pkg/reconciler/v1alpha1/revision/cruds.go

@@ -18,7 +18,6 @@ package revision
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/google/go-cmp/cmp"
 
@@ -28,14 +27,12 @@ import (
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config"
 	"github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources"
-	"go.uber.org/zap"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 )
 
 func (c *Reconciler) createDeployment(ctx context.Context, rev *v1alpha1.Revision) (*appsv1.Deployment, error) {
-	logger := logging.FromContext(ctx)
 	cfgs := config.FromContext(ctx)
 
 	deployment := resources.MakeDeployment(
@@ -47,13 +44,6 @@ func (c *Reconciler) createDeployment(ctx context.Context, rev *v1alpha1.Revisio
 		cfgs.Controller,
 	)
 
-	// Resolve tag image references to digests.
-	if err := c.resolver.Resolve(deployment, cfgs.Controller.RegistriesSkippingTagResolving); err != nil {
-		logger.Error("Error resolving deployment", zap.Error(err))
-		rev.Status.MarkContainerMissing(err.Error())
-		return nil, fmt.Errorf("Error resolving container to digest: %v", err)
-	}
-
 	return c.KubeClientSet.AppsV1().Deployments(deployment.Namespace).Create(deployment)
 }
 

pkg/reconciler/v1alpha1/revision/queueing_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-containerregistry/pkg/authn/k8schain"
 	fakecachingclientset "github.com/knative/caching/pkg/client/clientset/versioned/fake"
 	cachinginformers "github.com/knative/caching/pkg/client/informers/externalversions"
 	"github.com/knative/pkg/apis/duck"
@@ -34,7 +35,6 @@ import (
 	rclr "github.com/knative/serving/pkg/reconciler"
 	"github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config"
 	"github.com/knative/serving/pkg/system"
-	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -47,8 +47,8 @@ import (
 
 type nopResolver struct{}
 
-func (r *nopResolver) Resolve(*appsv1.Deployment, map[string]struct{}) error {
-	return nil
+func (r *nopResolver) Resolve(_ string, _ k8schain.Options, _ map[string]struct{}) (string, error) {
+	return "", nil
 }
 
 const (

pkg/reconciler/v1alpha1/revision/resolve.go

@@ -24,7 +24,6 @@ import (
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 
-	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/client-go/kubernetes"
 )
 
@@ -35,44 +34,36 @@ type digestResolver struct {
 
 // Resolve resolves the image references that use tags to digests.
 func (r *digestResolver) Resolve(
-	deploy *appsv1.Deployment,
+	image string,
+	opt k8schain.Options,
 	registriesToSkip map[string]struct{},
-) error {
-	pod := deploy.Spec.Template.Spec
-	opt := k8schain.Options{
-		Namespace:          deploy.Namespace,
-		ServiceAccountName: pod.ServiceAccountName,
-		// ImagePullSecrets: Not possible via RevisionSpec, since we
-		// don't expose such a field.
-	}
+) (string, error) {
 	kc, err := k8schain.New(r.client, opt)
 	if err != nil {
-		return err
+		return "", err
 	}
 
-	for i := range pod.Containers {
-		if _, err := name.NewDigest(pod.Containers[i].Image, name.WeakValidation); err == nil {
-			// Already a digest
-			continue
-		}
-		tag, err := name.NewTag(pod.Containers[i].Image, name.WeakValidation)
-		if err != nil {
-			return err
-		}
+	if _, err := name.NewDigest(image, name.WeakValidation); err == nil {
+		// Already a digest
+		return image, nil
+	}
 
-		if _, ok := registriesToSkip[tag.Registry.RegistryStr()]; ok {
-			continue
-		}
+	tag, err := name.NewTag(image, name.WeakValidation)
+	if err != nil {
+		return "", err
+	}
 
-		img, err := remote.Image(tag, remote.WithTransport(r.transport), remote.WithAuthFromKeychain(kc))
-		if err != nil {
-			return err
-		}
-		digest, err := img.Digest()
-		if err != nil {
-			return err
-		}
-		pod.Containers[i].Image = fmt.Sprintf("%s@%s", tag.Repository.String(), digest)
+	if _, ok := registriesToSkip[tag.Registry.RegistryStr()]; ok {
+		return "", nil
+	}
+
+	img, err := remote.Image(tag, remote.WithTransport(r.transport), remote.WithAuthFromKeychain(kc))
+	if err != nil {
+		return "", err
+	}
+	digest, err := img.Digest()
+	if err != nil {
+		return "", err
 	}
-	return nil
+	return fmt.Sprintf("%s@%s", tag.Repository.String(), digest), nil
 }