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

r2.Request should follow 307 on non-GET #177

Closed
dhermes opened this issue Feb 1, 2020 · 3 comments
Closed

r2.Request should follow 307 on non-GET #177

dhermes opened this issue Feb 1, 2020 · 3 comments
Assignees
Labels
bug Something isn't working r2 applies to the r2 package v3.x.y Version 3 series

Comments

@dhermes
Copy link
Contributor

dhermes commented Feb 1, 2020

See #175 for context. To test

package main

import (
	"fmt"
	"net/http"
	"net/http/httptest"
	"net/url"

	"github.com/blend/go-sdk/r2"
)

func main() {
	path := "/v1/auth/token/lookup"
	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.URL.Path == path {
			http.Redirect(w, r, "/more", http.StatusTemporaryRedirect)
			return
		}
		fmt.Fprintln(w, "Hello, client")
	}))
	defer ts.Close()
	u, err := url.Parse(ts.URL)
	if err != nil {
		panic(err)
	}

	// Do a GET.
	fmt.Println("--- Doing a GET ---")
	opts := []r2.Option{
		r2.OptScheme("http"),
		r2.OptHeaderValue("X-Vault-Token", "bar"),
		r2.OptHost(u.Host),
		r2.OptGet(),
		r2.OptPathf(path),
	}
	body, res, err := r2.New("", opts...).Bytes()
	if err != nil {
		panic(err)
	}
	fmt.Printf("body: %q\n\n%#v\n", string(body), res)

	data := map[string]string{"token": "foo"}
	// Do a POST.
	fmt.Println("--- Doing a POST ---")
	opts = []r2.Option{
		r2.OptScheme("http"),
		r2.OptHeaderValue("X-Vault-Token", "bar"),
		r2.OptHost(u.Host),
		r2.OptPost(),
		r2.OptPathf(path),
		r2.OptJSONBody(data),
	}
	r := r2.New("", opts...)
	body, res, err = r.Bytes()
	if err != nil {
		panic(err)
	}
	fmt.Printf("body: %q\n\n%#v\n", string(body), res)
}

which produces

--- Doing a GET ---
body: "Hello, client\n"

&http.Response{Status:"200 OK", StatusCode:200, Proto:"HTTP/1.1", ProtoMajor:1, ProtoMinor:1, Header:http.Header{"Content-Length":[]string{"14"}, "Content-Type":[]string{"text/plain; charset=utf-8"}, "Date":[]string{"Sat, 01 Feb 2020 00:01:16 GMT"}}, Body:(*http.bodyEOFSignal)(0xc0001bc100), ContentLength:14, TransferEncoding:[]string(nil), Close:false, Uncompressed:false, Trailer:http.Header(nil), Request:(*http.Request)(0xc0001c8000), TLS:(*tls.ConnectionState)(nil)}
--- Doing a POST ---
body: ""

&http.Response{Status:"307 Temporary Redirect", StatusCode:307, Proto:"HTTP/1.1", ProtoMajor:1, ProtoMinor:1, Header:http.Header{"Content-Length":[]string{"0"}, "Date":[]string{"Sat, 01 Feb 2020 00:01:16 GMT"}, "Location":[]string{"/more"}}, Body:http.noBody{}, ContentLength:0, TransferEncoding:[]string(nil), Close:false, Uncompressed:false, Trailer:http.Header(nil), Request:(*http.Request)(0xc000176420), TLS:(*tls.ConnectionState)(nil)}
@dhermes dhermes added bug Something isn't working r2 applies to the r2 package labels Feb 1, 2020
@dhermes dhermes self-assigned this Feb 1, 2020
@dhermes
Copy link
Contributor Author

dhermes commented Feb 1, 2020

OK, I tracked down what is caused this, it is the absence of r.Request.GetBody. This boils down to the same core issue as #154, #143, #141: We're trying to recreate what http.NewRequest() does by struct-embedding http.Request, but IMO we should not be struct embedding it because there are so many corner cases.

Applying the following updates to the script above

diff --git a/main.go b/main.go
index 8b95934..80a972e 100644
--- a/main.go
+++ b/main.go
@@ -1,7 +1,11 @@
 package main
 
 import (
+	"bytes"
+	"encoding/json"
 	"fmt"
+	"io"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -40,6 +44,10 @@ func main() {
 	fmt.Printf("body: %q\n\n%#v\n", string(body), res)
 
 	data := map[string]string{"token": "foo"}
+	bodyBytes, err := json.Marshal(data)
+	if err != nil {
+		panic(err)
+	}
 	// Do a POST.
 	fmt.Println("--- Doing a POST ---")
 	opts = []r2.Option{
@@ -51,6 +59,9 @@ func main() {
 		r2.OptJSONBody(data),
 	}
 	r := r2.New("", opts...)
+	r.Request.GetBody = func() (io.ReadCloser, error) {
+		return ioutil.NopCloser(bytes.NewReader(bodyBytes)), nil
+	}
 	body, res, err = r.Bytes()
 	if err != nil {
 		panic(err)

we get

--- Doing a GET ---
body: "Hello, client\n"

&http.Response{Status:"200 OK", StatusCode:200, Proto:"HTTP/1.1", ProtoMajor:1, ProtoMinor:1, Header:http.Header{"Content-Length":[]string{"14"}, "Content-Type":[]string{"text/plain; charset=utf-8"}, "Date":[]string{"Sat, 01 Feb 2020 06:09:57 GMT"}}, Body:(*http.bodyEOFSignal)(0xc0001da040), ContentLength:14, TransferEncoding:[]string(nil), Close:false, Uncompressed:false, Trailer:http.Header(nil), Request:(*http.Request)(0xc00019a200), TLS:(*tls.ConnectionState)(nil)}
--- Doing a POST ---
body: "Hello, client\n"

&http.Response{Status:"200 OK", StatusCode:200, Proto:"HTTP/1.1", ProtoMajor:1, ProtoMinor:1, Header:http.Header{"Content-Length":[]string{"14"}, "Content-Type":[]string{"text/plain; charset=utf-8"}, "Date":[]string{"Sat, 01 Feb 2020 06:09:57 GMT"}}, Body:(*http.bodyEOFSignal)(0xc0000d8640), ContentLength:14, TransferEncoding:[]string(nil), Close:false, Uncompressed:false, Trailer:http.Header(nil), Request:(*http.Request)(0xc00019a300), TLS:(*tls.ConnectionState)(nil)}

@dhermes dhermes added the v3.x.y Version 3 series label Feb 1, 2020
@dhermes
Copy link
Contributor Author

dhermes commented Feb 1, 2020

From the NewRequestWithContext in the standard library:

			req.GetBody = func() (io.ReadCloser, error) {
				r := bytes.NewReader(buf)
				return ioutil.NopCloser(r), nil
			}

@dhermes
Copy link
Contributor Author

dhermes commented Feb 2, 2020

This was fixed by #178.

@dhermes dhermes closed this as completed Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working r2 applies to the r2 package v3.x.y Version 3 series
Projects
None yet
Development

No branches or pull requests

1 participant