Remove duplicate X-Request-Id response header

This fixes performance bar not working with AJAX requests.

Both LabKit and Rails were injecting duplicate X-Request-Id headers into
the response stream, which caused the frontend to be interpret the value
as a comma-separated value (e.g. LdEgVXsp1R4, LdEgVXsp1R4).

The new flow behaves as the following:

1. Workhorse injects X-Request-Id to the inbound HTTP request to Rails
2. Rails looks up this value in its RequestId middleware. Since the
value exists, Rails sets the X-Request-Id response header to this value.
3. Workhorse doesn't append another header in the response.

Distributed tracing should still work fine because Workhorse still
generates the correlation ID.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/60111

internal/helper/helpers.go

@@ -14,6 +14,8 @@ import (
 	"strings"
 	"syscall"
 
+	"github.com/sebest/xff"
+
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/log"
 )
 
@@ -157,6 +159,15 @@ func AllowResponseBuffering(w http.ResponseWriter) {
 	w.Header().Del(NginxResponseBufferHeader)
 }
 
+func FixRemoteAddr(r *http.Request) {
+	// Unix domain sockets have a remote addr of @. This will make the
+	// xff package lookup the X-Forwarded-For address if available.
+	if r.RemoteAddr == "@" {
+		r.RemoteAddr = "127.0.0.1:0"
+	}
+	r.RemoteAddr = xff.GetRemoteAddr(r)
+}
+
 func SetForwardedFor(newHeaders *http.Header, originalRequest *http.Request) {
 	if clientIP, _, err := net.SplitHostPort(originalRequest.RemoteAddr); err == nil {
 		var header string

internal/helper/helpers_test.go

@@ -8,8 +8,39 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+func TestFixRemoteAddr(t *testing.T) {
+	testCases := []struct {
+		initial   string
+		forwarded string
+		expected  string
+	}{
+		{initial: "@", forwarded: "", expected: "127.0.0.1:0"},
+		{initial: "@", forwarded: "18.245.0.1", expected: "18.245.0.1:0"},
+		{initial: "@", forwarded: "127.0.0.1", expected: "127.0.0.1:0"},
+		{initial: "@", forwarded: "192.168.0.1", expected: "127.0.0.1:0"},
+		{initial: "192.168.1.1:0", forwarded: "", expected: "192.168.1.1:0"},
+		{initial: "192.168.1.1:0", forwarded: "18.245.0.1", expected: "18.245.0.1:0"},
+	}
+
+	for _, tc := range testCases {
+		req, err := http.NewRequest("POST", "unix:///tmp/test.socket/info/refs", nil)
+		require.NoError(t, err)
+
+		req.RemoteAddr = tc.initial
+
+		if tc.forwarded != "" {
+			req.Header.Add("X-Forwarded-For", tc.forwarded)
+		}
+
+		FixRemoteAddr(req)
+
+		assert.Equal(t, tc.expected, req.RemoteAddr)
+	}
+}
+
 func TestSetForwardedForGeneratesHeader(t *testing.T) {
 	testCases := []struct {
 		remoteAddr           string

internal/upstream/upstream.go

@@ -11,7 +11,7 @@ import (
 	"net/http"
 	"strings"
 
-	"github.com/sebest/xff"
+	"gitlab.com/gitlab-org/labkit/correlation"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
@@ -44,7 +44,8 @@ func NewUpstream(cfg config.Config) http.Handler {
 	up.RoundTripper = roundtripper.NewBackendRoundTripper(up.Backend, up.Socket, up.ProxyHeadersTimeout, cfg.DevelopmentMode)
 	up.configureURLPrefix()
 	up.configureRoutes()
-	return &up
+
+	return correlation.InjectCorrelationID(&up)
 }
 
 func (u *upstream) configureURLPrefix() {
@@ -56,12 +57,7 @@ func (u *upstream) configureURLPrefix() {
 }
 
 func (u *upstream) ServeHTTP(ow http.ResponseWriter, r *http.Request) {
-	// Unix domain sockets have a remote addr of @. This will make the
-	// xff package lookup the X-Forwarded-For address if available.
-	if r.RemoteAddr == "@" {
-		r.RemoteAddr = "127.0.0.1:0"
-	}
-	r.RemoteAddr = xff.GetRemoteAddr(r)
+	helper.FixRemoteAddr(r)
 
 	w := helper.NewStatsCollectingResponseWriter(ow)
 	defer w.RequestFinished(r)

internal/upstream/upstream_test.go

-package upstream
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// This test doesn't actually listen for a connection so there may be
-// some error spew that can be ignored.
-func TestXForwardedForHeaders(t *testing.T) {
-	testCases := []struct {
-		initial   string
-		forwarded string
-		expected  string
-	}{
-		{initial: "@", forwarded: "", expected: "127.0.0.1:0"},
-		{initial: "@", forwarded: "18.245.0.1", expected: "18.245.0.1:0"},
-		{initial: "@", forwarded: "127.0.0.1", expected: "127.0.0.1:0"},
-		{initial: "@", forwarded: "192.168.0.1", expected: "127.0.0.1:0"},
-		{initial: "192.168.1.1:0", forwarded: "", expected: "192.168.1.1:0"},
-		{initial: "192.168.1.1:0", forwarded: "18.245.0.1", expected: "18.245.0.1:0"},
-	}
-
-	for _, tc := range testCases {
-		req, err := http.NewRequest("POST", "unix:///tmp/test.socket/info/refs", nil)
-		require.NoError(t, err)
-
-		req.RemoteAddr = tc.initial
-
-		if tc.forwarded != "" {
-			req.Header.Add("X-Forwarded-For", tc.forwarded)
-		}
-
-		config := config.Config{}
-		u := NewUpstream(config)
-
-		resp := httptest.NewRecorder()
-		u.ServeHTTP(resp, req)
-
-		assert.Equal(t, tc.expected, req.RemoteAddr)
-	}
-}

main.go

@@ -25,7 +25,6 @@ import (
 
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 
-	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/tracing"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
@@ -152,12 +151,7 @@ func main() {
 		}
 	}
 
-	up := wrapRaven(
-		correlation.InjectCorrelationID(
-			upstream.NewUpstream(cfg),
-			correlation.WithSetResponseHeader(),
-		),
-	)
+	up := wrapRaven(upstream.NewUpstream(cfg))
 
 	logger.Fatal(http.Serve(listener, up))
 }

main_test.go

@@ -449,6 +449,26 @@ func TestAPIFalsePositivesAreProxied(t *testing.T) {
 	}
 }
 
+func TestCorrelationIdHeader(t *testing.T) {
+	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Add("X-Request-Id", "12345678")
+		w.WriteHeader(200)
+	})
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	for _, resource := range []string{
+		"/api/v3/projects/123/repository/not/special",
+	} {
+		resp, _ := httpGet(t, ws.URL+resource, nil)
+
+		assert.Equal(t, 200, resp.StatusCode, "GET %q: status code", resource)
+		requestIds := resp.Header["X-Request-Id"]
+		assert.Equal(t, 1, len(requestIds), "GET %q: One X-Request-Id present", resource)
+	}
+}
+
 func setupStaticFile(fpath, content string) error {
 	cwd, err := os.Getwd()
 	if err != nil {