By default, set Image to the image of the prev. revision by digest (#…

…373)

* Option to freeze revision to digest

* Tests pass. Checkpoint.

* Tests for env now check pinning to digest

* Bool flag using convention. Tweak usage.

* Describing the image carefully using the annotation and digest

* lint

* Test matrix of locking to digest behaviors

* Expose both flags, and rewrite help text again

* Unit tests.

* Removed unsed method

* Add tests for getting base revision

* Make tests actually test stuff better

* Make tests actually test stuff better

* A mergeout killed a returning of error. Restore it

* Help text again

docs/cmd/kn_service_create.md

@@ -49,9 +49,11 @@ kn service create NAME --image IMAGE [flags]
   -l, --label stringArray        Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-).
       --limits-cpu string        The limits on the requested CPU (e.g., 1000m).
       --limits-memory string     The limits on the requested memory (e.g., 1024Mi).
+      --lock-to-digest           keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) (default true)
       --max-scale int            Maximal number of replicas.
       --min-scale int            Minimal number of replicas.
   -n, --namespace string         List the requested object(s) in given namespace.
+      --no-lock-to-digest        do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
   -p, --port int32               The port where application listens on.
       --requests-cpu string      The requested CPU (e.g., 250m).
       --requests-memory string   The requested memory (e.g., 64Mi).

docs/cmd/kn_service_update.md

@@ -47,9 +47,11 @@ kn service update NAME [flags]
   -l, --label stringArray        Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-).
       --limits-cpu string        The limits on the requested CPU (e.g., 1000m).
       --limits-memory string     The limits on the requested memory (e.g., 1024Mi).
+      --lock-to-digest           keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) (default true)
       --max-scale int            Maximal number of replicas.
       --min-scale int            Minimal number of replicas.
   -n, --namespace string         List the requested object(s) in given namespace.
+      --no-lock-to-digest        do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
   -p, --port int32               The port where application listens on.
       --requests-cpu string      The requested CPU (e.g., 250m).
       --requests-memory string   The requested memory (e.g., 64Mi).

pkg/kn/commands/service/configuration_edit_flags.go

@@ -21,6 +21,7 @@ import (
 	"github.com/spf13/cobra"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"knative.dev/client/pkg/kn/flags"
 	servinglib "knative.dev/client/pkg/serving"
 	util "knative.dev/client/pkg/util"
 	servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
@@ -41,7 +42,9 @@ type ConfigurationEditFlags struct {
 	RevisionName               string
 
 	// Preferences about how to do the action.
-	ForceCreate bool
+	LockToDigest         bool
+	GenerateRevisionName bool
+	ForceCreate          bool
 
 	// Bookkeeping
 	flags []string
@@ -100,6 +103,11 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 			"Accepts golang templates, allowing {{.Service}} for the service name, "+
 			"{{.Generation}} for the generation, and {{.Random [n]}} for n random consonants.")
 	p.markFlagMakesRevision("revision-name")
+	flags.AddBothBoolFlagsUnhidden(command.Flags(), &p.LockToDigest, "lock-to-digest", "", true,
+		"keep the running image for the service constant when not explicitly specifying "+
+			"the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)")
+	// Don't mark as changing the revision.
+
 }
 
 // AddUpdateFlags adds the flags specific to update.
@@ -118,6 +126,7 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
 // Apply mutates the given service according to the flags in the command.
 func (p *ConfigurationEditFlags) Apply(
 	service *servingv1alpha1.Service,
+	baseRevision *servingv1alpha1.Revision,
 	cmd *cobra.Command) error {
 
 	template, err := servinglib.RevisionTemplateOfService(service)
@@ -157,12 +166,28 @@ func (p *ConfigurationEditFlags) Apply(
 			return err
 		}
 	}
+	imageSet := false
 	if cmd.Flags().Changed("image") {
 		err = servinglib.UpdateImage(template, p.Image)
 		if err != nil {
 			return err
 		}
+		imageSet = true
 	}
+	_, userImagePresent := template.Annotations[servinglib.UserImageAnnotationKey]
+	freezeMode := userImagePresent || cmd.Flags().Changed("lock-to-digest")
+	if p.LockToDigest && p.AnyMutation(cmd) && freezeMode {
+		servinglib.SetUserImageAnnot(template)
+		if !imageSet {
+			err = servinglib.FreezeImageToDigest(template, baseRevision)
+			if err != nil {
+				return err
+			}
+		}
+	} else if !p.LockToDigest {
+		servinglib.UnsetUserImageAnnot(template)
+	}
+
 	limitsResources, err := p.computeResources(p.LimitsFlags)
 	if err != nil {
 		return err

pkg/kn/commands/service/service_create.go

@@ -20,7 +20,9 @@ import (
 	"io"
 
 	"knative.dev/client/pkg/kn/commands"
+	servinglib "knative.dev/client/pkg/serving"
 	"knative.dev/client/pkg/serving/v1alpha1"
+
 	"knative.dev/serving/pkg/apis/serving"
 
 	"github.com/spf13/cobra"
@@ -208,10 +210,15 @@ func constructService(cmd *cobra.Command, editFlags ConfigurationEditFlags, name
 
 	service.Spec.Template = &serving_v1alpha1_api.RevisionTemplateSpec{
 		Spec: serving_v1alpha1_api.RevisionSpec{},
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				servinglib.UserImageAnnotationKey: "", // Placeholder. Will be replaced or deleted as we apply mutations.
+			},
+		},
 	}
 	service.Spec.Template.Spec.Containers = []corev1.Container{{}}
 
-	err := editFlags.Apply(&service, cmd)
+	err := editFlags.Apply(&service, nil, cmd)
 	if err != nil {
 		return nil, err
 	}

pkg/kn/commands/service/service_create_mock_test.go

@@ -68,7 +68,10 @@ func TestServiceCreateLabel(t *testing.T) {
 		"b":     "cookie",
 		"empty": "",
 	}
-	service.ObjectMeta.Labels = expected
+	service.Labels = expected
+	service.Spec.Template.Annotations = map[string]string{
+		servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz",
+	}
 	template, err := servinglib.RevisionTemplateOfService(service)
 	assert.NilError(t, err)
 	template.ObjectMeta.Labels = expected

pkg/kn/commands/service/service_describe.go

@@ -29,6 +29,7 @@ import (
 	"knative.dev/serving/pkg/apis/serving"
 
 	"knative.dev/client/pkg/printers"
+	client_serving "knative.dev/client/pkg/serving"
 	serving_kn_v1alpha1 "knative.dev/client/pkg/serving/v1alpha1"
 
 	"knative.dev/pkg/apis"
@@ -72,6 +73,7 @@ type revisionDesc struct {
 	timeoutSeconds *int64
 
 	image       string
+	userImage   string
 	imageDigest string
 	env         []string
 	port        *int32
@@ -321,8 +323,18 @@ func formatStatus(status corev1.ConditionStatus) string {
 // Return either image name with tag or together with its resolved digest
 func getImageDesc(desc *revisionDesc) string {
 	image := desc.image
-	if printDetails && desc.imageDigest != "" {
-		return fmt.Sprintf("%s (%s)", image, shortenDigest(desc.imageDigest))
+	// Check if the user image is likely a more user-friendly description
+	pinnedDesc := "at"
+	if desc.userImage != "" && strings.Contains(image, "@") && desc.imageDigest != "" {
+		parts := strings.Split(image, "@")
+		// Check if the user image refers to the same thing.
+		if strings.HasPrefix(desc.userImage, parts[0]) {
+			pinnedDesc = "pinned to"
+			image = desc.userImage
+		}
+	}
+	if desc.imageDigest != "" {
+		return fmt.Sprintf("%s (%s %s)", image, pinnedDesc, shortenDigest(desc.imageDigest))
 	}
 	return image
 }
@@ -503,6 +515,7 @@ func newRevisionDesc(revision *v1alpha1.Revision, target *v1alpha1.TrafficTarget
 		name:              revision.Name,
 		logURL:            revision.Status.LogURL,
 		timeoutSeconds:    revision.Spec.TimeoutSeconds,
+		userImage:         revision.Annotations[client_serving.UserImageAnnotationKey],
 		imageDigest:       revision.Status.ImageDigest,
 		creationTimestamp: revision.CreationTimestamp.Time,
 

pkg/kn/commands/service/service_describe_test.go

@@ -34,6 +34,7 @@ import (
 	"knative.dev/serving/pkg/apis/serving/v1alpha1"
 	"knative.dev/serving/pkg/apis/serving/v1beta1"
 
+	client_serving "knative.dev/client/pkg/serving"
 	knclient "knative.dev/client/pkg/serving/v1alpha1"
 	"knative.dev/client/pkg/util"
 )
@@ -63,6 +64,7 @@ func TestServiceDescribeBasic(t *testing.T) {
 
 	validateServiceOutput(t, "foo", output)
 	assert.Assert(t, util.ContainsAll(output, "Env:", "label1=lval1, label2=lval2\n"))
+	assert.Assert(t, util.ContainsAll(output, "1234567"))
 	assert.Assert(t, util.ContainsAll(output, "Annotations:", "anno1=aval1, anno2=aval2, anno3="))
 	assert.Assert(t, cmp.Regexp(`(?m)\s*Annotations:.*\.\.\.$`, output))
 	assert.Assert(t, util.ContainsAll(output, "Labels:", "label1=lval1, label2=lval2\n"))
@@ -196,6 +198,55 @@ func TestServiceDescribeResources(t *testing.T) {
 	}
 }
 
+func TestServiceDescribeUserImageVsImage(t *testing.T) {
+	// New mock client
+	client := knclient.NewMockKnClient(t)
+
+	// Recording:
+	r := client.Recorder()
+
+	// Prepare service
+	expectedService := createTestService("foo", []string{"rev1", "rev2", "rev3", "rev4"}, goodConditions())
+	r.GetService("foo", &expectedService, nil)
+
+	rev1 := createTestRevision("rev1", 1)
+	rev2 := createTestRevision("rev2", 2)
+	rev3 := createTestRevision("rev3", 3)
+	rev4 := createTestRevision("rev4", 4)
+
+	// Different combinations of image annotations and not.
+	rev1.Spec.Containers[0].Image = "gcr.io/test/image:latest"
+	rev1.Annotations[client_serving.UserImageAnnotationKey] = "gcr.io/test/image:latest"
+	rev2.Spec.Containers[0].Image = "gcr.io/test/image@" + imageDigest
+	rev2.Annotations[client_serving.UserImageAnnotationKey] = "gcr.io/test/image:latest"
+	// rev3 is as if we changed the image but didn't change the annotation
+	rev3.Annotations[client_serving.UserImageAnnotationKey] = "gcr.io/test/image:latest"
+	rev3.Spec.Containers[0].Image = "gcr.io/a/b"
+	// rev4 is without the annotation at all and no hash
+	rev4.Status.ImageDigest = ""
+	rev4.Spec.Containers[0].Image = "gcr.io/x/y"
+
+	// Fetch the revisions
+	r.GetRevision("rev1", &rev1, nil)
+	r.GetRevision("rev2", &rev2, nil)
+	r.GetRevision("rev3", &rev3, nil)
+	r.GetRevision("rev4", &rev4, nil)
+
+	// Testing:
+	output, err := executeServiceCommand(client, "describe", "foo")
+	assert.NilError(t, err)
+
+	validateServiceOutput(t, "foo", output)
+
+	assert.Assert(t, util.ContainsAll(output, "Image", "Name", "gcr.io/test/image:latest (at 123456789012)",
+		"gcr.io/test/image:latest (pinned to 123456789012)", "gcr.io/a/b (at 123456789012)", "gcr.io/x/y"))
+	assert.Assert(t, util.ContainsAll(output, "[1]", "[2]"))
+
+	// Validate that all recorded API methods have been called
+	r.Validate()
+
+}
+
 func TestServiceDescribeVerbose(t *testing.T) {
 
 	// New mock client
@@ -234,7 +285,7 @@ func TestServiceDescribeVerbose(t *testing.T) {
 
 	validateServiceOutput(t, "foo", output)
 
-	assert.Assert(t, util.ContainsAll(output, "Image", "Name", "(123456789012)", "50%", "gcr.io/test/image", "(0s)"))
+	assert.Assert(t, util.ContainsAll(output, "Image", "Name", "gcr.io/test/image (at 123456789012)", "50%", "(0s)"))
 	assert.Assert(t, util.ContainsAll(output, "Env:", "label1=lval1\n", "label2=lval2\n"))
 	assert.Assert(t, util.ContainsAll(output, "Annotations:", "anno1=aval1\n", "anno2=aval2\n"))
 	assert.Assert(t, util.ContainsAll(output, "Labels:", "label1=lval1\n", "label2=lval2\n"))
@@ -381,6 +432,7 @@ func createTestRevision(revision string, gen int64) v1alpha1.Revision {
 			Namespace:         "default",
 			Generation:        1,
 			Labels:            labels,
+			Annotations:       make(map[string]string),
 			CreationTimestamp: metav1.Time{Time: time.Now()},
 		},
 		Spec: v1alpha1.RevisionSpec{
@@ -402,7 +454,7 @@ func createTestRevision(revision string, gen int64) v1alpha1.Revision {
 			},
 		},
 		Status: v1alpha1.RevisionStatus{
-			ImageDigest: imageDigest,
+			ImageDigest: "gcr.io/test/image@" + imageDigest,
 		},
 	}
 }

pkg/kn/commands/service/service_update.go

@@ -24,6 +24,8 @@ import (
 	"knative.dev/client/pkg/kn/traffic"
 
 	"knative.dev/client/pkg/kn/commands"
+	serving "knative.dev/client/pkg/serving/v1alpha1"
+	"knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
 
 func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
@@ -77,7 +79,14 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 				}
 				service = service.DeepCopy()
 
-				err = editFlags.Apply(service, cmd)
+				var baseRevision *v1alpha1.Revision
+				if !cmd.Flags().Changed("image") && editFlags.LockToDigest {
+					baseRevision, err = client.GetBaseRevision(service)
+					if _, ok := err.(*serving.NoBaseRevisionError); ok {
+						fmt.Fprintf(cmd.OutOrStdout(), "Warning: No reivision found to update image digest")
+					}
+				}
+				err = editFlags.Apply(service, baseRevision, cmd)
 				if err != nil {
 					return err
 				}