Check DeleteTimestamp before updating resource (knative#805)

* Check DeleteTimestamp before updating resource. Include: service, apiserver source, sink binding, ping source, trigger.
maximilien · Apr 21, 2020 · f936c93 · f936c93
1 parent 286c6cd
commit f936c93
Show file tree

Hide file tree

Showing 11 changed files with 89 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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