Skip to content
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
28 changes: 26 additions & 2 deletions nmagent/internal/wireserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,32 @@ func (w *WireserverTransport) RoundTrip(inReq *http.Request) (*http.Response, er
}

// all POST requests (and by extension, PUT) must have a non-nil body
if req.Method == http.MethodPost && req.Body == nil {
req.Body = io.NopCloser(strings.NewReader(""))
if req.Method == http.MethodPost {
if req.Body == nil || req.Body == http.NoBody {
// the non-nil body that Wireserver expects is an empty JSON string. This
// is not the same as an empty Go string. The equivalent Go string is one
// with two quote characters:
emptyJSONString := `""`

// Body is expected to be an io.ReadCloser of some type, so this string
// needs to be dressed in appropriate wrapping types before it can be
// assigned:
req.Body = io.NopCloser(strings.NewReader(emptyJSONString))

// also, because the Body is a Reader, its length cannot be known apriori.
// Thus, we need to manually set this length, otherwise it will be sent
// with no Content-Length header (and Transfer-Encoding: chunked instead).
// Wireserver gets angry when there's no Content-Length header, and returns
// a 411 status code:
req.ContentLength = int64(len(emptyJSONString))

// the Content-Type must also be manually set, because the net/http
// Content-Type detection can't be run without draining the Reader set
// for the Request Body. This is a good idea anyway, because the
// detection uses heuristic methods to figure out the Content-Type, which
// may not necessarily be correct for a short JSON string like this:
req.Header.Set(HeaderContentType, MimeJSON)
}
}

// execute the request to the downstream transport
Expand Down
18 changes: 12 additions & 6 deletions nmagent/internal/wireserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package internal

import (
"encoding/json"
"io"
"net/http"
"net/http/httptest"
"testing"
Expand Down Expand Up @@ -220,12 +221,16 @@ func TestWireserverTransportPostBody(t *testing.T) {
// all PUT and POST requests must have an empty string body
t.Parallel()

bodyIsNil := false
var gotBody []byte
client := &http.Client{
Transport: &WireserverTransport{
Transport: &TestTripper{
RoundTripF: func(req *http.Request) (*http.Response, error) {
bodyIsNil = req.Body == nil
body, err := io.ReadAll(req.Body)
if err != nil {
t.Fatal("unexpected error reading request body: err:", err)
}
gotBody = body
rr := httptest.NewRecorder()
_, _ = rr.WriteString(`{"httpStatusCode": "200"}`)
rr.WriteHeader(http.StatusOK)
Expand All @@ -247,8 +252,9 @@ func TestWireserverTransportPostBody(t *testing.T) {
}
defer resp.Body.Close()

if bodyIsNil {
t.Error("downstream request body to wireserver was nil, but not expected to be")
exp := `""` // an empty JSON string
if got := string(gotBody); got != exp {
t.Errorf("unexpected body received: got: %q, exp: %q\n", got, exp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is the new line needed? same for others.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ramiro-gamarra do you mean the changed content, or a literal \n? There are no extra \ns (look at the line number sequence). It might be a GH display bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the \n in the error message

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so apparently, the presence or absence of it makes no difference in the output--they're stripped internally. I add them out of habit it because it matters for fmt.Println vs fmt.Printf. Since it won't create janky output, I'll keep it in mind for next time, but I won't fix it here to avoid blocking the merge.

}

// POST request
Expand All @@ -264,8 +270,8 @@ func TestWireserverTransportPostBody(t *testing.T) {
}
defer resp.Body.Close()

if bodyIsNil {
t.Error("downstream request body to wireserver was nil, but not expected to be")
if got := string(gotBody); got != exp {
t.Errorf("unexpected body received: got: %q, exp: %q\n", got, exp)
}
}

Expand Down