Revert "fix: retry on download errors (#274)" (#279)

This reverts commit 50ce5f8ecb.
2026-04-07 14:26:54 +00:00 · 2024-07-29 10:37:43 +02:00 · 2024-07-29 10:37:43 +02:00 · 706cd56e87
commit 706cd56e87
parent 50ce5f8ecb
21 changed files with 48 additions and 2130 deletions
--- a/pkg/registry/http.go
+++ b/pkg/registry/http.go
@ -10,7 +10,6 @@ import (
 	"os"
 	"time"

-	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/yannh/kubeconform/pkg/cache"
 )

@ -52,13 +51,8 @@ func newHTTPRegistry(schemaPathTemplate string, cacheFolder string, strict bool,
 		filecache = cache.NewOnDiskCache(cacheFolder)
 	}

-	// retriable http client
-	retryClient := retryablehttp.NewClient()
-	retryClient.RetryMax = 2
-	retryClient.HTTPClient = &http.Client{Transport: reghttp}
-
 	return &SchemaRegistry{
-		c:                  retryClient.StandardClient(),
+		c:                  &http.Client{Transport: reghttp},
 		schemaPathTemplate: schemaPathTemplate,
 		cache:              filecache,
 		strict:             strict,
--- a/pkg/registry/http_test.go
+++ b/pkg/registry/http_test.go
@ -3,98 +3,29 @@ package registry
 import (
 	"bytes"
 	"fmt"
-	"math/rand"
+	"io"
 	"net/http"
+	"strings"
 	"testing"
-	"time"
 )

 type mockHTTPGetter struct {
-	callNumber int
-	httpGet    func(mockHTTPGetter, string) (*http.Response, error)
+	httpGet func(string) (*http.Response, error)
 }

-func newMockHTTPGetter(f func(mockHTTPGetter, string) (*http.Response, error)) *mockHTTPGetter {
+func newMockHTTPGetter(f func(string) (*http.Response, error)) *mockHTTPGetter {
 	return &mockHTTPGetter{
-		callNumber: 0,
-		httpGet:    f,
+		httpGet: f,
 	}
 }
-func (m *mockHTTPGetter) Get(url string) (resp *http.Response, err error) {
-	m.callNumber = m.callNumber + 1
-	return m.httpGet(*m, url)
+func (m mockHTTPGetter) Get(url string) (resp *http.Response, err error) {
+	return m.httpGet(url)
 }

 func TestDownloadSchema(t *testing.T) {
-	callCounts := map[string]int{}
-
-	// http server to simulate different responses
-	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
-		var s int
-		callCounts[r.URL.Path]++
-		callCount := callCounts[r.URL.Path]
-
-		switch r.URL.Path {
-		case "/404":
-			s = http.StatusNotFound
-		case "/500":
-			s = http.StatusInternalServerError
-		case "/503":
-			if callCount < 2 {
-				s = http.StatusServiceUnavailable
-			} else {
-				s = http.StatusOK // Should succeed on 3rd try
-			}
-
-		case "/simulate-reset":
-			if callCount < 2 {
-				if hj, ok := w.(http.Hijacker); ok {
-					conn, _, err := hj.Hijack()
-					if err != nil {
-						fmt.Printf("Hijacking failed: %v\n", err)
-						return
-					}
-					conn.Close() // Close the connection to simulate a reset
-				}
-				return
-			}
-			s = http.StatusOK // Should succeed on third try
-
-		default:
-			s = http.StatusOK
-		}
-
-		w.WriteHeader(s)
-		w.Write([]byte(http.StatusText(s)))
-	})
-
-	port := fmt.Sprint(rand.Intn(1000) + 9000) // random port
-	server := &http.Server{Addr: "127.0.0.1:" + port}
-	url := fmt.Sprintf("http://localhost:%s", port)
-
-	go func() {
-		if err := server.ListenAndServe(); err != nil {
-			fmt.Printf("Failed to start server: %v\n", err)
-		}
-	}()
-	defer server.Shutdown(nil)
-
-	// Wait for the server to start
-	for i := 0; i < 20; i++ {
-		if _, err := http.Get(url); err == nil {
-			break
-		}
-
-		if i == 19 {
-			t.Error("http server did not start")
-			return
-		}
-
-		time.Sleep(50 * time.Millisecond)
-	}
-
 	for _, testCase := range []struct {
 		name                                         string
+		c                                            httpGetter
 		schemaPathTemplate                           string
 		strict                                       bool
 		resourceKind, resourceAPIVersion, k8sversion string
@ -102,78 +33,84 @@ func TestDownloadSchema(t *testing.T) {
 		expectErr                                    error
 	}{
 		{
-			"retry connection reset by peer",
-			fmt.Sprintf("%s/simulate-reset", url),
+			"error when downloading",
+			newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
+				return nil, fmt.Errorf("failed downloading from registry")
+			}),
+			"http://kubernetesjson.dev",
 			true,
 			"Deployment",
 			"v1",
 			"1.18.0",
-			[]byte(http.StatusText(http.StatusOK)),
 			nil,
+			fmt.Errorf("failed downloading schema at http://kubernetesjson.dev: failed downloading from registry"),
 		},
 		{
 			"getting 404",
-			fmt.Sprintf("%s/404", url),
+			newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
+				return &http.Response{
+					StatusCode: http.StatusNotFound,
+					Body:       io.NopCloser(strings.NewReader("http response mock body")),
+				}, nil
+			}),
+			"http://kubernetesjson.dev",
 			true,
 			"Deployment",
 			"v1",
 			"1.18.0",
 			nil,
-			fmt.Errorf("could not find schema at %s/404", url),
+			fmt.Errorf("could not find schema at http://kubernetesjson.dev"),
 		},
 		{
-			"getting 500",
-			fmt.Sprintf("%s/500", url),
+			"getting 503",
+			newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
+				return &http.Response{
+					StatusCode: http.StatusServiceUnavailable,
+					Body:       io.NopCloser(strings.NewReader("http response mock body")),
+				}, nil
+			}),
+			"http://kubernetesjson.dev",
 			true,
 			"Deployment",
 			"v1",
 			"1.18.0",
 			nil,
-			fmt.Errorf("failed downloading schema at %s/500: Get \"%s/500\": GET %s/500 giving up after 3 attempt(s)", url, url, url),
-		},
-		{
-			"retry 503",
-			fmt.Sprintf("%s/503", url),
-			true,
-			"Deployment",
-			"v1",
-			"1.18.0",
-			[]byte(http.StatusText(http.StatusOK)),
-			nil,
+			fmt.Errorf("error while downloading schema at http://kubernetesjson.dev - received HTTP status 503"),
 		},
 		{
 			"200",
-			url,
+			newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
+				return &http.Response{
+					StatusCode: http.StatusOK,
+					Body:       io.NopCloser(strings.NewReader("http response mock body")),
+				}, nil
+			}),
+			"http://kubernetesjson.dev",
 			true,
 			"Deployment",
 			"v1",
 			"1.18.0",
-			[]byte(http.StatusText(http.StatusOK)),
+			[]byte("http response mock body"),
 			nil,
 		},
 	} {
-		callCounts = map[string]int{} // Reinitialise counters
-
-		reg, err := newHTTPRegistry(testCase.schemaPathTemplate, "", testCase.strict, true, true)
-		if err != nil {
-			t.Errorf("during test '%s': failed to create registry: %s", testCase.name, err)
-			continue
+		reg := SchemaRegistry{
+			c:                  testCase.c,
+			schemaPathTemplate: testCase.schemaPathTemplate,
+			strict:             testCase.strict,
 		}

 		_, res, err := reg.DownloadSchema(testCase.resourceKind, testCase.resourceAPIVersion, testCase.k8sversion)
 		if err == nil || testCase.expectErr == nil {
-			if err == nil && testCase.expectErr != nil {
-				t.Errorf("during test '%s': expected error\n%s, got nil", testCase.name, testCase.expectErr)
-			}
-			if err != nil && testCase.expectErr == nil {
-				t.Errorf("during test '%s': expected no error, got\n%s\n", testCase.name, err)
+			if err != testCase.expectErr {
+				t.Errorf("during test '%s': expected error, got:\n%s\n%s\n", testCase.name, testCase.expectErr, err)
 			}
 		} else if err.Error() != testCase.expectErr.Error() {
-			t.Errorf("during test '%s': expected error\n%s, got:\n%s\n", testCase.name, testCase.expectErr, err)
+			t.Errorf("during test '%s': expected error, got:\n%s\n%s\n", testCase.name, testCase.expectErr, err)
 		}

 		if !bytes.Equal(res, testCase.expect) {
-			t.Errorf("during test '%s': expected '%s', got '%s'", testCase.name, testCase.expect, res)
+			t.Errorf("during test '%s': expected %s, got %s", testCase.name, testCase.expect, res)
 		}
 	}