From 72d4ebe7c3fe3fc9fc2fcebcf079cb4818678575 Mon Sep 17 00:00:00 2001 From: TAKAMI Torao Date: Tue, 22 Nov 2022 20:57:14 +0900 Subject: [PATCH 1/4] fix to apply timeout to HTTP request * If it have a general idea of what timeout to expect, apply it. * Applied 30 seconds for requests used in UPNP. * Applied 60 seconds for requests used in testing. * Do nothing for use cases where callers are using with and without timeouts. --- cmd/ostracon/commands/debug/util.go | 5 +-- libs/net/http.go | 26 +++++++++++++++ libs/net/http_test.go | 52 +++++++++++++++++++++++++++++ p2p/switch_test.go | 4 +-- p2p/upnp/upnp.go | 7 ++-- rpc/client/rpc_test.go | 6 ++-- rpc/jsonrpc/client/test_util.go | 1 + 7 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 libs/net/http.go create mode 100644 libs/net/http_test.go diff --git a/cmd/ostracon/commands/debug/util.go b/cmd/ostracon/commands/debug/util.go index cdbd3d029..1efbadffb 100644 --- a/cmd/ostracon/commands/debug/util.go +++ b/cmd/ostracon/commands/debug/util.go @@ -4,12 +4,13 @@ import ( "context" "fmt" "io/ioutil" - "net/http" "os" "path" "path/filepath" + "time" cfg "github.com/line/ostracon/config" + "github.com/line/ostracon/libs/net" rpchttp "github.com/line/ostracon/rpc/client/http" ) @@ -67,7 +68,7 @@ func copyConfig(home, dir string) error { func dumpProfile(dir, addr, profile string, debug int) error { endpoint := fmt.Sprintf("%s/debug/pprof/%s?debug=%d", addr, profile, debug) - resp, err := http.Get(endpoint) // nolint: gosec + resp, err := net.HttpGet(endpoint, time.Duration(frequency)*time.Second) if err != nil { return fmt.Errorf("failed to query for %s profile: %w", profile, err) } diff --git a/libs/net/http.go b/libs/net/http.go new file mode 100644 index 000000000..577feafe3 --- /dev/null +++ b/libs/net/http.go @@ -0,0 +1,26 @@ +package net + +import ( + "context" + "net/http" + "time" +) + +// HttpGet sends a GET request to the specified url with timeout and return the response. +func HttpGet(url string, timeout time.Duration) (*http.Response, error) { + request, err := http.NewRequestWithContext(context.Background(), "GET", url, nil) + if err != nil { + return nil, err + } + return HttpRequest(request, timeout) +} + +// HttpRequest sends the specified HTTP requests with timeout and return the response. +// For stability and security reason, we need to be available request timeouts, so http.Client{} and http.Get() are +// overridden with functions that rely on this function. +func HttpRequest(request *http.Request, timeout time.Duration) (*http.Response, error) { + client := &http.Client{ + Timeout: timeout, + } + return client.Do(request) +} diff --git a/libs/net/http_test.go b/libs/net/http_test.go new file mode 100644 index 000000000..c641cd2c0 --- /dev/null +++ b/libs/net/http_test.go @@ -0,0 +1,52 @@ +package net + +import ( + "github.com/stretchr/testify/require" + "io" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" +) + +func TestHttpGet(t *testing.T) { + expected := "hello, world" + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + w.Header().Set("Pragma", "no-cache") + w.Header().Set("Cache-Control", "no-cache") + w.Header().Set("Content-Type", "text/plain; charset=UTF-8") + _, err := w.Write([]byte(expected)) + require.NoError(t, err) + })) + defer server.Close() + + response, err := HttpGet(server.URL, 60*time.Second) + require.NoError(t, err) + bytes, err := io.ReadAll(response.Body) + require.NoError(t, err) + require.Equal(t, expected, string(bytes)) +} + +func TestHttpGetWithTimeout(t *testing.T) { + var mtx sync.Mutex + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mtx.Lock() + mtx.Unlock() + })) + defer server.Close() + + accuracy := 0.05 + timeout := 10 * time.Second + mtx.Lock() + defer mtx.Unlock() + t0 := time.Now() + _, err := HttpGet(server.URL, timeout) + t1 := time.Now() + require.Error(t, err) + delta := t1.Sub(t0).Seconds() + require.Greater(t, delta, timeout.Seconds()) + require.InDeltaf(t, timeout.Seconds(), delta, accuracy, + "response time of %.3f sec exceeded +%d%% of the expected timeout of %.3f sec", delta, uint(accuracy*100), timeout.Seconds()) +} diff --git a/p2p/switch_test.go b/p2p/switch_test.go index 85f48ae90..8829377b0 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -4,9 +4,9 @@ import ( "bytes" "errors" "fmt" + net2 "github.com/line/ostracon/libs/net" "io/ioutil" "net" - "net/http" "net/http/httptest" "regexp" "strconv" @@ -397,7 +397,7 @@ func TestSwitchStopPeerForError(t *testing.T) { defer s.Close() scrapeMetrics := func() string { - resp, err := http.Get(s.URL) + resp, err := net2.HttpGet(s.URL, 60*time.Second) require.NoError(t, err) defer resp.Body.Close() buf, _ := ioutil.ReadAll(resp.Body) diff --git a/p2p/upnp/upnp.go b/p2p/upnp/upnp.go index eadd0c0bb..83ba66e69 100644 --- a/p2p/upnp/upnp.go +++ b/p2p/upnp/upnp.go @@ -10,6 +10,7 @@ import ( "encoding/xml" "errors" "fmt" + net2 "github.com/line/ostracon/libs/net" "io/ioutil" "net" "net/http" @@ -18,6 +19,8 @@ import ( "time" ) +const HttpRequestTimeout = 30 * time.Second + type upnpNAT struct { serviceURL string ourIP string @@ -202,7 +205,7 @@ func localIPv4() (net.IP, error) { } func getServiceURL(rootURL string) (url, urnDomain string, err error) { - r, err := http.Get(rootURL) // nolint: gosec + r, err := net2.HttpGet(rootURL, HttpRequestTimeout) if err != nil { return } @@ -277,7 +280,7 @@ func soapRequest(url, function, message, domain string) (r *http.Response, err e // log.Stderr("soapRequest ", req) - r, err = http.DefaultClient.Do(req) + r, err = net2.HttpRequest(req, HttpRequestTimeout) if err != nil { return nil, err } diff --git a/rpc/client/rpc_test.go b/rpc/client/rpc_test.go index ed7cb6445..bf31a149d 100644 --- a/rpc/client/rpc_test.go +++ b/rpc/client/rpc_test.go @@ -18,6 +18,7 @@ import ( tmjson "github.com/line/ostracon/libs/json" "github.com/line/ostracon/libs/log" tmmath "github.com/line/ostracon/libs/math" + "github.com/line/ostracon/libs/net" mempl "github.com/line/ostracon/mempool" "github.com/line/ostracon/rpc/client" rpchttp "github.com/line/ostracon/rpc/client/http" @@ -75,7 +76,7 @@ func TestNilCustomHTTPClient(t *testing.T) { func TestCustomHTTPClient(t *testing.T) { remote := rpctest.GetConfig().RPC.ListenAddress - c, err := rpchttp.NewWithClient(remote, "/websocket", http.DefaultClient) + c, err := rpchttp.NewWithClient(remote, "/websocket", &http.Client{Timeout: 60 * time.Second}) require.Nil(t, err) status, err := c.Status(context.Background()) require.NoError(t, err) @@ -89,8 +90,7 @@ func TestCorsEnabled(t *testing.T) { req, err := http.NewRequest("GET", remote, nil) require.Nil(t, err, "%+v", err) req.Header.Set("Origin", origin) - c := &http.Client{} - resp, err := c.Do(req) + resp, err := net.HttpRequest(req, 60*time.Second) require.Nil(t, err, "%+v", err) defer resp.Body.Close() diff --git a/rpc/jsonrpc/client/test_util.go b/rpc/jsonrpc/client/test_util.go index 310186b96..ca0068c74 100644 --- a/rpc/jsonrpc/client/test_util.go +++ b/rpc/jsonrpc/client/test_util.go @@ -12,6 +12,7 @@ func HTTPClientForTest(remoteAddr string) (*http.Client, error) { dialFn, _ := makeHTTPDialer(remoteAddr) client := &http.Client{ + Timeout: 60 * time.Second, Transport: &http.Transport{ // Set to true to prevent GZIP-bomb DoS attacks DisableCompression: true, From 8dbfadac24285d62b9bfe7c21e7a6284031d3b56 Mon Sep 17 00:00:00 2001 From: TAKAMI Torao Date: Thu, 24 Nov 2022 11:53:55 +0900 Subject: [PATCH 2/4] change server-side timeout method from Lock to channel --- libs/net/http_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/libs/net/http_test.go b/libs/net/http_test.go index c641cd2c0..05f7e08e5 100644 --- a/libs/net/http_test.go +++ b/libs/net/http_test.go @@ -5,7 +5,6 @@ import ( "io" "net/http" "net/http/httptest" - "sync" "testing" "time" ) @@ -30,17 +29,15 @@ func TestHttpGet(t *testing.T) { } func TestHttpGetWithTimeout(t *testing.T) { - var mtx sync.Mutex + shutdown := make(chan string) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - mtx.Lock() - mtx.Unlock() + var _ = <-shutdown })) defer server.Close() accuracy := 0.05 timeout := 10 * time.Second - mtx.Lock() - defer mtx.Unlock() + defer func() { shutdown <- "shutdown" }() t0 := time.Now() _, err := HttpGet(server.URL, timeout) t1 := time.Now() From 42fcf13ce27e799738b5be196523a8d12ffb44a1 Mon Sep 17 00:00:00 2001 From: TAKAMI Torao Date: Thu, 24 Nov 2022 13:45:49 +0900 Subject: [PATCH 3/4] add test case that fails to create new request --- libs/net/http.go | 2 +- libs/net/http_test.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/libs/net/http.go b/libs/net/http.go index 577feafe3..5cf0505c7 100644 --- a/libs/net/http.go +++ b/libs/net/http.go @@ -8,7 +8,7 @@ import ( // HttpGet sends a GET request to the specified url with timeout and return the response. func HttpGet(url string, timeout time.Duration) (*http.Response, error) { - request, err := http.NewRequestWithContext(context.Background(), "GET", url, nil) + request, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) if err != nil { return nil, err } diff --git a/libs/net/http_test.go b/libs/net/http_test.go index 05f7e08e5..752a3c148 100644 --- a/libs/net/http_test.go +++ b/libs/net/http_test.go @@ -47,3 +47,8 @@ func TestHttpGetWithTimeout(t *testing.T) { require.InDeltaf(t, timeout.Seconds(), delta, accuracy, "response time of %.3f sec exceeded +%d%% of the expected timeout of %.3f sec", delta, uint(accuracy*100), timeout.Seconds()) } + +func TestHttpGetWithInvalidURL(t *testing.T) { + _, err := HttpGet("\n", 0*time.Second) + require.Error(t, err) +} From f690d36801fa20c4b795fc8ed89d460525c17952 Mon Sep 17 00:00:00 2001 From: TAKAMI Torao Date: Thu, 24 Nov 2022 17:05:19 +0900 Subject: [PATCH 4/4] rearrange imports --- p2p/switch_test.go | 2 +- p2p/upnp/upnp.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/p2p/switch_test.go b/p2p/switch_test.go index 8829377b0..2e6e9ddde 100644 --- a/p2p/switch_test.go +++ b/p2p/switch_test.go @@ -4,7 +4,6 @@ import ( "bytes" "errors" "fmt" - net2 "github.com/line/ostracon/libs/net" "io/ioutil" "net" "net/http/httptest" @@ -21,6 +20,7 @@ import ( "github.com/line/ostracon/config" "github.com/line/ostracon/crypto/ed25519" "github.com/line/ostracon/libs/log" + net2 "github.com/line/ostracon/libs/net" tmsync "github.com/line/ostracon/libs/sync" "github.com/line/ostracon/p2p/conn" ) diff --git a/p2p/upnp/upnp.go b/p2p/upnp/upnp.go index 83ba66e69..e3bbe5eda 100644 --- a/p2p/upnp/upnp.go +++ b/p2p/upnp/upnp.go @@ -10,13 +10,14 @@ import ( "encoding/xml" "errors" "fmt" - net2 "github.com/line/ostracon/libs/net" "io/ioutil" "net" "net/http" "strconv" "strings" "time" + + net2 "github.com/line/ostracon/libs/net" ) const HttpRequestTimeout = 30 * time.Second