diff --git a/pkg/apis/servicecatalog/validation/instance.go b/pkg/apis/servicecatalog/validation/instance.go index dd5a3be20d9..ce2ed3bb87b 100644 --- a/pkg/apis/servicecatalog/validation/instance.go +++ b/pkg/apis/servicecatalog/validation/instance.go @@ -23,6 +23,8 @@ import ( sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" "github.com/kubernetes-incubator/service-catalog/pkg/controller" + scfeatures "github.com/kubernetes-incubator/service-catalog/pkg/features" + utilfeature "k8s.io/apiserver/pkg/util/feature" ) // validateServiceInstanceName is the validation function for Instance names. @@ -254,13 +256,31 @@ 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. +// 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(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList { errors := field.ErrorList{} - if old.Generation != new.Generation && old.Status.CurrentOperation != "" { - errors = append(errors, field.Forbidden(field.NewPath("spec"), "Another update for this service instance is in progress")) + + // If the OriginatingIdentityLocking feature is not set then only allow the + // same user to edit the Instance. + if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentityLocking) { + oldUID, newUID := "", "" + + if old.Spec.UserInfo != nil { + oldUID = old.Spec.UserInfo.UID + } + + if new.Spec.UserInfo != nil { + newUID = new.Spec.UserInfo.UID + } + + 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 { errors = append(errors, field.Forbidden(field.NewPath("spec").Child("clusterServicePlanRef"), "clusterServicePlanRef must not be present when clusterServicePlanExternalName is being changed")) } diff --git a/pkg/apis/servicecatalog/validation/instance_test.go b/pkg/apis/servicecatalog/validation/instance_test.go index 8897fcb848d..a8619b8cbcc 100644 --- a/pkg/apis/servicecatalog/validation/instance_test.go +++ b/pkg/apis/servicecatalog/validation/instance_test.go @@ -744,30 +744,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,6 +820,13 @@ func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) { newInstance.Generation = oldInstance.Generation } + if tc.setUser { + newInstance.Spec.UserInfo = &servicecatalog.UserInfo{ + Username: "JohnDoe", + UID: "123", + } + } + errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) if len(errs) != 0 && tc.valid { t.Errorf("%v: unexpected error: %v", tc.name, errs) diff --git a/pkg/features/features.go b/pkg/features/features.go index 1cfe93c2af0..76d42f56e44 100644 --- a/pkg/features/features.go +++ b/pkg/features/features.go @@ -58,6 +58,17 @@ const ( // owner: @luksa // alpha: v0.1.12 ResponseSchema utilfeature.Feature = "ResponseSchema" + + // OriginatingIdentityLocking controls whether we lock OSB API resources + // that are being updated while we are still processing the update request. + // Meaning, we're still talking to the Broker to see if the requested + // change will be accepted or not. If locked, then no *other* user is + // allowed to update the resource. + // This lock was added in an attempt to fulfill the requirements + // of the OSBAPI OriginatingIdentity header. + // owner: @duglin + // alpha: v0.1.12 + OriginatingIdentityLocking utilfeature.Feature = "OriginatingIdentityLocking" ) func init() { @@ -68,9 +79,10 @@ func init() { // To add a new feature, define a key for it above and add it here. The features will be // available throughout service catalog binaries. var defaultServiceCatalogFeatureGates = map[utilfeature.Feature]utilfeature.FeatureSpec{ - PodPreset: {Default: false, PreRelease: utilfeature.Alpha}, - OriginatingIdentity: {Default: false, PreRelease: utilfeature.Alpha}, - AsyncBindingOperations: {Default: false, PreRelease: utilfeature.Alpha}, - NamespacedServiceBroker: {Default: false, PreRelease: utilfeature.Alpha}, - ResponseSchema: {Default: false, PreRelease: utilfeature.Alpha}, + PodPreset: {Default: false, PreRelease: utilfeature.Alpha}, + OriginatingIdentity: {Default: false, PreRelease: utilfeature.Alpha}, + AsyncBindingOperations: {Default: false, PreRelease: utilfeature.Alpha}, + NamespacedServiceBroker: {Default: false, PreRelease: utilfeature.Alpha}, + ResponseSchema: {Default: false, PreRelease: utilfeature.Alpha}, + OriginatingIdentityLocking: {Default: true, PreRelease: utilfeature.Alpha}, }