Merge branch 'gitaly-repository-fix' into 'master'

Gitaly cleanup

See merge request !152

internal/api/api.go

@@ -89,8 +89,6 @@ type Response struct {
 	Entry string `json:"entry"`
 	// Used to communicate terminal session details
 	Terminal *TerminalSettings
-	// 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
@@ -222,12 +220,6 @@ func (api *API) PreAuthorize(suffix string, r *http.Request) (httpResponse *http
 		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/receive-pack.go

@@ -51,7 +51,7 @@ func handleReceivePackWithGitaly(a *api.Response, clientRequest io.Reader, clien
 		return fmt.Errorf("smarthttp.ReceivePack: %v", err)
 	}
 
-	if err := smarthttp.ReceivePack(a, clientRequest, clientResponse); err != nil {
+	if err := smarthttp.ReceivePack(&a.Repository, a.GL_ID, clientRequest, clientResponse); err != nil {
 		return fmt.Errorf("smarthttp.ReceivePack: %v", err)
 	}
 

internal/git/upload-pack.go

@@ -63,7 +63,7 @@ func handleUploadPackWithGitaly(a *api.Response, clientRequest io.Reader, client
 		return fmt.Errorf("smarthttp.UploadPack: %v", err)
 	}
 
-	if err := smarthttp.UploadPack(a, clientRequest, clientResponse); err != nil {
+	if err := smarthttp.UploadPack(&a.Repository, clientRequest, clientResponse); err != nil {
 		return fmt.Errorf("smarthttp.UploadPack: %v", err)
 	}
 

internal/gitaly/smarthttp.go

@@ -4,8 +4,6 @@ import (
 	"fmt"
 	"io"
 
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
-
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	pbhelper "gitlab.com/gitlab-org/gitaly-proto/go/helper"
 
@@ -48,8 +46,7 @@ func (client *SmartHTTPClient) InfoRefsResponseWriterTo(repo *pb.Repository, rpc
 	return &pbhelper.InfoRefsClientWriterTo{c}, nil
 }
 
-func (client *SmartHTTPClient) ReceivePack(a *api.Response, clientRequest io.Reader, clientResponse io.Writer) error {
-	repo := &pb.Repository{Path: a.RepoPath}
+func (client *SmartHTTPClient) ReceivePack(repo *pb.Repository, GlId string, clientRequest io.Reader, clientResponse io.Writer) error {
 	stream, err := client.PostReceivePack(context.Background())
 	if err != nil {
 		return err
@@ -57,7 +54,7 @@ func (client *SmartHTTPClient) ReceivePack(a *api.Response, clientRequest io.Rea
 
 	rpcRequest := &pb.PostReceivePackRequest{
 		Repository: repo,
-		GlId:       a.GL_ID,
+		GlId:       GlId,
 	}
 
 	if err := stream.Send(rpcRequest); err != nil {
@@ -84,8 +81,7 @@ func (client *SmartHTTPClient) ReceivePack(a *api.Response, clientRequest io.Rea
 	return nil
 }
 
-func (client *SmartHTTPClient) UploadPack(a *api.Response, clientRequest io.Reader, clientResponse io.Writer) error {
-	repo := &pb.Repository{Path: a.RepoPath}
+func (client *SmartHTTPClient) UploadPack(repo *pb.Repository, clientRequest io.Reader, clientResponse io.Writer) error {
 	stream, err := client.PostUploadPack(context.Background())
 	if err != nil {
 		return err

internal/testhelper/gitaly.go

 package testhelper
 
 import (
+	"fmt"
 	"io"
 	"io/ioutil"
 	"log"
 	"path"
+	"strings"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 )
@@ -31,6 +33,10 @@ func NewGitalyServer() *GitalyTestServer {
 }
 
 func (s *GitalyTestServer) InfoRefsUploadPack(in *pb.InfoRefsRequest, stream pb.SmartHTTP_InfoRefsUploadPackServer) error {
+	if err := validateRepository(in.GetRepository()); err != nil {
+		return err
+	}
+
 	response := &pb.InfoRefsResponse{
 		Data: []byte(GitalyInfoRefsResponseMock),
 	}
@@ -38,6 +44,10 @@ func (s *GitalyTestServer) InfoRefsUploadPack(in *pb.InfoRefsRequest, stream pb.
 }
 
 func (s *GitalyTestServer) InfoRefsReceivePack(in *pb.InfoRefsRequest, stream pb.SmartHTTP_InfoRefsReceivePackServer) error {
+	if err := validateRepository(in.GetRepository()); err != nil {
+		return err
+	}
+
 	response := &pb.InfoRefsResponse{
 		Data: []byte(GitalyInfoRefsResponseMock),
 	}
@@ -50,8 +60,17 @@ func (s *GitalyTestServer) PostReceivePack(stream pb.SmartHTTP_PostReceivePackSe
 		return err
 	}
 
+	repo := req.GetRepository()
+	if err := validateRepository(req.GetRepository()); err != nil {
+		return err
+	}
 	response := &pb.PostReceivePackResponse{
-		Data: []byte(req.Repository.GetPath() + req.GlId),
+		Data: []byte(strings.Join([]string{
+			repo.GetPath(),
+			repo.GetStorageName(),
+			repo.GetRelativePath(),
+			req.GlId,
+		}, "\000") + "\000"),
 	}
 	if err := stream.Send(response); err != nil {
 		return err
@@ -84,8 +103,16 @@ func (s *GitalyTestServer) PostUploadPack(stream pb.SmartHTTP_PostUploadPackServ
 		return err
 	}
 
+	repo := req.GetRepository()
+	if err := validateRepository(req.GetRepository()); err != nil {
+		return err
+	}
 	response := &pb.PostUploadPackResponse{
-		Data: []byte(req.Repository.GetPath()),
+		Data: []byte(strings.Join([]string{
+			repo.GetPath(),
+			repo.GetStorageName(),
+			repo.GetRelativePath(),
+		}, "\000") + "\000"),
 	}
 	if err := stream.Send(response); err != nil {
 		return err
@@ -111,3 +138,16 @@ func (s *GitalyTestServer) PostUploadPack(stream pb.SmartHTTP_PostUploadPackServ
 
 	return nil
 }
+
+func validateRepository(repo *pb.Repository) error {
+	if len(repo.GetPath()) == 0 {
+		return fmt.Errorf("missing path: %v", repo)
+	}
+	if len(repo.GetStorageName()) == 0 {
+		return fmt.Errorf("missing storage_name: %v", repo)
+	}
+	if len(repo.GetRelativePath()) == 0 {
+		return fmt.Errorf("missing relative_path: %v", repo)
+	}
+	return nil
+}

main_test.go

@@ -145,7 +145,7 @@ func TestFailedCloneNoGitaly(t *testing.T) {
 		GL_ID:    "user-123",
 		RepoPath: repoPath(t),
 		// This will create a failure to connect to Gitaly
-		GitalySocketPath: "/nonexistent",
+		GitalyAddress: "unix:/nonexistent",
 	}
 
 	// Prepare test server and backend
@@ -599,66 +599,34 @@ func TestApiContentTypeBlock(t *testing.T) {
 
 func TestGetInfoRefsProxiedToGitalySuccessfully(t *testing.T) {
 	apiResponse := gitOkBody(t)
-	repoPath := apiResponse.RepoPath
-
 	gitalyServer, socketPath := startGitalyServer(t)
 	defer gitalyServer.Stop()
 
 	gitalyAddress := "unix://" + socketPath
+	apiResponse.GitalyAddress = gitalyAddress
 
-	addressCases := []struct {
-		socketPath string
-		address    string
-	}{
-		{socketPath: "/nonexistent,/should/be/ignored", address: gitalyAddress},
-		{socketPath: socketPath},
-	}
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
 
-	repoCases := []struct {
-		repoPath   string
-		repository pb.Repository
-	}{
-		{
-			repoPath: repoPath,
-		},
-		{
-			repoPath:   repoPath,
-			repository: pb.Repository{Path: repoPath, StorageName: "foobar", RelativePath: "baz.git"},
-		},
+	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)
 	}
 
-	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)
-				}
-			}()
-		}
+	expectedContent := testhelper.GitalyInfoRefsResponseMock
+	if !bytes.Equal(responseBody, []byte(expectedContent)) {
+		t.Errorf("GET %q: Expected %q, got %q", resource, expectedContent, responseBody)
 	}
+
 }
 
 func TestPostReceivePackProxiedToGitalySuccessfully(t *testing.T) {
@@ -692,9 +660,15 @@ func TestPostReceivePackProxiedToGitalySuccessfully(t *testing.T) {
 	if resp.StatusCode != 200 {
 		t.Errorf("GET %q: expected 200, got %d", resource, resp.StatusCode)
 	}
-
-	if string(responseBody) != apiResponse.RepoPath+apiResponse.GL_ID+string(testhelper.GitalyReceivePackResponseMock) {
-		t.Errorf("GET %q: Unexpected response", resource)
+	expectedResponse := strings.Join([]string{
+		apiResponse.RepoPath,
+		apiResponse.Repository.StorageName,
+		apiResponse.Repository.RelativePath,
+		apiResponse.GL_ID,
+		string(testhelper.GitalyReceivePackResponseMock),
+	}, "\000")
+	if string(responseBody) != expectedResponse {
+		t.Errorf("GET %q: Unexpected response %.100q", resource, responseBody)
 	}
 }
 
@@ -730,8 +704,14 @@ func TestPostUploadPackProxiedToGitalySuccessfully(t *testing.T) {
 		t.Errorf("GET %q: expected 200, got %d", resource, resp.StatusCode)
 	}
 
-	if string(responseBody) != apiResponse.RepoPath+string(testhelper.GitalyUploadPackResponseMock) {
-		t.Errorf("GET %q: Unexpected response", resource)
+	expected := strings.Join([]string{
+		apiResponse.RepoPath,
+		apiResponse.Repository.StorageName,
+		apiResponse.Repository.RelativePath,
+		string(testhelper.GitalyUploadPackResponseMock),
+	}, "\000")
+	if string(responseBody) != expected {
+		t.Errorf("GET %q: Unexpected response: %.100q", resource, responseBody)
 	}
 }
 
@@ -740,7 +720,7 @@ func TestGetInfoRefsHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
 	defer gitalyServer.Stop()
 
 	apiResponse := gitOkBody(t)
-	apiResponse.GitalySocketPath = ""
+	apiResponse.GitalyAddress = ""
 	ts := testAuthServer(nil, 200, apiResponse)
 	defer ts.Close()
 
@@ -773,7 +753,7 @@ func TestPostReceivePackHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
 	defer gitalyServer.Stop()
 
 	apiResponse := gitOkBody(t)
-	apiResponse.GitalySocketPath = ""
+	apiResponse.GitalyAddress = ""
 	ts := testAuthServer(nil, 200, apiResponse)
 	defer ts.Close()
 
@@ -804,7 +784,7 @@ func TestPostUploadPackHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
 	defer gitalyServer.Stop()
 
 	apiResponse := gitOkBody(t)
-	apiResponse.GitalySocketPath = ""
+	apiResponse.GitalyAddress = ""
 	ts := testAuthServer(nil, 200, apiResponse)
 	defer ts.Close()
 
@@ -1027,9 +1007,15 @@ func runOrFail(t *testing.T, cmd *exec.Cmd) {
 }
 
 func gitOkBody(t *testing.T) *api.Response {
+	repoPath := repoPath(t)
 	return &api.Response{
 		GL_ID:    "user-123",
-		RepoPath: repoPath(t),
+		RepoPath: repoPath,
+		Repository: pb.Repository{
+			Path:         repoPath,
+			StorageName:  "default",
+			RelativePath: "foo/bar.git",
+		},
 	}
 }