URL Path Fix

northwesternmutual · Aug 18, 2017 · b68ceb7 · b68ceb7
1 parent d81c39a
commit b68ceb7
Show file tree

Hide file tree

Showing 11 changed files with 76 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,10 +4,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
-## [1.1.2] - 2017-08-16
+## [Unreleased]
 ### Changed
 - If InfluxDB database doesn't exist when writing, it will be created.
-- Fixed bug causing `http: multiple response.WriteHeader calls` error if `io.Copy` failed when writing the response.
+- Raw URL path will be used as the primary path when proxying to upstream services.
 
 ## [1.1.1] - 2017-08-15
 ### Changed

diff --git a/handlers/handler.go b/handlers/handler.go
@@ -48,7 +48,7 @@ func (h Handler) serveHTTP(w http.ResponseWriter, r *http.Request) {
 	// start a global trace
 	sp := opentracing.StartSpan(fmt.Sprintf("%s %s",
 		r.Method,
-		r.URL.Path,
+		r.URL.EscapedPath(),
 	))
 
 	closer, str, err := utils.DupReaderAndString(r.Body)
@@ -60,7 +60,7 @@ func (h Handler) serveHTTP(w http.ResponseWriter, r *http.Request) {
 	r.Body = closer
 
 	sp.SetTag("http.request_body", str)
-	sp.SetTag("http.url", r.RequestURI)
+	sp.SetTag("http.url", r.URL.EscapedPath())
 	sp.SetTag("http.method", r.Method)
 
 	jsonHeaders, err := json.Marshal(utils.FlattenHTTPHeaders(utils.OmitHeaderValues(r.Header, viper.GetString(config.FlagHeaderMaskValue.GetLong()), viper.GetString(config.FlagApikeyHeaderKey.GetLong()))))
@@ -89,10 +89,10 @@ func (h Handler) serveHTTP(w http.ResponseWriter, r *http.Request) {
 			// log error
 			logrus.WithFields(logrus.Fields{
 				"method": r.Method,
-				"uri":    r.RequestURI,
+				"uri":    r.URL.EscapedPath(),
 			}).Error(e.Error())
 
-			h.Metrics.Add(metrics.Metric{"http_response_code", strconv.Itoa(e.Status()), true})
+			h.Metrics.Add(metrics.Metric{Name: "http_response_code", Value: strconv.Itoa(e.Status()), Index: true})
 
 			errStatus, err := json.Marshal(utils.JSONErr{Code: e.Status(), Msg: e.Error()})
 			if err != nil {
@@ -116,10 +116,10 @@ func (h Handler) serveHTTP(w http.ResponseWriter, r *http.Request) {
 			// log error
 			logrus.WithFields(logrus.Fields{
 				"method": r.Method,
-				"uri":    r.RequestURI,
+				"uri":    r.URL.EscapedPath(),
 			}).Error("unknown error")
 
-			h.Metrics.Add(metrics.Metric{"http_response_code", strconv.Itoa(http.StatusInternalServerError), true})
+			h.Metrics.Add(metrics.Metric{Name: "http_response_code", Value: strconv.Itoa(http.StatusInternalServerError), Index: true})
 
 			errStatus, err := json.Marshal(utils.JSONErr{Code: http.StatusInternalServerError, Msg: "unknown error"})
 			if err != nil {

diff --git a/handlers/logger.go b/handlers/logger.go
@@ -46,15 +46,15 @@ func Logger(influxCtlr *monitor.InfluxController, inner Handler) http.Handler {
 		logrus.WithFields(logrus.Fields{
 			"client ip": strings.Split(r.RemoteAddr, ":")[0],
 			"method":    r.Method,
-			"uri":       r.RequestURI,
+			"uri":       r.URL.EscapedPath(),
 			"totalTime": int(t1.Sub(t0) / time.Millisecond),
 		}).Info("request details")
 
 		inner.Metrics.Add(
-			metrics.Metric{"total_time", int(t1.Sub(t0) / time.Millisecond), false},
-			metrics.Metric{"http_method", r.Method, true},
-			metrics.Metric{"http_uri", r.RequestURI, false},
-			metrics.Metric{"client_ip", strings.Split(r.RemoteAddr, ":")[0], false},
+			metrics.Metric{Name: "total_time", Value: int(t1.Sub(t0) / time.Millisecond), Index: false},
+			metrics.Metric{Name: "http_method", Value: r.Method, Index: true},
+			metrics.Metric{Name: "http_uri", Value: r.URL.EscapedPath(), Index: false},
+			metrics.Metric{Name: "client_ip", Value: strings.Split(r.RemoteAddr, ":")[0], Index: false},
 		)
 
 		if err := influxCtlr.WriteRequestData(inner.Metrics); err != nil {

diff --git a/monitor/influxdb_test.go b/monitor/influxdb_test.go
@@ -68,8 +68,8 @@ func (c *mockClient) Close() error {
 func TestWriteRequestData(t *testing.T) {
 	ctlr := &InfluxController{Client: &mockClient{}}
 	m := &metrics.Metrics{
-		metrics.Metric{"metric-one", "value-one", true},
-		metrics.Metric{"metric-two", "value-two", false},
+		metrics.Metric{Name: "metric-one", Value: "value-one", Index: true},
+		metrics.Metric{Name: "metric-two", Value: "value-two", Index: false},
 	}
 	assert.Equal(t, ctlr.WriteRequestData(m).Error(), "no database name")
 	viper.SetDefault(config.FlagInfluxdbDatabase.GetLong(), "mydb")
@@ -98,8 +98,8 @@ func TestCreateDatabase(t *testing.T) {
 
 func TestGetFields(t *testing.T) {
 	assert.Equal(t, getFields(&metrics.Metrics{
-		metrics.Metric{"metric-one", "value-one", true},
-		metrics.Metric{"metric-two", "value-two", false},
+		metrics.Metric{Name: "metric-one", Value: "value-one", Index: true},
+		metrics.Metric{Name: "metric-two", Value: "value-two", Index: false},
 	}), map[string]interface{}{
 		"metric-one": "value-one",
 		"metric-two": "value-two",
@@ -108,16 +108,16 @@ func TestGetFields(t *testing.T) {
 
 func TestGetTags(t *testing.T) {
 	tags, err := getTags(&metrics.Metrics{
-		metrics.Metric{"metric-one", "value-one", true},
-		metrics.Metric{"metric-two", "value-two", false},
+		metrics.Metric{Name: "metric-one", Value: "value-one", Index: true},
+		metrics.Metric{Name: "metric-two", Value: "value-two", Index: false},
 	})
 	assert.Nil(t, err)
 	assert.Equal(t, tags, map[string]string{
 		"metric-one": "value-one",
 	})
 	tags, err = getTags(&metrics.Metrics{
-		metrics.Metric{"metric-one", 5, true},
-		metrics.Metric{"metric-two", "value-two", false},
+		metrics.Metric{Name: "metric-one", Value: 5, Index: true},
+		metrics.Metric{Name: "metric-two", Value: "value-two", Index: false},
 	})
 	assert.Nil(t, tags)
 	assert.Equal(t, err.Error(), "InfluxDB requires that the indexed field metric-one be of type string")

diff --git a/steps/mockservice.go b/steps/mockservice.go
@@ -61,7 +61,7 @@ func (step MockServiceStep) GetName() string {
 func (step MockServiceStep) Do(ctx context.Context, m *metrics.Metrics, c *controller.Controller, w http.ResponseWriter, r *http.Request, resp *http.Response, trace opentracing.Span) error {
 
 	// incoming proxy
-	untypedProxy, err := spec.ProxyStore.Get(r.URL.Path)
+	untypedProxy, err := spec.ProxyStore.Get(r.URL.EscapedPath())
 	if err != nil || untypedProxy == nil {
 		return utils.StatusError{Code: http.StatusNotFound, Err: errors.New("proxy not found")}
 	}
@@ -122,7 +122,7 @@ func (step MockServiceStep) Do(ctx context.Context, m *metrics.Metrics, c *contr
 
 	// alright so have an incoming path and we have a target path, if defined.
 	// figure out with the diff is and search for that in the map.
-	targetPath := utils.ComputeTargetPath(proxy.Spec.Path, proxy.Spec.Target, r.URL.Path)
+	targetPath := utils.ComputeTargetPath(proxy.Spec.Path, proxy.Spec.Target, r.URL.EscapedPath())
 
 	// this variable will hold the index in the array
 	// of the matching mock response, if any
@@ -143,7 +143,7 @@ func (step MockServiceStep) Do(ctx context.Context, m *metrics.Metrics, c *contr
 	if mockRespIndex < 0 {
 
 		return utils.StatusError{Code: http.StatusNotFound, Err: fmt.Errorf("no mock response defined for the incoming path %s and target path %s",
-			r.URL.Path,
+			r.URL.EscapedPath(),
 			targetPath,
 		)}
 
@@ -178,7 +178,7 @@ func (step MockServiceStep) Do(ctx context.Context, m *metrics.Metrics, c *contr
 		HeaderMap: upstreamHeaders,
 	}
 
-  m.Add(metrics.Metric{"http_response_code", strconv.Itoa(mok[mockRespIndex].Code), true})
+  m.Add(metrics.Metric{Name: "http_response_code", Value: strconv.Itoa(mok[mockRespIndex].Code), Index: true})
 
 	*resp = *responseRecorder.Result()
 

diff --git a/steps/pluginsonrequest.go b/steps/pluginsonrequest.go
@@ -46,7 +46,7 @@ func (step PluginsOnRequestStep) GetName() string {
 
 // Do executes the logic of the PluginsOnRequestStep step
 func (step PluginsOnRequestStep) Do(ctx context.Context, m *metrics.Metrics, c *controller.Controller, w http.ResponseWriter, r *http.Request, resp *http.Response, trace opentracing.Span) error {
-	untypedProxy, err := spec.ProxyStore.Get(r.URL.Path)
+	untypedProxy, err := spec.ProxyStore.Get(r.URL.EscapedPath())
 	if err != nil || untypedProxy == nil {
 		return utils.StatusError{Code: http.StatusNotFound, Err: errors.New("proxy not found")}
 	}

diff --git a/steps/pluginsonresponse.go b/steps/pluginsonresponse.go
@@ -46,7 +46,7 @@ func (step PluginsOnResponseStep) GetName() string {
 
 // Do executes the logic of the PluginsOnResponseStep step
 func (step PluginsOnResponseStep) Do(ctx context.Context, m *metrics.Metrics, c *controller.Controller, w http.ResponseWriter, r *http.Request, resp *http.Response, trace opentracing.Span) error {
-	untypedProxy, err := spec.ProxyStore.Get(r.URL.Path)
+	untypedProxy, err := spec.ProxyStore.Get(r.URL.EscapedPath())
 	if err != nil || untypedProxy == nil {
 		return utils.StatusError{Code: http.StatusNotFound, Err: errors.New("proxy not found")}
 	}

diff --git a/steps/proxypass.go b/steps/proxypass.go
@@ -82,7 +82,7 @@ func (step ProxyPassStep) Do(ctx context.Context, m *metrics.Metrics, c *control
 		Target: typedProxy, // shouldn't be nil (unless the proxy is removed within the microseconds it takes to get to this code)
 	}
 
-	up := create(p).setUpstreamURL(p, c).configureTLS(p).setUpstreamHeaders(p).performProxy(trace)
+	up := create(p).setUpstreamURL(p).configureTLS(p).setUpstreamHeaders(p).performProxy(trace)
 
 	if up.Error != (utils.StatusError{}) {
 		logrus.Errorf("oops: %s", up.Error)
@@ -196,7 +196,7 @@ func (up *upstream) configureTLS(p *proxy) *upstream {
 
 }
 
-func (up *upstream) setUpstreamURL(p *proxy, c *controller.Controller) *upstream {
+func (up *upstream) setUpstreamURL(p *proxy) *upstream {
 
 	if up.Error != (utils.StatusError{}) {
 		return up
@@ -212,8 +212,8 @@ func (up *upstream) setUpstreamURL(p *proxy, c *controller.Controller) *upstream
 			Err:  err,
 		}
 	} else {
-		// set path
-		u.Path = utils.ComputeTargetPath(p.Target.Spec.Path, p.Target.Spec.Target, p.Source.URL.Path)
+		u.Path = utils.ComputeTargetPath(p.Target.Spec.Path, p.Target.Spec.Target, p.Source.URL.EscapedPath())
+		u.RawPath = u.EscapedPath()
 		u.ForceQuery = p.Source.URL.ForceQuery
 		u.RawQuery = p.Source.URL.RawQuery
 		u.Fragment = p.Source.URL.Fragment

diff --git a/steps/proxypass_test.go b/steps/proxypass_test.go
@@ -50,6 +50,20 @@ func TestCreate(t *testing.T) {
 
 }
 
+func TestSetUpstreamURL(t *testing.T) {
+	spec.ServiceStore.Set(spec.Service{
+		Name:      "my-service",
+		Namespace: "foo",
+		ClusterIP: "1.2.3.4",
+		Port:      8080,
+	})
+
+	p := getTestInternalProxies()[3]
+	result := create(p).setUpstreamURL(p)
+	assert.Equal(t, result.Request.URL.Path, "/foo/bar/https%3A%2F%2Ffoo.bar.com")
+	assert.Equal(t, result.Request.URL.EscapedPath(), "/foo/bar/https%253A%252F%252Ffoo.bar.com")
+}
+
 func TestSetK8sDiscoveredURI(t *testing.T) {
 	assert := assert.New(t)
 
@@ -97,6 +111,7 @@ func getTestInternalProxies() []*proxy {
 
 	one, _ := url.Parse("http://www.foo.bar.com/api/v1/accounts")
 	two, _ := url.Parse("http://www.foo.bar.com/api/v1/accounts/one/two")
+	three, _ := url.Parse("http://www.foo.bar.com/api/v1/accounts/https%3A%2F%2Ffoo.bar.com")
 
 	return []*proxy{
 		{
@@ -203,6 +218,27 @@ func getTestInternalProxies() []*proxy {
 				},
 			},
 		},
+		{
+			Source: &http.Request{
+				URL: three,
+			},
+			Target: spec.APIProxy{
+				TypeMeta: unversioned.TypeMeta{},
+				ObjectMeta: api.ObjectMeta{
+					Name:      "exampleAPIProxyOne",
+					Namespace: "foo",
+				},
+				Spec: spec.APIProxySpec{
+					Path:   "/api/v1/accounts",
+					Target: "/foo/bar",
+					Service: spec.Service{
+						Name:      "my-service",
+						Namespace: "foo",
+						Port:      8080,
+					},
+				},
+			},
+		},
 	}
 
 }
diff --git a/steps/validateproxy.go b/steps/validateproxy.go
@@ -46,7 +46,7 @@ func (step ValidateProxyStep) GetName() string {
 // Do executes the logic of the ValidateProxyStep step
 func (step ValidateProxyStep) Do(ctx context.Context, m *metrics.Metrics, c *controller.Controller, w http.ResponseWriter, r *http.Request, resp *http.Response, trace opentracing.Span) error {
 
-	untypedProxy, err := spec.ProxyStore.Get(r.URL.Path)
+	untypedProxy, err := spec.ProxyStore.Get(r.URL.EscapedPath())
 	if err != nil || untypedProxy == nil {
 		if err != nil {
 			logrus.Error(err.Error())
@@ -56,8 +56,8 @@ func (step ValidateProxyStep) Do(ctx context.Context, m *metrics.Metrics, c *con
 		trace.SetTag("kanali.proxy_namespace", "unknown")
 
 		m.Add(
-			metrics.Metric{"proxy_name", "unknown", true},
-			metrics.Metric{"proxy_namespace", "unknown", true},
+			metrics.Metric{Name: "proxy_name", Value: "unknown", Index: true},
+			metrics.Metric{Name: "proxy_namespace", Value: "unknown", Index: true},
 		)
 
 		return utils.StatusError{Code: http.StatusNotFound, Err: errors.New("proxy not found")}
@@ -70,8 +70,8 @@ func (step ValidateProxyStep) Do(ctx context.Context, m *metrics.Metrics, c *con
 		trace.SetTag("kanali.proxy_namespace", "unknown")
 
 		m.Add(
-			metrics.Metric{"proxy_name", "unknown", true},
-			metrics.Metric{"proxy_namespace", "unknown", true},
+			metrics.Metric{Name: "proxy_name", Value: "unknown", Index: true},
+			metrics.Metric{Name: "proxy_namespace", Value: "unknown", Index: true},
 		)
 
 		return utils.StatusError{Code: http.StatusNotFound, Err: errors.New("proxy not found")}
@@ -81,8 +81,8 @@ func (step ValidateProxyStep) Do(ctx context.Context, m *metrics.Metrics, c *con
 	trace.SetTag("kanali.proxy_namespace", proxy.ObjectMeta.Namespace)
 
 	m.Add(
-		metrics.Metric{"proxy_name", proxy.ObjectMeta.Name, true},
-		metrics.Metric{"proxy_namespace", proxy.ObjectMeta.Namespace, true},
+		metrics.Metric{Name: "proxy_name", Value: proxy.ObjectMeta.Name, Index: true},
+		metrics.Metric{Name: "proxy_namespace", Value: proxy.ObjectMeta.Namespace, Index: true},
 	)
 
 	return nil

diff --git a/steps/writeresponse.go b/steps/writeresponse.go
@@ -51,7 +51,7 @@ func (step WriteResponseStep) Do(ctx context.Context, m *metrics.Metrics, c *con
 		}
 	}
 
-	m.Add(metrics.Metric{"http_response_code", strconv.Itoa(resp.StatusCode), true})
+	m.Add(metrics.Metric{Name: "http_response_code", Value: strconv.Itoa(resp.StatusCode), Index: true})
 
 	closer, str, err := utils.DupReaderAndString(resp.Body)
 	if err != nil {
@@ -64,7 +64,7 @@ func (step WriteResponseStep) Do(ctx context.Context, m *metrics.Metrics, c *con
 	w.WriteHeader(resp.StatusCode)
 
 	if _, err := io.Copy(w, closer); err != nil {
-		return nil
+		return err
 	}
 
 	return nil