Skip to content

jsonclient: surface HTTP Do and Read errors #1695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mhutchinson merged 1 commit into from
May 6, 2025
Merged

jsonclient: surface HTTP Do and Read errors #1695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 20 additions & 26 deletions jsonclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (

ct "github.com/google/certificate-transparency-go"
"github.com/google/certificate-transparency-go/x509"
"golang.org/x/net/context/ctxhttp"
"k8s.io/klog/v2"
)

Expand Down Expand Up @@ -171,7 +170,8 @@ func (c *JSONClient) BaseURI() string {
// GetAndParse makes a HTTP GET call to the given path, and attempts to parse
// the response as a JSON representation of the rsp structure. Returns the
// http.Response, the body of the response, and an error (which may be of
// type RspError if the HTTP response was available).
// type RspError if the HTTP response was available). It returns an error
// if the response status code is not 200 OK.
func (c *JSONClient) GetAndParse(ctx context.Context, path string, params map[string]string, rsp interface{}) (*http.Response, []byte, error) {
if ctx == nil {
return nil, nil, errors.New("context.Context required")
Expand All @@ -183,7 +183,7 @@ func (c *JSONClient) GetAndParse(ctx context.Context, path string, params map[st
}
fullURI := fmt.Sprintf("%s%s?%s", c.uri, path, vals.Encode())
klog.V(2).Infof("GET %s", fullURI)
httpReq, err := http.NewRequest(http.MethodGet, fullURI, nil)
httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, fullURI, nil)
if err != nil {
return nil, nil, err
}
Expand All @@ -194,20 +194,17 @@ func (c *JSONClient) GetAndParse(ctx context.Context, path string, params map[st
httpReq.Header.Add("Authorization", c.authorization)
}

httpRsp, err := ctxhttp.Do(ctx, c.httpClient, httpReq)
httpRsp, err := c.httpClient.Do(httpReq)
if err != nil {
return nil, nil, err
}

// Read everything now so http.Client can reuse the connection.
body, err := io.ReadAll(httpRsp.Body)
if err := httpRsp.Body.Close(); err != nil {
return nil, nil, err
}
if err != nil {
return nil, nil, RspError{Err: fmt.Errorf("failed to read response body: %v", err), StatusCode: httpRsp.StatusCode, Body: body}
return nil, nil, RspError{Err: fmt.Errorf("failed to read response body: %w", err), StatusCode: httpRsp.StatusCode, Body: body}
}
if err := httpRsp.Body.Close(); err != nil {
return nil, nil, RspError{Err: fmt.Errorf("failed to close response body: %w", err), StatusCode: httpRsp.StatusCode, Body: body}
}

if httpRsp.StatusCode != http.StatusOK {
return nil, nil, RspError{Err: fmt.Errorf("got HTTP Status %q", httpRsp.Status), StatusCode: httpRsp.StatusCode, Body: body}
}
Expand All @@ -223,6 +220,7 @@ func (c *JSONClient) GetAndParse(ctx context.Context, path string, params map[st
// parameters, and attempts to parse the response as a JSON representation of
// the rsp structure. Returns the http.Response, the body of the response, and
// an error (which may be of type RspError if the HTTP response was available).
// It does NOT return an error if the response status code is not 200 OK.
func (c *JSONClient) PostAndParse(ctx context.Context, path string, req, rsp interface{}) (*http.Response, []byte, error) {
if ctx == nil {
return nil, nil, errors.New("context.Context required")
Expand All @@ -234,7 +232,7 @@ func (c *JSONClient) PostAndParse(ctx context.Context, path string, req, rsp int
}
fullURI := fmt.Sprintf("%s%s", c.uri, path)
klog.V(2).Infof("POST %s", fullURI)
httpReq, err := http.NewRequest(http.MethodPost, fullURI, bytes.NewReader(postBody))
httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, fullURI, bytes.NewReader(postBody))
if err != nil {
return nil, nil, err
}
Expand All @@ -246,20 +244,16 @@ func (c *JSONClient) PostAndParse(ctx context.Context, path string, req, rsp int
}
httpReq.Header.Set("Content-Type", "application/json")

httpRsp, err := ctxhttp.Do(ctx, c.httpClient, httpReq)

// Read all of the body, if there is one, so that the http.Client can do Keep-Alive.
var body []byte
if httpRsp != nil {
body, err = io.ReadAll(httpRsp.Body)
if err := httpRsp.Body.Close(); err != nil {
return nil, nil, err
}
httpRsp, err := c.httpClient.Do(httpReq)
if err != nil {
return nil, nil, err
}
body, err := io.ReadAll(httpRsp.Body)
if err != nil {
if httpRsp != nil {
return nil, nil, RspError{StatusCode: httpRsp.StatusCode, Body: body, Err: err}
}
_ = httpRsp.Body.Close()
return nil, nil, err
}
if err := httpRsp.Body.Close(); err != nil {
return nil, nil, err
}
if httpRsp.Request.Method != http.MethodPost {
Expand All @@ -268,7 +262,7 @@ func (c *JSONClient) PostAndParse(ctx context.Context, path string, req, rsp int
}

if httpRsp.StatusCode == http.StatusOK {
if err = json.Unmarshal(body, &rsp); err != nil {
if err := json.Unmarshal(body, &rsp); err != nil {
return nil, nil, RspError{StatusCode: httpRsp.StatusCode, Body: body, Err: err}
}
}
Expand Down Expand Up @@ -302,7 +296,7 @@ func (c *JSONClient) PostAndParseWithRetry(ctx context.Context, path string, req
httpRsp, body, err := c.PostAndParse(ctx, path, req, rsp)
if err != nil {
// Don't retry context errors.
if err == context.Canceled || err == context.DeadlineExceeded {
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return nil, nil, err
}
wait := c.backoff.set(nil)
Expand Down
7 changes: 4 additions & 3 deletions jsonclient/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"encoding/json"
"encoding/pem"
"errors"
"fmt"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -516,15 +517,15 @@ func TestCancelledContext(t *testing.T) {

var result TestStruct
_, _, err = logClient.GetAndParse(ctx, "/struct/path", nil, &result)
if err != context.Canceled {
if !errors.Is(err, context.Canceled) {
t.Errorf("GetAndParse() = (_,_,%v), want %q", err, context.Canceled)
}
_, _, err = logClient.PostAndParse(ctx, "/struct/path", nil, &result)
if err != context.Canceled {
if !errors.Is(err, context.Canceled) {
t.Errorf("PostAndParse() = (_,_,%v), want %q", err, context.Canceled)
}
_, _, err = logClient.PostAndParseWithRetry(ctx, "/struct/path", nil, &result)
if err != context.Canceled {
if !errors.Is(err, context.Canceled) {
t.Errorf("PostAndParseWithRetry() = (_,_,%v), want %q", err, context.Canceled)
}
}
5 changes: 2 additions & 3 deletions trillian/integration/ct_integration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ import (
"github.com/transparency-dev/merkle"
"github.com/transparency-dev/merkle/proof"
"github.com/transparency-dev/merkle/rfc6962"
"golang.org/x/net/context/ctxhttp"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/protobuf/types/known/fieldmaskpb"
Expand Down Expand Up @@ -905,13 +904,13 @@ func (ls *logStats) fromServer(ctx context.Context, servers string) (*logStats,

got := newLogStats(int64(ls.logID))
for _, s := range strings.Split(servers, ",") {
httpReq, err := http.NewRequest(http.MethodGet, "http://"+s+"/metrics", nil)
httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://"+s+"/metrics", nil)
if err != nil {
return nil, fmt.Errorf("failed to build GET request: %v", err)
}
c := new(http.Client)

httpRsp, err := ctxhttp.Do(ctx, c, httpReq)
httpRsp, err := c.Do(httpReq)
if err != nil {
return nil, fmt.Errorf("getting stats failed: %v", err)
}
Expand Down
Loading