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

support not follow redirect for HTTP client #702

Merged
merged 11 commits into from May 1, 2020
7 changes: 6 additions & 1 deletion codegen/template_bundle/template_files.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions codegen/templates/http_client.tmpl
Expand Up @@ -90,6 +90,10 @@ func {{$exportName}}(deps *module.Dependencies) Client {
if deps.Default.Config.ContainsKey("http.clients.requestUUIDHeaderKey") {
requestUUIDHeaderKey = deps.Default.Config.MustGetString("http.clients.requestUUIDHeaderKey")
}
followRedirect := true
if deps.Default.Config.ContainsKey("clients.{{$clientID}}.followRedirect") {
followRedirect = deps.Default.Config.MustGetBoolean("clients.{{$clientID}}.followRedirect")
}


circuitBreakerDisabled := configureCicruitBreaker(deps, timeoutVal)
Expand All @@ -113,6 +117,7 @@ func {{$exportName}}(deps *module.Dependencies) Client {
baseURL,
defaultHeaders,
timeout,
followRedirect,
),
circuitBreakerDisabled: circuitBreakerDisabled,
requestUUIDHeaderKey: requestUUIDHeaderKey,
Expand Down
1 change: 1 addition & 0 deletions codegen/test_data/clients/bar.gogen
Expand Up @@ -248,6 +248,7 @@ func NewClient(deps *module.Dependencies) Client {
baseURL,
defaultHeaders,
timeout,
true,
),
}
}
Expand Down
5 changes: 5 additions & 0 deletions examples/example-gateway/build/clients/bar/bar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions examples/example-gateway/build/clients/contacts/contacts.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions examples/example-gateway/build/clients/multi/multi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
@@ -1,5 +1,6 @@
{
"dependencies": {
"eclint": "^1.1.5",
"uber-licence": "^3.1.1"
}
}
1 change: 1 addition & 0 deletions runtime/client_http_request_test.go
Expand Up @@ -75,6 +75,7 @@ func TestMakingClientWriteJSONWithBadJSON(t *testing.T) {
"/",
map[string]string{},
time.Second,
true,
)
ctx := context.Background()
req := zanzibar.NewClientHTTPRequest(ctx, "clientID", "DoStuff", "clientID::DoStuff", client)
Expand Down
91 changes: 88 additions & 3 deletions runtime/client_http_response_test.go
Expand Up @@ -78,6 +78,7 @@ func TestReadAndUnmarshalNonStructBody(t *testing.T) {
baseURL,
map[string]string{},
time.Second,
true,
)
ctx := context.Background()

Expand Down Expand Up @@ -139,6 +140,7 @@ func TestReadAndUnmarshalNonStructBodyUnmarshalError(t *testing.T) {
baseURL,
map[string]string{},
time.Second,
true,
)
ctx := context.Background()

Expand Down Expand Up @@ -177,16 +179,25 @@ func TestUnknownStatusCode(t *testing.T) {

bgateway := gateway.(*benchGateway.BenchGateway)

addr := bgateway.HTTPBackends()["bar"].RealAddr
baseURL := "http://" + addr

// test UnknownStatusCode along with follow redirect by default
fakeEcho := func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("Location", baseURL+"/bar-path")
w.WriteHeader(303)
_, err := w.Write([]byte(`false`))
assert.NoError(t, err)
}

fakeNormal := func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(999)
_, err := w.Write([]byte(`false`))
assert.NoError(t, err)
}

bgateway.HTTPBackends()["bar"].HandleFunc("POST", "/bar/echo", fakeEcho)

addr := bgateway.HTTPBackends()["bar"].RealAddr
baseURL := "http://" + addr
bgateway.HTTPBackends()["bar"].HandleFunc("GET", "/bar-path", fakeNormal)

client := zanzibar.NewHTTPClientContext(
bgateway.ActualGateway.Logger,
Expand All @@ -199,6 +210,7 @@ func TestUnknownStatusCode(t *testing.T) {
baseURL,
map[string]string{},
time.Second,
true,
)

ctx := context.Background()
Expand Down Expand Up @@ -230,6 +242,79 @@ func TestUnknownStatusCode(t *testing.T) {
assert.Len(t, logs["Finished an outgoing client HTTP request"], 1)
}

func TestNotFollowRedirect(t *testing.T) {
gateway, err := benchGateway.CreateGateway(
defaultTestConfig,
&testGateway.Options{
LogWhitelist: map[string]bool{
"Could not emit statusCode metric": true,
},
KnownHTTPBackends: []string{"bar"},
ConfigFiles: util.DefaultConfigFiles("example-gateway"),
},
exampleGateway.CreateGateway,
)
if !assert.NoError(t, err) {
return
}
defer gateway.Close()

bgateway := gateway.(*benchGateway.BenchGateway)

addr := bgateway.HTTPBackends()["bar"].RealAddr
baseURL := "http://" + addr

redirectURI := baseURL + "/bar-path"
fakeEcho := func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("Location", redirectURI)
w.WriteHeader(303)
_, err := w.Write([]byte(`false`))
assert.NoError(t, err)
}

fakeNormal := func(w http.ResponseWriter, r *http.Request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't seem to be used.

w.WriteHeader(999)
_, err := w.Write([]byte(`false`))
assert.NoError(t, err)
}

bgateway.HTTPBackends()["bar"].HandleFunc("POST", "/bar/echo", fakeEcho)
bgateway.HTTPBackends()["bar"].HandleFunc("GET", "/bar-path", fakeNormal)
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't seem to be used, remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense, since we don't follow the redirect, we don't need it anymore.


client := zanzibar.NewHTTPClientContext(
bgateway.ActualGateway.Logger,
bgateway.ActualGateway.ContextMetrics,
jsonwrapper.NewDefaultJSONWrapper(),
"bar",
map[string]string{
"echo": "bar::echo",
},
baseURL,
map[string]string{},
time.Second,
false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpatali does this test case address your ask? in this test case, it creates the gateway via exampleGateway and use example gateway's config files. And this test case is to verify the client call doesn't follow the redirect, so the caller will receive 303 status code.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine but additional also set the config value for clients.*.followRedirects = false for one of the clients in the .json to test the behavior since that is what you are going to need.

)

ctx := context.Background()

req := zanzibar.NewClientHTTPRequest(ctx, "bar", "echo", "bar::echo", client)

err = req.WriteJSON("POST", baseURL+"/bar/echo", nil, myJson{})
assert.NoError(t, err)

res, err := req.Do()
assert.NoError(t, err)

var resp string
assert.Error(t, res.ReadAndUnmarshalBody(&resp))
assert.Equal(t, "", resp)
assert.Equal(t, 303, res.StatusCode)
assert.Equal(t, redirectURI, res.Header["Location"][0])

logs := bgateway.AllLogs()
assert.Len(t, logs["Finished an outgoing client HTTP request"], 1)
}

type myJson struct{}

func (sj myJson) MarshalJSON() ([]byte, error) {
Expand Down
13 changes: 12 additions & 1 deletion runtime/http_client.go
Expand Up @@ -72,6 +72,7 @@ func NewHTTPClient(
baseURL,
defaultHeaders,
timeout,
true,
)
}

Expand All @@ -85,6 +86,7 @@ func NewHTTPClientContext(
baseURL string,
defaultHeaders map[string]string,
timeout time.Duration,
followRedirect bool,
) *HTTPClient {
loggers := make(map[string]*zap.Logger, len(methodToTargetEndpoint))

Expand All @@ -95,14 +97,23 @@ func NewHTTPClientContext(
zap.String(logFieldClientThriftMethod, targetEndpointName),
)
}

var checkRedirect func(req *http.Request, via []*http.Request) error
if !followRedirect {
checkRedirect = func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
}
}

return &HTTPClient{
Client: &http.Client{
Transport: &http.Transport{
DisableKeepAlives: false,
MaxIdleConns: 500,
MaxIdleConnsPerHost: 500,
},
Timeout: timeout,
Timeout: timeout,
CheckRedirect: checkRedirect,
},
BaseURL: baseURL,
DefaultHeaders: defaultHeaders,
Expand Down