From 3b4ddba551d697baa2d96796add3a9865b1da78a Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Thu, 2 Apr 2015 10:38:05 -0400 Subject: [PATCH] UPSTREAM Client must specify a resource version on finalize --- .../pkg/api/validation/validation.go | 26 ++---- .../pkg/api/validation/validation_test.go | 62 ++++++++++--- .../pkg/namespace/namespace_controller.go | 12 +-- .../kubernetes/pkg/registry/namespace/rest.go | 7 ++ .../pkg/registry/namespace/rest_test.go | 92 ++++++++++++++++++- 5 files changed, 161 insertions(+), 38 deletions(-) diff --git a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go index 9700b460d8c7..2899504f2b00 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go @@ -1116,21 +1116,13 @@ func validateFinalizerName(stringValue string) errs.ValidationErrorList { return errs.ValidationErrorList{} } -// ValidateNamespaceUpdate tests to make sure a namespace update can be applied. Modifies oldNamespace. -func ValidateNamespaceUpdate(oldNamespace *api.Namespace, namespace *api.Namespace) errs.ValidationErrorList { +// ValidateNamespaceUpdate tests to make sure a namespace update can be applied. +// newNamespace is updated with fields that cannot be changed +func ValidateNamespaceUpdate(newNamespace *api.Namespace, oldNamespace *api.Namespace) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNamespace.ObjectMeta, &namespace.ObjectMeta).Prefix("metadata")...) - - // TODO: move reset function to its own location - // Ignore metadata changes now that they have been tested - oldNamespace.ObjectMeta = namespace.ObjectMeta - oldNamespace.Spec.Finalizers = namespace.Spec.Finalizers - - // TODO: Add a 'real' ValidationError type for this error and provide print actual diffs. - if !api.Semantic.DeepEqual(oldNamespace, namespace) { - glog.V(4).Infof("Update failed validation %#v vs %#v", oldNamespace, namespace) - allErrs = append(allErrs, fmt.Errorf("update contains more than labels or annotation changes")) - } + allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNamespace.ObjectMeta, &newNamespace.ObjectMeta).Prefix("metadata")...) + newNamespace.Spec.Finalizers = oldNamespace.Spec.Finalizers + newNamespace.Status = oldNamespace.Status return allErrs } @@ -1143,17 +1135,15 @@ func ValidateNamespaceStatusUpdate(newNamespace, oldNamespace *api.Namespace) er return allErrs } -// ValidateNamespaceFinalizeUpdate tests to see if the update is legal for an end user to make. newNamespace is updated with fields -// that cannot be changed. +// ValidateNamespaceFinalizeUpdate tests to see if the update is legal for an end user to make. +// newNamespace is updated with fields that cannot be changed. func ValidateNamespaceFinalizeUpdate(newNamespace, oldNamespace *api.Namespace) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNamespace.ObjectMeta, &newNamespace.ObjectMeta).Prefix("metadata")...) for i := range newNamespace.Spec.Finalizers { allErrs = append(allErrs, validateFinalizerName(string(newNamespace.Spec.Finalizers[i]))...) } - newNamespace.ObjectMeta = oldNamespace.ObjectMeta newNamespace.Status = oldNamespace.Status - fmt.Printf("NEW NAMESPACE FINALIZERS : %v\n", newNamespace.Spec.Finalizers) return allErrs } diff --git a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation_test.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation_test.go index 798a3179d36e..6d543c49942d 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation_test.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation_test.go @@ -2495,6 +2495,9 @@ func TestValidateNamespace(t *testing.T) { }, { ObjectMeta: api.ObjectMeta{Name: "abc-123"}, + Spec: api.NamespaceSpec{ + Finalizers: []api.FinalizerName{"example.com/something", "example.com/other"}, + }, }, } for _, successCase := range successCases { @@ -2558,8 +2561,24 @@ func TestValidateNamespaceFinalizeUpdate(t *testing.T) { Finalizers: []api.FinalizerName{"foo.com/bar", "what.com/bar"}, }, }, true}, + {api.Namespace{ + ObjectMeta: api.ObjectMeta{ + Name: "fooemptyfinalizer"}, + Spec: api.NamespaceSpec{ + Finalizers: []api.FinalizerName{"foo.com/bar"}, + }, + }, + api.Namespace{ + ObjectMeta: api.ObjectMeta{ + Name: "fooemptyfinalizer"}, + Spec: api.NamespaceSpec{ + Finalizers: []api.FinalizerName{"", "foo.com/bar", "what.com/bar"}, + }, + }, false}, } for i, test := range tests { + test.namespace.ObjectMeta.ResourceVersion = "1" + test.oldNamespace.ObjectMeta.ResourceVersion = "1" errs := ValidateNamespaceFinalizeUpdate(&test.namespace, &test.oldNamespace) if test.valid && len(errs) > 0 { t.Errorf("%d: Unexpected error: %v", i, errs) @@ -2600,6 +2619,8 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { }, false}, } for i, test := range tests { + test.namespace.ObjectMeta.ResourceVersion = "1" + test.oldNamespace.ObjectMeta.ResourceVersion = "1" errs := ValidateNamespaceStatusUpdate(&test.oldNamespace, &test.namespace) if test.valid && len(errs) > 0 { t.Errorf("%d: Unexpected error: %v", i, errs) @@ -2620,57 +2641,76 @@ func TestValidateNamespaceUpdate(t *testing.T) { {api.Namespace{}, api.Namespace{}, true}, {api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo"}}, + Name: "foo1"}}, api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "bar"}, + Name: "bar1"}, }, false}, {api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo2", Labels: map[string]string{"foo": "bar"}, }, }, api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo2", Labels: map[string]string{"foo": "baz"}, }, }, true}, {api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo3", }, }, api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo3", Labels: map[string]string{"foo": "baz"}, }, }, true}, {api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo4", Labels: map[string]string{"bar": "foo"}, }, }, api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo4", Labels: map[string]string{"foo": "baz"}, }, }, true}, {api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo5", Labels: map[string]string{"foo": "baz"}, }, }, api.Namespace{ ObjectMeta: api.ObjectMeta{ - Name: "foo", + Name: "foo5", Labels: map[string]string{"Foo": "baz"}, }, }, true}, + {api.Namespace{ + ObjectMeta: api.ObjectMeta{ + Name: "foo6", + Labels: map[string]string{"foo": "baz"}, + }, + }, api.Namespace{ + ObjectMeta: api.ObjectMeta{ + Name: "foo6", + Labels: map[string]string{"Foo": "baz"}, + }, + Spec: api.NamespaceSpec{ + Finalizers: []api.FinalizerName{"kubernetes"}, + }, + Status: api.NamespaceStatus{ + Phase: api.NamespaceTerminating, + }, + }, true}, } for i, test := range tests { - errs := ValidateNamespaceUpdate(&test.oldNamespace, &test.namespace) + test.namespace.ObjectMeta.ResourceVersion = "1" + test.oldNamespace.ObjectMeta.ResourceVersion = "1" + errs := ValidateNamespaceUpdate(&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/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/namespace/namespace_controller.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/namespace/namespace_controller.go index 94518f5a99fb..2ecdbf1a0281 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/namespace/namespace_controller.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/namespace/namespace_controller.go @@ -99,20 +99,16 @@ func finalized(namespace api.Namespace) bool { // finalize will finalize the namespace for kubernetes func finalize(kubeClient client.Interface, namespace api.Namespace) (*api.Namespace, error) { - namespaceFinalize := api.Namespace{ - ObjectMeta: api.ObjectMeta{ - Name: namespace.Name, - ResourceVersion: namespace.ResourceVersion, - }, - Spec: api.NamespaceSpec{}, - } + namespaceFinalize := api.Namespace{} + namespaceFinalize.ObjectMeta = namespace.ObjectMeta + namespaceFinalize.Spec = namespace.Spec finalizerSet := util.NewStringSet() for i := range namespace.Spec.Finalizers { if namespace.Spec.Finalizers[i] != api.FinalizerKubernetes { finalizerSet.Insert(string(namespace.Spec.Finalizers[i])) } } - namespaceFinalize.Spec.Finalizers = make([]api.FinalizerName, len(finalizerSet), len(finalizerSet)) + namespaceFinalize.Spec.Finalizers = make([]api.FinalizerName, 0, len(finalizerSet)) for _, value := range finalizerSet.List() { namespaceFinalize.Spec.Finalizers = append(namespaceFinalize.Spec.Finalizers, api.FinalizerName(value)) } diff --git a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest.go index 75b69625e7b0..07acc72501c1 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest.go @@ -118,6 +118,13 @@ func (namespaceFinalizeStrategy) ValidateUpdate(ctx api.Context, obj, old runtim return validation.ValidateNamespaceFinalizeUpdate(obj.(*api.Namespace), old.(*api.Namespace)) } +// PrepareForUpdate clears fields that are not allowed to be set by end users on update. +func (namespaceFinalizeStrategy) PrepareForUpdate(obj, old runtime.Object) { + newNamespace := obj.(*api.Namespace) + oldNamespace := old.(*api.Namespace) + newNamespace.Status = oldNamespace.Status +} + // MatchNamespace returns a generic matcher for a given label and field selector. func MatchNamespace(label labels.Selector, field fields.Selector) generic.Matcher { return generic.MatcherFunc(func(obj runtime.Object) (bool, error) { diff --git a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest_test.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest_test.go index 9ac4d1384845..a99c5741d4da 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest_test.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/namespace/rest_test.go @@ -23,6 +23,7 @@ import ( ) func TestNamespaceStrategy(t *testing.T) { + ctx := api.NewDefaultContext() if Strategy.NamespaceScoped() { t.Errorf("Namespaces should not be namespace scoped") } @@ -30,11 +31,100 @@ func TestNamespaceStrategy(t *testing.T) { t.Errorf("Namespaces should not allow create on update") } namespace := &api.Namespace{ - ObjectMeta: api.ObjectMeta{Name: "foo"}, + ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}, Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, } Strategy.PrepareForCreate(namespace) if namespace.Status.Phase != api.NamespaceActive { t.Errorf("Namespaces do not allow setting phase on create") } + if len(namespace.Spec.Finalizers) != 1 || namespace.Spec.Finalizers[0] != api.FinalizerKubernetes { + t.Errorf("Prepare For Create should have added kubernetes finalizer") + } + errs := Strategy.Validate(ctx, namespace) + if len(errs) != 0 { + t.Errorf("Unexpected error validating %v", errs) + } + invalidNamespace := &api.Namespace{ + ObjectMeta: api.ObjectMeta{Name: "bar", ResourceVersion: "4"}, + } + // ensure we copy spec.finalizers from old to new + Strategy.PrepareForUpdate(invalidNamespace, namespace) + if len(invalidNamespace.Spec.Finalizers) != 1 || invalidNamespace.Spec.Finalizers[0] != api.FinalizerKubernetes { + t.Errorf("PrepareForUpdate should have preserved old.spec.finalizers") + } + errs = Strategy.ValidateUpdate(ctx, invalidNamespace, namespace) + if len(errs) == 0 { + t.Errorf("Expected a validation error") + } + if invalidNamespace.ResourceVersion != "4" { + t.Errorf("Incoming resource version on update should not be mutated") + } +} + +func TestNamespaceStatusStrategy(t *testing.T) { + ctx := api.NewDefaultContext() + if StatusStrategy.NamespaceScoped() { + t.Errorf("Namespaces should not be namespace scoped") + } + if StatusStrategy.AllowCreateOnUpdate() { + t.Errorf("Namespaces should not allow create on update") + } + 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"}, + Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, + } + StatusStrategy.PrepareForUpdate(namespace, oldNamespace) + if namespace.Status.Phase != api.NamespaceTerminating { + t.Errorf("Namespace status updates should allow change of phase: %v", namespace.Status.Phase) + } + if len(namespace.Spec.Finalizers) != 1 || namespace.Spec.Finalizers[0] != api.FinalizerKubernetes { + t.Errorf("PrepareForUpdate should have preserved old finalizers") + } + errs := StatusStrategy.ValidateUpdate(ctx, namespace, oldNamespace) + if len(errs) != 0 { + t.Errorf("Unexpected error %v", errs) + } + if namespace.ResourceVersion != "9" { + t.Errorf("Incoming resource version on update should not be mutated") + } +} + +func TestNamespaceFinalizeStrategy(t *testing.T) { + ctx := api.NewDefaultContext() + if FinalizeStrategy.NamespaceScoped() { + t.Errorf("Namespaces should not be namespace scoped") + } + if FinalizeStrategy.AllowCreateOnUpdate() { + t.Errorf("Namespaces should not allow create on update") + } + oldNamespace := &api.Namespace{ + ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}, + Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes", "example.com/org"}}, + Status: api.NamespaceStatus{Phase: api.NamespaceActive}, + } + namespace := &api.Namespace{ + ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "9"}, + Spec: api.NamespaceSpec{Finalizers: []api.FinalizerName{"example.com/foo"}}, + Status: api.NamespaceStatus{Phase: api.NamespaceTerminating}, + } + FinalizeStrategy.PrepareForUpdate(namespace, oldNamespace) + if namespace.Status.Phase != api.NamespaceActive { + t.Errorf("finalize updates should not allow change of phase: %v", namespace.Status.Phase) + } + if len(namespace.Spec.Finalizers) != 1 || string(namespace.Spec.Finalizers[0]) != "example.com/foo" { + t.Errorf("PrepareForUpdate should have modified finalizers") + } + errs := StatusStrategy.ValidateUpdate(ctx, namespace, oldNamespace) + if len(errs) != 0 { + t.Errorf("Unexpected error %v", errs) + } + if namespace.ResourceVersion != "9" { + t.Errorf("Incoming resource version on update should not be mutated") + } }