From 9ac0c0a7c513962fd9abb5f763830d3ab3ead8e9 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 20 Sep 2022 22:23:36 -0400 Subject: [PATCH 1/3] cli: set content length on `operator api` requests http.NewRequestWithContext will only set the right value for Content-Length if the input is *bytes.Buffer, *bytes.Reader, or *strings.Reader [0]. Since os.Stdin is an os.File, POST requests made with the `nomad operator api` command would always have Content-Length set to -1, which is interpreted as an unknown length by web servers. [0]: https://pkg.go.dev/net/http#NewRequestWithContext --- command/operator_api.go | 14 ++++++++-- command/operator_api_test.go | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/command/operator_api.go b/command/operator_api.go index 9ff7b9c9a5fc..b7812aad2ef1 100644 --- a/command/operator_api.go +++ b/command/operator_api.go @@ -1,9 +1,11 @@ package command import ( + "bytes" "crypto/tls" "fmt" "io" + "io/ioutil" "net" "net/http" "net/url" @@ -31,7 +33,7 @@ Usage: nomad operator api [options] api is a utility command for accessing Nomad's HTTP API and is inspired by the popular curl command line tool. Nomad's operator api command populates Nomad's standard environment variables into their appropriate HTTP headers. - If the 'path' does not begin with "http" then $NOMAD_ADDR will be used. + If the 'path' does not begin with "http" then $NOMAD_ADDR will be used. The 'path' can be in one of the following forms: @@ -142,7 +144,15 @@ func (c *OperatorAPICommand) Run(args []string) int { stat, _ := os.Stdin.Stat() if (stat.Mode() & os.ModeCharDevice) == 0 { verbose("* Reading request body from stdin.") - c.body = os.Stdin + + // Load stdin into a *bytes.Reader so that http.NewRequest can set the + // correct Content-Length value. + b, err := ioutil.ReadAll(os.Stdin) + if err != nil { + c.Ui.Error(fmt.Sprintf("Error reading stdin: %v", err)) + return 1 + } + c.body = bytes.NewReader(b) if c.method == "" { c.method = "POST" } diff --git a/command/operator_api_test.go b/command/operator_api_test.go index 4292cd586cec..9a17d31bd619 100644 --- a/command/operator_api_test.go +++ b/command/operator_api_test.go @@ -2,9 +2,11 @@ package command import ( "bytes" + "io/ioutil" "net/http" "net/http/httptest" "net/url" + "os" "testing" "time" @@ -170,3 +172,52 @@ func Test_pathToURL(t *testing.T) { }) } } + +// TestOperatorAPICommand_ContentLength tests that requests have the proper +// ContentLength set. +// +// Don't run in parallel since it messes with os.Stdin. +func TestOperatorAPICommand_ContentLength(t *testing.T) { + contentLength := make(chan int, 1) + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + contentLength <- int(r.ContentLength) + })) + defer ts.Close() + + // Setup os.Stdin to a known value. + // The command stats stdin, so we can't mock it as another io.Reader. + // https://stackoverflow.com/a/46365584 + input := []byte("test-input") + tmpfile, err := ioutil.TempFile("", "example") + require.NoError(t, err) + defer os.Remove(tmpfile.Name()) + + _, err = tmpfile.Write(input) + require.NoError(t, err) + _, err = tmpfile.Seek(0, 0) + require.NoError(t, err) + + oldStdin := os.Stdin + defer func() { os.Stdin = oldStdin }() + + os.Stdin = tmpfile + + // Setup command. + buf := bytes.NewBuffer(nil) + ui := &cli.BasicUi{ + ErrorWriter: buf, + Writer: buf, + } + cmd := &OperatorAPICommand{Meta: Meta{Ui: ui}} + + // Assert that a request has the expected content length. + exitCode := cmd.Run([]string{"-address=" + ts.URL, "/v1/jobs"}) + require.Zero(t, exitCode, buf.String()) + + select { + case l := <-contentLength: + require.Equal(t, len(input), l) + case <-time.After(10 * time.Second): + t.Fatalf("timed out waiting for request") + } +} From 26f06f01970e10ac6b85d833027d6dc173531667 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 20 Sep 2022 22:28:24 -0400 Subject: [PATCH 2/3] changelog: add entry for #14634 --- .changelog/14634.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/14634.txt diff --git a/.changelog/14634.txt b/.changelog/14634.txt new file mode 100644 index 000000000000..ccab596c8555 --- /dev/null +++ b/.changelog/14634.txt @@ -0,0 +1,3 @@ +```release-note:bug +cli: set content length on POST requests when using the `nomad operator api` command +``` From 1e31021977e2e3150d3c6e9fb12653e8510d8243 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Wed, 21 Sep 2022 12:33:06 -0400 Subject: [PATCH 3/3] test: don't override os.Stdin --- command/operator_api.go | 8 ++++++-- command/operator_api_test.go | 22 +++++++++------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/command/operator_api.go b/command/operator_api.go index b7812aad2ef1..36dd375b241b 100644 --- a/command/operator_api.go +++ b/command/operator_api.go @@ -18,6 +18,10 @@ import ( "github.com/posener/complete" ) +// Stdin represents the system's standard input, but it's declared as a +// variable here to allow tests to override it with a regular file. +var Stdin = os.Stdin + type OperatorAPICommand struct { Meta @@ -141,13 +145,13 @@ func (c *OperatorAPICommand) Run(args []string) int { // Opportunistically read from stdin and POST unless method has been // explicitly set. - stat, _ := os.Stdin.Stat() + stat, _ := Stdin.Stat() if (stat.Mode() & os.ModeCharDevice) == 0 { verbose("* Reading request body from stdin.") // Load stdin into a *bytes.Reader so that http.NewRequest can set the // correct Content-Length value. - b, err := ioutil.ReadAll(os.Stdin) + b, err := ioutil.ReadAll(Stdin) if err != nil { c.Ui.Error(fmt.Sprintf("Error reading stdin: %v", err)) return 1 diff --git a/command/operator_api_test.go b/command/operator_api_test.go index 9a17d31bd619..b71e65787f07 100644 --- a/command/operator_api_test.go +++ b/command/operator_api_test.go @@ -2,7 +2,6 @@ package command import ( "bytes" - "io/ioutil" "net/http" "net/http/httptest" "net/url" @@ -176,7 +175,7 @@ func Test_pathToURL(t *testing.T) { // TestOperatorAPICommand_ContentLength tests that requests have the proper // ContentLength set. // -// Don't run in parallel since it messes with os.Stdin. +// Don't run it in parallel as it modifies the package's Stdin variable. func TestOperatorAPICommand_ContentLength(t *testing.T) { contentLength := make(chan int, 1) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -184,23 +183,20 @@ func TestOperatorAPICommand_ContentLength(t *testing.T) { })) defer ts.Close() - // Setup os.Stdin to a known value. - // The command stats stdin, so we can't mock it as another io.Reader. - // https://stackoverflow.com/a/46365584 + // Setup a temp file to act as stdin. input := []byte("test-input") - tmpfile, err := ioutil.TempFile("", "example") + fakeStdin, err := os.CreateTemp("", "fake-stdin") require.NoError(t, err) - defer os.Remove(tmpfile.Name()) + defer os.Remove(fakeStdin.Name()) - _, err = tmpfile.Write(input) + _, err = fakeStdin.Write(input) require.NoError(t, err) - _, err = tmpfile.Seek(0, 0) + _, err = fakeStdin.Seek(0, 0) require.NoError(t, err) - oldStdin := os.Stdin - defer func() { os.Stdin = oldStdin }() - - os.Stdin = tmpfile + // Override the package's Stdin variable for testing. + Stdin = fakeStdin + defer func() { Stdin = os.Stdin }() // Setup command. buf := bytes.NewBuffer(nil)