Skip to content

Fix signer receiving a drained body on retries #620

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.

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
merged 1 commit into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Fix ISM Transition to omitempty Conditions field ([#609](https://github.com/opensearch-project/opensearch-go/pull/609))
- Fix ISM Allocation field types ([#609](https://github.com/opensearch-project/opensearch-go/pull/609))
- Fix ISM Error Notification types ([#612](https://github.com/opensearch-project/opensearch-go/pull/612))
- Fix signer receiving drained body on retries ([#620](https://github.com/opensearch-project/opensearch-go/pull/620))

### Security

Expand Down
8 changes: 4 additions & 4 deletions opensearchtransport/opensearchtransport.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,6 @@ func (c *Client) Perform(req *http.Request) (*http.Response, error) {
c.setReqURL(conn.URL, req)
c.setReqAuth(conn.URL, req)

if err = c.signRequest(req); err != nil {
return nil, fmt.Errorf("failed to sign request: %w", err)
}

if !c.disableRetry && i > 0 && req.Body != nil && req.Body != http.NoBody {
body, err := req.GetBody()
if err != nil {
Expand All @@ -304,6 +300,10 @@ func (c *Client) Perform(req *http.Request) (*http.Response, error) {
req.Body = body
}

if err = c.signRequest(req); err != nil {
return nil, fmt.Errorf("failed to sign request: %w", err)
}

// Set up time measures and execute the request
start := time.Now().UTC()
res, err = c.transport.RoundTrip(req)
Expand Down
45 changes: 45 additions & 0 deletions opensearchtransport/opensearchtransport_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,13 @@ type mockSigner struct {
SampleKey string
SampleValue string
ReturnError bool
testHook func(*http.Request)
}

func (m *mockSigner) SignRequest(req *http.Request) error {
if m.testHook != nil {
m.testHook(req)
}
if m.ReturnError {
return fmt.Errorf("invalid data")
}
Expand Down Expand Up @@ -732,6 +736,47 @@ func TestTransportPerformRetries(t *testing.T) {
}
})

t.Run("Signer can sign correctly during retry", func(t *testing.T) {
u, _ := url.Parse("https://foo.com/bar")
signer := mockSigner{}
callsToSigner := 0
expectedBody := "FOOBAR"

signer.testHook = func(req *http.Request) {
callsToSigner++
body, err := io.ReadAll(req.Body)
if err != nil {
panic(err)
}
if string(body) != expectedBody {
t.Fatalf("request %d body: expected %q, got %q", callsToSigner, expectedBody, body)
}
}

tp, _ := New(
Config{
URLs: []*url.URL{u},
Signer: &signer,
Transport: &mockTransp{
RoundTripFunc: func(req *http.Request) (*http.Response, error) {
return &http.Response{Status: "MOCK", StatusCode: http.StatusBadGateway}, nil
},
},
},
)

req, _ := http.NewRequest(http.MethodPost, "/abc", strings.NewReader(expectedBody))
//nolint:bodyclose // Mock response does not have a body to close
_, err := tp.Perform(req)
if err != nil {
t.Fatalf("Unexpected error: %s", err)
}

if callsToSigner != 4 {
t.Fatalf("expected 4 requests, got %d", callsToSigner)
}
})

t.Run("Don't retry request on regular error", func(t *testing.T) {
var i int

Expand Down
Loading