From c06559304c0c60ea443cd4b30492d6d01545ac22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Mon, 5 Jul 2021 20:35:32 +0200 Subject: [PATCH 1/6] Add a update timestamp anytime a new revision should be triggered. Previously client side revision names was this trigger but since we switched to server-side revision naming by default that didn't work anymore (and hasn't worked with server-side revision naming before, too). Fixes #1318. --- CHANGELOG.adoc | 30 +++++ go.mod | 8 +- go.sum | 20 +-- .../service/configuration_edit_flags.go | 43 +++++-- pkg/kn/commands/service/create_mock_test.go | 32 ++--- pkg/kn/commands/service/export.go | 17 +++ pkg/kn/commands/service/export_test.go | 4 + .../service/service_update_mock_test.go | 35 +++-- pkg/kn/commands/service/update.go | 6 +- pkg/kn/commands/service/update_test.go | 107 +++++++--------- pkg/kn/plugin/manager.go | 20 +++ pkg/printers/interface_test.go | 31 +++++ pkg/printers/tablegenerator_test.go | 120 ++++++++++++++++++ pkg/printers/tableprinter_test.go | 45 +++++++ pkg/serving/config_changes.go | 49 ++++--- pkg/serving/config_changes_test.go | 19 ++- pkg/serving/revision_template.go | 38 +++++- test/e2e/service_apply_test.go | 4 +- .../apis/networking/metadata_validation.go | 1 + .../pkg/apis/networking/register.go | 13 +- .../knative.dev/pkg/controller/controller.go | 12 +- vendor/knative.dev/pkg/controller/options.go | 3 + .../pkg/logging/warning_handler.go | 33 +++++ vendor/knative.dev/pkg/ptr/value.go | 91 +++++++++++++ vendor/knative.dev/pkg/test/spoof/spoof.go | 2 +- vendor/modules.txt | 8 +- 26 files changed, 644 insertions(+), 147 deletions(-) create mode 100644 pkg/printers/interface_test.go create mode 100644 pkg/printers/tablegenerator_test.go create mode 100644 pkg/printers/tableprinter_test.go create mode 100644 vendor/knative.dev/pkg/logging/warning_handler.go create mode 100644 vendor/knative.dev/pkg/ptr/value.go diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 30b4269811..b4f21bb708 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -12,6 +12,36 @@ | https://github.com/knative/client/pull/[#] //// +## Unreleased +[cols="1,10,3", options="header", width="100%"] +|=== +| | Description | PR + +| 🎁 +| Add an `client.knative.dev/updateTimestamp` annotation to trigger a new revision when required +| https://github.com/knative/client/pull/1364[#1364] + +| ✨ +| Update Kubernetes dependencies to v0.20.7 +| https://github.com/knative/client/pull/1344[#1344] + +| ✨ +| Increase code coverage for Sources #1343 +| https://github.com/knative/client/pull/1343[#1343] + +| ✨ +| Make e2e test run over other networks +| https://github.com/knative/client/pull/1339[#1339] + +| 🎁 +| Add `env-value-from` flag & keep order of env vars as provided +| https://github.com/knative/client/pull/1328[#1328] + +| 🐛 +| Fix Subscription's Channel to use KRefence type +| https://github.com/knative/client/pull/1326[#1326] +|=== + ## v0.24.0 (2021-06-29) [cols="1,10,3", options="header", width="100%"] |=== diff --git a/go.mod b/go.mod index bafc80b696..d98f95dfb0 100644 --- a/go.mod +++ b/go.mod @@ -22,11 +22,11 @@ require ( k8s.io/cli-runtime v0.20.7 k8s.io/client-go v0.20.7 k8s.io/code-generator v0.20.7 - knative.dev/eventing v0.24.0 + knative.dev/eventing v0.24.1-0.20210702080639-90932eb671e0 knative.dev/hack v0.0.0-20210622141627-e28525d8d260 - knative.dev/networking v0.0.0-20210622182128-53f45d6d2cfa - knative.dev/pkg v0.0.0-20210622173328-dd0db4b05c80 - knative.dev/serving v0.24.0 + knative.dev/networking v0.0.0-20210705111547-ca0a601fc900 + knative.dev/pkg v0.0.0-20210701025203-30f9568e894e + knative.dev/serving v0.24.1-0.20210705154247-d207fa3a92ac sigs.k8s.io/yaml v1.2.0 ) diff --git a/go.sum b/go.sum index a58eccca81..3076e31408 100644 --- a/go.sum +++ b/go.sum @@ -1593,19 +1593,21 @@ k8s.io/utils v0.0.0-20200729134348-d5654de09c73/go.mod h1:jPW/WVKK9YHAvNhRxK0md/ k8s.io/utils v0.0.0-20201110183641-67b214c5f920/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA= k8s.io/utils v0.0.0-20210111153108-fddb29f9d009 h1:0T5IaWHO3sJTEmCP6mUlBvMukxPKUQWqiI/YuiBNMiQ= k8s.io/utils v0.0.0-20210111153108-fddb29f9d009/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA= -knative.dev/caching v0.0.0-20210622183028-95f67e075071/go.mod h1:9gwZcCBtmo9pi1oTgchXcdMNAXAYWklwDmO9uhDsIJE= -knative.dev/eventing v0.24.0 h1:CoaQwZBizxZyOFJUvFcyb7vYSvpYBmfb4IYRNWUdTPE= -knative.dev/eventing v0.24.0/go.mod h1:9xo0SWkIfpXrx0lvGQO7MUlPF8cu+QCMd2gGxj6wxrU= +knative.dev/caching v0.0.0-20210701025702-e6e703d9557b/go.mod h1:RZs6mvIaZendqv7O0ewst35K8o13Wqy5J7oMDjAdiu4= +knative.dev/eventing v0.24.1-0.20210702080639-90932eb671e0 h1:A3/G1uDuRLTU2vYAb8Po9F3CKZ1Svmqk5hfr5/dxTJ8= +knative.dev/eventing v0.24.1-0.20210702080639-90932eb671e0/go.mod h1:93VHah2XghrfrDFr4jSN4KdZS7RtoZuv4qR2Jn2B6x8= knative.dev/hack v0.0.0-20210622141627-e28525d8d260 h1:f2eMtOubAOc/Q7JlvFPDKXiPlJVK+VpX2Cot8hRzCgQ= knative.dev/hack v0.0.0-20210622141627-e28525d8d260/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI= knative.dev/hack/schema v0.0.0-20210622141627-e28525d8d260/go.mod h1:ffjwmdcrH5vN3mPhO8RrF2KfNnbHeCE2C60A+2cv3U0= -knative.dev/networking v0.0.0-20210622182128-53f45d6d2cfa h1:MprAGBX3eRaBZFRXC3ZjsnhnjttfprRVXdxmTeEzC2o= -knative.dev/networking v0.0.0-20210622182128-53f45d6d2cfa/go.mod h1:vwPACNE712tyoEG4fjyUIgfL4xkbXFugx8bxW+QrKn4= -knative.dev/pkg v0.0.0-20210622173328-dd0db4b05c80 h1:GHJ3lglE0/YHfBMMJqluqUNLOmsNXh7s7DBnfrkpRMM= +knative.dev/networking v0.0.0-20210705111547-ca0a601fc900 h1:pjC4YxKR0mH5/GTORkdyN2U2oNoCwICGrX6AReUG0LE= +knative.dev/networking v0.0.0-20210705111547-ca0a601fc900/go.mod h1:lT2n243XZb7K59q4FLtCuAeuboOjoJeLYvTxxfO5rIY= knative.dev/pkg v0.0.0-20210622173328-dd0db4b05c80/go.mod h1:kGegTnbZ+ljFjAE3E1+8wgaH2LMv8qYi+72o3F3cbdc= -knative.dev/reconciler-test v0.0.0-20210623134345-88c84739abd9/go.mod h1:4wqv2WyWUC5yhTesRUVwgjv/fHTHny1RYBfdB6tVDok= -knative.dev/serving v0.24.0 h1:MZIXR0r2FCXlTuQQXwLuM9+tV6pl2K6YUtK6tEtDB58= -knative.dev/serving v0.24.0/go.mod h1:l/dhsWs+Y8PAssBxaS/hN4HRQQGy4zxfVHD1xYjl3ns= +knative.dev/pkg v0.0.0-20210628225612-51cfaabbcdf6/go.mod h1:kGegTnbZ+ljFjAE3E1+8wgaH2LMv8qYi+72o3F3cbdc= +knative.dev/pkg v0.0.0-20210701025203-30f9568e894e h1:2TofgD72tjBuWN3a4Rg3uzrMu1OWOlh2KY2m0fsjwqQ= +knative.dev/pkg v0.0.0-20210701025203-30f9568e894e/go.mod h1:kGegTnbZ+ljFjAE3E1+8wgaH2LMv8qYi+72o3F3cbdc= +knative.dev/reconciler-test v0.0.0-20210630182710-2a6d91dfee1e/go.mod h1:4wqv2WyWUC5yhTesRUVwgjv/fHTHny1RYBfdB6tVDok= +knative.dev/serving v0.24.1-0.20210705154247-d207fa3a92ac h1:z9xRHZxze66dnlNunH7BwKpBQq4eyyeBJcVvrFRrDAI= +knative.dev/serving v0.24.1-0.20210705154247-d207fa3a92ac/go.mod h1:ktSG08SjDYoGEIPw9GzzS6njQjr6bpWXQuViOOtmLkg= modernc.org/cc v1.0.0/go.mod h1:1Sk4//wdnYJiUIxnW8ddKpaOJCF37yAdqYnkxUpaYxw= modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk= modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k= diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index f8100b938d..78aa1f4c1a 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/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,12 +217,20 @@ 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 } @@ -230,7 +238,7 @@ func (p *ConfigurationEditFlags) Apply( } 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 { diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go index d3fdf078cc..9f678373ba 100644 --- a/pkg/kn/commands/service/create_mock_test.go +++ b/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", diff --git a/pkg/kn/commands/service/export.go b/pkg/kn/commands/service/export.go index e54a145309..e556899224 100644 --- a/pkg/kn/commands/service/export.go +++ b/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) + } +} diff --git a/pkg/kn/commands/service/export_test.go b/pkg/kn/commands/service/export_test.go index e2a4ef138e..7b2b2ee83f 100644 --- a/pkg/kn/commands/service/export_test.go +++ b/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( diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go index bf7eb45a1c..19db19c5a8 100644 --- a/pkg/kn/commands/service/service_update_mock_test.go +++ b/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", diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go index 08d2392693..ebbe22f1ff 100644 --- a/pkg/kn/commands/service/update.go +++ b/pkg/kn/commands/service/update.go @@ -85,7 +85,7 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command { updateFunc := func(service *servingv1.Service) (*servingv1.Service, error) { latestRevisionBeforeUpdate = service.Status.LatestReadyRevisionName var baseRevision *servingv1.Revision - if !cmd.Flags().Changed("image") && editFlags.LockToDigest { + if isImagePinned(cmd, editFlags) { baseRevision, err = client.GetBaseRevision(cmd.Context(), service) var errNoBaseRevision clientservingv1.NoBaseRevisionError if errors.As(err, &errNoBaseRevision) { @@ -151,6 +151,10 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command { return serviceUpdateCommand } +func isImagePinned(cmd *cobra.Command, editFlags ConfigurationEditFlags) bool { + return !cmd.Flags().Changed("image") && editFlags.LockToDigest +} + func preCheck(cmd *cobra.Command) error { if cmd.Flags().NFlag() == 0 { return fmt.Errorf("flag(s) not set\nUsage: %s", cmd.Use) diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index 1aa83e23a9..db4be8d7d5 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -93,6 +93,10 @@ func fakeServiceUpdate(original *servingv1.Service, args []string) ( rev.Spec = original.Spec.Template.Spec rev.ObjectMeta = original.Spec.Template.ObjectMeta rev.Name = original.Status.LatestCreatedRevisionName + rev.Status.ContainerStatuses = []servingv1.ContainerStatus{ + {ImageDigest: exampleImageByDigest, Name: "user-container"}, + } + rev.Status.DeprecatedImageDigest = exampleImageByDigest return true, rev, nil }) @@ -196,6 +200,14 @@ func TestServiceUpdateImage(t *testing.T) { !strings.Contains(output, "bar") { t.Fatalf("wrong or no success message: %s", output) } + + assert.Assert(t, template.Annotations != nil) + ts := template.Annotations[servinglib.UpdateTimestampAnnotationKey] + assert.Assert(t, ts != "") + updateTs, err := time.Parse(time.RFC3339, ts) + assert.NilError(t, err) + assert.Assert(t, updateTs.Before(time.Now())) + assert.Assert(t, updateTs.After(time.Now().Add(-5*time.Second))) } func TestServiceUpdateWithMultipleImages(t *testing.T) { @@ -353,9 +365,6 @@ func TestServiceUpdateMaxMinScale(t *testing.T) { } template := updated.Spec.Template - if err != nil { - t.Fatal(err) - } actualAnnos := template.Annotations expectedAnnos := []string{ @@ -393,9 +402,6 @@ func TestServiceUpdateScale(t *testing.T) { } template := updated.Spec.Template - if err != nil { - t.Fatal(err) - } actualAnnos := template.Annotations expectedAnnos := []string{ @@ -484,9 +490,6 @@ func TestServiceUpdateScaleWithRange(t *testing.T) { } template := updated.Spec.Template - if err != nil { - t.Fatal(err) - } actualAnnos := template.Annotations expectedAnnos := []string{ @@ -518,9 +521,6 @@ func TestServiceUpdateScaleMinWithRange(t *testing.T) { } template := updated.Spec.Template - if err != nil { - t.Fatal(err) - } actualAnnos := template.Annotations expectedAnnos := []string{ @@ -570,9 +570,6 @@ func TestServiceUpdateScaleMaxWithRange(t *testing.T) { } template := updated.Spec.Template - if err != nil { - t.Fatal(err) - } actualAnnos := template.Annotations expectedAnnos := []string{ @@ -780,7 +777,7 @@ func TestServiceUpdateDoesntPinToDigestWhenPreviouslyDidnt(t *testing.T) { } func TestServiceUpdateRequestsLimitsCPU(t *testing.T) { - service := createMockServiceWithResources(t, "250", "64Mi", "1000m", "1024Mi") + service := createMockServiceWithResources("250", "64Mi", "1000m", "1024Mi") action, updated, _, err := fakeServiceUpdate(service, []string{ "service", "update", "foo", "--request", "cpu=500m", "--limit", "cpu=1000m", "--no-wait"}) @@ -800,25 +797,23 @@ func TestServiceUpdateRequestsLimitsCPU(t *testing.T) { } newTemplate := updated.Spec.Template - if err != nil { - t.Fatal(err) - } else { - if !reflect.DeepEqual( - newTemplate.Spec.Containers[0].Resources.Requests, - expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) - } - if !reflect.DeepEqual( - newTemplate.Spec.Containers[0].Resources.Limits, - expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) - } + if !reflect.DeepEqual( + newTemplate.Spec.Containers[0].Resources.Requests, + expectedRequestsVars) { + t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) + } + + if !reflect.DeepEqual( + newTemplate.Spec.Containers[0].Resources.Limits, + expectedLimitsVars) { + t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) } + } func TestServiceUpdateRequestsLimitsMemory(t *testing.T) { - service := createMockServiceWithResources(t, "100m", "64Mi", "1000m", "1024Mi") + service := createMockServiceWithResources("100m", "64Mi", "1000m", "1024Mi") action, updated, _, err := fakeServiceUpdate(service, []string{ "service", "update", "foo", "--request", "memory=128Mi", "--limit", "memory=2048Mi", "--no-wait"}) @@ -838,25 +833,22 @@ func TestServiceUpdateRequestsLimitsMemory(t *testing.T) { } newTemplate := updated.Spec.Template - if err != nil { - t.Fatal(err) - } else { - if !reflect.DeepEqual( - newTemplate.Spec.Containers[0].Resources.Requests, - expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) - } - if !reflect.DeepEqual( - newTemplate.Spec.Containers[0].Resources.Limits, - expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) - } + if !reflect.DeepEqual( + newTemplate.Spec.Containers[0].Resources.Requests, + expectedRequestsVars) { + t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) + } + + if !reflect.DeepEqual( + newTemplate.Spec.Containers[0].Resources.Limits, + expectedLimitsVars) { + t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) } } func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) { - service := createMockServiceWithResources(t, "250m", "64Mi", "1000m", "1024Mi") + service := createMockServiceWithResources("250m", "64Mi", "1000m", "1024Mi") action, updated, _, err := fakeServiceUpdate(service, []string{ "service", "update", "foo", @@ -878,20 +870,16 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) { } newTemplate := updated.Spec.Template - if err != nil { - t.Fatal(err) - } else { - if !reflect.DeepEqual( - newTemplate.Spec.Containers[0].Resources.Requests, - expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) - } + if !reflect.DeepEqual( + newTemplate.Spec.Containers[0].Resources.Requests, + expectedRequestsVars) { + t.Fatalf("wrong requests vars %v", newTemplate.Spec.Containers[0].Resources.Requests) + } - if !reflect.DeepEqual( - newTemplate.Spec.Containers[0].Resources.Limits, - expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) - } + if !reflect.DeepEqual( + newTemplate.Spec.Containers[0].Resources.Limits, + expectedLimitsVars) { + t.Fatalf("wrong limits vars %v", newTemplate.Spec.Containers[0].Resources.Limits) } } @@ -1018,9 +1006,6 @@ func TestServiceUpdateNoClusterLocalOnPrivateService(t *testing.T) { assert.DeepEqual(t, expected, actual) newTemplate := updated.Spec.Template - if err != nil { - t.Fatal(err) - } actual = newTemplate.ObjectMeta.Labels assert.DeepEqual(t, expected, actual) @@ -1065,7 +1050,7 @@ func newEmptyService() *servingv1.Service { return ret } -func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, limitsCPU, limitsMemory string) *servingv1.Service { +func createMockServiceWithResources(requestCPU, requestMemory, limitsCPU, limitsMemory string) *servingv1.Service { service := newEmptyService() template := service.Spec.Template diff --git a/pkg/kn/plugin/manager.go b/pkg/kn/plugin/manager.go index 0ecb72f1a9..4ad8eed19b 100644 --- a/pkg/kn/plugin/manager.go +++ b/pkg/kn/plugin/manager.go @@ -52,6 +52,26 @@ type Plugin interface { Path() string } +type ContextData map[string]string + +type PluginManifest struct { + // ProducesContextDataKeys is a list of keys for the ContextData that + // a plugin can produce. Nil or an empty list declares that this + // plugin is not ContextDataProducer + ProducesContextDataKeys []string + + // ConsumesContextDataKeys is a list of keys from a ContextData that a + // plugin is interested in to consume. Nil or an empty list declares + // that this plugin is not a ContextDataConsumer + ConsumesContextDataKeys []string +} + +type ContextDataConsumer interface { + // ExecuteWithContextData executes the plugin with the given args much like + // Execute() but with an additional argument that holds the ContextData + ExecuteWithContextData(args []string, data ContextData) error +} + type Manager struct { // Dedicated plugin directory as configured pluginsDir string diff --git a/pkg/printers/interface_test.go b/pkg/printers/interface_test.go new file mode 100644 index 0000000000..d7105c60a2 --- /dev/null +++ b/pkg/printers/interface_test.go @@ -0,0 +1,31 @@ +// Copyright © 2021 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package printers + +import ( + "fmt" + "io" + "testing" + + "gotest.tools/v3/assert" + "k8s.io/apimachinery/pkg/runtime" +) + +func TestResourcePrintObj(t *testing.T) { + mockFunc := ResourcePrinterFunc(func(runtime.Object, io.Writer) error { + return fmt.Errorf(mockErrorString) + }) + assert.Error(t, mockFunc.PrintObj(nil, nil), mockErrorString) +} diff --git a/pkg/printers/tablegenerator_test.go b/pkg/printers/tablegenerator_test.go new file mode 100644 index 0000000000..fca8aaf89d --- /dev/null +++ b/pkg/printers/tablegenerator_test.go @@ -0,0 +1,120 @@ +// Copyright © 2021 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package printers + +import ( + "fmt" + "reflect" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/duration" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" + + "gotest.tools/v3/assert" + metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" +) + +const ( + mockErrorString = "This function returns an error" +) + +var ( + invalidPrintTypeFunc = true + invalidPrintFuncIncorrectInCount = func() ([]metav1beta1.TableRow, error) { return nil, nil } + invalidPrintFuncIncorrectInType = func(*servingv1.Service, bool) ([]metav1beta1.TableRow, error) { return nil, nil } + validPrintFunc = func(obj *servingv1.Service, opts PrintOptions) ([]metav1beta1.TableRow, error) { + tableRow := metav1beta1.TableRow{ + Object: runtime.RawExtension{Object: obj}, + } + tableRow.Cells = append(tableRow.Cells, obj.Namespace, obj.Name, duration.HumanDuration(time.Since(obj.CreationTimestamp.Time))) + return []metav1.TableRow{tableRow}, nil + } + validPrintFuncErrOutput = func(obj *servingv1.Service, opts PrintOptions) ([]metav1beta1.TableRow, error) { + return nil, fmt.Errorf(mockErrorString) + } + columnDefs = []metav1beta1.TableColumnDefinition{ + {Name: "Namespace", Type: "string", Description: "Namespace of mock instance", Priority: 0}, + {Name: "Name", Type: "string", Description: "Name of the mock instance", Priority: 1}, + {Name: "Age", Type: "string", Description: "Age of the mock instance", Priority: 1}, + } +) + +// Do we need this function? +func TestValidateRowPrintHandlerFunc(t *testing.T) { + assert.Error(t, ValidateRowPrintHandlerFunc(reflect.ValueOf(invalidPrintTypeFunc)), fmt.Sprintf("invalid print handler. %#v is not a function", reflect.ValueOf(invalidPrintTypeFunc))) + assert.Error(t, ValidateRowPrintHandlerFunc(reflect.ValueOf(invalidPrintFuncIncorrectInCount)), fmt.Sprintf("invalid print handler."+ + "Must accept 2 parameters and return 2 value.")) + assert.Error(t, ValidateRowPrintHandlerFunc(reflect.ValueOf(invalidPrintFuncIncorrectInType)), fmt.Sprintf("invalid print handler. The expected signature is: "+ + "func handler(obj %v, options PrintOptions) ([]metav1beta1.TableRow, error)", reflect.TypeOf(&servingv1.Service{}))) + assert.NilError(t, ValidateRowPrintHandlerFunc(reflect.ValueOf(validPrintFunc))) +} + +func TestTableHandler(t *testing.T) { + h := NewTableGenerator() + assert.ErrorContains(t, h.TableHandler(columnDefs, invalidPrintTypeFunc), "invalid print handler") + assert.ErrorContains(t, h.TableHandler(columnDefs, invalidPrintFuncIncorrectInCount), "invalid print handler") + assert.ErrorContains(t, h.TableHandler(columnDefs, invalidPrintFuncIncorrectInType), "invalid print handler") + assert.NilError(t, h.TableHandler(columnDefs, validPrintFunc)) + assert.ErrorContains(t, h.TableHandler(columnDefs, validPrintFunc), "registered duplicate printer") +} + +func TestGenerateTable(t *testing.T) { + h := NewTableGenerator() + myksvc := &servingv1.Service{ + TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "serving.knative.dev/v1"}, + ObjectMeta: metav1.ObjectMeta{Name: "myksvc", Namespace: "default"}, + } + _, err := h.GenerateTable(myksvc, h.options) + assert.ErrorContains(t, err, "no table handler registered for this type") + err = h.TableHandler(columnDefs, validPrintFunc) + assert.NilError(t, err) + table, err := h.GenerateTable(myksvc, h.options) + assert.NilError(t, err) + expected := metav1beta1.TableRow{ + Object: runtime.RawExtension{Object: myksvc}, + } + expected.Cells = append(expected.Cells, myksvc.Namespace, myksvc.Name, duration.HumanDuration(time.Since(myksvc.CreationTimestamp.Time))) + assert.DeepEqual(t, table.Rows[0], expected) + + myksvcList := &servingv1.ServiceList{ + TypeMeta: metav1.TypeMeta{}, + ListMeta: metav1.ListMeta{}, + Items: []servingv1.Service{*myksvc}, + } + + printServiceList := func(objList *servingv1.ServiceList, opts PrintOptions) ([]metav1beta1.TableRow, error) { + rows := make([]metav1beta1.TableRow, 0, len(objList.Items)) + for _, obj := range objList.Items { + row, err := validPrintFunc(&obj, h.options) + assert.NilError(t, err) + rows = append(rows, row...) + } + return rows, nil + } + err = h.TableHandler(columnDefs, printServiceList) + assert.NilError(t, err) + table, err = h.GenerateTable(myksvcList, h.options) + assert.NilError(t, err) + assert.DeepEqual(t, table.Rows[0], expected) + + // Create new human readable printer + h = NewTableGenerator() + assert.NilError(t, h.TableHandler(columnDefs, validPrintFuncErrOutput)) + _, err = h.GenerateTable(myksvc, h.options) + assert.Error(t, err, mockErrorString) +} diff --git a/pkg/printers/tableprinter_test.go b/pkg/printers/tableprinter_test.go new file mode 100644 index 0000000000..5f4d31a78f --- /dev/null +++ b/pkg/printers/tableprinter_test.go @@ -0,0 +1,45 @@ +// Copyright © 2021 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package printers + +import ( + "bytes" + "os" + "testing" + + "gotest.tools/v3/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/client/pkg/util" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" +) + +func TestPrintObj(t *testing.T) { + h := NewTablePrinter(PrintOptions{}) + assert.Assert(t, h != nil) + assert.NilError(t, h.PrintObj(nil, nil)) + + myksvc := &servingv1.Service{ + TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "serving.knative.dev/v1"}, + ObjectMeta: metav1.ObjectMeta{Name: "myksvc", Namespace: "default"}, + } + assert.ErrorContains(t, h.PrintObj(myksvc, os.Stdout), "unknown type") + h.TableHandler(columnDefs, validPrintFunc) + var out bytes.Buffer + assert.NilError(t, h.PrintObj(myksvc, &out)) + assert.Assert(t, util.ContainsAll(out.String(), "myksvc")) + h = NewTablePrinter(PrintOptions{}) + h.TableHandler(columnDefs, validPrintFuncErrOutput) + assert.Error(t, h.PrintObj(myksvc, os.Stdout), mockErrorString) +} diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index e8d429ff02..31ebec94ba 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -41,8 +41,9 @@ const ( ) var ( - UserImageAnnotationKey = "client.knative.dev/user-image" - ApiTooOldError = errors.New("the service is using too old of an API format for the operation") + UserImageAnnotationKey = "client.knative.dev/user-image" + UpdateTimestampAnnotationKey = "client.knative.dev/updateTimestamp" + APITooOldError = errors.New("the service is using too old of an API format for the operation") ) func (vt VolumeSourceType) String() string { @@ -91,31 +92,46 @@ func UpdateConcurrencyLimit(template *servingv1.RevisionTemplateSpec, limit int6 return nil } -// UnsetUserImageAnnot removes the user image annotation -func UnsetUserImageAnnot(template *servingv1.RevisionTemplateSpec) { +// UnsetUserImageAnnotation removes the user image annotation +func UnsetUserImageAnnotation(template *servingv1.RevisionTemplateSpec) { delete(template.Annotations, UserImageAnnotationKey) } -// SetUserImageAnnot sets the user image annotation if the image isn't by-digest already. -func SetUserImageAnnot(template *servingv1.RevisionTemplateSpec) { +// UpdateUserImageAnnotation sets the user image annotation if the image isn't by-digest already. +func UpdateUserImageAnnotation(template *servingv1.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] + currentContainer, err := ContainerOfRevisionTemplate(template) + if err != nil { + return + } + image := currentContainer.Image + if strings.Contains(image, "@") { + // Ensure that the non-digestified image is used + storedImage, ok := template.Annotations[UserImageAnnotationKey] if ok { - ui = prev + image = storedImage } } + ensureAnnotations(template) + template.Annotations[UserImageAnnotationKey] = image +} + +// UpdateTimestampAnnotation update the annotation for the last update with the current timestamp +func UpdateTimestampAnnotation(template *servingv1.RevisionTemplateSpec) { + ensureAnnotations(template) + + template.Annotations[UpdateTimestampAnnotationKey] = time.Now().UTC().Format(time.RFC3339) +} + +func ensureAnnotations(template *servingv1.RevisionTemplateSpec) { 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 *servingv1.RevisionTemplateSpec, baseRevision *servingv1.Revision) error { +// PinImageToDigest sets the image on the template to the image digest of the base revision. +func PinImageToDigest(template *servingv1.RevisionTemplateSpec, baseRevision *servingv1.Revision) error { if baseRevision == nil { return nil } @@ -133,8 +149,9 @@ func FreezeImageToDigest(template *servingv1.RevisionTemplateSpec, baseRevision return fmt.Errorf("could not freeze image to digest since current revision contains unexpected image") } - if baseRevision.Status.DeprecatedImageDigest != "" { - return flags.UpdateImage(&template.Spec.PodSpec, baseRevision.Status.DeprecatedImageDigest) + containerStatus := ContainerStatus(baseRevision) + if containerStatus.ImageDigest != "" { + return flags.UpdateImage(&template.Spec.PodSpec, containerStatus.ImageDigest) } return nil } diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 2830f59e18..9b3cadbfe9 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -79,7 +79,7 @@ type userImageAnnotCase struct { set bool } -func TestSetUserImageAnnot(t *testing.T) { +func TestSetUserImageAnnotation(t *testing.T) { cases := []userImageAnnotCase{ {"foo/bar", "", "foo/bar", true}, {"foo/bar@sha256:asdfsf", "", "foo/bar@sha256:asdfsf", true}, @@ -99,9 +99,9 @@ func TestSetUserImageAnnot(t *testing.T) { } container.Image = c.image if c.set { - SetUserImageAnnot(template) + UpdateUserImageAnnotation(template) } else { - UnsetUserImageAnnot(template) + UnsetUserImageAnnotation(template) } assert.Equal(t, template.Annotations[UserImageAnnotationKey], c.result) } @@ -112,12 +112,21 @@ func TestFreezeImageToDigest(t *testing.T) { revision := &servingv1.Revision{} revision.Spec = template.Spec revision.ObjectMeta = template.ObjectMeta - revision.Status.DeprecatedImageDigest = "gcr.io/foo/bar@sha256:deadbeef" + revision.Status.ContainerStatuses = []servingv1.ContainerStatus{ + {Name: "user-container", ImageDigest: "gcr.io/foo/bar@sha256:deadbeef"}, + } container.Image = "gcr.io/foo/bar:latest" - FreezeImageToDigest(template, revision) + err := PinImageToDigest(template, revision) + assert.NilError(t, err) assert.Equal(t, container.Image, "gcr.io/foo/bar@sha256:deadbeef") } +func TestUpdateTimestampAnnotation(t *testing.T) { + template, _ := getRevisionTemplate() + UpdateTimestampAnnotation(template) + assert.Assert(t, template.Annotations[UpdateTimestampAnnotationKey] != "") +} + func TestUpdateMinScale(t *testing.T) { template, _ := getRevisionTemplate() err := UpdateMinScale(template, 10) diff --git a/pkg/serving/revision_template.go b/pkg/serving/revision_template.go index 553ebc9e95..e99aff3ad0 100644 --- a/pkg/serving/revision_template.go +++ b/pkg/serving/revision_template.go @@ -33,13 +33,49 @@ func ContainerOfRevisionTemplate(template *servingv1.RevisionTemplateSpec) (*cor return ContainerOfRevisionSpec(&template.Spec) } +// ContainerOfRevisionSpec returns the 'main' container of a revision specification and +// use GetServingContainerIndex to identify the container. +// An error is returned if no such container could be found func ContainerOfRevisionSpec(revisionSpec *servingv1.RevisionSpec) (*corev1.Container, error) { - if len(revisionSpec.Containers) == 0 { + idx := ContainerIndexOfRevisionSpec(revisionSpec) + if idx == -1 { return nil, fmt.Errorf("internal: no container set in spec.template.spec.containers") } return &revisionSpec.Containers[0], nil } +// ContainerIndexOfRevisionSpec returns the index of the "main" container if +// multiple containers are present. The main container is either the single +// container when there is only ony container in the list or the first container +// which has a ports declaration (validation guarantees that there is only one +// such container) +// If no container could be found (list is empty or no container has a port declaration) +// then -1 is returned +// This method's logic is taken from RevisionSpec.GetContainer() +func ContainerIndexOfRevisionSpec(revisionSpec *servingv1.RevisionSpec) int { + switch { + case len(revisionSpec.Containers) == 1: + return 0 + case len(revisionSpec.Containers) > 1: + for i := range revisionSpec.Containers { + if len(revisionSpec.Containers[i].Ports) != 0 { + return i + } + } + } + return -1 +} + +// ContainerStatus returns the status of the main container or nil of no +// such status could be found +func ContainerStatus(r *servingv1.Revision) *servingv1.ContainerStatus { + idx := ContainerIndexOfRevisionSpec(&r.Spec) + if idx == -1 { + return nil + } + return &r.Status.ContainerStatuses[idx] +} + func ScalingInfo(m *metav1.ObjectMeta) (*Scaling, error) { ret := &Scaling{} var err error diff --git a/test/e2e/service_apply_test.go b/test/e2e/service_apply_test.go index bbdb5d0e86..f5e74c8b0c 100644 --- a/test/e2e/service_apply_test.go +++ b/test/e2e/service_apply_test.go @@ -41,11 +41,11 @@ func TestServiceApply(t *testing.T) { t.Log("apply hello service (initially)") result := serviceApply(r, "hello-apply") - r.AssertNoError(result) assert.Check(r.T(), util.ContainsAllIgnoreCase(result.Stdout, "creating", "service", "hello-apply", "ready", "http")) t.Log("apply hello service (unchanged)") - result = serviceApply(r, "hello-apply") + result = serviceApply(r, "hello-apply", "--annotation-service", "foo=bar") r.AssertNoError(result) + result = serviceApply(r, "hello-apply", "--annotation-service", "foo=bar") assert.Check(r.T(), util.ContainsAllIgnoreCase(result.Stdout, "no changes", "service", "hello-apply", "http")) t.Log("apply hello service (update env)") diff --git a/vendor/knative.dev/networking/pkg/apis/networking/metadata_validation.go b/vendor/knative.dev/networking/pkg/apis/networking/metadata_validation.go index de861c32d2..e7ea78925a 100644 --- a/vendor/knative.dev/networking/pkg/apis/networking/metadata_validation.go +++ b/vendor/knative.dev/networking/pkg/apis/networking/metadata_validation.go @@ -28,6 +28,7 @@ var ( allowedAnnotations = sets.NewString( DisableAutoTLSAnnotationKey, CertificateClassAnnotationKey, + HTTPOptionAnnotationKey, ) ) diff --git a/vendor/knative.dev/networking/pkg/apis/networking/register.go b/vendor/knative.dev/networking/pkg/apis/networking/register.go index 59771e5796..ec18c4bc00 100644 --- a/vendor/knative.dev/networking/pkg/apis/networking/register.go +++ b/vendor/knative.dev/networking/pkg/apis/networking/register.go @@ -35,9 +35,14 @@ const ( // Istio-based Ingress will reconcile into a VirtualService). IngressClassAnnotationKey = "networking.knative.dev/ingress.class" - // DisableAutoTLSAnnotationKey is the label key attached to a namespace to indicate that - // AutoTLS should not be enabled for it. + // DisableAutoTLSAnnotationKey is the annotation key attached to a Knative Service/DomainMapping + // to indicate that AutoTLS should not be enabled for it. DisableAutoTLSAnnotationKey = "networking.knative.dev/disableAutoTLS" + + // HTTPOptionAnnotationKey is the annotation key attached to a Knative Service/DomainMapping + // to indicate the HTTP option of it. + HTTPOptionAnnotationKey = "networking.knative.dev/httpOption" + // IngressLabelKey is the label key attached to underlying network programming // resources to indicate which Ingress triggered their creation. IngressLabelKey = GroupName + "/ingress" @@ -69,10 +74,6 @@ const ( // Cert-Manager-based Certificate will reconcile into a Cert-Manager Certificate). CertificateClassAnnotationKey = "networking.knative.dev/certificate.class" - // DeprecatedDisableWildcardCertLabelKey is the deprecated label key attached to a namespace to indicate that - // a wildcard certificate should be not created for it. - DeprecatedDisableWildcardCertLabelKey = GroupName + "/disableWildcardCert" - // DisableWildcardCertLabelKey is the label key attached to a namespace to indicate that // a wildcard certificate should be not created for it. DisableWildcardCertLabelKey = "networking.knative.dev/disableWildcardCert" diff --git a/vendor/knative.dev/pkg/controller/controller.go b/vendor/knative.dev/pkg/controller/controller.go index 9593eef28d..593d00a47d 100644 --- a/vendor/knative.dev/pkg/controller/controller.go +++ b/vendor/knative.dev/pkg/controller/controller.go @@ -57,6 +57,7 @@ var ( // when processing the controller's workqueue. Controller binaries // may adjust this process-wide default. For finer control, invoke // Run on the controller directly. + // TODO rename the const to Concurrency and deprecated this DefaultThreadsPerController = 2 ) @@ -203,6 +204,9 @@ type Impl struct { // which are not required to complete at the highest priority. workQueue *twoLaneQueue + // Concurrency - The number of workers to use when processing the controller's workqueue. + Concurrency int + // Sugared logger is easier to use but is not as performant as the // raw logger. In performance critical paths, call logger.Desugar() // and use the returned raw logger instead. In addition to the @@ -221,6 +225,7 @@ type ControllerOptions struct { //nolint // for backcompat. Logger *zap.SugaredLogger Reporter StatsReporter RateLimiter workqueue.RateLimiter + Concurrency int } // NewImpl instantiates an instance of our controller that will feed work to the @@ -244,12 +249,16 @@ func NewImplFull(r Reconciler, options ControllerOptions) *Impl { if options.Reporter == nil { options.Reporter = MustNewStatsReporter(options.WorkQueueName, options.Logger) } + if options.Concurrency == 0 { + options.Concurrency = DefaultThreadsPerController + } return &Impl{ Name: options.WorkQueueName, Reconciler: r, workQueue: newTwoLaneWorkQueue(options.WorkQueueName, options.RateLimiter), logger: options.Logger, statsReporter: options.Reporter, + Concurrency: options.Concurrency, } } @@ -723,9 +732,10 @@ func StartAll(ctx context.Context, controllers ...*Impl) { // Start all of the controllers. for _, ctrlr := range controllers { wg.Add(1) + concurrency := ctrlr.Concurrency go func(c *Impl) { defer wg.Done() - c.RunContext(ctx, DefaultThreadsPerController) + c.RunContext(ctx, concurrency) }(ctrlr) } wg.Wait() diff --git a/vendor/knative.dev/pkg/controller/options.go b/vendor/knative.dev/pkg/controller/options.go index 8d61835e69..dbacd74799 100644 --- a/vendor/knative.dev/pkg/controller/options.go +++ b/vendor/knative.dev/pkg/controller/options.go @@ -38,6 +38,9 @@ type Options struct { // DemoteFunc configures the demote function this reconciler uses DemoteFunc func(b reconciler.Bucket) + + // Concurrency - The number of workers to use when processing the controller's workqueue. + Concurrency int } // OptionsFn is a callback method signature that accepts an Impl and returns diff --git a/vendor/knative.dev/pkg/logging/warning_handler.go b/vendor/knative.dev/pkg/logging/warning_handler.go new file mode 100644 index 0000000000..7ea7aeeff6 --- /dev/null +++ b/vendor/knative.dev/pkg/logging/warning_handler.go @@ -0,0 +1,33 @@ +/* +Copyright 2021 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package logging + +import "go.uber.org/zap" + +// WarningHandler is a warning handler to forward client-go's warning logs into a zap logger. +type WarningHandler struct { + Logger *zap.SugaredLogger +} + +func (h *WarningHandler) HandleWarningHeader(code int, agent string, message string) { + // This condition is copied from K8s' default WarningLogger. + if code != 299 || len(message) == 0 { + return + } + + h.Logger.Warn("API Warning: " + message) +} diff --git a/vendor/knative.dev/pkg/ptr/value.go b/vendor/knative.dev/pkg/ptr/value.go new file mode 100644 index 0000000000..148bf2105c --- /dev/null +++ b/vendor/knative.dev/pkg/ptr/value.go @@ -0,0 +1,91 @@ +/* +Copyright 2021 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package ptr + +import "time" + +// Int32Value is a helper for turning pointers to integers into values for use +// in API types that want int32. +func Int32Value(i *int32) int32 { + if i == nil { + return 0 + } + return *i +} + +// Int64Value is a helper for turning pointers to integers into values for use +// in API types that want int64. +func Int64Value(i *int64) int64 { + if i == nil { + return 0 + } + return *i +} + +// Float32Value is a helper for turning pointers to floats into values for use +// in API types that want float32. +func Float32Value(f *float32) float32 { + if f == nil { + return 0 + } + return *f +} + +// Float64Value is a helper for turning pointers to floats into values for use +// in API types that want float64. +func Float64Value(f *float64) float64 { + if f == nil { + return 0 + } + return *f +} + +// BoolValue is a helper for turning pointers to bools into values for use in +// API types that want bool. +func BoolValue(b *bool) bool { + if b == nil { + return false + } + return *b +} + +// StringValue is a helper for turning pointers to strings into values for use +// in API types that want string. +func StringValue(s *string) string { + if s == nil { + return "" + } + return *s +} + +// DurationValue is a helper for turning *time.Duration into values for use in +// API types that want time.Duration. +func DurationValue(t *time.Duration) time.Duration { + if t == nil { + return 0 + } + return *t +} + +// TimeValue is a helper for turning *time.Time into values for use in API +// types that want API types that want time.Time. +func TimeValue(t *time.Time) time.Time { + if t == nil { + return time.Time{} + } + return *t +} diff --git a/vendor/knative.dev/pkg/test/spoof/spoof.go b/vendor/knative.dev/pkg/test/spoof/spoof.go index 48ffcffa85..0513f7e0f1 100644 --- a/vendor/knative.dev/pkg/test/spoof/spoof.go +++ b/vendor/knative.dev/pkg/test/spoof/spoof.go @@ -208,7 +208,7 @@ func DefaultErrorRetryChecker(err error) (bool, error) { if isTCPTimeout(err) { return true, fmt.Errorf("retrying for TCP timeout: %w", err) } - // Retrying on DNS error, since we may be using xip.io or nip.io in tests. + // Retrying on DNS error, since we may be using sslip.io or nip.io in tests. if isDNSError(err) { return true, fmt.Errorf("retrying for DNS error: %w", err) } diff --git a/vendor/modules.txt b/vendor/modules.txt index deeadd96c6..c0a68035af 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -776,7 +776,7 @@ k8s.io/utils/buffer k8s.io/utils/integer k8s.io/utils/pointer k8s.io/utils/trace -# knative.dev/eventing v0.24.0 +# knative.dev/eventing v0.24.1-0.20210702080639-90932eb671e0 ## explicit knative.dev/eventing/pkg/apis/config knative.dev/eventing/pkg/apis/duck @@ -805,12 +805,12 @@ knative.dev/eventing/pkg/client/clientset/versioned/typed/sources/v1beta2/fake # knative.dev/hack v0.0.0-20210622141627-e28525d8d260 ## explicit knative.dev/hack -# knative.dev/networking v0.0.0-20210622182128-53f45d6d2cfa +# knative.dev/networking v0.0.0-20210705111547-ca0a601fc900 ## explicit knative.dev/networking/pkg knative.dev/networking/pkg/apis/networking knative.dev/networking/pkg/apis/networking/v1alpha1 -# knative.dev/pkg v0.0.0-20210622173328-dd0db4b05c80 +# knative.dev/pkg v0.0.0-20210701025203-30f9568e894e ## explicit knative.dev/pkg/apis knative.dev/pkg/apis/duck @@ -856,7 +856,7 @@ knative.dev/pkg/tracing/propagation knative.dev/pkg/tracing/propagation/tracecontextb3 knative.dev/pkg/tracker knative.dev/pkg/unstructured -# knative.dev/serving v0.24.0 +# knative.dev/serving v0.24.1-0.20210705154247-d207fa3a92ac ## explicit knative.dev/serving/pkg/apis/autoscaling knative.dev/serving/pkg/apis/autoscaling/v1alpha1 From b54266502346f0d5e1f0c4614368adfb9946a45f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Sun, 18 Jul 2021 11:00:12 +0200 Subject: [PATCH 2/6] fix e2e test --- test/e2e/service_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index 6b483c2ff2..e4fc0f1b4b 100644 --- a/test/e2e/service_test.go +++ b/test/e2e/service_test.go @@ -93,8 +93,7 @@ func serviceCreatePrivateUpdatePublic(r *test.KnRunResultCollector, serviceName r.AssertNoError(out) assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, network.VisibilityLabelKey, serving.VisibilityClusterLocal)) - out = r.KnTest().Kn().Run("service", "update", serviceName, - "--image", pkgtest.ImagePath("helloworld"), "--no-cluster-local") + out = r.KnTest().Kn().Run("service", "update", serviceName, "--no-cluster-local") r.AssertNoError(out) assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "no new revision", "namespace", r.KnTest().Kn().Namespace())) From 3264f6f30be84e1aa8da2c59b4d9b091e3871a29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Mon, 19 Jul 2021 15:20:06 +0200 Subject: [PATCH 3/6] add some unit tests --- pkg/serving/config_changes.go | 1 + pkg/serving/config_changes_test.go | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 31ebec94ba..441c681808 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -145,6 +145,7 @@ func PinImageToDigest(template *servingv1.RevisionTemplateSpec, baseRevision *se 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") } diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 9b3cadbfe9..6ec2220d28 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -107,7 +107,7 @@ func TestSetUserImageAnnotation(t *testing.T) { } } -func TestFreezeImageToDigest(t *testing.T) { +func TestPinImageToDigest(t *testing.T) { template, container := getRevisionTemplate() revision := &servingv1.Revision{} revision.Spec = template.Spec @@ -119,6 +119,26 @@ func TestFreezeImageToDigest(t *testing.T) { err := PinImageToDigest(template, revision) assert.NilError(t, err) assert.Equal(t, container.Image, "gcr.io/foo/bar@sha256:deadbeef") + + // No base revision --> no-op + err = PinImageToDigest(template, nil) + assert.NilError(t, err) +} + +func TestPinImageToDigestInvalidImages(t *testing.T) { + template, container := getRevisionTemplate() + container.Image = "gcr.io/A" + revision := &servingv1.Revision{ + Spec: servingv1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Image: "gcr.io/B"}, + }, + }, + }, + } + err := PinImageToDigest(template, revision) + assert.ErrorContains(t, err, "unexpected image") } func TestUpdateTimestampAnnotation(t *testing.T) { From fcee305c4f068c5d88fea362d1e3ec15b9dc9080 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Wed, 21 Jul 2021 10:33:04 +0200 Subject: [PATCH 4/6] Update error handling + added some tests --- pkg/kn/commands/revision/describe.go | 16 ++++---- pkg/kn/commands/service/update_test.go | 18 +++------ pkg/serving/config_changes.go | 39 ++++++++++++------- pkg/serving/revision_template.go | 17 +++----- pkg/serving/revision_template_test.go | 54 ++++++++++++++++++++++++++ pkg/serving/v1/client.go | 11 +----- pkg/serving/v1/client_test.go | 2 +- 7 files changed, 103 insertions(+), 54 deletions(-) diff --git a/pkg/kn/commands/revision/describe.go b/pkg/kn/commands/revision/describe.go index ff5e5fa4ce..42488ae66e 100644 --- a/pkg/kn/commands/revision/describe.go +++ b/pkg/kn/commands/revision/describe.go @@ -153,8 +153,8 @@ func WriteConcurrencyOptions(dw printers.PrefixWriter, revision *servingv1.Revis // Write the image attribute (with func WriteImage(dw printers.PrefixWriter, revision *servingv1.Revision) { - c, err := clientserving.ContainerOfRevisionSpec(&revision.Spec) - if err != nil { + c := clientserving.ContainerOfRevisionSpec(&revision.Spec) + if c == nil { dw.WriteAttribute("Image", "Unknown") return } @@ -223,8 +223,8 @@ func WriteScale(dw printers.PrefixWriter, revision *servingv1.Revision) { } func WriteResources(dw printers.PrefixWriter, r *servingv1.Revision) { - c, err := clientserving.ContainerOfRevisionSpec(&r.Spec) - if err != nil { + c := clientserving.ContainerOfRevisionSpec(&r.Spec) + if c == nil { return } requests := c.Resources.Requests @@ -280,8 +280,8 @@ func formatScale(minScale *int, maxScale *int) string { } func stringifyEnv(revision *servingv1.Revision) []string { - container, err := clientserving.ContainerOfRevisionSpec(&revision.Spec) - if err != nil { + container := clientserving.ContainerOfRevisionSpec(&revision.Spec) + if container == nil { return nil } @@ -297,8 +297,8 @@ func stringifyEnv(revision *servingv1.Revision) []string { } func stringifyEnvFrom(revision *servingv1.Revision) []string { - container, err := clientserving.ContainerOfRevisionSpec(&revision.Spec) - if err != nil { + container := clientserving.ContainerOfRevisionSpec(&revision.Spec) + if container == nil { return nil } diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index c79520995e..d514cdd346 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -187,10 +187,8 @@ func TestServiceUpdateImage(t *testing.T) { } template = &updated.Spec.Template - container, err := servinglib.ContainerOfRevisionTemplate(template) - if err != nil { - t.Fatal(err) - } + container := servinglib.ContainerOfRevisionSpec(&template.Spec) + assert.Assert(t, container != nil) assert.Equal(t, container.Image, "gcr.io/foo/quux:xyzzy") if !strings.Contains(strings.ToLower(output), "update") || @@ -886,10 +884,8 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) { func TestServiceUpdateLabelWhenEmpty(t *testing.T) { original := newEmptyService() origTemplate := original.Spec.Template - origContainer, err := servinglib.ContainerOfRevisionTemplate(&origTemplate) - if err != nil { - t.Fatal(err) - } + origContainer := servinglib.ContainerOfRevisionSpec(&origTemplate.Spec) + assert.Assert(t, origContainer != nil) origContainer.Image = "gcr.io/foo/bar:latest" action, updated, _, err := fakeServiceUpdate(original, []string{ @@ -912,10 +908,8 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { template := updated.Spec.Template actual = template.ObjectMeta.Labels assert.DeepEqual(t, expected, actual) - container, err := servinglib.ContainerOfRevisionTemplate(&template) - if err != nil { - t.Fatal(err) - } + container := servinglib.ContainerOfRevisionSpec(&template.Spec) + assert.Assert(t, container != nil) assert.Equal(t, container.Image, exampleImageByDigest) } diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 441c681808..e040a9ee0c 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -101,8 +101,9 @@ func UnsetUserImageAnnotation(template *servingv1.RevisionTemplateSpec) { func UpdateUserImageAnnotation(template *servingv1.RevisionTemplateSpec) { // If the current image isn't by-digest, set the user-image annotation to it // so we remember what it was. - currentContainer, err := ContainerOfRevisionTemplate(template) - if err != nil { + currentContainer := ContainerOfRevisionSpec(&template.Spec) + if currentContainer == nil { + // No container set in the template, so return } image := currentContainer.Image @@ -131,28 +132,40 @@ func ensureAnnotations(template *servingv1.RevisionTemplateSpec) { } // PinImageToDigest sets the image on the template to the image digest of the base revision. -func PinImageToDigest(template *servingv1.RevisionTemplateSpec, baseRevision *servingv1.Revision) error { +func PinImageToDigest(currentRevisionTemplate *servingv1.RevisionTemplateSpec, baseRevision *servingv1.Revision) error { + // If there is no base revision then there is nothing to pin to. It's not an error so let's return + // silently if baseRevision == nil { return nil } - currentContainer, err := ContainerOfRevisionTemplate(template) + err := VerifyThatContainersMatchInCurrentAndBaseRevision(currentRevisionTemplate, baseRevision) if err != nil { - return err + return fmt.Errorf("can not pin image to digest: %e", err) } - baseContainer, err := ContainerOfRevisionSpec(&baseRevision.Spec) - if err != nil { - return err + containerStatus := ContainerStatus(baseRevision) + if containerStatus.ImageDigest != "" { + return flags.UpdateImage(¤tRevisionTemplate.Spec.PodSpec, containerStatus.ImageDigest) + } + return nil +} + +// VerifyThatContainersMatchInCurrentAndBaseRevision checks if the image in the current revision matches +// matches the one in a given base revision +func VerifyThatContainersMatchInCurrentAndBaseRevision(template *servingv1.RevisionTemplateSpec, baseRevision *servingv1.Revision) error { + currentContainer := ContainerOfRevisionSpec(&template.Spec) + if currentContainer == nil { + return fmt.Errorf("no container given in current revision %s", template.Name) } - if currentContainer.Image != baseContainer.Image { - return fmt.Errorf("could not freeze image to digest since current revision contains unexpected image") + baseContainer := ContainerOfRevisionSpec(&baseRevision.Spec) + if baseContainer == nil { + return fmt.Errorf("no container found in base revision %s", baseRevision.Name) } - containerStatus := ContainerStatus(baseRevision) - if containerStatus.ImageDigest != "" { - return flags.UpdateImage(&template.Spec.PodSpec, containerStatus.ImageDigest) + if currentContainer.Image != baseContainer.Image { + return fmt.Errorf("current revision %s contains unexpected image (%s) that does not fit to the base revision's %s image (%s)", template.Name, currentContainer.Image, baseRevision.Name, baseContainer.Image) } return nil } diff --git a/pkg/serving/revision_template.go b/pkg/serving/revision_template.go index e99aff3ad0..0339a22fe4 100644 --- a/pkg/serving/revision_template.go +++ b/pkg/serving/revision_template.go @@ -15,7 +15,6 @@ package serving import ( - "fmt" "strconv" corev1 "k8s.io/api/core/v1" @@ -29,19 +28,15 @@ type Scaling struct { Max *int } -func ContainerOfRevisionTemplate(template *servingv1.RevisionTemplateSpec) (*corev1.Container, error) { - return ContainerOfRevisionSpec(&template.Spec) -} - // ContainerOfRevisionSpec returns the 'main' container of a revision specification and // use GetServingContainerIndex to identify the container. -// An error is returned if no such container could be found -func ContainerOfRevisionSpec(revisionSpec *servingv1.RevisionSpec) (*corev1.Container, error) { +// Nil is returned if no such container could be found +func ContainerOfRevisionSpec(revisionSpec *servingv1.RevisionSpec) *corev1.Container { idx := ContainerIndexOfRevisionSpec(revisionSpec) if idx == -1 { - return nil, fmt.Errorf("internal: no container set in spec.template.spec.containers") + return nil } - return &revisionSpec.Containers[0], nil + return &revisionSpec.Containers[0] } // ContainerIndexOfRevisionSpec returns the index of the "main" container if @@ -109,8 +104,8 @@ func AutoscaleWindow(m *metav1.ObjectMeta) string { } func Port(revisionSpec *servingv1.RevisionSpec) *int32 { - c, err := ContainerOfRevisionSpec(revisionSpec) - if err != nil { + c := ContainerOfRevisionSpec(revisionSpec) + if c == nil { return nil } if len(c.Ports) > 0 { diff --git a/pkg/serving/revision_template_test.go b/pkg/serving/revision_template_test.go index 80ca2583ec..ad8ee13684 100644 --- a/pkg/serving/revision_template_test.go +++ b/pkg/serving/revision_template_test.go @@ -17,6 +17,9 @@ package serving import ( "testing" + corev1 "k8s.io/api/core/v1" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" + "gotest.tools/v3/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -71,3 +74,54 @@ func TestScalingInfo(t *testing.T) { } } + +func TestContainerIndexOfRevisionSpec(t *testing.T) { + tests := []struct { + name string + revSpec *servingv1.RevisionSpec + want int + }{ + { + "no container", + &servingv1.RevisionSpec{}, + -1, + }, + { + "1 container", + &servingv1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "user-container", + Ports: []corev1.ContainerPort{{ContainerPort: 80}}, + }, + }}}, + 0, + }, + { + "2 containers", + &servingv1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "sidecar-container-1", + }, + { + Name: "user-container", + Ports: []corev1.ContainerPort{{ContainerPort: 80}}, + }, + { + Name: "sidecar-container-2", + }, + }}}, + 1, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ContainerIndexOfRevisionSpec(tt.revSpec); got != tt.want { + t.Errorf("ContainerIndexOfRevisionSpec() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index caf303e78e..e1116f13f0 100644 --- a/pkg/serving/v1/client.go +++ b/pkg/serving/v1/client.go @@ -424,18 +424,11 @@ func getBaseRevision(ctx context.Context, cl KnServingClient, service *servingv1 if err != nil { return nil, err } - latestContainer, err := serving.ContainerOfRevisionSpec(&latestCreated.Spec) - if err != nil { - return nil, err - } - container, err := serving.ContainerOfRevisionTemplate(&template) + + err = serving.VerifyThatContainersMatchInCurrentAndBaseRevision(&template, latestCreated) 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 diff --git a/pkg/serving/v1/client_test.go b/pkg/serving/v1/client_test.go index ef3969a013..bb04abe081 100644 --- a/pkg/serving/v1/client_test.go +++ b/pkg/serving/v1/client_test.go @@ -655,7 +655,7 @@ func TestGetBaseRevision(t *testing.T) { {"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"}, + {"", "gcr.io/foo/bar", "foo-asdf", "foo-asdf", "gcr.io/foo/baz", "unexpected image"}, } var c baseRevisionCase servingFake.AddReactor("get", "revisions", func(a clienttesting.Action) (bool, runtime.Object, error) { From 6f97cbb1373bada0dd744f34cb3e956a4be30fbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Wed, 21 Jul 2021 10:46:46 +0200 Subject: [PATCH 5/6] fixed compile error in test --- pkg/serving/revision_template_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/serving/revision_template_test.go b/pkg/serving/revision_template_test.go index f586195621..69a4ad681b 100644 --- a/pkg/serving/revision_template_test.go +++ b/pkg/serving/revision_template_test.go @@ -21,11 +21,9 @@ import ( servingv1 "knative.dev/serving/pkg/apis/serving/v1" "gotest.tools/v3/assert" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/serving/pkg/apis/autoscaling" - servingv1 "knative.dev/serving/pkg/apis/serving/v1" ) type scalingInfoTest struct { @@ -126,6 +124,7 @@ func TestContainerIndexOfRevisionSpec(t *testing.T) { } }) } +} func TestAnnotations(t *testing.T) { m := &metav1.ObjectMeta{} From ba2500e62af879ffd88d60a6f71a83759844f515 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Fri, 23 Jul 2021 15:53:49 +0200 Subject: [PATCH 6/6] lint fixes --- pkg/kn/commands/flags/listprint_test.go | 4 +--- pkg/kn/commands/service/update_test.go | 7 ++----- pkg/serving/config_changes.go | 2 +- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/pkg/kn/commands/flags/listprint_test.go b/pkg/kn/commands/flags/listprint_test.go index c9109485c9..a5e8e0cd46 100644 --- a/pkg/kn/commands/flags/listprint_test.go +++ b/pkg/kn/commands/flags/listprint_test.go @@ -76,10 +76,8 @@ func TestListPrintFlags(t *testing.T) { assert.NilError(t, err) assert.Assert(t, allowMissingTemplateKeys == true) - p, err := flags.ToPrinter() + _, err = flags.ToPrinter() assert.NilError(t, err) - _, ok := p.(hprinters.ResourcePrinter) - assert.Check(t, ok == true) } func TestListPrintFlagsPrint(t *testing.T) { diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index d514cdd346..dd2037ea85 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -188,8 +188,7 @@ func TestServiceUpdateImage(t *testing.T) { template = &updated.Spec.Template container := servinglib.ContainerOfRevisionSpec(&template.Spec) - assert.Assert(t, container != nil) - assert.Equal(t, container.Image, "gcr.io/foo/quux:xyzzy") + assert.Assert(t, container != nil && container.Image == "gcr.io/foo/quux:xyzzy") if !strings.Contains(strings.ToLower(output), "update") || !strings.Contains(output, "foo") || @@ -885,7 +884,6 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { original := newEmptyService() origTemplate := original.Spec.Template origContainer := servinglib.ContainerOfRevisionSpec(&origTemplate.Spec) - assert.Assert(t, origContainer != nil) origContainer.Image = "gcr.io/foo/bar:latest" action, updated, _, err := fakeServiceUpdate(original, []string{ @@ -909,8 +907,7 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) { actual = template.ObjectMeta.Labels assert.DeepEqual(t, expected, actual) container := servinglib.ContainerOfRevisionSpec(&template.Spec) - assert.Assert(t, container != nil) - assert.Equal(t, container.Image, exampleImageByDigest) + assert.Assert(t, container != nil && container.Image == exampleImageByDigest) } func TestServiceUpdateLabelExisting(t *testing.T) { diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index e040a9ee0c..0629b65a75 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -141,7 +141,7 @@ func PinImageToDigest(currentRevisionTemplate *servingv1.RevisionTemplateSpec, b err := VerifyThatContainersMatchInCurrentAndBaseRevision(currentRevisionTemplate, baseRevision) if err != nil { - return fmt.Errorf("can not pin image to digest: %e", err) + return fmt.Errorf("can not pin image to digest: %w", err) } containerStatus := ContainerStatus(baseRevision)