Add flag for making instance/binding locks optional (#1917)

kubernetes-retired · Apr 13, 2018 · 92dda3d · 92dda3d
1 parent b977005
commit 92dda3d
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 15 deletions.
diff --git a/pkg/apis/servicecatalog/validation/binding.go b/pkg/apis/servicecatalog/validation/binding.go
@@ -18,10 +18,11 @@ package validation
 
 import (
 	"github.com/ghodss/yaml"
+	sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
+	scfeatures "github.com/kubernetes-incubator/service-catalog/pkg/features"
 	apivalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
-
-	sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 )
 
 // validateServiceBindingName is the validation function for ServiceBinding names.
@@ -219,9 +220,15 @@ func validateServiceBindingUpdate(binding *sc.ServiceBinding) field.ErrorList {
 // to the spec to go through.
 func internalValidateServiceBindingUpdateAllowed(new *sc.ServiceBinding, old *sc.ServiceBinding) field.ErrorList {
 	errors := field.ErrorList{}
-	if old.Generation != new.Generation && old.Status.ReconciledGeneration != old.Generation {
-		errors = append(errors, field.Forbidden(field.NewPath("spec"), "another change to the spec is in progress"))
+
+	// If the OriginatingIdentityLocking feature is set then don't allow spec updates
+	// if processing of the current generation hasn't finished yet
+	if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentityLocking) {
+		if old.Generation != new.Generation && old.Status.ReconciledGeneration != old.Generation {
+			errors = append(errors, field.Forbidden(field.NewPath("spec"), "another change to the spec is in progress"))
+		}
 	}
+
 	return errors
 }
 

diff --git a/pkg/apis/servicecatalog/validation/instance.go b/pkg/apis/servicecatalog/validation/instance.go
@@ -18,11 +18,12 @@ package validation
 
 import (
 	"github.com/ghodss/yaml"
-	apivalidation "k8s.io/apimachinery/pkg/api/validation"
-	"k8s.io/apimachinery/pkg/util/validation/field"
-
 	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"
+	apivalidation "k8s.io/apimachinery/pkg/api/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 )
 
 // validateServiceInstanceName is the validation function for Instance names.
@@ -258,9 +259,19 @@ func validateServiceInstanceUpdate(instance *sc.ServiceInstance) field.ErrorList
 // to the spec to go through.
 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 set then don't allow spec updates
+	// if processing of the current generation hasn't finished yet
+	if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentityLocking) {
+		// TODO nilebox: The condition for locking should not be based on whether
+		// there is an operation in progress. It should be based on whether controller
+		// has finished processing the current generation (i.e. either succeeded, or failed and won't retry).
+		// In other words, check for ObservedGeneration + conditions instead of CurrentOperation
+		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"))
+		}
 	}
+
 	planUpdated := old.Spec.ClusterServicePlanExternalName != new.Spec.ClusterServicePlanExternalName
 	planUpdated = planUpdated || old.Spec.ClusterServicePlanExternalID != new.Spec.ClusterServicePlanExternalID
 	planUpdated = planUpdated || old.Spec.ClusterServicePlanName != new.Spec.ClusterServicePlanName

diff --git a/pkg/features/features.go b/pkg/features/features.go
@@ -64,6 +64,12 @@ const (
 	// owner: @jberkhahn
 	// alpha: v0.1.13
 	UpdateDashboardURL utilfeature.Feature = "UpdateDashboardURL"
+
+	// OriginatingIdentityLocking controls whether we lock OSB API resources
+	// for updating while we are still processing the current spec.
+	// owner: @nilebox
+	// alpha: v0.1.14
+	OriginatingIdentityLocking utilfeature.Feature = "OriginatingIdentityLocking"
 )
 
 func init() {
@@ -74,10 +80,11 @@ 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},
-	UpdateDashboardURL:      {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},
+	UpdateDashboardURL:         {Default: false, PreRelease: utilfeature.Alpha},
+	OriginatingIdentityLocking: {Default: true, PreRelease: utilfeature.Alpha},
 }