From f936c93698116bb61062add83cd365a78c6487d4 Mon Sep 17 00:00:00 2001 From: Lv Jiawei Date: Tue, 21 Apr 2020 19:54:50 +0800 Subject: [PATCH] Check DeleteTimestamp before updating resource (#805) * Check DeleteTimestamp before updating resource. Include: service, apiserver source, sink binding, ping source, trigger. --- CHANGELOG.adoc | 4 ++++ pkg/kn/commands/service/update_test.go | 11 +++++++++++ pkg/kn/commands/source/apiserver/update.go | 3 +++ pkg/kn/commands/source/apiserver/update_test.go | 15 +++++++++++++++ pkg/kn/commands/source/binding/update.go | 3 +++ pkg/kn/commands/source/binding/update_test.go | 14 ++++++++++++++ pkg/kn/commands/source/ping/update.go | 3 +++ pkg/kn/commands/source/ping/update_test.go | 14 ++++++++++++++ pkg/kn/commands/trigger/update.go | 3 +++ pkg/kn/commands/trigger/update_test.go | 16 ++++++++++++++++ pkg/serving/v1/client.go | 3 +++ 11 files changed, 89 insertions(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index c50b8a6762..626667ab8f 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -22,6 +22,10 @@ | Add `-a` flag as an alias for `--annotation` | https://github.com/knative/client/pull/782[#782] +| 🐛 +| Check DeleteTimestamp before updating resource +| https://github.com/knative/client/pull/805[#805] + |=== ## v0.13.2 (2020-04-15) diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index 9364ff7914..888f68320c 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -20,6 +20,7 @@ import ( "reflect" "strings" "testing" + "time" "gotest.tools/assert" "gotest.tools/assert/cmp" @@ -726,6 +727,16 @@ func TestServiceUpdateNoClusterLocalOnPrivateService(t *testing.T) { assert.DeepEqual(t, expected, actual) } +func TestServiceUpdateDeletionTimestampNotNil(t *testing.T) { + original := newEmptyService() + original.DeletionTimestamp = &metav1.Time{Time: time.Now()} + _, _, _, err := fakeServiceUpdate(original, []string{ + "service", "update", "foo", "--revision-name", "foo-v1"}) + assert.ErrorContains(t, err, original.Name) + assert.ErrorContains(t, err, "deletion") + assert.ErrorContains(t, err, "service") +} + func newEmptyService() *servingv1.Service { ret := &servingv1.Service{ TypeMeta: metav1.TypeMeta{ diff --git a/pkg/kn/commands/source/apiserver/update.go b/pkg/kn/commands/source/apiserver/update.go index 146dc1cf12..6ec97bc0fc 100644 --- a/pkg/kn/commands/source/apiserver/update.go +++ b/pkg/kn/commands/source/apiserver/update.go @@ -63,6 +63,9 @@ func NewAPIServerUpdateCommand(p *commands.KnParams) *cobra.Command { if err != nil { return err } + if source.GetDeletionTimestamp() != nil { + return fmt.Errorf("can't update apiserver source %s because it has been marked for deletion", name) + } b := v1alpha2.NewAPIServerSourceBuilderFromExisting(source) if cmd.Flags().Changed("service-account") { diff --git a/pkg/kn/commands/source/apiserver/update_test.go b/pkg/kn/commands/source/apiserver/update_test.go index 3b6bf807fd..2b0d9ee49c 100644 --- a/pkg/kn/commands/source/apiserver/update_test.go +++ b/pkg/kn/commands/source/apiserver/update_test.go @@ -16,6 +16,7 @@ package apiserver import ( "testing" + "time" "gotest.tools/assert" @@ -48,3 +49,17 @@ func TestApiServerSourceUpdate(t *testing.T) { apiServerRecorder.Validate() } + +func TestApiServerSourceUpdateDeletionTimestampNotNil(t *testing.T) { + apiServerClient := v1alpha2.NewMockKnAPIServerSourceClient(t) + apiServerRecorder := apiServerClient.Recorder() + + present := createAPIServerSource("testsource", "Event", "v1", "testsa1", "Ref", "svc1", false) + present.DeletionTimestamp = &metav1.Time{Time: time.Now()} + apiServerRecorder.GetAPIServerSource("testsource", present, nil) + + _, err := executeAPIServerSourceCommand(apiServerClient, nil, "update", "testsource", "--service-account", "testsa2", "--sink", "svc:svc2") + assert.ErrorContains(t, err, present.Name) + assert.ErrorContains(t, err, "deletion") + assert.ErrorContains(t, err, "apiserver") +} diff --git a/pkg/kn/commands/source/binding/update.go b/pkg/kn/commands/source/binding/update.go index 93f490f49f..b8b8526f33 100644 --- a/pkg/kn/commands/source/binding/update.go +++ b/pkg/kn/commands/source/binding/update.go @@ -61,6 +61,9 @@ func NewBindingUpdateCommand(p *commands.KnParams) *cobra.Command { if err != nil { return err } + if source.GetDeletionTimestamp() != nil { + return fmt.Errorf("can't update binding %s because it has been marked for deletion", name) + } b := v1alpha12.NewSinkBindingBuilderFromExisting(source) if cmd.Flags().Changed("sink") { diff --git a/pkg/kn/commands/source/binding/update_test.go b/pkg/kn/commands/source/binding/update_test.go index b7c006fa0f..fba37abc77 100644 --- a/pkg/kn/commands/source/binding/update_test.go +++ b/pkg/kn/commands/source/binding/update_test.go @@ -17,6 +17,7 @@ package binding import ( "errors" "testing" + "time" "gotest.tools/assert" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -66,3 +67,16 @@ func TestUpdateError(t *testing.T) { bindingRecorder.Validate() } + +func TestBindingUpdateDeletionTimestampNotNil(t *testing.T) { + sinkBindingClient := clientsourcesv1alpha1.NewMockKnSinkBindingClient(t) + bindingRecorder := sinkBindingClient.Recorder() + present := createSinkBinding("testbinding", "", deploymentGvk, "", nil) + present.DeletionTimestamp = &v1.Time{Time: time.Now()} + bindingRecorder.GetSinkBinding("testbinding", present, nil) + + _, err := executeSinkBindingCommand(sinkBindingClient, nil, "update", "testbinding") + assert.ErrorContains(t, err, present.Name) + assert.ErrorContains(t, err, "deletion") + assert.ErrorContains(t, err, "binding") +} diff --git a/pkg/kn/commands/source/ping/update.go b/pkg/kn/commands/source/ping/update.go index 60ea091827..80ce941df0 100644 --- a/pkg/kn/commands/source/ping/update.go +++ b/pkg/kn/commands/source/ping/update.go @@ -61,6 +61,9 @@ func NewPingUpdateCommand(p *commands.KnParams) *cobra.Command { if err != nil { return err } + if source.GetDeletionTimestamp() != nil { + return fmt.Errorf("can't update ping source %s because it has been marked for deletion", name) + } b := v1alpha2.NewPingSourceBuilderFromExisting(source) if cmd.Flags().Changed("schedule") { diff --git a/pkg/kn/commands/source/ping/update_test.go b/pkg/kn/commands/source/ping/update_test.go index ca3de723fd..59db0b9a9c 100644 --- a/pkg/kn/commands/source/ping/update_test.go +++ b/pkg/kn/commands/source/ping/update_test.go @@ -17,6 +17,7 @@ package ping import ( "errors" "testing" + "time" "gotest.tools/assert" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -74,3 +75,16 @@ func TestUpdateError(t *testing.T) { pingRecorder.Validate() } + +func TestPingUpdateDeletionTimestampNotNil(t *testing.T) { + pingSourceClient := clientv1alpha2.NewMockKnPingSourceClient(t) + present := createPingSource("test", "", "", "") + present.DeletionTimestamp = &v1.Time{Time: time.Now()} + pingRecorder := pingSourceClient.Recorder() + pingRecorder.GetPingSource("test", present, nil) + + _, err := executePingSourceCommand(pingSourceClient, nil, "update", "test") + assert.ErrorContains(t, err, present.Name) + assert.ErrorContains(t, err, "deletion") + assert.ErrorContains(t, err, "ping") +} diff --git a/pkg/kn/commands/trigger/update.go b/pkg/kn/commands/trigger/update.go index fa5a036eae..2f6313b6d1 100644 --- a/pkg/kn/commands/trigger/update.go +++ b/pkg/kn/commands/trigger/update.go @@ -75,6 +75,9 @@ func NewTriggerUpdateCommand(p *commands.KnParams) *cobra.Command { if err != nil { return err } + if trigger.GetDeletionTimestamp() != nil { + return fmt.Errorf("can't update trigger %s because it has been marked for deletion", name) + } b := client_v1alpha1.NewTriggerBuilderFromExisting(trigger) diff --git a/pkg/kn/commands/trigger/update_test.go b/pkg/kn/commands/trigger/update_test.go index eb953f57d9..6aab9bcb9c 100644 --- a/pkg/kn/commands/trigger/update_test.go +++ b/pkg/kn/commands/trigger/update_test.go @@ -17,6 +17,7 @@ package trigger import ( "fmt" "testing" + "time" "gotest.tools/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -74,3 +75,18 @@ func TestTriggerUpdateInvalidBroker(t *testing.T) { eventingRecorder.Validate() } + +func TestTriggerUpdateDeletionTimestampNotNil(t *testing.T) { + eventingClient := clienteventingv1alpha1.NewMockKnEventingClient(t) + + eventingRecorder := eventingClient.Recorder() + present := createTrigger("default", triggerName, map[string]string{"type": "dev.knative.foo"}, "mybroker", "mysvc") + present.DeletionTimestamp = &metav1.Time{Time: time.Now()} + eventingRecorder.GetTrigger(triggerName, present, nil) + + _, err := executeTriggerCommand(eventingClient, nil, "update", triggerName, + "--filter", "type=dev.knative.new", "--sink", "svc:mysvc") + assert.ErrorContains(t, err, present.Name) + assert.ErrorContains(t, err, "deletion") + assert.ErrorContains(t, err, "trigger") +} diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index a4bb53a1ac..ca2d0f5986 100644 --- a/pkg/serving/v1/client.go +++ b/pkg/serving/v1/client.go @@ -237,6 +237,9 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU if err != nil { return err } + if service.GetDeletionTimestamp() != nil { + return fmt.Errorf("can't update service %s because it has been marked for deletion", name) + } updatedService, err := updateFunc(service.DeepCopy()) if err != nil { return err