Merge branch 'gitaly-tcp-insecure' into 'master'

Support insecure TCP connections to Gitaly

See merge request !150

internal/api/api.go

@@ -89,8 +89,10 @@ type Response struct {
 	Entry string `json:"entry"`
 	// Used to communicate terminal session details
 	Terminal *TerminalSettings
-	// Path to Gitaly Socket
+	// Path to Gitaly Socket (deprecated in favor of GitalyAddress)
 	GitalySocketPath string
+	// GitalyAddress is a unix:// or tcp:// address to reach a Gitaly service on
+	GitalyAddress string
 	// Repository object for making gRPC requests to Gitaly. This will
 	// eventually replace the RepoPath field.
 	Repository pb.Repository
@@ -216,11 +218,16 @@ func (api *API) PreAuthorize(suffix string, r *http.Request) (httpResponse *http
 	// This is for backwards compatiblity, can be depracated when Rails
 	// always sends a 'Repository' message. For the time being we cannot
 	// count on this, so we put some minimal data in the Repository struct.
-
 	if authResponse.Repository.Path == "" {
 		authResponse.Repository.Path = authResponse.RepoPath
 	}
 
+	if socketPath := authResponse.GitalySocketPath; socketPath != "" && authResponse.GitalyAddress == "" {
+		// We are transitioning away from the GitalySocketPath response field.
+		// Until all the new code is in place, keep backwards compatibility.
+		authResponse.GitalyAddress = "unix://" + socketPath
+	}
+
 	return httpResponse, authResponse, nil
 }
 

internal/git/info-refs.go

@@ -29,7 +29,7 @@ func handleGetInfoRefs(rw http.ResponseWriter, r *http.Request, a *api.Response)
 	w.Header().Set("Cache-Control", "no-cache")
 
 	var err error
-	if a.GitalySocketPath == "" {
+	if a.GitalyAddress == "" {
 		err = handleGetInfoRefsLocally(w, a, rpc)
 	} else {
 		err = handleGetInfoRefsWithGitaly(w, a, rpc)
@@ -62,7 +62,7 @@ func handleGetInfoRefsLocally(w http.ResponseWriter, a *api.Response, rpc string
 }
 
 func handleGetInfoRefsWithGitaly(w http.ResponseWriter, a *api.Response, rpc string) error {
-	smarthttp, err := gitaly.NewSmartHTTPClient(a.GitalySocketPath)
+	smarthttp, err := gitaly.NewSmartHTTPClient(a.GitalyAddress)
 	if err != nil {
 		return fmt.Errorf("GetInfoRefsHandler: %v", err)
 	}

internal/git/receive-pack.go

@@ -20,7 +20,7 @@ func handleReceivePack(w *GitHttpResponseWriter, r *http.Request, a *api.Respons
 	defer cw.Flush()
 
 	var err error
-	if a.GitalySocketPath == "" {
+	if a.GitalyAddress == "" {
 		err = handleReceivePackLocally(a, r, cr, cw, action)
 	} else {
 		err = handleReceivePackWithGitaly(a, cr, cw)
@@ -46,7 +46,7 @@ func handleReceivePackLocally(a *api.Response, r *http.Request, stdin io.Reader,
 }
 
 func handleReceivePackWithGitaly(a *api.Response, clientRequest io.Reader, clientResponse io.Writer) error {
-	smarthttp, err := gitaly.NewSmartHTTPClient(a.GitalySocketPath)
+	smarthttp, err := gitaly.NewSmartHTTPClient(a.GitalyAddress)
 	if err != nil {
 		return fmt.Errorf("smarthttp.ReceivePack: %v", err)
 	}

internal/git/upload-pack.go

@@ -27,7 +27,7 @@ func handleUploadPack(w *GitHttpResponseWriter, r *http.Request, a *api.Response
 	action := getService(r)
 	writePostRPCHeader(w, action)
 
-	if a.GitalySocketPath == "" {
+	if a.GitalyAddress == "" {
 		err = handleUploadPackLocally(a, r, buffer, w, action)
 	} else {
 		err = handleUploadPackWithGitaly(a, buffer, w)
@@ -58,7 +58,7 @@ func handleUploadPackLocally(a *api.Response, r *http.Request, stdin *os.File, s
 }
 
 func handleUploadPackWithGitaly(a *api.Response, clientRequest io.Reader, clientResponse io.Writer) error {
-	smarthttp, err := gitaly.NewSmartHTTPClient(a.GitalySocketPath)
+	smarthttp, err := gitaly.NewSmartHTTPClient(a.GitalyAddress)
 	if err != nil {
 		return fmt.Errorf("smarthttp.UploadPack: %v", err)
 	}

internal/gitaly/gitaly.go

 package gitaly
 
 import (
+	"fmt"
 	"net"
+	"net/url"
+	"strings"
 	"sync"
 	"time"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"google.golang.org/grpc"
-
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 )
 
 type connectionsCache struct {
@@ -20,8 +21,8 @@ var cache = connectionsCache{
 	connections: make(map[string]*grpc.ClientConn),
 }
 
-func NewSmartHTTPClient(socketPath string) (*SmartHTTPClient, error) {
-	conn, err := getOrCreateConnection(socketPath)
+func NewSmartHTTPClient(address string) (*SmartHTTPClient, error) {
+	conn, err := getOrCreateConnection(address)
 	if err != nil {
 		return nil, err
 	}
@@ -29,20 +30,20 @@ func NewSmartHTTPClient(socketPath string) (*SmartHTTPClient, error) {
 	return &SmartHTTPClient{grpcClient}, nil
 }
 
-func getOrCreateConnection(socketPath string) (*grpc.ClientConn, error) {
+func getOrCreateConnection(address string) (*grpc.ClientConn, error) {
 	cache.Lock()
 	defer cache.Unlock()
 
-	if conn := cache.connections[socketPath]; conn != nil {
+	if conn := cache.connections[address]; conn != nil {
 		return conn, nil
 	}
 
-	conn, err := newConnection(socketPath)
+	conn, err := newConnection(address)
 	if err != nil {
 		return nil, err
 	}
 
-	cache.connections[socketPath] = conn
+	cache.connections[address] = conn
 
 	return conn, nil
 }
@@ -56,17 +57,48 @@ func CloseConnections() {
 	}
 }
 
-func newConnection(socketPath string) (*grpc.ClientConn, error) {
+func newConnection(rawAddress string) (*grpc.ClientConn, error) {
+	network, addr, err := parseAddress(rawAddress)
+	if err != nil {
+		return nil, err
+	}
+
 	connOpts := []grpc.DialOption{
 		grpc.WithInsecure(), // Since we're connecting to Gitaly over UNIX, we don't need to use TLS credentials.
-		grpc.WithDialer(func(addr string, _ time.Duration) (net.Conn, error) {
-			return badgateway.DefaultDialer.Dial("unix", addr)
+		grpc.WithDialer(func(a string, _ time.Duration) (net.Conn, error) {
+			return net.Dial(network, a)
 		}),
 	}
-	conn, err := grpc.Dial(socketPath, connOpts...)
+	conn, err := grpc.Dial(addr, connOpts...)
 	if err != nil {
 		return nil, err
 	}
 
 	return conn, nil
 }
+
+func parseAddress(rawAddress string) (network, addr string, err error) {
+	// Parsing unix:// URL's with url.Parse does not give the result we want
+	// so we do it manually.
+	for _, prefix := range []string{"unix://", "unix:"} {
+		if strings.HasPrefix(rawAddress, prefix) {
+			return "unix", strings.TrimPrefix(rawAddress, prefix), nil
+		}
+	}
+
+	u, err := url.Parse(rawAddress)
+	if err != nil {
+		return "", "", err
+	}
+
+	if u.Scheme != "tcp" {
+		return "", "", fmt.Errorf("unknown scheme: %q", rawAddress)
+	}
+	if u.Host == "" {
+		return "", "", fmt.Errorf("network tcp requires host: %q", rawAddress)
+	}
+	if u.Path != "" {
+		return "", "", fmt.Errorf("network tcp should have no path: %q", rawAddress)
+	}
+	return "tcp", u.Host, nil
+}

internal/gitaly/gitaly_test.go

+package gitaly
+
+import (
+	"testing"
+)
+
+func TestParseAddress(t *testing.T) {
+	testCases := []struct {
+		raw     string
+		network string
+		addr    string
+		invalid bool
+	}{
+		{raw: "unix:/foo/bar.socket", network: "unix", addr: "/foo/bar.socket"},
+		{raw: "unix:///foo/bar.socket", network: "unix", addr: "/foo/bar.socket"},
+		// Mainly for test purposes we explicitly want to support relative paths
+		{raw: "unix://foo/bar.socket", network: "unix", addr: "foo/bar.socket"},
+		{raw: "unix:foo/bar.socket", network: "unix", addr: "foo/bar.socket"},
+		{raw: "tcp://1.2.3.4", network: "tcp", addr: "1.2.3.4"},
+		{raw: "tcp://1.2.3.4:567", network: "tcp", addr: "1.2.3.4:567"},
+		{raw: "tcp://foobar", network: "tcp", addr: "foobar"},
+		{raw: "tcp://foobar:567", network: "tcp", addr: "foobar:567"},
+		{raw: "tcp://1.2.3.4/foo/bar.socket", invalid: true},
+		{raw: "tcp:///foo/bar.socket", invalid: true},
+		{raw: "tcp:/foo/bar.socket", invalid: true},
+	}
+
+	for _, tc := range testCases {
+		network, addr, err := parseAddress(tc.raw)
+
+		if err == nil && tc.invalid {
+			t.Errorf("%v: expected error, got none", tc)
+		} else if err != nil && !tc.invalid {
+			t.Errorf("%v: parse error: %v", tc, err)
+			continue
+		}
+
+		if tc.invalid {
+			continue
+		}
+
+		if tc.network != network {
+			t.Errorf("%v: expected %q, got %q", tc, tc.network, network)
+		}
+
+		if tc.addr != addr {
+			t.Errorf("%v: expected %q, got %q", tc, tc.addr, addr)
+		}
+	}
+}

main_test.go

@@ -604,46 +604,60 @@ func TestGetInfoRefsProxiedToGitalySuccessfully(t *testing.T) {
 	gitalyServer, socketPath := startGitalyServer(t)
 	defer gitalyServer.Stop()
 
-	apiResponse.GitalySocketPath = socketPath
-	ts := testAuthServer(nil, 200, apiResponse)
-	defer ts.Close()
+	gitalyAddress := "unix://" + socketPath
 
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
+	addressCases := []struct {
+		socketPath string
+		address    string
+	}{
+		{socketPath: "/nonexistent,/should/be/ignored", address: gitalyAddress},
+		{socketPath: socketPath},
+	}
 
-	for _, testCase := range []struct {
+	repoCases := []struct {
 		repoPath   string
 		repository pb.Repository
 	}{
-		{repoPath: repoPath},
-		{repoPath: repoPath, repository: pb.Repository{Path: repoPath, StorageName: "foobar", RelativePath: "baz.git"}},
-	} {
-		func() {
-			apiResponse.RepoPath = testCase.repoPath
-			apiResponse.Repository = testCase.repository
-			apiResponse.GitalySocketPath = socketPath
-			ts := testAuthServer(nil, 200, apiResponse)
-			defer ts.Close()
-
-			ws := startWorkhorseServer(ts.URL)
-			defer ws.Close()
-
-			resource := "/gitlab-org/gitlab-test.git/info/refs?service=git-upload-pack"
-			resp, err := http.Get(ws.URL + resource)
-			if err != nil {
-				t.Fatal(err)
-			}
-			defer resp.Body.Close()
-			responseBody, err := ioutil.ReadAll(resp.Body)
-			if err != nil {
-				t.Error(err)
-			}
-
-			expectedContent := testhelper.GitalyInfoRefsResponseMock
-			if !bytes.Equal(responseBody, []byte(expectedContent)) {
-				t.Errorf("GET %q: Expected %q, got %q", resource, expectedContent, responseBody)
-			}
-		}()
+		{
+			repoPath: repoPath,
+		},
+		{
+			repoPath:   repoPath,
+			repository: pb.Repository{Path: repoPath, StorageName: "foobar", RelativePath: "baz.git"},
+		},
+	}
+
+	for _, ac := range addressCases {
+		for _, rc := range repoCases {
+			func() {
+				apiResponse.RepoPath = rc.repoPath
+				apiResponse.Repository = rc.repository
+				apiResponse.GitalySocketPath = ac.socketPath
+				apiResponse.GitalyAddress = ac.address
+
+				ts := testAuthServer(nil, 200, apiResponse)
+				defer ts.Close()
+
+				ws := startWorkhorseServer(ts.URL)
+				defer ws.Close()
+
+				resource := "/gitlab-org/gitlab-test.git/info/refs?service=git-upload-pack"
+				resp, err := http.Get(ws.URL + resource)
+				if err != nil {
+					t.Fatal(err)
+				}
+				defer resp.Body.Close()
+				responseBody, err := ioutil.ReadAll(resp.Body)
+				if err != nil {
+					t.Error(err)
+				}
+
+				expectedContent := testhelper.GitalyInfoRefsResponseMock
+				if !bytes.Equal(responseBody, []byte(expectedContent)) {
+					t.Errorf("GET %q: Expected %q, got %q", resource, expectedContent, responseBody)
+				}
+			}()
+		}
 	}
 }
 
@@ -653,7 +667,7 @@ func TestPostReceivePackProxiedToGitalySuccessfully(t *testing.T) {
 	gitalyServer, socketPath := startGitalyServer(t)
 	defer gitalyServer.Stop()
 
-	apiResponse.GitalySocketPath = socketPath
+	apiResponse.GitalyAddress = "unix://" + socketPath
 	ts := testAuthServer(nil, 200, apiResponse)
 	defer ts.Close()
 
@@ -690,7 +704,7 @@ func TestPostUploadPackProxiedToGitalySuccessfully(t *testing.T) {
 	gitalyServer, socketPath := startGitalyServer(t)
 	defer gitalyServer.Stop()
 
-	apiResponse.GitalySocketPath = socketPath
+	apiResponse.GitalyAddress = "unix://" + socketPath
 	ts := testAuthServer(nil, 200, apiResponse)
 	defer ts.Close()