Bug [net/http]: client.Timeout exceeded errors are not wrapped

yunginnanet commented 11 months ago

What version of Go are you using (`go version`)?

$ go version
go version go1.21.1 linux/amd64

issue also exists in master branch

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE='auto'
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/kayos/.cache/go-build'
GOENV='/home/kayos/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/kayos/go/pkg/mod'
GOOS='linux'
GOPATH='/home/kayos/go'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD=''
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build464817929=/tmp/go-build -gno-record-gcc-switches'

What did you do?

if _, err = GetPublicIP(); !errors.Is(err, context.DeadlineExceeded) {
    t.Errorf("expected error to be %v, got %v", context.DeadlineExceeded, 
}

from: https://github.com/gravitl/netmaker/pull/2616

Full Context

```golang package servercfg import ( "context" "errors" "net/http" "net/http/httptest" "os" "strings" "testing" "time" "github.com/gravitl/netmaker/config" ) func TestGetPublicIP(t *testing.T) { var testIP = "55.55.55.55" server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if _, err := w.Write([]byte(testIP)); err != nil { t.Errorf("expected no error, got %v", err) } })) defer server.Close() if err := os.Setenv("NETMAKER_TEST_IP_SERVICE", server.URL); err != nil { t.Logf("WARNING: could not set NETMAKER_TEST_IP_SERVICE env var") } t.Run("Use PUBLIC_IP_SERVICE if set", func(t *testing.T) { // set the environment variable if err := os.Setenv("PUBLIC_IP_SERVICE", server.URL); err != nil { t.Fatalf("expected no error, got %v", err) } defer func() { _ = os.Unsetenv("PUBLIC_IP_SERVICE") }() var ip string var err error if ip, err = GetPublicIP(); err != nil { t.Fatalf("expected no error, got %v", err) } if !strings.EqualFold(ip, testIP) { t.Errorf("expected IP to be %s, got %s", testIP, ip) } }) t.Run("Use config.Config.Server.PublicIPService if PUBLIC_IP_SERVICE isn't set", func(t *testing.T) { config.Config.Server.PublicIPService = server.URL defer func() { config.Config.Server.PublicIPService = "" }() ip, err := GetPublicIP() if err != nil { t.Fatalf("expected no error, got %v", err) } if ip != testIP { t.Fatalf("expected IP to be %s, got %s", testIP, ip) } }) t.Run("Handle service timeout", func(t *testing.T) { if os.Getenv("NETMAKER_TEST_IP_SERVICE") == "" { t.Skip("NETMAKER_TEST_IP_SERVICE not set") } var badTestIP = "123.45.67.91" badServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // intentionally delay to simulate a timeout time.Sleep(3 * time.Second) _, _ = w.Write([]byte(badTestIP)) })) defer badServer.Close() // we set this so that we can lower the timeout for the test to not hold up CI if err := os.Setenv("NETMAKER_TEST_BAD_IP_SERVICE", badServer.URL); err != nil { // but if we can't set it, we can't test it t.Skip("failed to set NETMAKER_TEST_BAD_IP_SERVICE, skipping test because we won't timeout") } defer func() { _ = os.Unsetenv("NETMAKER_TEST_BAD_IP_SERVICE") }() // mock the config oldConfig := config.Config.Server.PublicIPService config.Config.Server.PublicIPService = badServer.URL defer func() { config.Config.Server.PublicIPService = oldConfig }() res, err := GetPublicIP() if err != nil { t.Errorf("GetPublicIP() fallback has failed: %v", err) } if strings.EqualFold(res, badTestIP) { t.Errorf("GetPublicIP() returned the response from the server that should have timed out: %v", res) } if !strings.EqualFold(res, testIP) { t.Errorf("GetPublicIP() did not fallback to the correct IP: %v", res) } t.Run("Assert error is passed down", func(t *testing.T) { oldConfig = config.Config.Server.PublicIPService oldEnv := os.Getenv("NETMAKER_TEST_IP_SERVICE") // make sure that even the fallback fails if err = os.Setenv("NETMAKER_TEST_IP_SERVICE", badServer.URL); err != nil { t.Skipf("could not set NETMAKER_TEST_IP_SERVICE env var") } defer func() { _ = os.Setenv("NETMAKER_TEST_IP_SERVICE", oldEnv) }() if _, err = GetPublicIP(); !errors.Is(err, context.DeadlineExceeded) { t.Errorf("expected error to be %v, got %v", context.DeadlineExceeded, err) } }) }) } ```

What did you expect to see?

errors.Is(err, context.DeadlineExceeded) == true

What did you see instead?

    serverconf_test.go:110: expected error to be context deadline exceeded, got Get "http://127.0.0.1:38513": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

Proposed Fix

https://github.com/golang/go/compare/master...yunginnanet:go:net-http-error-wrapping

yunginnanet commented 11 months ago

ohp looks like i duplicated https://github.com/golang/go/issues/50856

seankhliao commented 11 months ago

Duplicate of #50856

yunginnanet commented 11 months ago

https://github.com/golang/go/pull/63448

golang / go