cli: set content length on operator api requests

[lgfa29]

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.

[lgfa29]

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.

[lgfa29]

I'm not sure if this is actually the best way to handle this, so feel free to suggest any alternative implementation 😅

[schmichael]

Hm, a generic HTTP client intended to work with any HTTP server should use chunked TransferEncoding here...

...but that's not the world this code lives in! This is a client for the Nomad API, and the Nomad API (1) peeks at ContentLength in some places as you note, and (2) has no APIs that expect large request bodies.

So this seems like a good approach and has the added benefit of failing early if stdin breaks.

[lgfa29]

I also don't know if this is the best way to test it. Messing with os.Stdin seems dangerous 😬

[shoenig]

I haven't tried this, but maybe we could leave os.Stdin alone and just write the content of tmpfile to /dev/stdin ?

[lgfa29]

Hum...it doesn't seem to like it (or maybe I'm doing it wrong 😅):

write /dev/stdin: bad file descriptor

Here's what I tried:

diff --git a/command/operator_api_test.go b/command/operator_api_test.go
index 21c390602..1e3ff5e6e 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"
@@ -120,24 +119,15 @@ func TestOperatorAPICommand_ContentLength(t *testing.T) {
 	}))
 	defer ts.Close()
 
+	input := "test-input"
+
 	// 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)
+	_, err := os.Stdin.WriteString(input)
 	require.NoError(t, err)
-	_, err = tmpfile.Seek(0, 0)
+	_, err = os.Stdin.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{

One thing that did work was to do something like this:

diff --git a/command/operator_api.go b/command/operator_api.go
index 7544b1807..ebc322ba4 100644
--- a/command/operator_api.go
+++ b/command/operator_api.go
@@ -18,6 +18,8 @@ import (
        "github.com/posener/complete"
 )

+var Stdin = os.Stdin
+
 type OperatorAPICommand struct {
        Meta

@@ -141,13 +143,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 21c390602..e58de06c6 100644
--- a/command/operator_api_test.go
+++ b/command/operator_api_test.go
@@ -133,10 +133,7 @@ func TestOperatorAPICommand_ContentLength(t *testing.T) {
        _, err = tmpfile.Seek(0, 0)
        require.NoError(t, err)

-       oldStdin := os.Stdin
-       defer func() { os.Stdin = oldStdin }()
-
-       os.Stdin = tmpfile
+       Stdin = tmpfile

        // Setup command.
        buf := bytes.NewBuffer(nil)

But kind of weird to have that random var Stdin = os.Stdin just for testing? 😬

[shoenig]

Yeah, at most we should .Skip this test out; overriding a global like that is just asking for trouble later.

Seems like the only thing we can really do is replace the direct use of Stdin with a plumbed through Reader that we can then configure in the test. Depending on how deep that hole goes, might not be worth it just for this one test case (which we could prove works in e2e anyway).

[lgfa29]

Seems like the only thing we can really do is replace the direct use of Stdin with a plumbed through Reader that we can then configure in the test.

Yeah, that was the second approach I sent. Not sure how I feel about it 😅

Testing it in E2E could work, thought I'm not sure how to intercept the request to check the content length. I will see if I can figure something out.

[lgfa29]

I couldn't find a way to check the content lengh of a request in the E2E suite. How do you find the approach in 1e31021? It doesn't modify os.Stdin anymore, but it does touch a global package variable.

[schmichael]

Hm, a generic HTTP client intended to work with any HTTP server should use chunked TransferEncoding here...

...but that's not the world this code lives in! This is a client for the Nomad API, and the Nomad API (1) peeks at ContentLength in some places as you note, and (2) has no APIs that expect large request bodies.

So this seems like a good approach and has the added benefit of failing early if stdin breaks.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.