Skip to content

Commit

Permalink
Revert "fix: retry on download errors (#274)"
Browse files Browse the repository at this point in the history
This reverts commit 50ce5f8.
  • Loading branch information
yannh authored Jul 29, 2024
1 parent 50ce5f8 commit 322ba4f
Show file tree
Hide file tree
Showing 21 changed files with 48 additions and 2,130 deletions.
5 changes: 0 additions & 5 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,3 @@ 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
)
4 changes: 0 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
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=
Expand Down
8 changes: 1 addition & 7 deletions pkg/registry/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"os"
"time"

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

Expand Down Expand Up @@ -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,
Expand Down
157 changes: 47 additions & 110 deletions pkg/registry/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,177 +3,114 @@ 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
expect []byte
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)
}
}

Expand Down
Loading

0 comments on commit 322ba4f

Please sign in to comment.