From 8364055f0fff47fc7aa3593585e532c4386d8ce2 Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Fri, 10 Apr 2015 11:41:18 -0400 Subject: [PATCH] Improve validation and fix not throwing an error during ns delete --- pkg/api/validation/validation.go | 9 ++++ pkg/api/validation/validation_test.go | 23 +++++++-- pkg/registry/namespace/etcd/etcd.go | 5 +- pkg/registry/namespace/etcd/etcd_test.go | 60 +++++++++++++++++++++++- pkg/registry/namespace/rest_test.go | 4 +- 5 files changed, 93 insertions(+), 8 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 6c69051f341..8c12512612b 100644 --- a/pkg/api/validation/validation.go +++ b/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 } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 87759f60c3e..1076f97087f 100644 --- a/pkg/api/validation/validation_test.go +++ b/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) diff --git a/pkg/registry/namespace/etcd/etcd.go b/pkg/registry/namespace/etcd/etcd.go index a04fa96e6b8..a1548630313 100644 --- a/pkg/registry/namespace/etcd/etcd.go +++ b/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) } diff --git a/pkg/registry/namespace/etcd/etcd_test.go b/pkg/registry/namespace/etcd/etcd_test.go index f3df2d1562e..a6fbf4c13ec 100644 --- a/pkg/registry/namespace/etcd/etcd_test.go +++ b/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) + } } diff --git a/pkg/registry/namespace/rest_test.go b/pkg/registry/namespace/rest_test.go index a99c5741d4d..afc2e8b5ee6 100644 --- a/pkg/registry/namespace/rest_test.go +++ b/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)