Merge pull request #6686 from derekwaynecarr/terminate_must_have_deletion_timestamp

Improve validation and fix not throwing an error during ns delete

pkg/api/validation/validation.go

@@ -1240,6 +1240,15 @@ func ValidateNamespaceStatusUpdate(newNamespace, oldNamespace *api.Namespace) er
 	allErrs := errs.ValidationErrorList{}
 	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNamespace.ObjectMeta, &newNamespace.ObjectMeta).Prefix("metadata")...)
 	newNamespace.Spec = oldNamespace.Spec
+	if newNamespace.DeletionTimestamp.IsZero() {
+		if newNamespace.Status.Phase != api.NamespaceActive {
+			allErrs = append(allErrs, errs.NewFieldInvalid("Status.Phase", newNamespace.Status.Phase, "A namespace may only be in active status if it does not have a deletion timestamp."))
+		}
+	} else {
+		if newNamespace.Status.Phase != api.NamespaceTerminating {
+			allErrs = append(allErrs, errs.NewFieldInvalid("Status.Phase", newNamespace.Status.Phase, "A namespace may only be in terminating status if it has a deletion timestamp."))
+		}
+	}
 	return allErrs
 }
 

pkg/api/validation/validation_test.go

@@ -2629,12 +2629,29 @@ func TestValidateNamespaceFinalizeUpdate(t *testing.T) {
 }
 
 func TestValidateNamespaceStatusUpdate(t *testing.T) {
+	now := util.Now()
+
 	tests := []struct {
 		oldNamespace api.Namespace
 		namespace    api.Namespace
 		valid        bool
 	}{
-		{api.Namespace{}, api.Namespace{}, true},
+		{api.Namespace{}, api.Namespace{
+			Status: api.NamespaceStatus{
+				Phase: api.NamespaceActive,
+			},
+		}, true},
+		{api.Namespace{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo"}},
+			api.Namespace{
+				ObjectMeta: api.ObjectMeta{
+					Name:              "foo",
+					DeletionTimestamp: &now},
+				Status: api.NamespaceStatus{
+					Phase: api.NamespaceTerminating,
+				},
+			}, true},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
 				Name: "foo"}},
@@ -2644,7 +2661,7 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) {
 				Status: api.NamespaceStatus{
 					Phase: api.NamespaceTerminating,
 				},
-			}, true},
+			}, false},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
 				Name: "foo"}},
@@ -2659,7 +2676,7 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) {
 	for i, test := range tests {
 		test.namespace.ObjectMeta.ResourceVersion = "1"
 		test.oldNamespace.ObjectMeta.ResourceVersion = "1"
-		errs := ValidateNamespaceStatusUpdate(&test.oldNamespace, &test.namespace)
+		errs := ValidateNamespaceStatusUpdate(&test.namespace, &test.oldNamespace)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)
 			t.Logf("%#v vs %#v", test.oldNamespace.ObjectMeta, test.namespace.ObjectMeta)

pkg/registry/namespace/etcd/etcd.go

@@ -89,7 +89,7 @@ func (r *REST) Delete(ctx api.Context, name string, options *api.DeleteOptions)
 	namespace := nsObj.(*api.Namespace)
 
 	// upon first request to delete, we switch the phase to start namespace termination
-	if namespace.DeletionTimestamp == nil {
+	if namespace.DeletionTimestamp.IsZero() {
 		now := util.Now()
 		namespace.DeletionTimestamp = &now
 		namespace.Status.Phase = api.NamespaceTerminating
@@ -99,7 +99,8 @@ func (r *REST) Delete(ctx api.Context, name string, options *api.DeleteOptions)
 
 	// prior to final deletion, we must ensure that finalizers is empty
 	if len(namespace.Spec.Finalizers) != 0 {
-		err = fmt.Errorf("Unable to delete namespace %v because finalizers is not empty %v", namespace.Name, namespace.Spec.Finalizers)
+		err = fmt.Errorf("Namespace %v termination is in progress, waiting for %v", namespace.Name, namespace.Spec.Finalizers)
+		return nil, err
 	}
 	return r.Etcd.Delete(ctx, name, nil)
 }

pkg/registry/namespace/etcd/etcd_test.go

@@ -319,6 +319,62 @@ func TestDeleteNamespace(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-
+}
-	// TODO: when we add life-cycle, this will go to Terminating, and then we need to test Terminating to gone
+
+func TestDeleteNamespaceWithIncompleteFinalizers(t *testing.T) {
+	now := util.Now()
+	fakeEtcdClient, helper := newHelper(t)
+	fakeEtcdClient.ChangeIndex = 1
+	fakeEtcdClient.Data["/registry/namespaces/foo"] = tools.EtcdResponseWithError{
+		R: &etcd.Response{
+			Node: &etcd.Node{
+				Value: runtime.EncodeOrDie(latest.Codec, &api.Namespace{
+					ObjectMeta: api.ObjectMeta{
+						Name:              "foo",
+						DeletionTimestamp: &now,
+					},
+					Spec: api.NamespaceSpec{
+						Finalizers: []api.FinalizerName{api.FinalizerKubernetes},
+					},
+					Status: api.NamespaceStatus{Phase: api.NamespaceActive},
+				}),
+				ModifiedIndex: 1,
+				CreatedIndex:  1,
+			},
+		},
+	}
+	storage, _, _ := NewStorage(helper)
+	_, err := storage.Delete(api.NewDefaultContext(), "foo", nil)
+	if err == nil {
+		t.Fatalf("expected error: %v", err)
+	}
+}
+
+func TestDeleteNamespaceWithCompleteFinalizers(t *testing.T) {
+	now := util.Now()
+	fakeEtcdClient, helper := newHelper(t)
+	fakeEtcdClient.ChangeIndex = 1
+	fakeEtcdClient.Data["/registry/namespaces/foo"] = tools.EtcdResponseWithError{
+		R: &etcd.Response{
+			Node: &etcd.Node{
+				Value: runtime.EncodeOrDie(latest.Codec, &api.Namespace{
+					ObjectMeta: api.ObjectMeta{
+						Name:              "foo",
+						DeletionTimestamp: &now,
+					},
+					Spec: api.NamespaceSpec{
+						Finalizers: []api.FinalizerName{},
+					},
+					Status: api.NamespaceStatus{Phase: api.NamespaceActive},
+				}),
+				ModifiedIndex: 1,
+				CreatedIndex:  1,
+			},
+		},
+	}
+	storage, _, _ := NewStorage(helper)
+	_, err := storage.Delete(api.NewDefaultContext(), "foo", nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 }

pkg/registry/namespace/rest_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 func TestNamespaceStrategy(t *testing.T) {
@@ -70,13 +71,14 @@ func TestNamespaceStatusStrategy(t *testing.T) {
 	if StatusStrategy.AllowCreateOnUpdate() {
 		t.Errorf("Namespaces should not allow create on update")
 	}
+	now := util.Now()
 	oldNamespace := &api.Namespace{
 		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"},
 		Spec:       api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes"}},
 		Status:     api.NamespaceStatus{Phase: api.NamespaceActive},
 	}
 	namespace := &api.Namespace{
-		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "9"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "9", DeletionTimestamp: &now},
 		Status:     api.NamespaceStatus{Phase: api.NamespaceTerminating},
 	}
 	StatusStrategy.PrepareForUpdate(namespace, oldNamespace)