Skip to content

Commit 706cd56

Browse files
yannhyannh
authored
Revert "fix: retry on download errors (#274)" (#279)
This reverts commit 50ce5f8.
1 parent 50ce5f8 commit 706cd56

File tree

21 files changed

+48
-2130
lines changed

21 files changed

+48
-2130
lines changed

‎go.mod‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,3 @@ require (
66
github.com/santhosh-tekuri/jsonschema/v5 v5.3.1
77
sigs.k8s.io/yaml v1.4.0
88
)
9-
10-
require (
11-
github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
12-
github.com/hashicorp/go-retryablehttp v0.7.7 // indirect
13-
)

‎go.sum‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,5 @@
11
github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
22
github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
3-
github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ=
4-
github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48=
5-
github.com/hashicorp/go-retryablehttp v0.7.7 h1:C8hUCYzor8PIfXHa4UrZkU4VvK8o9ISHxT2Q8+VepXU=
6-
github.com/hashicorp/go-retryablehttp v0.7.7/go.mod h1:pkQpWZeYWskR+D1tR2O5OcBFOxfA7DoAO6xtkuQnHTk=
73
github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 h1:lZUw3E0/J3roVtGQ+SCrUrg3ON6NgVqpn3+iol9aGu4=
84
github.com/santhosh-tekuri/jsonschema/v5 v5.3.1/go.mod h1:uToXkOrWAZ6/Oc07xWQrPOhJotwFIyu2bBVN41fcDUY=
95
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=

‎pkg/registry/http.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"os"
1111
"time"
1212

13-
retryablehttp "github.com/hashicorp/go-retryablehttp"
1413
"github.com/yannh/kubeconform/pkg/cache"
1514
)
1615

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

55-
// retriable http client
56-
retryClient := retryablehttp.NewClient()
57-
retryClient.RetryMax = 2
58-
retryClient.HTTPClient = &http.Client{Transport: reghttp}
59-
6054
return &SchemaRegistry{
61-
c: retryClient.StandardClient(),
55+
c: &http.Client{Transport: reghttp},
6256
schemaPathTemplate: schemaPathTemplate,
6357
cache: filecache,
6458
strict: strict,

‎pkg/registry/http_test.go‎

Lines changed: 47 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -3,177 +3,114 @@ package registry
33
import (
44
"bytes"
55
"fmt"
6-
"math/rand"
6+
"io"
77
"net/http"
8+
"strings"
89
"testing"
9-
"time"
1010
)
1111

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

17-
func newMockHTTPGetter(f func(mockHTTPGetter, string) (*http.Response, error)) *mockHTTPGetter {
16+
func newMockHTTPGetter(f func(string) (*http.Response, error)) *mockHTTPGetter {
1817
return &mockHTTPGetter{
19-
callNumber: 0,
20-
httpGet: f,
18+
httpGet: f,
2119
}
2220
}
23-
func (m *mockHTTPGetter) Get(url string) (resp *http.Response, err error) {
24-
m.callNumber = m.callNumber + 1
25-
return m.httpGet(*m, url)
21+
func (m mockHTTPGetter) Get(url string) (resp *http.Response, err error) {
22+
return m.httpGet(url)
2623
}
2724

2825
func TestDownloadSchema(t *testing.T) {
29-
callCounts := map[string]int{}
30-
31-
// http server to simulate different responses
32-
http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
33-
var s int
34-
callCounts[r.URL.Path]++
35-
callCount := callCounts[r.URL.Path]
36-
37-
switch r.URL.Path {
38-
case "/404":
39-
s = http.StatusNotFound
40-
case "/500":
41-
s = http.StatusInternalServerError
42-
case "/503":
43-
if callCount < 2 {
44-
s = http.StatusServiceUnavailable
45-
} else {
46-
s = http.StatusOK // Should succeed on 3rd try
47-
}
48-
49-
case "/simulate-reset":
50-
if callCount < 2 {
51-
if hj, ok := w.(http.Hijacker); ok {
52-
conn, _, err := hj.Hijack()
53-
if err != nil {
54-
fmt.Printf("Hijacking failed: %v\n", err)
55-
return
56-
}
57-
conn.Close() // Close the connection to simulate a reset
58-
}
59-
return
60-
}
61-
s = http.StatusOK // Should succeed on third try
62-
63-
default:
64-
s = http.StatusOK
65-
}
66-
67-
w.WriteHeader(s)
68-
w.Write([]byte(http.StatusText(s)))
69-
})
70-
71-
port := fmt.Sprint(rand.Intn(1000) + 9000) // random port
72-
server := &http.Server{Addr: "127.0.0.1:" + port}
73-
url := fmt.Sprintf("http://localhost:%s", port)
74-
75-
go func() {
76-
if err := server.ListenAndServe(); err != nil {
77-
fmt.Printf("Failed to start server: %v\n", err)
78-
}
79-
}()
80-
defer server.Shutdown(nil)
81-
82-
// Wait for the server to start
83-
for i := 0; i < 20; i++ {
84-
if _, err := http.Get(url); err == nil {
85-
break
86-
}
87-
88-
if i == 19 {
89-
t.Error("http server did not start")
90-
return
91-
}
92-
93-
time.Sleep(50 * time.Millisecond)
94-
}
95-
9626
for _, testCase := range []struct {
9727
name string
28+
c httpGetter
9829
schemaPathTemplate string
9930
strict bool
10031
resourceKind, resourceAPIVersion, k8sversion string
10132
expect []byte
10233
expectErr error
10334
}{
10435
{
105-
"retry connection reset by peer",
106-
fmt.Sprintf("%s/simulate-reset", url),
36+
"error when downloading",
37+
newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
38+
return nil, fmt.Errorf("failed downloading from registry")
39+
}),
40+
"http://kubernetesjson.dev",
10741
true,
10842
"Deployment",
10943
"v1",
11044
"1.18.0",
111-
[]byte(http.StatusText(http.StatusOK)),
11245
nil,
46+
fmt.Errorf("failed downloading schema at http://kubernetesjson.dev: failed downloading from registry"),
11347
},
11448
{
11549
"getting 404",
116-
fmt.Sprintf("%s/404", url),
50+
newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
51+
return &http.Response{
52+
StatusCode: http.StatusNotFound,
53+
Body: io.NopCloser(strings.NewReader("http response mock body")),
54+
}, nil
55+
}),
56+
"http://kubernetesjson.dev",
11757
true,
11858
"Deployment",
11959
"v1",
12060
"1.18.0",
12161
nil,
122-
fmt.Errorf("could not find schema at %s/404", url),
62+
fmt.Errorf("could not find schema at http://kubernetesjson.dev"),
12363
},
12464
{
125-
"getting 500",
126-
fmt.Sprintf("%s/500", url),
65+
"getting 503",
66+
newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
67+
return &http.Response{
68+
StatusCode: http.StatusServiceUnavailable,
69+
Body: io.NopCloser(strings.NewReader("http response mock body")),
70+
}, nil
71+
}),
72+
"http://kubernetesjson.dev",
12773
true,
12874
"Deployment",
12975
"v1",
13076
"1.18.0",
13177
nil,
132-
fmt.Errorf("failed downloading schema at %s/500: Get \"%s/500\": GET %s/500 giving up after 3 attempt(s)", url, url, url),
133-
},
134-
{
135-
"retry 503",
136-
fmt.Sprintf("%s/503", url),
137-
true,
138-
"Deployment",
139-
"v1",
140-
"1.18.0",
141-
[]byte(http.StatusText(http.StatusOK)),
142-
nil,
78+
fmt.Errorf("error while downloading schema at http://kubernetesjson.dev - received HTTP status 503"),
14379
},
14480
{
14581
"200",
146-
url,
82+
newMockHTTPGetter(func(url string) (resp *http.Response, err error) {
83+
return &http.Response{
84+
StatusCode: http.StatusOK,
85+
Body: io.NopCloser(strings.NewReader("http response mock body")),
86+
}, nil
87+
}),
88+
"http://kubernetesjson.dev",
14789
true,
14890
"Deployment",
14991
"v1",
15092
"1.18.0",
151-
[]byte(http.StatusText(http.StatusOK)),
93+
[]byte("http response mock body"),
15294
nil,
15395
},
15496
} {
155-
callCounts = map[string]int{} // Reinitialise counters
156-
157-
reg, err := newHTTPRegistry(testCase.schemaPathTemplate, "", testCase.strict, true, true)
158-
if err != nil {
159-
t.Errorf("during test '%s': failed to create registry: %s", testCase.name, err)
160-
continue
97+
reg := SchemaRegistry{
98+
c: testCase.c,
99+
schemaPathTemplate: testCase.schemaPathTemplate,
100+
strict: testCase.strict,
161101
}
162102

163103
_, res, err := reg.DownloadSchema(testCase.resourceKind, testCase.resourceAPIVersion, testCase.k8sversion)
164104
if err == nil || testCase.expectErr == nil {
165-
if err == nil && testCase.expectErr != nil {
166-
t.Errorf("during test '%s': expected error\n%s, got nil", testCase.name, testCase.expectErr)
167-
}
168-
if err != nil && testCase.expectErr == nil {
169-
t.Errorf("during test '%s': expected no error, got\n%s\n", testCase.name, err)
105+
if err != testCase.expectErr {
106+
t.Errorf("during test '%s': expected error, got:\n%s\n%s\n", testCase.name, testCase.expectErr, err)
170107
}
171108
} else if err.Error() != testCase.expectErr.Error() {
172-
t.Errorf("during test '%s': expected error\n%s, got:\n%s\n", testCase.name, testCase.expectErr, err)
109+
t.Errorf("during test '%s': expected error, got:\n%s\n%s\n", testCase.name, testCase.expectErr, err)
173110
}
174111

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

0 commit comments

Comments
 (0)