Refactor http client properly (#481)

Signed-off-by: Adrian Cole <[email protected]>

Author: codefromthecrypt

internal/envoy/http.go

@@ -13,14 +13,14 @@ import (
 )
 
 // httpGet adds the userAgent header to the request, so that we can tell what is a dev build vs release.
-func httpGet(ctx context.Context, url string, p version.Platform, v string) (*http.Response, error) {
+func httpGet(ctx context.Context, client *http.Client, url string, p version.Platform, v string) (*http.Response, error) {
 	// #nosec -> url can be anywhere by design
 	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
 	if err != nil {
 		return nil, err
 	}
 	req.Header.Add("User-Agent", userAgent(p, v))
-	return http.DefaultClient.Do(req)
+	return client.Do(req)
 }
 
 // userAgent returns the 'User-Agent' header value used in HTTP requests. This is useful in log, metrics, analytics, or

internal/envoy/http_test.go

@@ -4,7 +4,6 @@
 package envoy
 
 import (
-	"context"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -22,7 +21,7 @@ func TestHttpGet_AddsDefaultHeaders(t *testing.T) {
 	}))
 	defer ts.Close()
 
-	res, err := httpGet(context.Background(), ts.URL, globals.DefaultPlatform, "dev")
+	res, err := httpGet(t.Context(), ts.Client(), ts.URL, globals.DefaultPlatform, "dev")
 	require.NoError(t, err)
 
 	defer res.Body.Close() //nolint:errcheck

internal/envoy/install.go

@@ -82,7 +82,7 @@ func verifyEnvoy(installPath string) (string, error) {
 func untarEnvoy(ctx context.Context, dst string, src version.TarballURL, // dst, src order like io.Copy
 	sha256Sum version.SHA256Sum, p version.Platform, v string,
 ) error {
-	res, err := httpGet(ctx, string(src), p, v)
+	res, err := httpGet(ctx, http.DefaultClient, string(src), p, v)
 	if err != nil {
 		return err
 	}

internal/envoy/runtime.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -48,9 +49,11 @@ const (
 // opts allows a user running envoy to control the working directory by ID or path, allowing explicit cleanup.
 func NewRuntime(opts *globals.RunOpts, logf LogFunc) *Runtime {
 	safeHook := &safeStartupHook{
-		delegate: collectConfigDump,
-		logf:     logf,
-		timeout:  3 * time.Second,
+		delegate: func(ctx context.Context, runDir, adminAddress string) error {
+			return collectConfigDump(ctx, http.DefaultClient, runDir, adminAddress)
+		},
+		logf:    logf,
+		timeout: 3 * time.Second,
 	}
 	return &Runtime{o: opts, logf: logf, startupHook: safeHook.Hook}
 }

internal/envoy/startup.go

@@ -52,13 +52,13 @@ func (s *safeStartupHook) Hook(ctx context.Context, runDir, adminAddress string)
 // - Endpoints (EDS)
 // - Secrets (SDS)
 // This provides a comprehensive snapshot of Envoy's dynamic configuration state.
-func collectConfigDump(ctx context.Context, runDir, adminAddress string) error {
+func collectConfigDump(ctx context.Context, client *http.Client, runDir, adminAddress string) error {
 	url := fmt.Sprintf("http://%s/config_dump?include_eds", adminAddress)
 	file := filepath.Join(runDir, "config_dump.json")
-	return copyURLToFile(ctx, url, file)
+	return copyURLToFile(ctx, client, url, file)
 }
 
-func copyURLToFile(ctx context.Context, url, fullPath string) error {
+func copyURLToFile(ctx context.Context, client *http.Client, url, fullPath string) error {
 	// #nosec -> runDir is allowed to be anywhere
 	f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_WRONLY, 0o600)
 	if err != nil {
@@ -71,7 +71,7 @@ func copyURLToFile(ctx context.Context, url, fullPath string) error {
 	if err != nil {
 		return fmt.Errorf("could not create request %v: %w", url, err)
 	}
-	res, err := http.DefaultClient.Do(req)
+	res, err := client.Do(req)
 	if err != nil {
 		return fmt.Errorf("could not read %v: %w", url, err)
 	}

internal/envoy/startup_test.go

@@ -133,7 +133,7 @@ func TestCollectConfigDump(t *testing.T) {
 				ctx, cancel = context.WithTimeout(ctx, 3*time.Second)
 				defer cancel()
 			}
-			err := collectConfigDump(ctx, tempDir, adminAddress)
+			err := collectConfigDump(ctx, ts.Client(), tempDir, adminAddress)
 
 			if tt.wantErr != "" {
 				require.Error(t, err)
@@ -211,7 +211,7 @@ func TestCopyURLToFile(t *testing.T) {
 				filePath = "/invalid\x00path/test.txt"
 			}
 
-			err := copyURLToFile(ctx, ts.URL, filePath)
+			err := copyURLToFile(ctx, ts.Client(), ts.URL, filePath)
 
 			if tt.wantErr != "" {
 				require.Error(t, err)
@@ -236,7 +236,7 @@ func TestCopyURLToFile_InvalidURL(t *testing.T) {
 	filePath := filepath.Join(tempDir, "test.txt")
 
 	// Test with invalid URL
-	err := copyURLToFile(t.Context(), "://invalid-url", filePath)
+	err := copyURLToFile(t.Context(), http.DefaultClient, "://invalid-url", filePath)
 	require.Error(t, err)
 	require.Contains(t, err.Error(), "could not create request")
 }

internal/envoy/versions.go

@@ -18,7 +18,7 @@ import (
 func NewGetVersions(envoyVersionsURL string, p version.Platform, v string) version.GetReleaseVersions {
 	return func(ctx context.Context) (*version.ReleaseVersions, error) {
 		// #nosec => This is by design, users can call out to wherever they like!
-		resp, err := httpGet(ctx, envoyVersionsURL, p, v)
+		resp, err := httpGet(ctx, http.DefaultClient, envoyVersionsURL, p, v)
 		if err != nil {
 			return nil, err
 		}