From 0ff537ad982391df7bb9efab7a2c5195d0bdedcf Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Tue, 27 Aug 2019 11:42:40 -0700 Subject: [PATCH] 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 | 2 + docs/cmd/kn_service_update.md | 2 + .../service/configuration_edit_flags.go | 27 +- pkg/kn/commands/service/service_create.go | 9 +- .../service/service_create_mock_test.go | 5 +- pkg/kn/commands/service/service_describe.go | 17 +- .../commands/service/service_describe_test.go | 56 ++- pkg/kn/commands/service/service_update.go | 11 +- .../commands/service/service_update_test.go | 363 ++++++++++++------ pkg/kn/commands/testing_helper.go | 6 + pkg/kn/flags/bool.go | 15 +- pkg/serving/config_changes.go | 52 ++- pkg/serving/config_changes_test.go | 50 ++- pkg/serving/v1alpha1/client.go | 60 +++ pkg/serving/v1alpha1/client_mock.go | 4 + pkg/serving/v1alpha1/client_test.go | 50 +++ test/e2e/service_options_test.go | 2 +- 17 files changed, 589 insertions(+), 142 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 7800ca32c..39defc7bc 100644 --- a/docs/cmd/kn_service_create.md +++ b/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). diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 48c590843..d448d8ab9 100644 --- a/docs/cmd/kn_service_update.md +++ b/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). diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index fd376a15b..15ba637f1 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/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 diff --git a/pkg/kn/commands/service/service_create.go b/pkg/kn/commands/service/service_create.go index e5c942859..50db8e432 100644 --- a/pkg/kn/commands/service/service_create.go +++ b/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 } diff --git a/pkg/kn/commands/service/service_create_mock_test.go b/pkg/kn/commands/service/service_create_mock_test.go index ee6d9a33f..7cbd77d0c 100644 --- a/pkg/kn/commands/service/service_create_mock_test.go +++ b/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 diff --git a/pkg/kn/commands/service/service_describe.go b/pkg/kn/commands/service/service_describe.go index acfefaafd..56e8b3d17 100644 --- a/pkg/kn/commands/service/service_describe.go +++ b/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, diff --git a/pkg/kn/commands/service/service_describe_test.go b/pkg/kn/commands/service/service_describe_test.go index 34548aac2..4ccad4c31 100644 --- a/pkg/kn/commands/service/service_describe_test.go +++ b/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, }, } } diff --git a/pkg/kn/commands/service/service_update.go b/pkg/kn/commands/service/service_update.go index 9b9a995e6..8922d8b64 100644 --- a/pkg/kn/commands/service/service_update.go +++ b/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 } diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index 6b708aa74..8e42888b5 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -38,11 +38,16 @@ import ( "knative.dev/serving/pkg/apis/serving/v1alpha1" ) +var exampleImageByDigest = "gcr.io/foo/bar@sha256:deadbeefdeadbeef" +var exampleRevisionName = "foo-asdf" +var exampleRevisionName2 = "foo-xyzzy" + func fakeServiceUpdate(original *v1alpha1.Service, args []string, sync bool) ( action client_testing.Action, updated *v1alpha1.Service, output string, err error) { + var reconciled v1alpha1.Service knParams := &commands.KnParams{} cmd, fakeServing, buf := commands.CreateTestKnCommand(NewServiceCommand(knParams), knParams) fakeServing.AddReactor("update", "*", @@ -58,9 +63,30 @@ func fakeServiceUpdate(original *v1alpha1.Service, args []string, sync bool) ( } return true, updated, nil }) - fakeServing.AddReactor("get", "*", + fakeServing.AddReactor("get", "services", func(a client_testing.Action) (bool, runtime.Object, error) { - return true, original, nil + if updated == nil { + original.Status.LatestCreatedRevisionName = exampleRevisionName + return true, original, nil + } + reconciled = *updated + if updated.Spec.Template.Name == "" { + reconciled.Status.LatestCreatedRevisionName = exampleRevisionName2 + } else { + reconciled.Status.LatestCreatedRevisionName = updated.Spec.Template.Name + } + + return true, &reconciled, nil + }) + fakeServing.AddReactor("get", "revisions", + // This is important for the way we set images to their image digest + func(a client_testing.Action) (bool, runtime.Object, error) { + rev := &v1alpha1.Revision{} + rev.Spec = original.Spec.Template.Spec + rev.ObjectMeta = original.Spec.Template.ObjectMeta + rev.Name = original.Status.LatestCreatedRevisionName + rev.Status.ImageDigest = exampleImageByDigest + return true, rev, nil }) if sync { fakeServing.AddWatchReactor("services", @@ -128,42 +154,46 @@ func TestServiceUpdateImageSync(t *testing.T) { template, err = servinglib.RevisionTemplateOfService(updated) assert.NilError(t, err) - assert.Equal(t, template.Spec.DeprecatedContainer.Image, "gcr.io/foo/quux:xyzzy") + assert.Equal(t, template.Spec.Containers[0].Image, "gcr.io/foo/quux:xyzzy") assert.Assert(t, util.ContainsAll(strings.ToLower(output), "update", "foo", "service", "namespace", "bar", "ok", "waiting")) } func TestServiceUpdateImage(t *testing.T) { - orig := newEmptyService() + for _, orig := range []*v1alpha1.Service{newEmptyServiceBetaAPIStyle(), newEmptyServiceAlphaAPIStyle()} { - template, err := servinglib.RevisionTemplateOfService(orig) - if err != nil { - t.Fatal(err) - } + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } - servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") - action, updated, output, err := fakeServiceUpdate(orig, []string{ - "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--async"}, false) + action, updated, output, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--async"}, false) - if err != nil { - t.Fatal(err) - } else if !action.Matches("update", "services") { - t.Fatalf("Bad action %v", action) - } + if err != nil { + t.Fatal(err) + } else if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } - template, err = servinglib.RevisionTemplateOfService(updated) - if err != nil { - t.Fatal(err) - } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/quux:xyzzy" { - t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) - } + template, err = servinglib.RevisionTemplateOfService(updated) + if err != nil { + t.Fatal(err) + } + container, err := servinglib.ContainerOfRevisionTemplate(template) + if err != nil { + t.Fatal(err) + } + assert.Equal(t, container.Image, "gcr.io/foo/quux:xyzzy") - if !strings.Contains(strings.ToLower(output), "update") || - !strings.Contains(output, "foo") || - !strings.Contains(strings.ToLower(output), "service") || - !strings.Contains(strings.ToLower(output), "namespace") || - !strings.Contains(output, "bar") { - t.Fatalf("wrong or no success message: %s", output) + if !strings.Contains(strings.ToLower(output), "update") || + !strings.Contains(output, "foo") || + !strings.Contains(strings.ToLower(output), "service") || + !strings.Contains(strings.ToLower(output), "namespace") || + !strings.Contains(output, "bar") { + t.Fatalf("wrong or no success message: %s", output) + } } } @@ -299,33 +329,13 @@ func TestServiceUpdateMaxMinScale(t *testing.T) { } func TestServiceUpdateEnv(t *testing.T) { - orig := &v1alpha1.Service{ - TypeMeta: metav1.TypeMeta{ - Kind: "Service", - APIVersion: "knative.dev/v1alpha1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", - }, - Spec: v1alpha1.ServiceSpec{ - DeprecatedRunLatest: &v1alpha1.RunLatestType{ - Configuration: v1alpha1.ConfigurationSpec{ - DeprecatedRevisionTemplate: &v1alpha1.RevisionTemplateSpec{ - Spec: v1alpha1.RevisionSpec{ - DeprecatedContainer: &corev1.Container{}, - }, - }, - }, - }, - }, - } + orig := newEmptyService() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { t.Fatal(err) } - template.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ + template.Spec.Containers[0].Env = []corev1.EnvVar{ {Name: "EXISTING", Value: "thing"}, {Name: "OTHEREXISTING"}, } @@ -346,13 +356,115 @@ func TestServiceUpdateEnv(t *testing.T) { } template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + // Test that we pinned to digest + assert.Equal(t, template.Spec.Containers[0].Image, exampleImageByDigest) + assert.Equal(t, template.Spec.Containers[0].Env[0], expectedEnvVar) +} + +func TestServiceUpdatePinsToDigestWhenAsked(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + delete(template.Annotations, servinglib.UserImageAnnotationKey) if err != nil { t.Fatal(err) - } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) - } else if template.Spec.DeprecatedContainer.Env[0] != expectedEnvVar { - t.Fatalf("wrong env set: %v", template.Spec.DeprecatedContainer.Env) } + + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "-e", "TARGET=Awesome", "--lock-to-digest", "--async"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + // Test that we pinned to digest + assert.Equal(t, template.Spec.Containers[0].Image, exampleImageByDigest) +} + +func TestServiceUpdatePinsToDigestWhenPreviouslyDidSo(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "-e", "TARGET=Awesome", "--async"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + // Test that we pinned to digest + assert.Equal(t, template.Spec.Containers[0].Image, exampleImageByDigest) +} + +func TestServiceUpdateDoesntPinToDigestWhenUnAsked(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "-e", "TARGET=Awesome", "--no-lock-to-digest", "--async"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + // Test that we pinned to digest + assert.Equal(t, template.Spec.Containers[0].Image, "gcr.io/foo/bar:baz") + _, present := template.Annotations[servinglib.UserImageAnnotationKey] + assert.Assert(t, !present) +} +func TestServiceUpdateDoesntPinToDigestWhenPreviouslyDidnt(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + delete(template.Annotations, servinglib.UserImageAnnotationKey) + if err != nil { + t.Fatal(err) + } + + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "-e", "TARGET=Awesome", "--async"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + // Test that we pinned to digest + assert.Equal(t, template.Spec.Containers[0].Image, "gcr.io/foo/bar:baz") + _, present := template.Annotations[servinglib.UserImageAnnotationKey] + assert.Assert(t, !present) } func TestServiceUpdateRequestsLimitsCPU(t *testing.T) { @@ -380,15 +492,15 @@ func TestServiceUpdateRequestsLimitsCPU(t *testing.T) { t.Fatal(err) } else { if !reflect.DeepEqual( - newTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.Containers[0].Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) } if !reflect.DeepEqual( - newTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.Containers[0].Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) } } } @@ -418,15 +530,15 @@ func TestServiceUpdateRequestsLimitsMemory(t *testing.T) { t.Fatal(err) } else { if !reflect.DeepEqual( - newTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.Containers[0].Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) } if !reflect.DeepEqual( - newTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.Containers[0].Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) } } } @@ -458,21 +570,30 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) { t.Fatal(err) } else { if !reflect.DeepEqual( - newTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.Containers[0].Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) } if !reflect.DeepEqual( - newTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.Containers[0].Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) } } } func TestServiceUpdateLabelWhenEmpty(t *testing.T) { original := newEmptyService() + origTemplate, err := servinglib.RevisionTemplateOfService(original) + if err != nil { + t.Fatal(err) + } + origContainer, err := servinglib.ContainerOfRevisionTemplate(origTemplate) + if err != nil { + t.Fatal(err) + } + origContainer.Image = "gcr.io/foo/bar:latest" action, updated, _, err := fakeServiceUpdate(original, []string{ "service", "update", "foo", "-l", "a=mouse", "--label", "b=cookie", "-l=single", "--async"}, false) @@ -495,6 +616,11 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { assert.NilError(t, err) actual = template.ObjectMeta.Labels assert.DeepEqual(t, expected, actual) + container, err := servinglib.ContainerOfRevisionTemplate(template) + if err != nil { + t.Fatal(err) + } + assert.Equal(t, container.Image, exampleImageByDigest) } func TestServiceUpdateLabelExisting(t *testing.T) { @@ -526,6 +652,52 @@ func TestServiceUpdateLabelExisting(t *testing.T) { } func newEmptyService() *v1alpha1.Service { + return newEmptyServiceBetaAPIStyle() +} + +func newEmptyServiceBetaAPIStyle() *v1alpha1.Service { + ret := &v1alpha1.Service{ + TypeMeta: metav1.TypeMeta{ + Kind: "Service", + APIVersion: "knative.dev/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + }, + Spec: v1alpha1.ServiceSpec{}, + } + ret.Spec.Template = &v1alpha1.RevisionTemplateSpec{} + ret.Spec.Template.Annotations = map[string]string{ + servinglib.UserImageAnnotationKey: "", + } + ret.Spec.Template.Spec.Containers = []corev1.Container{{}} + return ret +} + +func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, limitsCPU, limitsMemory string) *v1alpha1.Service { + service := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(service) + if err != nil { + t.Fatal(err) + } + + template.Spec.Containers[0].Resources = corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(requestCPU), + corev1.ResourceMemory: resource.MustParse(requestMemory), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(limitsCPU), + corev1.ResourceMemory: resource.MustParse(limitsMemory), + }, + } + + return service +} + +func newEmptyServiceAlphaAPIStyle() *v1alpha1.Service { return &v1alpha1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", @@ -548,62 +720,3 @@ func newEmptyService() *v1alpha1.Service { }, } } - -func newEmptyServiceBetaAPIStyle() *v1alpha1.Service { - ret := &v1alpha1.Service{ - TypeMeta: metav1.TypeMeta{ - Kind: "Service", - APIVersion: "knative.dev/v1alpha1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", - }, - Spec: v1alpha1.ServiceSpec{}, - } - ret.Spec.Template = &v1alpha1.RevisionTemplateSpec{} - ret.Spec.Template.Spec.Containers = []corev1.Container{{}} - return ret -} - -func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, limitsCPU, limitsMemory string) *v1alpha1.Service { - service := &v1alpha1.Service{ - TypeMeta: metav1.TypeMeta{ - Kind: "Service", - APIVersion: "knative.dev/v1alpha1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - Namespace: "default", - }, - Spec: v1alpha1.ServiceSpec{ - DeprecatedRunLatest: &v1alpha1.RunLatestType{ - Configuration: v1alpha1.ConfigurationSpec{ - DeprecatedRevisionTemplate: &v1alpha1.RevisionTemplateSpec{ - Spec: v1alpha1.RevisionSpec{ - DeprecatedContainer: &corev1.Container{}, - }, - }, - }, - }, - }, - } - - template, err := servinglib.RevisionTemplateOfService(service) - if err != nil { - t.Fatal(err) - } - - template.Spec.DeprecatedContainer.Resources = corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(requestCPU), - corev1.ResourceMemory: resource.MustParse(requestMemory), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(limitsCPU), - corev1.ResourceMemory: resource.MustParse(limitsMemory), - }, - } - - return service -} diff --git a/pkg/kn/commands/testing_helper.go b/pkg/kn/commands/testing_helper.go index af9cf1765..f47569376 100644 --- a/pkg/kn/commands/testing_helper.go +++ b/pkg/kn/commands/testing_helper.go @@ -25,6 +25,8 @@ import ( "github.com/spf13/viper" "gotest.tools/assert" client_testing "k8s.io/client-go/testing" + "knative.dev/client/pkg/kn/flags" + "knative.dev/client/pkg/serving/v1alpha1" "knative.dev/serving/pkg/client/clientset/versioned/typed/serving/v1alpha1/fake" ) @@ -107,6 +109,10 @@ Eventing: Manage event subscriptions and channels. Connect up event sources.`, // Prevents Cobra from dealing with errors as we deal with them in main.go SilenceErrors: true, + + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return flags.ReconcileBoolFlags(cmd.Flags()) + }, } if params.Output != nil { rootCmd.SetOutput(params.Output) diff --git a/pkg/kn/flags/bool.go b/pkg/kn/flags/bool.go index a11f1741a..49ec4bf44 100644 --- a/pkg/kn/flags/bool.go +++ b/pkg/kn/flags/bool.go @@ -24,16 +24,23 @@ import ( var negPrefix = "no-" -// AddBothBoolFlags adds the given flag in both `--foo` and `--no-foo` variants. -// If you do this, make sure you call ReconcileBoolFlags later to catch errors and -// set the relationship between the flag values. -func AddBothBoolFlags(f *pflag.FlagSet, p *bool, name, short string, value bool, usage string) { +// AddBothBoolFlagsUnhidden is just like AddBothBoolFlags but shows both flags. +func AddBothBoolFlagsUnhidden(f *pflag.FlagSet, p *bool, name, short string, value bool, usage string) { negativeName := negPrefix + name f.BoolVarP(p, name, short, value, usage) f.Bool(negativeName, !value, "do not "+usage) +} + +// AddBothBoolFlags adds the given flag in both `--foo` and `--no-foo` variants. +// If you do this, make sure you call ReconcileBoolFlags later to catch errors +// and set the relationship between the flag values. Only the flag that does the +// non-default behavior is visible; the other is hidden. +func AddBothBoolFlags(f *pflag.FlagSet, p *bool, name, short string, value bool, usage string) { + AddBothBoolFlagsUnhidden(f, p, name, short, value, usage) + negativeName := negPrefix + name if value { err := f.MarkHidden(name) if err != nil { diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 346747cf1..d438d1792 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "strconv" + "strings" corev1 "k8s.io/api/core/v1" "knative.dev/serving/pkg/apis/autoscaling" @@ -26,6 +27,8 @@ import ( servingv1beta1 "knative.dev/serving/pkg/apis/serving/v1beta1" ) +var UserImageAnnotationKey = "client.knative.dev/user-image" + // UpdateEnvVars gives the configuration all the env var values listed in the given map of // vars. Does not touch any environment variables not mentioned, but it can add // new env vars and change the values of existing ones. @@ -54,7 +57,7 @@ func UpdateConcurrencyTarget(template *servingv1alpha1.RevisionTemplateSpec, tar // TODO(toVersus): Remove the following validation once serving library is updated to v0.8.0 // and just rely on ValidateAnnotations method. if target < autoscaling.TargetMin { - return fmt.Errorf("Invalid 'concurrency-target' value: must be an integer greater than 0: %s", + return fmt.Errorf("invalid 'concurrency-target' value: must be an integer greater than 0: %s", autoscaling.TargetAnnotationKey) } @@ -67,7 +70,7 @@ func UpdateConcurrencyLimit(template *servingv1alpha1.RevisionTemplateSpec, limi // Validate input limit ctx := context.Background() if err := cc.Validate(ctx).ViaField("spec.containerConcurrency"); err != nil { - return fmt.Errorf("Invalid 'concurrency-limit' value: %s", err) + return fmt.Errorf("invalid 'concurrency-limit' value: %s", err) } template.Spec.ContainerConcurrency = cc return nil @@ -120,6 +123,7 @@ func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) { // UpdateImage a given image func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) error { + // When not setting the image to a digest, add the user image annotation. container, err := ContainerOfRevisionTemplate(template) if err != nil { return err @@ -128,6 +132,50 @@ func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) e return nil } +// UnsetUserImageAnnot removes the user image annotation +func UnsetUserImageAnnot(template *servingv1alpha1.RevisionTemplateSpec) { + delete(template.Annotations, UserImageAnnotationKey) +} + +// SetUserImageAnnot sets the user image annotation if the image isn't by-digest already. +func SetUserImageAnnot(template *servingv1alpha1.RevisionTemplateSpec) { + // If the current image isn't by-digest, set the user-image annotation to it + // so we remember what it was. + currentContainer, _ := ContainerOfRevisionTemplate(template) + ui := currentContainer.Image + if strings.Contains(ui, "@") { + prev, ok := template.Annotations[UserImageAnnotationKey] + if ok { + ui = prev + } + } + if template.Annotations == nil { + template.Annotations = make(map[string]string) + } + template.Annotations[UserImageAnnotationKey] = ui +} + +// FreezeImageToDigest sets the image on the template to the image digest of the base revision. +func FreezeImageToDigest(template *servingv1alpha1.RevisionTemplateSpec, baseRevision *servingv1alpha1.Revision) error { + currentContainer, err := ContainerOfRevisionTemplate(template) + + if baseRevision == nil { + return nil + } + baseContainer, err := ContainerOfRevisionSpec(&baseRevision.Spec) + if err != nil { + return err + } + if currentContainer.Image != baseContainer.Image { + return fmt.Errorf("could not freeze image to digest since current revision contains unexpected image.") + } + + if baseRevision.Status.ImageDigest != "" { + return UpdateImage(template, baseRevision.Status.ImageDigest) + } + return nil +} + // UpdateContainerPort updates container with a give port func UpdateContainerPort(template *servingv1alpha1.RevisionTemplateSpec, port int32) error { container, err := ContainerOfRevisionTemplate(template) diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 8cafe0f2e..b7fab9cab 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -99,6 +99,52 @@ func TestUpdateEnvVarsAppendOld(t *testing.T) { assertNoV1alpha1Old(t, template) } +type userImageAnnotCase struct { + image string + annot string + result string + set bool +} + +func TestSetUserImageAnnot(t *testing.T) { + cases := []userImageAnnotCase{ + {"foo/bar", "", "foo/bar", true}, + {"foo/bar@sha256:asdfsf", "", "foo/bar@sha256:asdfsf", true}, + {"foo/bar@sha256:asdf", "foo/bar", "foo/bar", true}, + {"foo/bar", "baz/quux", "foo/bar", true}, + {"foo/bar", "", "", false}, + {"foo/bar", "baz/quux", "", false}, + } + for _, c := range cases { + template, container := getV1alpha1Config() + if c.annot == "" { + template.Annotations = nil + } else { + template.Annotations = map[string]string{ + UserImageAnnotationKey: c.annot, + } + } + container.Image = c.image + if c.set { + SetUserImageAnnot(template) + } else { + UnsetUserImageAnnot(template) + } + assert.Equal(t, template.Annotations[UserImageAnnotationKey], c.result) + } +} + +func TestFreezeImageToDigest(t *testing.T) { + template, container := getV1alpha1Config() + revision := &servingv1alpha1.Revision{} + revision.Spec = template.Spec + revision.ObjectMeta = template.ObjectMeta + revision.Status.ImageDigest = "gcr.io/foo/bar@sha256:deadbeef" + container.Image = "gcr.io/foo/bar:latest" + FreezeImageToDigest(template, revision) + assert.Equal(t, container.Image, "gcr.io/foo/bar@sha256:deadbeef") +} + func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { container.Env = []corev1.EnvVar{ {Name: "a", Value: "foo"}, @@ -205,7 +251,7 @@ func TestUpdateConcurrencyTarget(t *testing.T) { checkAnnotationValue(t, template, autoscaling.TargetAnnotationKey, 10) // Update with invalid value err = UpdateConcurrencyTarget(template, -1) - assert.ErrorContains(t, err, "Invalid") + assert.ErrorContains(t, err, "invalid") } func TestUpdateConcurrencyLimit(t *testing.T) { @@ -216,7 +262,7 @@ func TestUpdateConcurrencyLimit(t *testing.T) { checkContainerConcurrency(t, template, 10) // Update with invalid value err = UpdateConcurrencyLimit(template, -1) - assert.ErrorContains(t, err, "Invalid") + assert.ErrorContains(t, err, "invalid") } func TestUpdateContainerImage(t *testing.T) { diff --git a/pkg/serving/v1alpha1/client.go b/pkg/serving/v1alpha1/client.go index e74f79ac1..2bf968837 100644 --- a/pkg/serving/v1alpha1/client.go +++ b/pkg/serving/v1alpha1/client.go @@ -61,6 +61,10 @@ type KnClient interface { // Get a revision by name GetRevision(name string) (*v1alpha1.Revision, error) + // Get the "base" revision for a Service; the one that corresponds to the + // current template. + GetBaseRevision(service *v1alpha1.Service) (*v1alpha1.Revision, error) + // List revisions ListRevisions(opts ...ListConfig) (*v1alpha1.RevisionList, error) @@ -230,6 +234,62 @@ func (cl *knClient) GetRevision(name string) (*v1alpha1.Revision, error) { return revision, nil } +type NoBaseRevisionError struct { + msg string +} + +func (e NoBaseRevisionError) Error() string { + return e.msg +} + +var noBaseRevisionError = &NoBaseRevisionError{"base revision not found"} + +// Get a "base" revision. This is the revision corresponding to the template of +// a Service. It may not be findable with our heuristics, in which case this +// method returns Errors()["no-base-revision"]. If it simply doesn't exist (like +// it wasn't yet created or was deleted), return the usual not found error. +func (cl *knClient) GetBaseRevision(service *v1alpha1.Service) (*v1alpha1.Revision, error) { + return getBaseRevision(cl, service) +} + +func getBaseRevision(cl KnClient, service *v1alpha1.Service) (*v1alpha1.Revision, error) { + template, err := serving.RevisionTemplateOfService(service) + if err != nil { + return nil, err + } + // First, try to get it by name. If the template has a particular name, the + // base revision is the one created with that name. + if template.Name != "" { + return cl.GetRevision(template.Name) + } + // Next, let's try the LatestCreatedRevision, and see if that matches the + // template, at least in terms of the image (which is what we care about here). + if service.Status.LatestCreatedRevisionName != "" { + latestCreated, err := cl.GetRevision(service.Status.LatestCreatedRevisionName) + if err != nil { + return nil, err + } + latestContainer, err := serving.ContainerOfRevisionSpec(&latestCreated.Spec) + if err != nil { + return nil, err + } + container, err := serving.ContainerOfRevisionTemplate(template) + if err != nil { + return nil, err + } + if latestContainer.Image != container.Image { + // At this point we know the latestCreatedRevision is out of date. + return nil, noBaseRevisionError + } + // There is still some chance the latestCreatedRevision is out of date, + // but we can't check the whole thing for equality because of + // server-side defaulting. Since what we probably want it for is to + // check the image digest anyway, keep it as good enough. + return latestCreated, nil + } + return nil, noBaseRevisionError +} + // Delete a revision by name func (cl *knClient) DeleteRevision(name string) error { err := cl.client.Revisions(cl.namespace).Delete(name, &v1.DeleteOptions{}) diff --git a/pkg/serving/v1alpha1/client_mock.go b/pkg/serving/v1alpha1/client_mock.go index 194ba2986..b8a2caa9f 100644 --- a/pkg/serving/v1alpha1/client_mock.go +++ b/pkg/serving/v1alpha1/client_mock.go @@ -194,6 +194,10 @@ func (r *Recorder) GetConfiguration(name string, config *v1alpha1.Configuration, } +func (c *MockKnClient) GetBaseRevision(service *v1alpha1.Service) (*v1alpha1.Revision, error) { + return getBaseRevision(c, service) +} + // GetConfiguration returns a configuration looked up by name func (c *MockKnClient) GetConfiguration(name string) (*v1alpha1.Configuration, error) { call := c.getCall("GetConfiguration") diff --git a/pkg/serving/v1alpha1/client_test.go b/pkg/serving/v1alpha1/client_test.go index 613229268..18ba13390 100644 --- a/pkg/serving/v1alpha1/client_test.go +++ b/pkg/serving/v1alpha1/client_test.go @@ -394,6 +394,56 @@ func TestWaitForService(t *testing.T) { }) } +type baseRevisionCase struct { + templateName string + templateImage string + latestCreated string + requestedRevisionName string + + foundRevisionImage string + errText string +} + +func TestGetBaseRevision(t *testing.T) { + serving, client := setup() + cases := []baseRevisionCase{ + {"foo-asdf", "gcr.io/foo/bar", "foo-asdf", "foo-asdf", "gcr.io/foo/bar", ""}, + {"", "gcr.io/foo/bar", "foo-asdf", "foo-asdf", "gcr.io/foo/bar", ""}, + {"foo-qwer", "gcr.io/foo/bar", "foo-asdf", "foo-qwer", "gcr.io/foo/bar", ""}, + {"", "gcr.io/foo/bar", "foo-asdf", "foo-asdf", "gcr.io/foo/baz", "base revision not found"}, + } + var c baseRevisionCase + serving.AddReactor("get", "revisions", func(a client_testing.Action) (bool, runtime.Object, error) { + revision := &v1alpha1.Revision{ObjectMeta: metav1.ObjectMeta{Name: c.requestedRevisionName, Namespace: testNamespace}} + revision.Spec.Containers = []corev1.Container{{}} + name := a.(client_testing.GetAction).GetName() + assert.Equal(t, name, c.requestedRevisionName) + + if c.foundRevisionImage != "" { + revision.Spec.Containers[0].Image = c.foundRevisionImage + return true, revision, nil + } else { + return true, nil, errors.NewNotFound(v1alpha1.Resource("revision"), name) + } + }) + for _, c = range cases { + service := v1alpha1.Service{} + service.Spec.Template = &v1alpha1.RevisionTemplateSpec{} + service.Spec.Template.Name = c.templateName + service.Status.LatestCreatedRevisionName = c.latestCreated + service.Spec.Template.Spec.Containers = []corev1.Container{{}} + service.Spec.Template.Spec.Containers[0].Image = c.templateImage + + r, err := client.GetBaseRevision(&service) + if err == nil { + assert.Equal(t, r.Spec.Containers[0].Image, c.foundRevisionImage) + } else { + assert.Assert(t, c.errText != "") + assert.ErrorContains(t, err, c.errText) + } + } +} + func TestGetConfiguration(t *testing.T) { serving, client := setup() diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go index 41c5a6317..040e43e26 100644 --- a/test/e2e/service_options_test.go +++ b/test/e2e/service_options_test.go @@ -43,7 +43,7 @@ func TestServiceOptions(t *testing.T) { t.Run("update concurrency options with invalid values for service", func(t *testing.T) { command := []string{"service", "update", "svc1", "--concurrency-limit", "-1", "--concurrency-target", "0"} _, err := test.kn.RunWithOpts(command, runOpts{NoNamespace: false, AllowError: true}) - assert.ErrorContains(t, err, "Invalid") + assert.ErrorContains(t, err, "invalid") }) t.Run("returns steady concurrency options for service", func(t *testing.T) {