Make timeout of any request to the broker globally configurable

We find the delay in creating instance binding varies. The current default is to wait one minute, otherwise the creation is considered a failure. So I do this patch to solve this problem. And I think this delay is configured for different usage scenarios or brokers. For example, when the maximum response time of my service is 1 minute, we may set it to be 60, While yours is 3 minutes, and then you should set it to be 180.
kubernetes-retired · Jun 21, 2019 · 840e137 · 840e137
1 parent 48e6805
commit 840e137
Show file tree

Hide file tree

Showing 13 changed files with 41 additions and 3 deletions.
diff --git a/charts/catalog/README.md b/charts/catalog/README.md
@@ -80,6 +80,7 @@ chart and their default values.
 | `controllerManager.healthcheck.enabled` | Enable readiness and liveliness probes | `true` |
 | `controllerManager.verbosity` | Log level; valid values are in the range 0 - 10 | `10` |
 | `controllerManager.resyncInterval` | How often the controller should resync informers; duration format (`20m`, `1h`, etc) | `5m` |
+| `controllerManager.osbApiRequestTimeout` | The maximum amount of timeout to any request to the broker; duration format (`60s`, `3m`, etc) | `60s` |
 | `controllerManager.brokerRelistInterval` | How often the controller should relist the catalogs of ready brokers; duration format (`20m`, `1h`, etc) | `24h` |
 | `controllerManager.brokerRelistIntervalActivated` | Whether or not the controller supports a --broker-relist-interval flag. If this is set to true, brokerRelistInterval will be used as the value for that flag. | `true` |
 | `controllerManager.profiling.disabled` | Disable profiling via web interface host:port/debug/pprof/ | `false` |

diff --git a/charts/catalog/templates/controller-manager-deployment.yaml b/charts/catalog/templates/controller-manager-deployment.yaml
@@ -80,6 +80,10 @@ spec:
         - --operation-polling-maximum-backoff-duration
         - {{ .Values.controllerManager.operationPollingMaximumBackoffDuration }}
         {{- end }}
+        {{ if .Values.controllerManager.osbApiRequestTimeout -}}
+        - --osb-api-request-timeout
+        - {{ .Values.controllerManager.osbApiRequestTimeout }}
+        {{- end }}
         - --feature-gates
         - OriginatingIdentity={{.Values.originatingIdentityEnabled}}
         - --feature-gates

diff --git a/charts/catalog/values.yaml b/charts/catalog/values.yaml
@@ -149,6 +149,8 @@ controllerManager:
   brokerRelistIntervalActivated: true
    # The maximum amount of time to back-off while polling an OSB API operation; format is a duration (`20m`, `1h`, etc)
   operationPollingMaximumBackoffDuration: 20m
+  # The maximum amount of timeout to any request to the broker; format is a duration (`60s`, `3m`, etc)
+  osbApiRequestTimeout: 60s
   # enables profiling via web interface host:port/debug/pprof/
   profiling:
     # Disable profiling via web interface host:port/debug/pprof/

diff --git a/cmd/controller-manager/app/controller_manager.go b/cmd/controller-manager/app/controller_manager.go
@@ -402,6 +402,7 @@ func StartControllers(s *options.ControllerManagerServer,
 		s.OperationPollingMaximumBackoffDuration,
 		s.ClusterIDConfigMapName,
 		s.ClusterIDConfigMapNamespace,
+		s.OSBAPITimeOut,
 	)
 	if err != nil {
 		return err

diff --git a/cmd/controller-manager/app/options/options.go b/cmd/controller-manager/app/options/options.go
@@ -60,6 +60,7 @@ const (
 	defaultLeaderElectionNamespace                = "kube-system"
 	defaultReconciliationRetryDuration            = 7 * 24 * time.Hour
 	defaultOperationPollingMaximumBackoffDuration = 20 * time.Minute
+	defaultOSBAPITimeOut                          = 60 * time.Second
 )
 
 var defaultOSBAPIPreferredVersion = osb.LatestAPIVersion().HeaderValue()
@@ -78,6 +79,7 @@ func NewControllerManagerServer() *ControllerManagerServer {
 			ServiceBrokerRelistInterval:            defaultServiceBrokerRelistInterval,
 			OSBAPIContextProfile:                   defaultOSBAPIContextProfile,
 			OSBAPIPreferredVersion:                 defaultOSBAPIPreferredVersion,
+			OSBAPITimeOut:                          defaultOSBAPITimeOut,
 			ConcurrentSyncs:                        defaultConcurrentSyncs,
 			LeaderElection:                         leaderelectionconfig.DefaultLeaderElectionConfiguration(),
 			LeaderElectionNamespace:                defaultLeaderElectionNamespace,
@@ -119,6 +121,7 @@ func (s *ControllerManagerServer) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&s.LeaderElectionNamespace, "leader-election-namespace", s.LeaderElectionNamespace, "Namespace to use for leader election lock")
 	fs.DurationVar(&s.ReconciliationRetryDuration, "reconciliation-retry-duration", s.ReconciliationRetryDuration, "The maximum amount of time to retry reconciliations on a resource before failing")
 	fs.DurationVar(&s.OperationPollingMaximumBackoffDuration, "operation-polling-maximum-backoff-duration", s.OperationPollingMaximumBackoffDuration, "The maximum amount of time to back-off while polling an OSB API operation")
+	fs.DurationVar(&s.OSBAPITimeOut, "osb-api-request-timeout", s.OSBAPITimeOut, "The maximum amount of timeout to any request to the broker.")
 	s.SecureServingOptions.AddFlags(fs)
 	utilfeature.DefaultMutableFeatureGate.AddFlag(fs)
 	fs.StringVar(&s.ClusterIDConfigMapName, "cluster-id-configmap-name", controller.DefaultClusterIDConfigMapName, "k8s name for clusterid configmap")

diff --git a/pkg/apis/componentconfig/types.go b/pkg/apis/componentconfig/types.go
@@ -77,6 +77,9 @@ type ControllerManagerConfiguration struct {
 	OSBAPIContextProfile   bool
 	OSBAPIPreferredVersion string
 
+	// OSBAPITimeOut the length of the timeout of any request to the broker.
+	OSBAPITimeOut time.Duration
+
 	// ConcurrentSyncs is the number of resources, per resource type,
 	// that are allowed to sync concurrently. Larger number = more responsive
 	// SC operations, but more CPU (and network) load.

diff --git a/pkg/controller/case_test.go b/pkg/controller/case_test.go
@@ -131,6 +131,7 @@ func newControllerTest(t *testing.T) *controllerTest {
 		7*24*time.Hour,
 		"DefaultClusterIDConfigMapName",
 		"DefaultClusterIDConfigMapNamespace",
+		60*time.Second,
 	)
 	if err != nil {
 		t.Fatal(err)

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -95,13 +95,15 @@ func NewController(
 	operationPollingMaximumBackoffDuration time.Duration,
 	clusterIDConfigMapName string,
 	clusterIDConfigMapNamespace string,
+	osbAPITimeOut time.Duration,
 ) (Controller, error) {
 	controller := &controller{
 		kubeClient:                  kubeClient,
 		secretLister:                secretInformer.Lister(),
 		serviceCatalogClient:        serviceCatalogClient,
 		brokerRelistInterval:        brokerRelistInterval,
 		OSBAPIPreferredVersion:      osbAPIPreferredVersion,
+		OSBAPITimeOut:               osbAPITimeOut,
 		recorder:                    recorder,
 		reconciliationRetryDuration: reconciliationRetryDuration,
 		clusterServiceBrokerQueue:   workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(pollingStartInterval, operationPollingMaximumBackoffDuration), "cluster-service-broker"),
@@ -205,6 +207,7 @@ type controller struct {
 	secretLister                v1.SecretLister
 	brokerRelistInterval        time.Duration
 	OSBAPIPreferredVersion      string
+	OSBAPITimeOut               time.Duration
 	recorder                    record.EventRecorder
 	reconciliationRetryDuration time.Duration
 	clusterServiceBrokerQueue   workqueue.RateLimitingInterface
@@ -1271,14 +1274,15 @@ func isServiceInstanceOrphanMitigation(instance *v1beta1.ServiceInstance) bool {
 
 // NewClientConfigurationForBroker creates a new ClientConfiguration for connecting
 // to the specified Broker
-func NewClientConfigurationForBroker(meta metav1.ObjectMeta, commonSpec *v1beta1.CommonServiceBrokerSpec, authConfig *osb.AuthConfig) *osb.ClientConfiguration {
+func NewClientConfigurationForBroker(meta metav1.ObjectMeta, commonSpec *v1beta1.CommonServiceBrokerSpec, authConfig *osb.AuthConfig, osbAPITimeOut time.Duration) *osb.ClientConfiguration {
 	clientConfig := osb.DefaultClientConfiguration()
 	clientConfig.Name = meta.Name
 	clientConfig.URL = commonSpec.URL
 	clientConfig.AuthConfig = authConfig
 	clientConfig.EnableAlphaFeatures = true
 	clientConfig.Insecure = commonSpec.InsecureSkipTLSVerify
 	clientConfig.CAData = commonSpec.CABundle
+	clientConfig.TimeoutSeconds = int(osbAPITimeOut.Seconds())
 	return clientConfig
 }
 

diff --git a/pkg/controller/controller_clusterservicebroker.go b/pkg/controller/controller_clusterservicebroker.go
@@ -137,7 +137,7 @@ func (c *controller) clusterServiceBrokerClient(broker *v1beta1.ClusterServiceBr
 		}
 		return nil, err
 	}
-	clientConfig := NewClientConfigurationForBroker(broker.ObjectMeta, &broker.Spec.CommonServiceBrokerSpec, authConfig)
+	clientConfig := NewClientConfigurationForBroker(broker.ObjectMeta, &broker.Spec.CommonServiceBrokerSpec, authConfig, c.OSBAPITimeOut)
 	brokerClient, err := c.brokerClientManager.UpdateBrokerClient(NewClusterServiceBrokerKey(broker.Name), clientConfig)
 	if err != nil {
 		s := fmt.Sprintf("Error creating client for broker %q: %s", broker.Name, err)

diff --git a/pkg/controller/controller_clusterservicebroker_test.go b/pkg/controller/controller_clusterservicebroker_test.go
@@ -232,6 +232,22 @@ func TestShouldReconcileClusterServiceBroker(t *testing.T) {
 	}
 }
 
+// TestReconcileClusterServiceBrokerSetOSBTimeOut
+// verifies that timeout of any request to the
+// broker takes effect.
+func TestReconcileClusterServiceBrokerSetOSBTimeOut(t *testing.T) {
+	const testOSBTimeOut = time.Second * 80
+	_, _, _, testController, _ := newTestController(t, getTestCatalogConfig())
+	testController.OSBAPITimeOut = testOSBTimeOut
+	if err := reconcileClusterServiceBroker(t, testController, getTestClusterServiceBroker()); err == nil {
+		t.Fatalf("This should not fail : %v", err)
+	}
+	createdClient := testController.brokerClientManager.clients[NewClusterServiceBrokerKey(getTestClusterServiceBroker().Name)]
+	if createdClient.clientConfig.TimeoutSeconds != int(testController.OSBAPITimeOut.Seconds()) {
+		t.Errorf("Unexpected OSBTimeOut: got %v, expeted %v", createdClient.clientConfig.TimeoutSeconds, int(testController.OSBAPITimeOut.Seconds()))
+	}
+}
+
 // TestReconcileClusterServiceBrokerExistingServiceClassAndServicePlan
 // verifies a simple, successful run of reconcileClusterServiceBroker() when a
 // ClusterServiceClass and plan already exist.  This test will cause

diff --git a/pkg/controller/controller_servicebroker.go b/pkg/controller/controller_servicebroker.go
@@ -127,7 +127,7 @@ func (c *controller) serviceBrokerClient(broker *v1beta1.ServiceBroker) (osb.Cli
 		return nil, err
 	}
 
-	clientConfig := NewClientConfigurationForBroker(broker.ObjectMeta, &broker.Spec.CommonServiceBrokerSpec, authConfig)
+	clientConfig := NewClientConfigurationForBroker(broker.ObjectMeta, &broker.Spec.CommonServiceBrokerSpec, authConfig, c.OSBAPITimeOut)
 
 	brokerClient, err := c.brokerClientManager.UpdateBrokerClient(NewServiceBrokerKey(broker.Namespace, broker.Name), clientConfig)
 	if err != nil {

diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
@@ -2354,6 +2354,7 @@ func newTestController(t *testing.T, config fakeosb.FakeClientConfiguration) (
 		7*24*time.Hour,
 		DefaultClusterIDConfigMapName,
 		DefaultClusterIDConfigMapNamespace,
+		60*time.Second,
 	)
 
 	if err != nil {

diff --git a/test/integration/controller_test.go b/test/integration/controller_test.go
@@ -791,6 +791,7 @@ func newControllerTestTestController(ct *controllerTest) (
 		7*24*time.Hour,
 		controller.DefaultClusterIDConfigMapName,
 		controller.DefaultClusterIDConfigMapNamespace,
+		60*time.Second,
 	)
 	t.Log("controller start")
 	if err != nil {
@@ -957,6 +958,7 @@ func newTestController(t *testing.T) (
 		7*24*time.Hour,
 		controller.DefaultClusterIDConfigMapName,
 		controller.DefaultClusterIDConfigMapNamespace,
+		60*time.Second,
 	)
 	t.Log("controller start")
 	if err != nil {