Skip to content

Commit

Permalink
fix to apply timeout to HTTP request
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
torao committed Nov 24, 2022
1 parent e6f57a1 commit 72d4ebe
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 9 deletions.
5 changes: 3 additions & 2 deletions cmd/ostracon/commands/debug/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
Expand Down
26 changes: 26 additions & 0 deletions libs/net/http.go
Original file line number Diff line number Diff line change
@@ -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)
}
52 changes: 52 additions & 0 deletions libs/net/http_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
4 changes: 2 additions & 2 deletions p2p/switch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions p2p/upnp/upnp.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"encoding/xml"
"errors"
"fmt"
net2 "github.com/line/ostracon/libs/net"
"io/ioutil"
"net"
"net/http"
Expand All @@ -18,6 +19,8 @@ import (
"time"
)

const HttpRequestTimeout = 30 * time.Second

type upnpNAT struct {
serviceURL string
ourIP string
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
6 changes: 3 additions & 3 deletions rpc/client/rpc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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()

Expand Down
1 change: 1 addition & 0 deletions rpc/jsonrpc/client/test_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 72d4ebe

Please sign in to comment.