Retry (#282)

* fix: use hashicorp/go-retryablehttp to retry failed schema downloads


---------

Co-authored-by: Carlos Sanchez <carlos@apache.org>

go.mod

@@ -6,3 +6,8 @@ require (
 	github.com/santhosh-tekuri/jsonschema/v5 v5.3.1
 	sigs.k8s.io/yaml v1.4.0
 )
+
+require (
+	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
+	github.com/hashicorp/go-retryablehttp v0.7.7 // indirect
+)

go.sum

@@ -1,5 +1,9 @@
 github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
 github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ=
+github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48=
+github.com/hashicorp/go-retryablehttp v0.7.7 h1:C8hUCYzor8PIfXHa4UrZkU4VvK8o9ISHxT2Q8+VepXU=
+github.com/hashicorp/go-retryablehttp v0.7.7/go.mod h1:pkQpWZeYWskR+D1tR2O5OcBFOxfA7DoAO6xtkuQnHTk=
 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 h1:lZUw3E0/J3roVtGQ+SCrUrg3ON6NgVqpn3+iol9aGu4=
 github.com/santhosh-tekuri/jsonschema/v5 v5.3.1/go.mod h1:uToXkOrWAZ6/Oc07xWQrPOhJotwFIyu2bBVN41fcDUY=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=

pkg/registry/http.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"time"
 
+	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/yannh/kubeconform/pkg/cache"
 )
 
@@ -51,8 +52,14 @@ 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}
+	retryClient.Logger = nil
+
 	return &SchemaRegistry{
-		c:                  &http.Client{Transport: reghttp},
+		c:                  retryClient.StandardClient(),
 		schemaPathTemplate: schemaPathTemplate,
 		cache:              filecache,
 		strict:             strict,

pkg/registry/http_test.go

@@ -3,114 +3,177 @@ package registry
 import (
 	"bytes"
 	"fmt"
-	"io"
+	"math/rand"
 	"net/http"
-	"strings"
 	"testing"
+	"time"
 )
 
 type mockHTTPGetter struct {
-	httpGet func(string) (*http.Response, error)
+	callNumber int
+	httpGet    func(mockHTTPGetter, string) (*http.Response, error)
 }
 
-func newMockHTTPGetter(f func(string) (*http.Response, error)) *mockHTTPGetter {
+func newMockHTTPGetter(f func(mockHTTPGetter, string) (*http.Response, error)) *mockHTTPGetter {
 	return &mockHTTPGetter{
-		httpGet: f,
+		callNumber: 0,
+		httpGet:    f,
 	}
 }
-func (m mockHTTPGetter) Get(url string) (resp *http.Response, err error) {
-	return m.httpGet(url)
+func (m *mockHTTPGetter) Get(url string) (resp *http.Response, err error) {
+	m.callNumber = m.callNumber + 1
+	return m.httpGet(*m, 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
 		expect                                       []byte
 		expectErr                                    error
 	}{
 		{
-			"error when downloading",
-			newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
-				return nil, fmt.Errorf("failed downloading from registry")
-			}),
-			"http://kubernetesjson.dev",
+			"retry connection reset by peer",
+			fmt.Sprintf("%s/simulate-reset", url),
 			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",
-			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",
+			fmt.Sprintf("%s/404", url),
 			true,
 			"Deployment",
 			"v1",
 			"1.18.0",
 			nil,
-			fmt.Errorf("could not find schema at http://kubernetesjson.dev"),
+			fmt.Errorf("could not find schema at %s/404", 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",
+			"getting 500",
+			fmt.Sprintf("%s/500", url),
 			true,
 			"Deployment",
 			"v1",
 			"1.18.0",
 			nil,
-			fmt.Errorf("error while downloading schema at http://kubernetesjson.dev - received HTTP status 503"),
+			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,
 		},
 		{
 			"200",
-			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",
+			url,
 			true,
 			"Deployment",
 			"v1",
 			"1.18.0",
-			[]byte("http response mock body"),
+			[]byte(http.StatusText(http.StatusOK)),
 			nil,
 		},
 	} {
-		reg := SchemaRegistry{
-			c:                  testCase.c,
-			schemaPathTemplate: testCase.schemaPathTemplate,
-			strict:             testCase.strict,
+		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
 		}
 
 		_, res, err := reg.DownloadSchema(testCase.resourceKind, testCase.resourceAPIVersion, testCase.k8sversion)
 		if err == nil || testCase.expectErr == nil {
-			if err != testCase.expectErr {
-				t.Errorf("during test '%s': expected error, got:\n%s\n%s\n", testCase.name, testCase.expectErr, err)
+			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)
 			}
 		} else if err.Error() != testCase.expectErr.Error() {
-			t.Errorf("during test '%s': expected error, got:\n%s\n%s\n", testCase.name, testCase.expectErr, err)
+			t.Errorf("during test '%s': expected error\n%s, got:\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)
 		}
 	}