Add an update timestamp to trigger the creation of a revision when needed.

CHANGELOG.adoc

@@ -17,9 +17,14 @@
 |===
 | | Description | PR
 
+| 🎁
+| Add an `client.knative.dev/updateTimestamp` annotation to trigger a new revision when required
+| https://github.com/knative/client/pull/1364[#1364]
+
 | ✨
 | make --cmd flag as an array instead of string
 | https://github.com/knative/client/pull/1380[#1380]
+
 |===
 
 ## v0.24.0 (2021-06-29)

pkg/kn/commands/service/configuration_edit_flags.go

@@ -205,9 +205,9 @@ func (p *ConfigurationEditFlags) Apply(
 		return err
 	}
 
-	name := ""
+	// Client side revision naming requires to set a new revisionname
 	if p.RevisionName != "" {
-		name, err = servinglib.GenerateRevisionName(p.RevisionName, service)
+		name, err := servinglib.GenerateRevisionName(p.RevisionName, service)
 		if err != nil {
 			return err
 		}
@@ -217,20 +217,28 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 	}
 
-	_, userImagePresent := template.Annotations[servinglib.UserImageAnnotationKey]
-	freezeMode := userImagePresent || cmd.Flags().Changed("lock-to-digest")
-	if p.LockToDigest && p.AnyMutation(cmd) && freezeMode {
-		servinglib.SetUserImageAnnot(template)
-		if !cmd.Flags().Changed("image") {
-			err = servinglib.FreezeImageToDigest(template, baseRevision)
+	// If some change happened that can cause a revision, set the update timestamp
+	// But not for "apply", this would destroy idempotency
+	if p.AnyMutation(cmd) && cmd.Name() != "apply" {
+		servinglib.UpdateTimestampAnnotation(template)
+	}
+
+	if p.shouldPinToImageDigest(template, cmd) {
+		servinglib.UpdateUserImageAnnotation(template)
+		// Don't copy over digest of base revision if an image is specified.
+		// If an --image is given, always use the tagged named to cause a re-resolving
+		// of the digest by the serving backend (except when you use "apply" where you
+		// always have to provide an image
+		if !cmd.Flags().Changed("image") || cmd.Name() == "apply" {
+			err = servinglib.PinImageToDigest(template, baseRevision)
 			if err != nil {
 				return err
 			}
 		}
 	}
 
 	if !p.LockToDigest {
-		servinglib.UnsetUserImageAnnot(template)
+		servinglib.UnsetUserImageAnnotation(template)
 	}
 
 	if cmd.Flags().Changed("limits-cpu") || cmd.Flags().Changed("limits-memory") {
@@ -432,6 +440,23 @@ func (p *ConfigurationEditFlags) Apply(
 	return nil
 }
 
+// shouldPinToImageDigest checks whether the image digest that has been resolved in the current active
+// revision should be used for the image update. This is useful if an update of the image
+// should be prevented.
+func (p *ConfigurationEditFlags) shouldPinToImageDigest(template *servingv1.RevisionTemplateSpec, cmd *cobra.Command) bool {
+	// The user-image annotation is an indicator that the service has been
+	// created with lock-to-digest. If this is not the case and neither --lock-to-digest
+	// has been given explitly then we won't change the image
+	_, userImagePresent := template.Annotations[servinglib.UserImageAnnotationKey]
+	if !userImagePresent && !cmd.Flags().Changed("lock-to-digest") {
+		return false
+	}
+
+	// When we want an update and --lock-to-digest is set (either given or the default), then
+	// the image should be pinned to its digest
+	return p.LockToDigest && p.AnyMutation(cmd)
+}
+
 func (p *ConfigurationEditFlags) updateLabels(obj *metav1.ObjectMeta, flagLabels []string, labelsAllMap map[string]string) error {
 	labelFlagMap, err := util.MapFromArrayAllowingSingles(flagLabels, "=")
 	if err != nil {

pkg/kn/commands/service/create_mock_test.go

@@ -78,7 +78,7 @@ func TestServiceCreateEnvMock(t *testing.T) {
 	template.Spec.Containers[0].Env = envVars
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "a=mouse", "--env", "b=cookie", "--env", "empty=", "--no-wait", "--revision-name=")
 	assert.NilError(t, err)
@@ -106,7 +106,7 @@ func TestServiceCreateLabel(t *testing.T) {
 	template := &service.Spec.Template
 	template.ObjectMeta.Labels = expected
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--label=empty", "--no-wait", "--revision-name=")
 	assert.NilError(t, err)
@@ -134,7 +134,7 @@ func TestServiceCreateWithEnvFromConfigMap(t *testing.T) {
 	}
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "--env-from", "config-map:config-map-name", "--no-wait", "--revision-name=")
 	assert.NilError(t, err)
@@ -154,7 +154,7 @@ func TestServiceCreateWithEnvFromConfigMapRemoval(t *testing.T) {
 	template.Spec.Containers[0].EnvFrom = nil
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "--env-from", "config-map:config-map-name-", "--no-wait", "--revision-name=")
 	assert.NilError(t, err)
@@ -199,7 +199,7 @@ func TestServiceCreateWithEnvFromSecret(t *testing.T) {
 	}
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "--env-from", "secret:secret-name", "--no-wait", "--revision-name=")
 	assert.NilError(t, err)
@@ -219,7 +219,7 @@ func TestServiceCreateWithEnvFromSecretRemoval(t *testing.T) {
 	template.Spec.Containers[0].EnvFrom = nil
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "--env-from", "secret:secret-name-", "--no-wait", "--revision-name=")
 	assert.NilError(t, err)
@@ -259,7 +259,7 @@ func TestServiceCreateWithVolumeAndMountConfigMap(t *testing.T) {
 
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--mount", "/mount/path=volume-name", "--volume", "volume-name=cm:config-map-name", "--no-wait", "--revision-name=")
@@ -300,7 +300,7 @@ func TestServiceCreateWithMountConfigMap(t *testing.T) {
 
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--mount", "/mount/path=cm:config-map-name", "--no-wait", "--revision-name=")
@@ -339,7 +339,7 @@ func TestServiceCreateWithVolumeAndMountSecret(t *testing.T) {
 
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--mount", "/mount/path=volume-name", "--volume", "volume-name=secret:secret-name", "--no-wait", "--revision-name=")
@@ -378,7 +378,7 @@ func TestServiceCreateWithMountSecret(t *testing.T) {
 
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--mount", "/mount/path=sc:secret-name", "--no-wait", "--revision-name=")
@@ -402,7 +402,7 @@ func TestServiceCreateWithUser(t *testing.T) {
 	}
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "--user", "1001", "--no-wait", "--revision-name=")
 	assert.NilError(t, err)
@@ -433,7 +433,7 @@ func TestServiceCreateWithResources(t *testing.T) {
 
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
 	template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"}
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--request", "cpu=250m,memory=64Mi",
@@ -534,7 +534,7 @@ func TestServiceCreateWithAnnotations(t *testing.T) {
 		servinglib.UserImageAnnotationKey:     "gcr.io/foo/bar:baz",
 	}
 
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--annotation", "foo=bar",
@@ -565,7 +565,7 @@ func TestServiceCreateWithRevisionAndServiceAnnotations(t *testing.T) {
 		servinglib.UserImageAnnotationKey:     "gcr.io/foo/bar:baz",
 	}
 
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--annotation-service", "foo=bar",
@@ -592,7 +592,7 @@ func TestServiceCreateWithRevisionAnnotations(t *testing.T) {
 		servinglib.UserImageAnnotationKey:     "gcr.io/foo/bar:baz",
 	}
 
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--annotation-revision", autoscaling.InitialScaleAnnotationKey+"=1",
@@ -621,7 +621,7 @@ func TestServiceCreateWithServiceAnnotations(t *testing.T) {
 		servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz",
 	}
 
-	r.CreateService(service, nil)
+	r.CreateService(verifyService(service, true), nil)
 
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--annotation-service", "foo=bar",

pkg/kn/commands/service/export.go

@@ -19,6 +19,8 @@ import (
 	"errors"
 	"fmt"
 
+	clientserving "knative.dev/client/pkg/serving"
+
 	"sort"
 	"strconv"
 
@@ -50,6 +52,7 @@ var IgnoredRevisionAnnotations = []string{
 	"serving.knative.dev/lastPinned",
 	"serving.knative.dev/creator",
 	"serving.knative.dev/routingStateModified",
+	clientserving.UpdateTimestampAnnotationKey,
 }
 
 // IgnoredServiceLabels defines the label keys which should be removed
@@ -178,6 +181,8 @@ func exportLatestService(latestSvc *servingv1.Service, withRoutes bool) *serving
 
 	stripIgnoredAnnotationsFromService(&exportedSvc)
 	stripIgnoredLabelsFromService(&exportedSvc)
+	stripIgnoredAnnotationsFromRevisionTemplate(&exportedSvc.Spec.Template)
+	stripIgnoredLabelsFromRevisionTemplate(&exportedSvc.Spec.Template)
 	return &exportedSvc
 }
 
@@ -357,6 +362,12 @@ func stripIgnoredAnnotationsFromRevision(revision *servingv1.Revision) {
 	}
 }
 
+func stripIgnoredAnnotationsFromRevisionTemplate(template *servingv1.RevisionTemplateSpec) {
+	for _, annotation := range IgnoredRevisionAnnotations {
+		delete(template.ObjectMeta.Annotations, annotation)
+	}
+}
+
 func stripIgnoredLabelsFromService(svc *servingv1.Service) {
 	for _, label := range IgnoredServiceLabels {
 		delete(svc.ObjectMeta.Labels, label)
@@ -368,3 +379,9 @@ func stripIgnoredLabelsFromRevision(rev *servingv1.Revision) {
 		delete(rev.ObjectMeta.Labels, label)
 	}
 }
+
+func stripIgnoredLabelsFromRevisionTemplate(template *servingv1.RevisionTemplateSpec) {
+	for _, label := range IgnoredRevisionLabels {
+		delete(template.ObjectMeta.Labels, label)
+	}
+}

pkg/kn/commands/service/export_test.go

@@ -19,6 +19,8 @@ import (
 	"encoding/json"
 	"testing"
 
+	"knative.dev/client/pkg/serving"
+
 	"gotest.tools/v3/assert"
 
 	v1 "k8s.io/api/core/v1"
@@ -127,11 +129,13 @@ func TestServiceExportwithMultipleRevisions(t *testing.T) {
 				servingtest.WithRevisionLabel(apiserving.ConfigurationGenerationLabelKey, "1"),
 				servingtest.WithRevisionAnn("client.knative.dev/user-image", "busybox:v1"),
 				servingtest.WithRevisionAnn("serving.knative.dev/lastPinned", "1111132"),
+				servingtest.WithRevisionAnn(serving.UpdateTimestampAnnotationKey, "now"),
 			))),
 			libtest.WithRevision(*(libtest.BuildRevision("foo-rev-2",
 				servingtest.WithRevisionLabel(apiserving.ServiceLabelKey, "foo"),
 				servingtest.WithRevisionLabel(apiserving.ConfigurationGenerationLabelKey, "1"),
 				servingtest.WithRevisionAnn("client.knative.dev/user-image", "busybox:v2"),
+				servingtest.WithRevisionAnn(serving.UpdateTimestampAnnotationKey, "now"),
 			))),
 		),
 		expectedKNExport: libtest.BuildKNExportWithOptions(

pkg/kn/commands/service/service_update_mock_test.go

@@ -129,9 +129,22 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) {
 
 func recordServiceUpdateWithSuccess(r *clientservingv1.ServingRecorder, svcName string, newService *servingv1.Service, updatedService *servingv1.Service) {
 	r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName))
-	r.CreateService(newService, nil)
+	r.CreateService(verifyService(newService, true), nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService, true, nil)
+	r.UpdateService(verifyService(updatedService, true), true, nil)
+}
+
+// verifyService checks for a service, and also whether the updateTimestamp is set
+func verifyService(srv *servingv1.Service, assertUpdateTimeCheck bool) func(t *testing.T, toCheck *servingv1.Service) {
+	return func(t *testing.T, toCheck *servingv1.Service) {
+		updateTimestamp := toCheck.Spec.Template.Annotations[clientserving.UpdateTimestampAnnotationKey]
+		// the update timestamp should be there
+		assert.Assert(t, (assertUpdateTimeCheck && updateTimestamp != "") || (!assertUpdateTimeCheck && updateTimestamp == ""))
+		// Delete the timestamps before doing a deep equal to avoid a conflict on this volatile value
+		delete(toCheck.Spec.Template.Annotations, clientserving.UpdateTimestampAnnotationKey)
+		assert.DeepEqual(t, srv, toCheck)
+	}
+
 }
 
 func TestServiceUpdateEnvFromAddingWithConfigMap(t *testing.T) {
@@ -283,7 +296,7 @@ func TestServiceUpdateEnvFromRemovalWithConfigMap(t *testing.T) {
 	r.GetService(svcName, updatedService1, nil)
 	//r.UpdateService(updatedService2, nil) // since an error happens, update is not triggered here
 	r.GetService(svcName, updatedService2, nil)
-	r.UpdateService(updatedService3, true, nil)
+	r.UpdateService(verifyService(updatedService3, true), true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",
@@ -369,10 +382,10 @@ func TestServiceUpdateEnvFromRemovalWithEmptyName(t *testing.T) {
 
 	r := client.Recorder()
 	r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName))
-	r.CreateService(newService, nil)
+	r.CreateService(verifyService(newService, true), nil)
 	r.GetService(svcName, newService, nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService1, true, nil)
+	r.UpdateService(verifyService(updatedService1, true), true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",
@@ -623,13 +636,13 @@ func TestServiceUpdateEnvFromRemovalWithSecret(t *testing.T) {
 
 	r := client.Recorder()
 	r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName))
-	r.CreateService(newService, nil)
+	r.CreateService(verifyService(newService, true), nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService1, true, nil)
+	r.UpdateService(verifyService(updatedService1, true), true, nil)
 	r.GetService(svcName, updatedService1, nil)
 	//r.UpdateService(updatedService2, nil) // since an error happens, update is not triggered here
 	r.GetService(svcName, updatedService2, nil)
-	r.UpdateService(updatedService3, true, nil)
+	r.UpdateService(verifyService(updatedService3, true), true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",
@@ -1381,11 +1394,11 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) {
 
 	r := client.Recorder()
 	r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName))
-	r.CreateService(newService, nil)
+	r.CreateService(verifyService(newService, true), nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService1, true, nil)
+	r.UpdateService(verifyService(updatedService1, true), true, nil)
 	r.GetService(svcName, updatedService1, nil)
-	r.UpdateService(updatedService2, true, nil)
+	r.UpdateService(verifyService(updatedService2, true), true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",