reenable the controller default relist interval (#2125)

* reenable the controller default relist interval - dump apiserver defaults + validation - dump checks in controller for a set value * fixup unit tests due to changed behavior
kubernetes-retired · Jun 25, 2018 · ea84fa5 · ea84fa5
1 parent 1d86700
commit ea84fa5
Show file tree

Hide file tree

Showing 6 changed files with 17 additions and 47 deletions.
diff --git a/pkg/apis/servicecatalog/v1beta1/defaults.go b/pkg/apis/servicecatalog/v1beta1/defaults.go
@@ -17,9 +17,6 @@ limitations under the License.
 package v1beta1
 
 import (
-	"time"
-
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
@@ -39,10 +36,6 @@ func setCommonServiceBrokerDefaults(spec *CommonServiceBrokerSpec) {
 	if spec.RelistBehavior == "" {
 		spec.RelistBehavior = ServiceBrokerRelistBehaviorDuration
 	}
-
-	if spec.RelistBehavior == ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {
-		spec.RelistDuration = &metav1.Duration{Duration: 15 * time.Minute}
-	}
 }
 
 func SetDefaults_ServiceBinding(binding *ServiceBinding) {

diff --git a/pkg/apis/servicecatalog/v1beta1/defaults_test.go b/pkg/apis/servicecatalog/v1beta1/defaults_test.go
@@ -116,14 +116,5 @@ func TestSetDefaultClusterServiceBroker(t *testing.T) {
 				tc.name, tc.behavior, actualSpec.RelistBehavior,
 			)
 		}
-
-		if tc.duration == nil && actualSpec.RelistDuration == nil {
-			continue
-		} else if *tc.duration != *actualSpec.RelistDuration {
-			t.Errorf(
-				"%v: unexpected RelistDuration: expected %v, got %v",
-				tc.name, tc.duration, actualSpec.RelistDuration,
-			)
-		}
 	}
 }
diff --git a/pkg/apis/servicecatalog/validation/broker.go b/pkg/apis/servicecatalog/validation/broker.go
@@ -188,20 +188,6 @@ func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath *
 		)
 	}
 
-	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorDuration && spec.RelistDuration == nil {
-		commonErrs = append(
-			commonErrs,
-			field.Required(fldPath.Child("relistDuration"), "relistDuration must be set if relistBehavior is set to Duration"),
-		)
-	}
-
-	if spec.RelistBehavior == sc.ServiceBrokerRelistBehaviorManual && spec.RelistDuration != nil {
-		commonErrs = append(
-			commonErrs,
-			field.Required(fldPath.Child("relistDuration"), "relistDuration must not be set if relistBehavior is set to Manual"),
-		)
-	}
-
 	if spec.RelistRequests < 0 {
 		commonErrs = append(
 			commonErrs,

diff --git a/pkg/apis/servicecatalog/validation/broker_test.go b/pkg/apis/servicecatalog/validation/broker_test.go
@@ -259,7 +259,7 @@ func TestValidateClusterServiceBroker(t *testing.T) {
 			valid: true,
 		},
 		{
-			name: "invalid clusterservicebroker - manual behavior with RelistDuration",
+			name: "valid clusterservicebroker - manual behavior with RelistDuration",
 			broker: &servicecatalog.ClusterServiceBroker{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "test-clusterservicebroker",
@@ -272,7 +272,7 @@ func TestValidateClusterServiceBroker(t *testing.T) {
 					},
 				},
 			},
-			valid: false,
+			valid: true,
 		},
 		{
 			name: "valid clusterservicebroker - manual behavior without RelistDuration",
@@ -291,7 +291,7 @@ func TestValidateClusterServiceBroker(t *testing.T) {
 			valid: true,
 		},
 		{
-			name: "invalid clusterservicebroker - duration behavior without duration",
+			name: "valid clusterservicebroker - duration behavior defaulting to controller provided value",
 			broker: &servicecatalog.ClusterServiceBroker{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "test-clusterservicebroker",
@@ -304,7 +304,7 @@ func TestValidateClusterServiceBroker(t *testing.T) {
 					},
 				},
 			},
-			valid: false,
+			valid: true,
 		},
 		{
 			name: "invalid clusterservicebroker - relistBehavior is invalid",
@@ -825,7 +825,7 @@ func TestValidateServiceBroker(t *testing.T) {
 			valid: true,
 		},
 		{
-			name: "invalid servicebroker - manual behavior with RelistDuration",
+			name: "valid servicebroker - manual behavior with RelistDuration",
 			broker: &servicecatalog.ServiceBroker{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-clusterservicebroker",
@@ -839,7 +839,7 @@ func TestValidateServiceBroker(t *testing.T) {
 					},
 				},
 			},
-			valid: false,
+			valid: true,
 		},
 		{
 			name: "valid servicebroker - manual behavior without RelistDuration",
@@ -859,7 +859,7 @@ func TestValidateServiceBroker(t *testing.T) {
 			valid: true,
 		},
 		{
-			name: "invalid servicebroker - duration behavior without duration",
+			name: "valid servicebroker - duration behavior defaulting to controller provided value",
 			broker: &servicecatalog.ServiceBroker{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-clusterservicebroker",
@@ -873,7 +873,7 @@ func TestValidateServiceBroker(t *testing.T) {
 					},
 				},
 			},
-			valid: false,
+			valid: true,
 		},
 		{
 			name: "invalid servicebroker - relistBehavior is invalid",

diff --git a/pkg/controller/controller_clusterservicebroker.go b/pkg/controller/controller_clusterservicebroker.go
@@ -93,7 +93,7 @@ func (c *controller) clusterServiceBrokerDelete(obj interface{}) {
 // returns true unless the broker has a ready condition with status true and
 // the controller's broker relist interval has not elapsed since the broker's
 // ready condition became true, or if the broker's RelistBehavior is set to Manual.
-func shouldReconcileClusterServiceBroker(broker *v1beta1.ClusterServiceBroker, now time.Time) bool {
+func shouldReconcileClusterServiceBroker(broker *v1beta1.ClusterServiceBroker, now time.Time, defaultRelistInterval time.Duration) bool {
 	pcb := pretty.NewClusterServiceBrokerContextBuilder(broker)
 	if broker.Status.ReconciledGeneration != broker.Generation {
 		// If the spec has changed, we should reconcile the broker.
@@ -122,14 +122,14 @@ func shouldReconcileClusterServiceBroker(broker *v1beta1.ClusterServiceBroker, n
 					return false
 				}
 
-				if broker.Spec.RelistDuration == nil {
-					glog.Error(pcb.Message("Unable to process because RelistBehavior is set to Duration with a nil RelistDuration value"))
-					return false
-				}
-
 				// By default, the broker should relist if it has been longer than the
 				// RelistDuration since the last time we fetched the Catalog
-				duration := broker.Spec.RelistDuration.Duration
+				duration := defaultRelistInterval
+
+				if broker.Spec.RelistDuration != nil {
+					duration = broker.Spec.RelistDuration.Duration
+				}
+
 				intervalPassed := true
 				if broker.Status.LastCatalogRetrievalTime != nil {
 					intervalPassed = now.After(broker.Status.LastCatalogRetrievalTime.Time.Add(duration))
@@ -176,7 +176,7 @@ func (c *controller) reconcileClusterServiceBroker(broker *v1beta1.ClusterServic
 	// set to Manual, do not reconcile it.
 	// * If the broker's ready condition is true and the relist interval has not
 	// elapsed, do not reconcile it.
-	if !shouldReconcileClusterServiceBroker(broker, time.Now()) {
+	if !shouldReconcileClusterServiceBroker(broker, time.Now(), c.brokerRelistInterval) {
 		return nil
 	}
 

diff --git a/pkg/controller/controller_clusterservicebroker_test.go b/pkg/controller/controller_clusterservicebroker_test.go
@@ -209,7 +209,7 @@ func TestShouldReconcileClusterServiceBroker(t *testing.T) {
 			t.Logf("broker.Spec.RelistDuration set to nil")
 		}
 
-		actual := shouldReconcileClusterServiceBroker(tc.broker, tc.now)
+		actual := shouldReconcileClusterServiceBroker(tc.broker, tc.now, 24*time.Hour)
 
 		if e, a := tc.reconcile, actual; e != a {
 			t.Errorf("%v: unexpected result: %s", tc.name, expectedGot(e, a))