Handle upgrade-check panics in a single place

Author: cbandy

internal/upgradecheck/http.go

@@ -83,14 +83,6 @@ func checkForUpgrades(ctx context.Context, url, versionString string, backoff wa
 	isOpenShift bool) (message string, header string, err error) {
 	var headerPayloadStruct *clientUpgradeData
 
-	// Guard against panics within the checkForUpgrades function to allow the
-	// checkForUpgradesScheduler to reschedule a check
-	defer func() {
-		if panicErr := recover(); panicErr != nil {
-			err = fmt.Errorf("%s", panicErr)
-		}
-	}()
-
 	// Prep request
 	req, err := http.NewRequest("GET", url, nil)
 	if err == nil {
@@ -151,13 +143,6 @@ func CheckForUpgradesScheduler(ctx context.Context,
 	cacheClient CacheWithWait,
 ) {
 	log := logging.FromContext(ctx)
-	defer func() {
-		if err := recover(); err != nil {
-			log.V(1).Info("encountered panic in upgrade check",
-				"response", err,
-			)
-		}
-	}()
 
 	if url == "" {
 		url = upgradeCheckURL
@@ -174,30 +159,38 @@ func CheckForUpgradesScheduler(ctx context.Context,
 		return
 	}
 
-	info, header, err := checkForUpgrades(ctx, url, versionString, backoff,
-		crclient, cfg, isOpenShift)
-	if err != nil {
-		log.V(1).Info("could not complete upgrade check",
-			"response", err.Error())
-	} else {
-		log.Info(info, clientHeader, header)
-	}
+	check(ctx, versionString, url, crclient, cfg, isOpenShift)
 
 	ticker := time.NewTicker(upgradeCheckPeriod)
 	for {
 		select {
 		case <-ticker.C:
-			info, header, err = checkForUpgrades(ctx, url, versionString, backoff,
-				crclient, cfg, isOpenShift)
-			if err != nil {
-				log.V(1).Info("could not complete scheduled upgrade check",
-					"response", err.Error())
-			} else {
-				log.Info(info, clientHeader, header)
-			}
+			check(ctx, versionString, url, crclient, cfg, isOpenShift)
 		case <-ctx.Done():
 			ticker.Stop()
 			return
 		}
 	}
 }
+
+func check(ctx context.Context,
+	versionString, url string, crclient crclient.Client,
+	cfg *rest.Config, isOpenShift bool,
+) {
+	log := logging.FromContext(ctx)
+
+	defer func() {
+		if v := recover(); v != nil {
+			log.V(1).Info("encountered panic in upgrade check", "response", v)
+		}
+	}()
+
+	info, header, err := checkForUpgrades(ctx,
+		url, versionString, backoff, crclient, cfg, isOpenShift)
+
+	if err != nil {
+		log.V(1).Info("could not complete upgrade check", "response", err.Error())
+	} else {
+		log.Info(info, clientHeader, header)
+	}
+}

internal/upgradecheck/http_test.go

@@ -113,24 +113,6 @@ func TestCheckForUpgrades(t *testing.T) {
 		checkData(t, header)
 	})
 
-	t.Run("recovers from panic", func(t *testing.T) {
-		var counter int
-		// A panicking call
-		funcFoo = func() (*http.Response, error) {
-			counter++
-			panic(fmt.Errorf("oh no!"))
-		}
-
-		res, header, err := checkForUpgrades(ctx, "", "4.7.3", backoff,
-			fakeClient, cfg, false)
-		// One call because of panic
-		assert.Equal(t, counter, 1)
-		assert.Equal(t, res, "")
-		assert.Equal(t, err.Error(), `oh no!`)
-		// no http response returned, so don't perform full check
-		assert.Assert(t, header == "")
-	})
-
 	t.Run("total failure, bad StatusCode", func(t *testing.T) {
 		var counter int
 		// A call returning bad StatusCode
@@ -213,7 +195,7 @@ func TestCheckForUpgradesScheduler(t *testing.T) {
 		// Sleeping leads to some non-deterministic results, but we expect at least 1 execution
 		// plus one log for the failure to apply the configmap
 		assert.Assert(t, len(calls) >= 2)
-		assert.Assert(t, cmp.Contains(calls[1], `could not complete upgrade check`))
+		assert.Assert(t, cmp.Contains(calls[1], `encountered panic in upgrade check`))
 	})
 
 	t.Run("cache sync fail leads to log and exit", func(t *testing.T) {