feat: adds the --instance-timeout flag to control timeout value

integrationtests/upgrade_test.go

@@ -305,4 +305,36 @@ var _ = Describe("upgrade", func() {
 			Expect(second.Sub(first)).To(BeNumerically("~", retryInterval, accuracy))
 		})
 	})
+
+	Context("instance timeout", func() {
+		BeforeEach(func() {
+			capi.AddBroker(
+				fakecapi.ServiceBroker{Name: brokerName},
+				fakecapi.WithServiceOffering(
+					fakecapi.ServiceOffering{Name: "service-offering-1"},
+					fakecapi.WithServicePlan(
+						fakecapi.ServicePlan{Name: "service-plan1", Version: "1.2.3"},
+						// Instance takes 5 seconds to update, which will exceed our short timeout
+						fakecapi.WithServiceInstances(fakecapi.ServiceInstance{UpgradeAvailable: true, Version: "1.2.2", UpdateTime: 5 * time.Second}),
+					),
+				),
+			)
+		})
+
+		It("respects the -instance-timeout flag", func() {
+			// Use a short timeout (1 second) that will be exceeded by the 5 second update time
+			session := cf("upgrade-all-services", brokerName, "--instance-polling-interval", "100ms", "--instance-timeout", "1s")
+			Eventually(session).WithTimeout(time.Minute).Should(Exit(1))
+
+			Expect(session.Out).To(Say("error upgrade request timeout"))
+		})
+
+		It("succeeds when timeout is long enough", func() {
+			// Use a timeout longer than the update time
+			session := cf("upgrade-all-services", brokerName, "--instance-polling-interval", "100ms", "--instance-timeout", "10s")
+			Eventually(session).WithTimeout(time.Minute).Should(Exit(0))
+
+			Expect(session.Out).To(Say("successfully upgraded 1 instances"))
+		})
+	})
 })

internal/ccapi/ccapi.go

@@ -8,11 +8,13 @@ import (
 type CCAPI struct {
 	requester       requester.Requester
 	pollingInterval time.Duration
+	instanceTimeout time.Duration
 }
 
-func NewCCAPI(req requester.Requester, pollingInterval time.Duration) CCAPI {
+func NewCCAPI(req requester.Requester, pollingInterval time.Duration, instanceTimeout time.Duration) CCAPI {
 	return CCAPI{
 		requester:       req,
 		pollingInterval: pollingInterval,
+		instanceTimeout: instanceTimeout,
 	}
 }

internal/ccapi/service_instances_test.go

@@ -24,7 +24,7 @@ var _ = Describe("GetServiceInstances", func() {
 		fakeServer = ghttp.NewServer()
 		DeferCleanup(fakeServer.Close)
 		req = requester.NewRequester(fakeServer.URL(), "fake-token", false)
-		ccapiClient = ccapi.NewCCAPI(req, time.Millisecond)
+		ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, 10*time.Minute)
 	})
 
 	When("service instances exist in the given plans", func() {

internal/ccapi/service_plans_test.go

@@ -25,7 +25,7 @@ var _ = Describe("GetServicePlans", func() {
 		fakeServer = ghttp.NewServer()
 		DeferCleanup(fakeServer.Close)
 		req = requester.NewRequester(fakeServer.URL(), "fake-token", false)
-		ccapiClient = ccapi.NewCCAPI(req, time.Millisecond)
+		ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, 10*time.Minute)
 	})
 
 	When("Given a valid brokername", func() {

internal/ccapi/upgrade_instance.go

@@ -17,7 +17,7 @@ func (c CCAPI) UpgradeServiceInstance(guid, miVersion string) error {
 		return fmt.Errorf("upgrade request error: %s", err)
 	}
 
-	for timeout := time.After(time.Minute * 10); ; {
+	for timeout := time.After(c.instanceTimeout); ; {
 		select {
 		case <-timeout:
 			return fmt.Errorf("error upgrade request timeout")

internal/ccapi/upgrade_instance_test.go

@@ -87,7 +87,7 @@ var _ = Describe("UpgradeServiceInstance", func() {
 		fakeServer = ghttp.NewServer()
 		DeferCleanup(fakeServer.Close)
 		req = requester.NewRequester(fakeServer.URL(), "fake-token", false)
-		ccapiClient = ccapi.NewCCAPI(req, time.Millisecond)
+		ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, 10*time.Minute)
 	})
 
 	When("given an upgradeable instance", func() {
@@ -217,7 +217,7 @@ var _ = Describe("UpgradeServiceInstance", func() {
 				),
 			)
 
-			ccapiClient = ccapi.NewCCAPI(req, interval)
+			ccapiClient = ccapi.NewCCAPI(req, interval, 10*time.Minute)
 
 			const accuracy = 25 * time.Millisecond
 			start := time.Now()
@@ -229,4 +229,67 @@ var _ = Describe("UpgradeServiceInstance", func() {
 		// big enough that we can see a change of behavior from above, but small enough that tests are still snappy
 		Entry("moderate interval", 100*time.Millisecond),
 	)
+
+	When("the upgrade times out", func() {
+		BeforeEach(func() {
+			fakeServer.AppendHandlers(
+				ghttp.CombineHandlers(
+					ghttp.VerifyHeaderKV("Authorization", "fake-token"),
+					ghttp.VerifyRequest("PATCH", "/v3/service_instances/test-guid"),
+					ghttp.VerifyBody([]byte(`{"maintenance_info":{"version":"test-mi-version"}}`)),
+					ghttp.RespondWith(http.StatusAccepted, ``, nil),
+				),
+			)
+			// Keep returning "in progress" indefinitely
+			fakeServer.RouteToHandler("GET", "/v3/service_instances/test-guid",
+				ghttp.CombineHandlers(
+					ghttp.VerifyHeaderKV("Authorization", "fake-token"),
+					ghttp.RespondWith(http.StatusOK, instanceUpdatingResponse, nil),
+				),
+			)
+		})
+
+		It("returns a timeout error", func() {
+			// Use a very short timeout for the test
+			const timeout = 10 * time.Millisecond
+			ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, timeout)
+
+			err := ccapiClient.UpgradeServiceInstance("test-guid", "test-mi-version")
+			Expect(err).To(MatchError("error upgrade request timeout"))
+		})
+	})
+
+	When("a custom timeout is configured", func() {
+		BeforeEach(func() {
+			fakeServer.AppendHandlers(
+				ghttp.CombineHandlers(
+					ghttp.VerifyHeaderKV("Authorization", "fake-token"),
+					ghttp.VerifyRequest("PATCH", "/v3/service_instances/test-guid"),
+					ghttp.VerifyBody([]byte(`{"maintenance_info":{"version":"test-mi-version"}}`)),
+					ghttp.RespondWith(http.StatusAccepted, ``, nil),
+				),
+			)
+			// Keep returning "in progress" indefinitely
+			fakeServer.RouteToHandler("GET", "/v3/service_instances/test-guid",
+				ghttp.CombineHandlers(
+					ghttp.VerifyHeaderKV("Authorization", "fake-token"),
+					ghttp.RespondWith(http.StatusOK, instanceUpdatingResponse, nil),
+				),
+			)
+		})
+
+		It("respects the configured timeout", func() {
+			// Use a custom timeout
+			const customTimeout = 50 * time.Millisecond
+			ccapiClient = ccapi.NewCCAPI(req, time.Millisecond, customTimeout)
+
+			const accuracy = 25 * time.Millisecond
+			start := time.Now()
+			err := ccapiClient.UpgradeServiceInstance("test-guid", "test-mi-version")
+			duration := time.Since(start)
+
+			Expect(err).To(MatchError("error upgrade request timeout"))
+			Expect(duration).To(BeNumerically("~", customTimeout, accuracy))
+		})
+	})
 })

internal/config/config.go

@@ -29,6 +29,7 @@ type Config struct {
 	RetryInterval           time.Duration
 	IgnoreInstanceErrors    bool
 	InstancePollingInterval time.Duration
+	InstanceTimeout         time.Duration
 }
 
 // ParseConfig combines and validates data from the command line and CLIConnection object
@@ -54,6 +55,7 @@ func ParseConfig(conn CLIConnection, args []string) (Config, error) {
 	flagSet.DurationVar(&cfg.RetryInterval, retryIntervalFlag, retryIntervalDefault, retryIntervalDescription)
 	flagSet.BoolVar(&cfg.IgnoreInstanceErrors, ignoreInstanceErrorsFlag, ignoreInstanceErrorsDefault, ignoreInstanceErrorsDescription)
 	flagSet.DurationVar(&cfg.InstancePollingInterval, instancePollingIntervalFlag, instancePollingIntervalDefault, instancePollingIntervalDescription)
+	flagSet.DurationVar(&cfg.InstanceTimeout, instanceTimeoutFlag, instanceTimeoutDefault, instanceTimeoutDescription)
 
 	// This ranges over a chain of functions, each of which performs a single action and may return an error.
 	// The chain breaks at the first error received. It arguably reads better than repetitive error handling logic.
@@ -82,6 +84,7 @@ func ParseConfig(conn CLIConnection, args []string) (Config, error) {
 		func() error { return validateAttempts(cfg.Attempts) },
 		func() error { return validateRetryInterval(cfg.RetryInterval) },
 		func() error { return validateInstancePollingInterval(cfg.InstancePollingInterval) },
+		func() error { return validateInstanceTimeout(cfg.InstanceTimeout) },
 	} {
 		if err := s(); err != nil {
 			return Config{}, err

internal/config/config_test.go

@@ -3,6 +3,7 @@ package config_test
 import (
 	"fmt"
 	"strings"
+	"time"
 	"upgrade-all-services-cli-plugin/internal/config"
 	"upgrade-all-services-cli-plugin/internal/config/configfakes"
 
@@ -525,4 +526,44 @@ var _ = Describe("Config", func() {
 			})
 		})
 	})
+
+	Describe("-instance-timeout", func() {
+		Context("default value", func() {
+			It("is set to 10 minutes", func() {
+				Expect(cfgErr).NotTo(HaveOccurred())
+				Expect(cfg.InstanceTimeout).To(Equal(10 * time.Minute))
+			})
+		})
+
+		Context("custom value", func() {
+			BeforeEach(func() {
+				fakeArgs = append(fakeArgs, "-instance-timeout", "5m")
+			})
+
+			It("uses the custom value", func() {
+				Expect(cfgErr).NotTo(HaveOccurred())
+				Expect(cfg.InstanceTimeout).To(Equal(5 * time.Minute))
+			})
+		})
+
+		Context("too low", func() {
+			BeforeEach(func() {
+				fakeArgs = append(fakeArgs, "-instance-timeout", "500ms")
+			})
+
+			It("returns an error", func() {
+				Expect(cfgErr).To(MatchError(`instance timeout must be greater or equal to 1s`))
+			})
+		})
+
+		Context("too high", func() {
+			BeforeEach(func() {
+				fakeArgs = append(fakeArgs, "-instance-timeout", "25h")
+			})
+
+			It("returns an error", func() {
+				Expect(cfgErr).To(MatchError(`instance timeout must be less than or equal to 24h0m0s`))
+			})
+		})
+	})
 })

internal/config/flags.go

@@ -59,4 +59,10 @@ const (
 	instancePollingIntervalDescription = "polling interval for service instances during the upgrade process. Default is 10s"
 	instancePollingIntervalMinimum     = time.Millisecond
 	instancePollingIntervalMaximum     = time.Minute
+
+	instanceTimeoutDefault     = 10 * time.Minute
+	instanceTimeoutFlag        = "instance-timeout"
+	instanceTimeoutDescription = "timeout for service instance upgrade operations. Default is 10m"
+	instanceTimeoutMinimum     = time.Second
+	instanceTimeoutMaximum     = 24 * time.Hour
 )

internal/config/usage.go

@@ -22,6 +22,7 @@ func UsageOptions() map[string]string {
 		attemptsFlag:                attemptsDescription,
 		retryIntervalFlag:           retryIntervalDescription,
 		instancePollingIntervalFlag: instancePollingIntervalDescription,
+		instanceTimeoutFlag:         instanceTimeoutDescription,
 		ignoreInstanceErrorsFlag:    ignoreInstanceErrorsDescription,
 	}
 }

internal/config/validations.go

@@ -135,3 +135,14 @@ func validateInstancePollingInterval(interval time.Duration) error {
 		return nil
 	}
 }
+
+func validateInstanceTimeout(timeout time.Duration) error {
+	switch {
+	case timeout > instanceTimeoutMaximum:
+		return fmt.Errorf("instance timeout must be less than or equal to %s", instanceTimeoutMaximum)
+	case timeout < instanceTimeoutMinimum:
+		return fmt.Errorf("instance timeout must be greater or equal to %s", instanceTimeoutMinimum)
+	default:
+		return nil
+	}
+}

upgrade_all.go

@@ -39,7 +39,7 @@ func upgradeAllServices(cliConnection plugin.CliConnection, args []string) int {
 		reqr.Logger = logr
 	}
 
-	err = upgrader.Upgrade(ccapi.NewCCAPI(reqr, cfg.InstancePollingInterval), logr, upgrader.UpgradeConfig{
+	err = upgrader.Upgrade(ccapi.NewCCAPI(reqr, cfg.InstancePollingInterval, cfg.InstanceTimeout), logr, upgrader.UpgradeConfig{
 		BrokerName:       cfg.BrokerName,
 		ParallelUpgrades: cfg.ParallelUpgrades,
 		Action:           cfg.Action,