From e31953a92b1255a0fd75ff63427bcd64bf2303e2 Mon Sep 17 00:00:00 2001 From: Vladimir Lyubitelev Date: Tue, 15 May 2018 02:42:25 +0200 Subject: [PATCH] fixed issue with proxing response - default response was updated and affected responses of all baskets, rework of forwarding logic, more tests, documented 'proxy_response' flag --- baskets.go | 80 ++++++++++++++------------- baskets_test.go | 22 +++++--- doc/api-swagger.json | 6 +- doc/api-swagger.yaml | 5 ++ handlers.go | 108 ++++++++++++++++++++++-------------- handlers_test.go | 128 +++++++++++++++++++++++++++++++++++++++++-- server.go | 7 +++ 7 files changed, 264 insertions(+), 92 deletions(-) diff --git a/baskets.go b/baskets.go index a534f3d..1bbc238 100644 --- a/baskets.go +++ b/baskets.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "io/ioutil" "log" "net/http" @@ -126,55 +127,58 @@ func ToRequestData(req *http.Request) *RequestData { } // Forward forwards request data to specified URL -func (req *RequestData) Forward(client *http.Client, config BasketConfig, basket string) *http.Response { - body := strings.NewReader(req.Body) +func (req *RequestData) Forward(client *http.Client, config BasketConfig, basket string) (*http.Response, error) { forwardURL, err := url.ParseRequestURI(config.ForwardURL) - if err != nil { - log.Printf("[warn] invalid forward URL: %s; basket: %s", config.ForwardURL, basket) - } else { - // expand path - if config.ExpandPath && len(req.Path) > len(basket)+1 { - forwardURL.Path = expand(forwardURL.Path, req.Path, basket) - } + return nil, fmt.Errorf("Invalid forward URL: %s - %s", config.ForwardURL, err) + } - // append query - if len(req.Query) > 0 { - if len(forwardURL.RawQuery) > 0 { - forwardURL.RawQuery += "&" + req.Query - } else { - forwardURL.RawQuery = req.Query - } - } + // expand path + if config.ExpandPath && len(req.Path) > len(basket)+1 { + forwardURL.Path = expandURL(forwardURL.Path, req.Path, basket) + } - forwardReq, err := http.NewRequest(req.Method, forwardURL.String(), body) - if err != nil { - log.Printf("[error] failed to create forward request: %s", err) + // append query + if len(req.Query) > 0 { + if len(forwardURL.RawQuery) > 0 { + forwardURL.RawQuery += "&" + req.Query } else { - // copy headers - for header, vals := range req.Header { - for _, val := range vals { - forwardReq.Header.Add(header, val) - } - } - // set do not forward header - forwardReq.Header.Set(DoNotForwardHeader, "1") - - var response *http.Response - response, err = client.Do(forwardReq) + forwardURL.RawQuery = req.Query + } + } - if err != nil { - log.Printf("[error] failed to forward request: %s", err) - return &http.Response{} - } + forwardReq, err := http.NewRequest(req.Method, forwardURL.String(), strings.NewReader(req.Body)) + if err != nil { + return nil, fmt.Errorf("Failed to create forward request: %s", err) + } - return response + // copy headers + for header, vals := range req.Header { + for _, val := range vals { + forwardReq.Header.Add(header, val) } } - return &http.Response{} + // set do not forward header + forwardReq.Header.Set(DoNotForwardHeader, "1") + + // forward request + response, err := client.Do(forwardReq) + if err != nil { + // HTTP issue during forwarding - HTTP 502 Bad Gateway + log.Printf("[warn] failed to forward request for basket: %s - %s", basket, err) + badGatewayResp := &http.Response{ + StatusCode: http.StatusBadGateway, + Header: http.Header{}, + Body: ioutil.NopCloser(strings.NewReader(fmt.Sprintf("Failed to forward request: %s", err)))} + badGatewayResp.Header.Set("Content-Type", "text/plain") + + return badGatewayResp, nil + } + + return response, nil } -func expand(url string, original string, basket string) string { +func expandURL(url string, original string, basket string) string { return strings.TrimSuffix(url, "/") + strings.TrimPrefix(original, "/"+basket) } diff --git a/baskets_test.go b/baskets_test.go index 8b8cfec..5f59fdb 100644 --- a/baskets_test.go +++ b/baskets_test.go @@ -101,10 +101,13 @@ func TestRequestData_Forward_BrokenURL(t *testing.T) { data.Path = "/" + basket // Config to forward requests to broken URL - config := BasketConfig{ForwardURL: "-.'", ExpandPath: false, Capacity: 20} + config := BasketConfig{ForwardURL: "abc", ExpandPath: false, Capacity: 20} // Should not fail, warning in log is expected - data.Forward(new(http.Client), config, basket) + r, e := data.Forward(new(http.Client), config, basket) + assert.Nil(t, r, "response is not expected") + assert.NotNil(t, e, "error is expected") + assert.EqualError(t, e, "Invalid forward URL: abc - parse abc: invalid URI for request", "wrong error") } func TestRequestData_Forward_UnreachableURL(t *testing.T) { @@ -124,12 +127,15 @@ func TestRequestData_Forward_UnreachableURL(t *testing.T) { config := BasketConfig{ForwardURL: "http://localhost:81/should/fail/to/forward", ExpandPath: false, Capacity: 20} // Should not fail, warning in log is expected - data.Forward(new(http.Client), config, basket) + r, e := data.Forward(new(http.Client), config, basket) + assert.Nil(t, e, "error is not expected") + assert.NotNil(t, r, "response is expected") + assert.Equal(t, 502, r.StatusCode, "wrong status code") } -func TestExpand(t *testing.T) { - assert.Equal(t, "/notify/abc/123-123", expand("/notify", "/sniffer/abc/123-123", "sniffer")) - assert.Equal(t, "/hello/world", expand("/", "/mybasket/hello/world", "mybasket")) - assert.Equal(t, "/notify/hello/world", expand("/notify", "/notify/hello/world", "notify")) - assert.Equal(t, "/receive/notification/test/", expand("/receive/notification/", "/basket/test/", "basket")) +func TestExpandURL(t *testing.T) { + assert.Equal(t, "/notify/abc/123-123", expandURL("/notify", "/sniffer/abc/123-123", "sniffer")) + assert.Equal(t, "/hello/world", expandURL("/", "/mybasket/hello/world", "mybasket")) + assert.Equal(t, "/notify/hello/world", expandURL("/notify", "/notify/hello/world", "notify")) + assert.Equal(t, "/receive/notification/test/", expandURL("/receive/notification/", "/basket/test/", "basket")) } diff --git a/doc/api-swagger.json b/doc/api-swagger.json index 0532b52..7fd59f0 100644 --- a/doc/api-swagger.json +++ b/doc/api-swagger.json @@ -417,6 +417,10 @@ "type" : "string", "description" : "URL to forward all incoming requests of the basket, `empty` value disables forwarding" }, + "proxy_response" : { + "type" : "boolean", + "description" : "If set to `true` this basket behaves as a full proxy: responses from underlying service configured in `forward_url`\nare passed back to clients of original requests. The configuration of basket responses is ignored in this case.\n" + }, "insecure_tls" : { "type" : "boolean", "description" : "If set to `true` the certificate verification will be disabled if forward URL indicates HTTPS scheme.\n**Warning:** enabling this feature has known security implications.\n" @@ -531,4 +535,4 @@ } } } -} \ No newline at end of file +} diff --git a/doc/api-swagger.yaml b/doc/api-swagger.yaml index 0cefe98..9237675 100644 --- a/doc/api-swagger.yaml +++ b/doc/api-swagger.yaml @@ -369,6 +369,11 @@ definitions: forward_url: type: string description: URL to forward all incoming requests of the basket, `empty` value disables forwarding + proxy_response: + type: boolean + description: | + If set to `true` this basket behaves as a full proxy: responses from underlying service configured in `forward_url` + are passed back to clients of original requests. The configuration of basket responses is ignored in this case. insecure_tls: type: boolean description: | diff --git a/handlers.go b/handlers.go index b11fa8a..6a09b9f 100644 --- a/handlers.go +++ b/handlers.go @@ -17,7 +17,7 @@ import ( ) var validBasketName = regexp.MustCompile(basketNamePattern) -var defaultResponse = ResponseConfig{Status: 200, Headers: http.Header{}, IsTemplate: false} +var defaultResponse = ResponseConfig{Status: http.StatusOK, Headers: http.Header{}, IsTemplate: false} var basketPageTemplate = template.Must(template.New("basket").Parse(basketPageContentTemplate)) // writeJSON writes JSON content to HTTP response @@ -351,60 +351,86 @@ func AcceptBasketRequests(w http.ResponseWriter, r *http.Request) { if basket := basketsDb.Get(name); basket != nil { request := basket.Add(r) - responseConfig := basket.GetResponse(r.Method) - if responseConfig == nil { - responseConfig = &defaultResponse - } - - // forward request in separate thread + // forward request if configured and it's a first forwarding config := basket.Config() if len(config.ForwardURL) > 0 && r.Header.Get(DoNotForwardHeader) != "1" { - client := httpClient - if config.InsecureTLS { - client = httpInsecureClient + if config.ProxyResponse { + forwardAndProxyResponse(w, request, config, name) + return } - if config.ProxyResponse { - response := request.Forward(client, config, name) - defer response.Body.Close() + go forwardAndForget(request, config, name) + } - body, err := ioutil.ReadAll(response.Body) - if err != nil { - http.Error(w, "Error in "+err.Error(), http.StatusInternalServerError) - } + writeBasketResponse(w, r, name, basket) + } else { + w.WriteHeader(http.StatusNotFound) + } +} - responseConfig.Headers = response.Header - responseConfig.Status = response.StatusCode - responseConfig.Body = string(body) - } else { - go request.Forward(client, config, name) - } - } +func forwardAndForget(request *RequestData, config BasketConfig, name string) { + // forward request and discard the response + response, err := request.Forward(getHTTPClient(config.InsecureTLS), config, name) + if err != nil { + log.Printf("[warn] failed to forward request for basket: %s - %s", name, err) + } else { + io.Copy(ioutil.Discard, response.Body) + response.Body.Close() + } +} +func forwardAndProxyResponse(w http.ResponseWriter, request *RequestData, config BasketConfig, name string) { + // forward request in a full proxy mode + response, err := request.Forward(getHTTPClient(config.InsecureTLS), config, name) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + } else { // headers - for k, v := range responseConfig.Headers { + for k, v := range response.Header { w.Header()[k] = v } + + // status + w.WriteHeader(response.StatusCode) + // body - if responseConfig.IsTemplate && len(responseConfig.Body) > 0 { - // template - t, err := template.New(name + "-" + r.Method).Parse(responseConfig.Body) - if err != nil { - // invalid template - http.Error(w, "Error in "+err.Error(), http.StatusInternalServerError) - } else { - // status - w.WriteHeader(responseConfig.Status) - // templated body - t.Execute(w, r.URL.Query()) - } + _, err := io.Copy(w, response.Body) + if err != nil { + log.Printf("[warn] failed to proxy response body for basket: %s - %s", name, err) + io.Copy(ioutil.Discard, response.Body) + } + response.Body.Close() + } +} + +func writeBasketResponse(w http.ResponseWriter, r *http.Request, name string, basket Basket) { + response := basket.GetResponse(r.Method) + if response == nil { + response = &defaultResponse + } + + // headers + for k, v := range response.Headers { + w.Header()[k] = v + } + + // body + if response.IsTemplate && len(response.Body) > 0 { + // template + t, err := template.New(name + "-" + r.Method).Parse(response.Body) + if err != nil { + // invalid template + http.Error(w, "Error in "+err.Error(), http.StatusInternalServerError) } else { // status - w.WriteHeader(responseConfig.Status) - // plain body - w.Write([]byte(responseConfig.Body)) + w.WriteHeader(response.Status) + // templated body + t.Execute(w, r.URL.Query()) } } else { - w.WriteHeader(http.StatusNotFound) + // status + w.WriteHeader(response.Status) + // plain body + w.Write([]byte(response.Body)) } } diff --git a/handlers_test.go b/handlers_test.go index 0582952..56b8d6b 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -10,9 +10,10 @@ import ( "testing/iotest" "time" + "io/ioutil" + "github.com/julienschmidt/httprouter" "github.com/stretchr/testify/assert" - "io/ioutil" ) func TestWriteJSON(t *testing.T) { @@ -1584,14 +1585,14 @@ func TestAcceptBasketRequests_WithForwardExpand(t *testing.T) { } } -func TestAcceptBasketRequests_WithProxyRequest(t *testing.T) { +func TestAcceptBasketRequests_WithProxyResponse(t *testing.T) { basket := "accept07" method := "DELETE" // Test HTTP server ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusAccepted) - w.Write([]byte("test")) + w.Write([]byte("server test response")) })) defer ts.Close() @@ -1621,8 +1622,127 @@ func TestAcceptBasketRequests_WithProxyRequest(t *testing.T) { // validate expected response assert.Equal(t, 202, w.Code, "wrong HTTP response code") - assert.Equal(t, "test", string(responseBody), "wrong response body") + assert.Equal(t, "server test response", string(responseBody), "wrong response body") time.Sleep(100 * time.Millisecond) } } } + +func TestAcceptBasketRequests_WithForward_BadGateway(t *testing.T) { + basket := "accept08" + method := "GET" + + // assuming that nothing is running at port 55556 + forwardURL := "http://localhost:55556/notify" + + r, err := http.NewRequest("POST", "http://localhost:55555/baskets/"+basket, + strings.NewReader("{\"forward_url\":\""+forwardURL+"\",\"capacity\":200}")) + if assert.NoError(t, err) { + ps := append(make(httprouter.Params, 0), httprouter.Param{Key: "basket", Value: basket}) + w := httptest.NewRecorder() + + CreateBasket(w, r, ps) + assert.Equal(t, 201, w.Code, "wrong HTTP result code") + + // send request and validate forwarding + r, err = http.NewRequest(method, "http://localhost:55555/"+basket+"/faile_to_forward", strings.NewReader("")) + if assert.NoError(t, err) { + w = httptest.NewRecorder() + AcceptBasketRequests(w, r) + + // validate expected response: forwarding errors are not exposed unless ForwardResponse is enabled + assert.Equal(t, 200, w.Code, "wrong HTTP response code") + assert.Equal(t, "", w.Body.String(), "wrong HTTP response body") + } + } +} + +func TestAcceptBasketRequests_WithProxyResponse_BadGateway(t *testing.T) { + basket := "accept09" + method := "POST" + + // assuming that nothing is running at port 55556 + forwardURL := "http://localhost:55556/notify" + + r, err := http.NewRequest("POST", "http://localhost:55555/baskets/"+basket, + strings.NewReader("{\"forward_url\":\""+forwardURL+"\",\"proxy_response\":true,\"capacity\":20}")) + if assert.NoError(t, err) { + ps := append(make(httprouter.Params, 0), httprouter.Param{Key: "basket", Value: basket}) + w := httptest.NewRecorder() + + CreateBasket(w, r, ps) + assert.Equal(t, 201, w.Code, "wrong HTTP result code") + + // send request and validate forwarding + r, err = http.NewRequest(method, "http://localhost:55555/"+basket+"/faile_to_forward", strings.NewReader("")) + if assert.NoError(t, err) { + w = httptest.NewRecorder() + AcceptBasketRequests(w, r) + + // validate expected response + assert.Equal(t, 502, w.Code, "wrong HTTP response code") + assert.Contains(t, w.Body.String(), "Failed to forward request", "wrong HTTP response body") + assert.Contains(t, w.Body.String(), forwardURL, "wrong HTTP response body") + assert.Contains(t, w.Body.String(), "connection refused", "wrong HTTP response body") + } + } +} + +func TestAcceptBasketRequests_WithForward_InternalServerError(t *testing.T) { + basket := "accept10" + method := "PUT" + + r, err := http.NewRequest("POST", "http://localhost:55555/baskets/"+basket, strings.NewReader("")) + if assert.NoError(t, err) { + ps := append(make(httprouter.Params, 0), httprouter.Param{Key: "basket", Value: basket}) + w := httptest.NewRecorder() + + CreateBasket(w, r, ps) + assert.Equal(t, 201, w.Code, "wrong HTTP result code") + + // set invalid forward URL directly + b := basketsDb.Get(basket) + b.Update(BasketConfig{Capacity: 20, ForwardURL: "qwert"}) + + // send request and validate forwarding + r, err = http.NewRequest(method, "http://localhost:55555/"+basket+"/internal_error", strings.NewReader("abc")) + if assert.NoError(t, err) { + w = httptest.NewRecorder() + AcceptBasketRequests(w, r) + + // validate expected response: forwarding errors are not exposed unless ForwardResponse is enabled + assert.Equal(t, 200, w.Code, "wrong HTTP response code") + assert.Equal(t, "", w.Body.String(), "wrong HTTP response body") + } + } +} + +func TestAcceptBasketRequests_WithProxyResponse_InternalServerError(t *testing.T) { + basket := "accept11" + method := "PATCH" + + r, err := http.NewRequest("POST", "http://localhost:55555/baskets/"+basket, strings.NewReader("")) + if assert.NoError(t, err) { + ps := append(make(httprouter.Params, 0), httprouter.Param{Key: "basket", Value: basket}) + w := httptest.NewRecorder() + + CreateBasket(w, r, ps) + assert.Equal(t, 201, w.Code, "wrong HTTP result code") + + // set invalid forward URL directly + b := basketsDb.Get(basket) + b.Update(BasketConfig{Capacity: 20, ForwardURL: "qwert", ProxyResponse: true}) + + // send request and validate forwarding + r, err = http.NewRequest(method, "http://localhost:55555/"+basket+"/internal_error", strings.NewReader("abc")) + if assert.NoError(t, err) { + w = httptest.NewRecorder() + AcceptBasketRequests(w, r) + + // validate expected response + assert.Equal(t, 500, w.Code, "wrong HTTP response code") + assert.Contains(t, w.Body.String(), "Invalid forward URL: qwert", "wrong HTTP response body") + assert.Contains(t, w.Body.String(), "invalid URI for request", "wrong HTTP response body") + } + } +} diff --git a/server.go b/server.go index b3aded3..0e33bad 100644 --- a/server.go +++ b/server.go @@ -99,3 +99,10 @@ func shutdownHook() { log.Printf("[info] terminating server") os.Exit(0) } + +func getHTTPClient(insecure bool) *http.Client { + if insecure { + return httpInsecureClient + } + return httpClient +}