From a323ae31f1ff2ba1a20c106108fe04bad96149d4 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Sat, 24 Mar 2018 08:34:32 -0700 Subject: [PATCH] Allow the same user to edit an instance Signed-off-by: Doug Davis --- .../servicecatalog/validation/instance.go | 28 +++++++++++++++---- .../validation/instance_test.go | 27 ++++++++++++++++-- .../servicecatalog/instance/strategy.go | 2 +- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/pkg/apis/servicecatalog/validation/instance.go b/pkg/apis/servicecatalog/validation/instance.go index dd5a3be20d9..e6234d8f270 100644 --- a/pkg/apis/servicecatalog/validation/instance.go +++ b/pkg/apis/servicecatalog/validation/instance.go @@ -23,6 +23,7 @@ import ( sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" "github.com/kubernetes-incubator/service-catalog/pkg/controller" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" ) // validateServiceInstanceName is the validation function for Instance names. @@ -254,11 +255,26 @@ func validateServiceInstanceUpdate(instance *sc.ServiceInstance) field.ErrorList } // internalValidateServiceInstanceUpdateAllowed ensures there is not a -// pending update on-going with the spec of the instance before allowing an update -// to the spec to go through. -func internalValidateServiceInstanceUpdateAllowed(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { +// pending update on-going with the spec of the instance before allowing an +// update to the spec to go through unless its the same user who made the +// original update. +func internalValidateServiceInstanceUpdateAllowed(ctx genericapirequest.Context, new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { errors := field.ErrorList{} - if old.Generation != new.Generation && old.Status.CurrentOperation != "" { + + oldUID := "" + newUID := "" + + if old.Spec.UserInfo != nil { + oldUID = old.Spec.UserInfo.UID + } + + if ctx != nil { + if user, _ := genericapirequest.UserFrom(ctx); user != nil { + newUID = user.GetUID() + } + } + + if old.Generation != new.Generation && old.Status.CurrentOperation != "" && oldUID != newUID { errors = append(errors, field.Forbidden(field.NewPath("spec"), "Another update for this service instance is in progress")) } if old.Spec.ClusterServicePlanExternalName != new.Spec.ClusterServicePlanExternalName && new.Spec.ClusterServicePlanRef != nil { @@ -268,13 +284,13 @@ func internalValidateServiceInstanceUpdateAllowed(new *sc.ServiceInstance, old * } // ValidateServiceInstanceUpdate validates a change to the Instance's spec. -func ValidateServiceInstanceUpdate(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { +func ValidateServiceInstanceUpdate(ctx genericapirequest.Context, new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { allErrs := field.ErrorList{} specFieldPath := field.NewPath("spec") allErrs = append(allErrs, validatePlanReferenceUpdate(&new.Spec.PlanReference, &old.Spec.PlanReference, specFieldPath)...) - allErrs = append(allErrs, internalValidateServiceInstanceUpdateAllowed(new, old)...) + allErrs = append(allErrs, internalValidateServiceInstanceUpdateAllowed(ctx, new, old)...) allErrs = append(allErrs, internalValidateServiceInstance(new, false)...) allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ClusterServiceClassExternalName, old.Spec.ClusterServiceClassExternalName, specFieldPath.Child("clusterServiceClassExternalName"))...) diff --git a/pkg/apis/servicecatalog/validation/instance_test.go b/pkg/apis/servicecatalog/validation/instance_test.go index 8897fcb848d..f00ce80a6c5 100644 --- a/pkg/apis/servicecatalog/validation/instance_test.go +++ b/pkg/apis/servicecatalog/validation/instance_test.go @@ -25,6 +25,8 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" + "k8s.io/apiserver/pkg/authentication/user" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" ) const ( @@ -744,30 +746,42 @@ func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) { name string specChange bool onGoingOperation bool + setUser bool valid bool }{ { name: "spec change when no on-going operation", specChange: true, onGoingOperation: false, + setUser: false, valid: true, }, { - name: "spec change when on-going operation", + name: "spec change when on-going operation, same user", specChange: true, onGoingOperation: true, + setUser: false, + valid: true, + }, + { + name: "spec change when on-going operation, diff user", + specChange: true, + onGoingOperation: true, + setUser: true, valid: false, }, { name: "meta change when no on-going operation", specChange: false, onGoingOperation: false, + setUser: false, valid: true, }, { name: "meta change when on-going operation", specChange: false, onGoingOperation: true, + setUser: false, valid: true, }, } @@ -808,7 +822,14 @@ func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) { newInstance.Generation = oldInstance.Generation } - errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) + user := user.DefaultInfo{} + if tc.setUser { + user.Name = "JohnDoe" + user.UID = "123" + } + ctx := genericapirequest.WithUser(genericapirequest.NewContext(), &user) + + errs := internalValidateServiceInstanceUpdateAllowed(ctx, newInstance, oldInstance) if len(errs) != 0 && tc.valid { t.Errorf("%v: unexpected error: %v", tc.name, errs) continue @@ -880,7 +901,7 @@ func TestInternalValidateServiceInstanceUpdateAllowedForPlanChange(t *testing.T) }, } - errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) + errs := internalValidateServiceInstanceUpdateAllowed(nil, newInstance, oldInstance) if len(errs) != 0 && tc.valid { t.Errorf("%v: unexpected error: %v", tc.name, errs) continue diff --git a/pkg/registry/servicecatalog/instance/strategy.go b/pkg/registry/servicecatalog/instance/strategy.go index 06f594daba0..f0607fc03b8 100644 --- a/pkg/registry/servicecatalog/instance/strategy.go +++ b/pkg/registry/servicecatalog/instance/strategy.go @@ -186,7 +186,7 @@ func (instanceRESTStrategy) ValidateUpdate(ctx genericapirequest.Context, new, o glog.Fatal("received a non-instance object to validate from") } - return scv.ValidateServiceInstanceUpdate(newServiceInstance, oldServiceInstance) + return scv.ValidateServiceInstanceUpdate(ctx, newServiceInstance, oldServiceInstance) } // CheckGracefulDelete sets the UserInfo on the resource to that of the user that