Fix closing the body for HTTP requests

.golangci.yml

@@ -29,6 +29,7 @@ linters:
     - gofmt
     - goimports
     - exportloopref
+    - bodyclose
 
     # revive is a replacement for golint, but we do not run it in CI for now.
     # This is only enabled as a post-commit hook

go/test/endtoend/clustertest/main_test.go

@@ -107,9 +107,10 @@ func testURL(t *testing.T, url string, testCaseName string) {
 
 // getStatusForUrl returns the status code for the URL
 func getStatusForURL(url string) int {
-	resp, _ := http.Get(url)
-	if resp != nil {
-		return resp.StatusCode
+	resp, err := http.Get(url)
+	if err != nil {
+		return 0
 	}
-	return 0
+	defer resp.Body.Close()
+	return resp.StatusCode
 }

go/test/endtoend/clustertest/vtctld_test.go

@@ -62,13 +62,15 @@ func TestVtctldProcess(t *testing.T) {
 
 func testTopoDataAPI(t *testing.T, url string) {
 	resp, err := http.Get(url)
-	require.Nil(t, err)
+	require.NoError(t, err)
+	defer resp.Body.Close()
 	assert.Equal(t, resp.StatusCode, 200)
 
 	resultMap := make(map[string]any)
-	respByte, _ := io.ReadAll(resp.Body)
+	respByte, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
 	err = json.Unmarshal(respByte, &resultMap)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	errorValue := reflect.ValueOf(resultMap["Error"])
 	assert.Empty(t, errorValue.String())
@@ -83,7 +85,7 @@ func testTopoDataAPI(t *testing.T, url string) {
 func testListAllTablets(t *testing.T) {
 	// first w/o any filters, aside from cell
 	result, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ListAllTablets", clusterInstance.Cell)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	tablets := getAllTablets()
 
@@ -104,7 +106,7 @@ func testListAllTablets(t *testing.T) {
 		"ListAllTablets", "--", "--keyspace", clusterInstance.Keyspaces[0].Name,
 		"--tablet_type", "primary",
 		clusterInstance.Cell)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	// We should only return a single primary tablet per shard in the first keyspace
 	tabletsFromCMD = strings.Split(result, "\n")
@@ -115,9 +117,10 @@ func testListAllTablets(t *testing.T) {
 
 func testTabletStatus(t *testing.T) {
 	resp, err := http.Get(fmt.Sprintf("http://%s:%d", clusterInstance.Hostname, clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].HTTPPort))
-	require.Nil(t, err)
+	require.NoError(t, err)
+	defer resp.Body.Close()
 	respByte, err := io.ReadAll(resp.Body)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	result := string(respByte)
 	log.Infof("Tablet status response: %v", result)
 	assert.True(t, strings.Contains(result, `Alias: <a href="http://localhost:`))
@@ -126,13 +129,13 @@ func testTabletStatus(t *testing.T) {
 
 func testExecuteAsDba(t *testing.T) {
 	result, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ExecuteFetchAsDba", clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].Alias, `SELECT 1 AS a`)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, result, oneTableOutput)
 }
 
 func testExecuteAsApp(t *testing.T) {
 	result, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ExecuteFetchAsApp", clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].Alias, `SELECT 1 AS a`)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, result, oneTableOutput)
 }
 

go/test/endtoend/clustertest/vtgate_test.go

@@ -40,7 +40,7 @@ func TestVtgateProcess(t *testing.T) {
 	verifyVtgateVariables(t, clusterInstance.VtgateProcess.VerifyURL)
 	ctx := context.Background()
 	conn, err := mysql.Connect(ctx, &vtParams)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	defer conn.Close()
 
 	utils.Exec(t, conn, "insert into customer(id, email) values(1,'email1')")
@@ -52,41 +52,42 @@ func TestVtgateProcess(t *testing.T) {
 }
 
 func verifyVtgateVariables(t *testing.T, url string) {
-	resp, _ := http.Get(url)
-	if resp != nil && resp.StatusCode == 200 {
-		resultMap := make(map[string]any)
-		respByte, _ := io.ReadAll(resp.Body)
-		err := json.Unmarshal(respByte, &resultMap)
-		require.Nil(t, err)
-		if resultMap["VtgateVSchemaCounts"] == nil {
-			t.Error("Vschema count should be present in variables")
-		}
-		vschemaCountMap := getMapFromJSON(resultMap, "VtgateVSchemaCounts")
-		if _, present := vschemaCountMap["Reload"]; !present {
-			t.Error("Reload count should be present in vschemacount")
-		} else if object := reflect.ValueOf(vschemaCountMap["Reload"]); object.NumField() <= 0 {
-			t.Error("Reload count should be greater than 0")
-		}
-		if _, present := vschemaCountMap["WatchError"]; present {
-			t.Error("There should not be any WatchError in VschemaCount")
-		}
-		if _, present := vschemaCountMap["Parsing"]; present {
-			t.Error("There should not be any Parsing in VschemaCount")
-		}
+	resp, err := http.Get(url)
+	require.NoError(t, err)
+	defer resp.Body.Close()
 
-		if resultMap["HealthcheckConnections"] == nil {
-			t.Error("HealthcheckConnections count should be present in variables")
-		}
+	require.Equal(t, 200, resp.StatusCode)
+	resultMap := make(map[string]any)
+	respByte, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	err = json.Unmarshal(respByte, &resultMap)
+	require.NoError(t, err)
+	if resultMap["VtgateVSchemaCounts"] == nil {
+		t.Error("Vschema count should be present in variables")
+	}
+	vschemaCountMap := getMapFromJSON(resultMap, "VtgateVSchemaCounts")
+	if _, present := vschemaCountMap["Reload"]; !present {
+		t.Error("Reload count should be present in vschemacount")
+	} else if object := reflect.ValueOf(vschemaCountMap["Reload"]); object.NumField() <= 0 {
+		t.Error("Reload count should be greater than 0")
+	}
+	if _, present := vschemaCountMap["WatchError"]; present {
+		t.Error("There should not be any WatchError in VschemaCount")
+	}
+	if _, present := vschemaCountMap["Parsing"]; present {
+		t.Error("There should not be any Parsing in VschemaCount")
+	}
 
-		healthCheckConnection := getMapFromJSON(resultMap, "HealthcheckConnections")
-		if len(healthCheckConnection) <= 0 {
-			t.Error("Atleast one healthy tablet needs to be present")
-		}
-		if !isPrimaryTabletPresent(healthCheckConnection) {
-			t.Error("Atleast one PRIMARY tablet needs to be present")
-		}
-	} else {
-		t.Error("Vtgate api url response not found")
+	if resultMap["HealthcheckConnections"] == nil {
+		t.Error("HealthcheckConnections count should be present in variables")
+	}
+
+	healthCheckConnection := getMapFromJSON(resultMap, "HealthcheckConnections")
+	if len(healthCheckConnection) <= 0 {
+		t.Error("Atleast one healthy tablet needs to be present")
+	}
+	if !isPrimaryTabletPresent(healthCheckConnection) {
+		t.Error("Atleast one PRIMARY tablet needs to be present")
 	}
 }
 

go/test/endtoend/clustertest/vttablet_test.go

@@ -33,13 +33,15 @@ func TestVttabletProcess(t *testing.T) {
 	defer cluster.PanicHandler(t)
 	firstTabletPort := clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].HTTPPort
 	testURL(t, fmt.Sprintf("http://localhost:%d/debug/vars/", firstTabletPort), "tablet debug var url")
-	resp, _ := http.Get(fmt.Sprintf("http://localhost:%d/debug/vars", firstTabletPort))
+	resp, err := http.Get(fmt.Sprintf("http://localhost:%d/debug/vars", firstTabletPort))
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
 	resultMap := make(map[string]any)
-	respByte, _ := io.ReadAll(resp.Body)
-	err := json.Unmarshal(respByte, &resultMap)
-	if err != nil {
-		panic(err)
-	}
+	respByte, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	err = json.Unmarshal(respByte, &resultMap)
+	require.NoError(t, err)
 	if got, want := resultMap["TabletKeyspace"], "commerce"; got != want {
 		t.Errorf("select:\n%v want\n%v for %s", got, want, "Keyspace of tablet should match")
 	}
@@ -50,5 +52,5 @@ func TestDeleteTablet(t *testing.T) {
 	primary := clusterInstance.Keyspaces[0].Shards[0].PrimaryTablet()
 	require.NotNil(t, primary)
 	_, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("DeleteTablet", "--", "--allow_primary", primary.Alias)
-	require.Nil(t, err, "Error: %v", err)
+	require.NoError(t, err)
 }

go/test/endtoend/messaging/message_test.go

@@ -648,8 +648,13 @@ func parseDebugVars(t *testing.T, output interface{}, vttablet *cluster.Vttablet
 	if err != nil {
 		t.Fatalf("failed to fetch %q: %v", debugVarURL, err)
 	}
+	defer resp.Body.Close()
+
+	respByte, err := io.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatalf("failed to read body %q: %v", debugVarURL, err)
+	}
 
-	respByte, _ := io.ReadAll(resp.Body)
 	if resp.StatusCode != 200 {
 		t.Fatalf("status code %d while fetching %q:\n%s", resp.StatusCode, debugVarURL, respByte)
 	}

go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go

@@ -20,7 +20,6 @@ import (
 	"flag"
 	"fmt"
 	"io"
-	"net/http"
 	"os"
 	"path"
 	"strings"
@@ -221,24 +220,25 @@ func TestMain(m *testing.M) {
 }
 
 // direct per-tablet throttler API instruction
-func throttleResponse(tablet *cluster.Vttablet, path string) (resp *http.Response, respBody string, err error) {
+func throttleResponse(tablet *cluster.Vttablet, path string) (respBody string, err error) {
 	apiURL := fmt.Sprintf("http://%s:%d/%s", tablet.VttabletProcess.TabletHostname, tablet.HTTPPort, path)
-	resp, err = httpClient.Get(apiURL)
+	resp, err := httpClient.Get(apiURL)
 	if err != nil {
-		return resp, respBody, err
+		return "", err
 	}
+	defer resp.Body.Close()
 	b, err := io.ReadAll(resp.Body)
 	respBody = string(b)
-	return resp, respBody, err
+	return respBody, err
 }
 
 // direct per-tablet throttler API instruction
-func throttleApp(tablet *cluster.Vttablet, app string) (*http.Response, string, error) {
+func throttleApp(tablet *cluster.Vttablet, app string) (string, error) {
 	return throttleResponse(tablet, fmt.Sprintf("throttler/throttle-app?app=%s&duration=1h", app))
 }
 
 // direct per-tablet throttler API instruction
-func unthrottleApp(tablet *cluster.Vttablet, app string) (*http.Response, string, error) {
+func unthrottleApp(tablet *cluster.Vttablet, app string) (string, error) {
 	return throttleResponse(tablet, fmt.Sprintf("throttler/unthrottle-app?app=%s", app))
 }
 
@@ -398,7 +398,7 @@ func TestSchemaChange(t *testing.T) {
 				// vstreamer source; but it's OK to be on the safe side and throttle on all tablets. Doesn't
 				// change the essence of this test.
 				for _, tablet := range shard.Vttablets {
-					_, body, err := throttleApp(tablet, vstreamerThrottlerAppName)
+					body, err := throttleApp(tablet, vstreamerThrottlerAppName)
 					defer unthrottleApp(tablet, vstreamerThrottlerAppName)
 
 					assert.NoError(t, err)
@@ -498,12 +498,12 @@ func TestSchemaChange(t *testing.T) {
 				case 0:
 					// this is the shard where we run PRS
 					// Use per-tablet throttling API
-					_, body, err = throttleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
+					body, err = throttleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
 					defer unthrottleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
 				case 1:
 					// no PRS on this shard
 					// Use per-tablet throttling API
-					_, body, err = throttleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
+					body, err = throttleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
 					defer unthrottleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
 				}
 				assert.NoError(t, err)
@@ -554,11 +554,11 @@ func TestSchemaChange(t *testing.T) {
 					case 0:
 						// this is the shard where we run PRS
 						// Use per-tablet throttling API
-						_, body, err = unthrottleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
+						body, err = unthrottleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
 					case 1:
 						// no PRS on this shard
 						// Use per-tablet throttling API
-						_, body, err = unthrottleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
+						body, err = unthrottleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
 					}
 					assert.NoError(t, err)
 					assert.Contains(t, body, onlineDDLThrottlerAppName)
@@ -683,7 +683,7 @@ func TestSchemaChange(t *testing.T) {
 		// shard 0 will run normally, shard 1 will be throttled
 		defer unthrottleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
 		t.Run("throttle shard 1", func(t *testing.T) {
-			_, body, err := throttleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
+			body, err := throttleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
 			assert.NoError(t, err)
 			assert.Contains(t, body, onlineDDLThrottlerAppName)
 		})
@@ -707,7 +707,7 @@ func TestSchemaChange(t *testing.T) {
 			onlineddl.CheckCancelAllMigrations(t, &vtParams, 1)
 		})
 		t.Run("unthrottle shard 1", func(t *testing.T) {
-			_, body, err := unthrottleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
+			body, err := unthrottleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
 			assert.NoError(t, err)
 			assert.Contains(t, body, onlineDDLThrottlerAppName)
 		})

go/test/endtoend/tabletgateway/buffer/buffer_test_helpers.go

@@ -288,7 +288,7 @@ func (bt *BufferingTest) Test(t *testing.T) {
 	// Healthcheck interval on tablet is set to 1s, so sleep for 2s
 	time.Sleep(2 * time.Second)
 	conn, err := mysql.Connect(context.Background(), &vtParams)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	defer conn.Close()
 
 	// Insert two rows for the later threads (critical read, update).
@@ -350,11 +350,14 @@ func (bt *BufferingTest) Test(t *testing.T) {
 	//At least one thread should have been buffered.
 	//This may fail if a failover is too fast. Add retries then.
 	resp, err := http.Get(clusterInstance.VtgateProcess.VerifyURL)
-	require.Nil(t, err)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
 	require.Equal(t, 200, resp.StatusCode)
 
 	var metadata VTGateBufferingStats
-	respByte, _ := io.ReadAll(resp.Body)
+	respByte, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
 	err = json.Unmarshal(respByte, &metadata)
 	require.NoError(t, err)
 

go/test/endtoend/tabletgateway/vtgate_test.go

@@ -45,7 +45,7 @@ func TestVtgateHealthCheck(t *testing.T) {
 	verifyVtgateVariables(t, clusterInstance.VtgateProcess.VerifyURL)
 	ctx := context.Background()
 	conn, err := mysql.Connect(ctx, &vtParams)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	defer conn.Close()
 
 	qr := utils.Exec(t, conn, "show vitess_tablets")
@@ -59,7 +59,7 @@ func TestVtgateReplicationStatusCheck(t *testing.T) {
 	verifyVtgateVariables(t, clusterInstance.VtgateProcess.VerifyURL)
 	ctx := context.Background()
 	conn, err := mysql.Connect(ctx, &vtParams)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	defer conn.Close()
 
 	// Only returns rows for REPLICA and RDONLY tablets -- so should be 2 of them
@@ -72,10 +72,12 @@ func TestVtgateReplicationStatusCheck(t *testing.T) {
 func verifyVtgateVariables(t *testing.T, url string) {
 	resp, err := http.Get(url)
 	require.NoError(t, err)
+	defer resp.Body.Close()
 	require.Equal(t, 200, resp.StatusCode, "Vtgate api url response not found")
 
 	resultMap := make(map[string]any)
-	respByte, _ := io.ReadAll(resp.Body)
+	respByte, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
 	err = json.Unmarshal(respByte, &resultMap)
 	require.NoError(t, err)
 	assert.Contains(t, resultMap, "VtgateVSchemaCounts", "Vschema count should be present in variables")
@@ -203,7 +205,7 @@ func TestReplicaTransactions(t *testing.T) {
 	// been restarted and the session lost
 	replicaTablet.VttabletProcess.ServingStatus = "SERVING"
 	err = replicaTablet.VttabletProcess.Setup()
-	require.Nil(t, err)
+	require.NoError(t, err)
 	serving := replicaTablet.VttabletProcess.WaitForStatus("SERVING", 60*time.Second)
 	assert.Equal(t, serving, true, "Tablet did not become ready within a reasonable time")
 	utils.AssertContainsError(t, readConn, fetchAllCustomers, "not found")