Skip to content
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

cli: set content length on operator api requests #14634

Merged
merged 3 commits into from
Sep 26, 2022
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
3 changes: 3 additions & 0 deletions .changelog/14634.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was debating if this should be a bug or a improvement, but since it did cause an issue for me (using nomad operator api in a non-TTY session would send a -1 content length, causing endpoints like /v1/acl/bootstrap to attempt decode an empty string), I set it as bug.

cli: set content length on POST requests when using the `nomad operator api` command
```
20 changes: 17 additions & 3 deletions command/operator_api.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package command

import (
"bytes"
"crypto/tls"
"fmt"
"io"
"io/ioutil"
"net"
"net/http"
"net/url"
Expand All @@ -16,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

Expand All @@ -31,7 +37,7 @@ Usage: nomad operator api [options] <path>
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:

Expand Down Expand Up @@ -139,10 +145,18 @@ 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.")
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(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"
}
Expand Down
47 changes: 47 additions & 0 deletions command/operator_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net/http"
"net/http/httptest"
"net/url"
"os"
"testing"
"time"

Expand Down Expand Up @@ -170,3 +171,49 @@ func Test_pathToURL(t *testing.T) {
})
}
}

// TestOperatorAPICommand_ContentLength tests that requests have the proper
// ContentLength set.
//
// 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) {
contentLength <- int(r.ContentLength)
}))
defer ts.Close()

// Setup a temp file to act as stdin.
input := []byte("test-input")
fakeStdin, err := os.CreateTemp("", "fake-stdin")
require.NoError(t, err)
defer os.Remove(fakeStdin.Name())

_, err = fakeStdin.Write(input)
require.NoError(t, err)
_, err = fakeStdin.Seek(0, 0)
require.NoError(t, err)

// Override the package's Stdin variable for testing.
Stdin = fakeStdin
defer func() { Stdin = os.Stdin }()

// 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")
}
}