Merge branch 'id-clean-up-gitaly-unavailable-err' into 'master'

Improve Git HTTP err message given when Gitaly unavailable

See merge request gitlab-org/gitlab!71120

app/controllers/repositories/git_http_controller.rb

@@ -11,6 +11,9 @@ module Repositories
     rescue_from Gitlab::GitAccess::NotFoundError, with: :render_404_with_exception
     rescue_from Gitlab::GitAccessProject::CreationError, with: :render_422_with_exception
     rescue_from Gitlab::GitAccess::TimeoutError, with: :render_503_with_exception
+    rescue_from GRPC::Unavailable do |e|
+      render_503_with_exception(e, message: 'The git server, Gitaly, is not available at this time. Please contact your administrator.')
+    end
 
     # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
     # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
@@ -71,8 +74,8 @@ module Repositories
       render plain: exception.message, status: :unprocessable_entity
     end
 
-    def render_503_with_exception(exception)
-      render plain: exception.message, status: :service_unavailable
+    def render_503_with_exception(exception, message: nil)
+      render plain: message || exception.message, status: :service_unavailable
     end
 
     def update_fetch_statistics

spec/controllers/repositories/git_http_controller_spec.rb

@@ -7,12 +7,33 @@ RSpec.describe Repositories::GitHttpController do
   let_it_be(:personal_snippet) { create(:personal_snippet, :public, :repository) }
   let_it_be(:project_snippet) { create(:project_snippet, :public, :repository, project: project) }
 
+  shared_examples 'handles unavailable Gitaly' do
+    let(:params) { super().merge(service: 'git-upload-pack') }
+
+    before do
+      request.headers.merge! auth_env(user.username, user.password, nil)
+    end
+
+    context 'when Gitaly is unavailable' do
+      it 'responds with a 503 message' do
+        expect(Gitlab::GitalyClient).to receive(:call).and_raise(GRPC::Unavailable)
+
+        get :info_refs, params: params
+
+        expect(response).to have_gitlab_http_status(:service_unavailable)
+        expect(response.body).to eq('The git server, Gitaly, is not available at this time. Please contact your administrator.')
+      end
+    end
+  end
+
   context 'when repository container is a project' do
     it_behaves_like Repositories::GitHttpController do
       let(:container) { project }
       let(:user) { project.owner }
       let(:access_checker_class) { Gitlab::GitAccess }
 
+      it_behaves_like 'handles unavailable Gitaly'
+
       describe 'POST #git_upload_pack' do
         before do
           allow(controller).to receive(:verify_workhorse_api!).and_return(true)
@@ -84,6 +105,8 @@ RSpec.describe Repositories::GitHttpController do
       let(:container) { personal_snippet }
       let(:user) { personal_snippet.author }
       let(:access_checker_class) { Gitlab::GitAccessSnippet }
+
+      it_behaves_like 'handles unavailable Gitaly'
     end
   end
 
@@ -92,6 +115,8 @@ RSpec.describe Repositories::GitHttpController do
       let(:container) { project_snippet }
       let(:user) { project_snippet.author }
       let(:access_checker_class) { Gitlab::GitAccessSnippet }
+
+      it_behaves_like 'handles unavailable Gitaly'
     end
   end
 end

workhorse/internal/git/info-refs.go

@@ -8,8 +8,8 @@ import (
 	"net/http"
 
 	"github.com/golang/gddo/httputil"
-
-	"gitlab.com/gitlab-org/labkit/log"
+	grpccodes "google.golang.org/grpc/codes"
+	grpcstatus "google.golang.org/grpc/status"
 
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/gitaly"
@@ -26,6 +26,7 @@ func handleGetInfoRefs(rw http.ResponseWriter, r *http.Request, a *api.Response)
 	defer responseWriter.Log(r, 0)
 
 	rpc := getService(r)
+
 	if !(rpc == "git-upload-pack" || rpc == "git-receive-pack") {
 		// The 'dumb' Git HTTP protocol is not supported
 		http.Error(responseWriter, "Not Found", 404)
@@ -41,19 +42,26 @@ func handleGetInfoRefs(rw http.ResponseWriter, r *http.Request, a *api.Response)
 	encoding := httputil.NegotiateContentEncoding(r, offers)
 
 	if err := handleGetInfoRefsWithGitaly(r.Context(), responseWriter, a, rpc, gitProtocol, encoding); err != nil {
-		helper.Fail500(responseWriter, r, fmt.Errorf("handleGetInfoRefs: %v", err))
+		status := grpcstatus.Convert(err)
+		err = fmt.Errorf("handleGetInfoRefs: %v", err)
+
+		if status != nil && status.Code() == grpccodes.Unavailable {
+			helper.CaptureAndFail(responseWriter, r, err, "The git server, Gitaly, is not available at this time. Please contact your administrator.", http.StatusServiceUnavailable)
+		} else {
+			helper.Fail500(responseWriter, r, err)
+		}
 	}
 }
 
 func handleGetInfoRefsWithGitaly(ctx context.Context, responseWriter *HttpResponseWriter, a *api.Response, rpc, gitProtocol, encoding string) error {
 	ctx, smarthttp, err := gitaly.NewSmartHTTPClient(ctx, a.GitalyServer)
 	if err != nil {
-		return fmt.Errorf("GetInfoRefsHandler: %v", err)
+		return err
 	}
 
 	infoRefsResponseReader, err := smarthttp.InfoRefsResponseReader(ctx, &a.Repository, rpc, gitConfigOptions(a), gitProtocol)
 	if err != nil {
-		return fmt.Errorf("GetInfoRefsHandler: %v", err)
+		return err
 	}
 
 	var w io.Writer
@@ -69,7 +77,7 @@ func handleGetInfoRefsWithGitaly(ctx context.Context, responseWriter *HttpRespon
 	}
 
 	if _, err = io.Copy(w, infoRefsResponseReader); err != nil {
-		log.WithError(err).Error("GetInfoRefsHandler: error copying gitaly response")
+		return err
 	}
 
 	return nil

workhorse/internal/git/info-refs_test.go

+package git
+
+import (
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	grpccodes "google.golang.org/grpc/codes"
+	grpcstatus "google.golang.org/grpc/status"
+
+	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
+
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/gitaly"
+)
+
+type smartHTTPServiceServerWithInfoRefs struct {
+	gitalypb.UnimplementedSmartHTTPServiceServer
+	InfoRefsUploadPackFunc func(*gitalypb.InfoRefsRequest, gitalypb.SmartHTTPService_InfoRefsUploadPackServer) error
+}
+
+func (srv *smartHTTPServiceServerWithInfoRefs) InfoRefsUploadPack(r *gitalypb.InfoRefsRequest, s gitalypb.SmartHTTPService_InfoRefsUploadPackServer) error {
+	return srv.InfoRefsUploadPackFunc(r, s)
+}
+
+func TestGetInfoRefsHandler(t *testing.T) {
+	addr := startSmartHTTPServer(t, &smartHTTPServiceServerWithInfoRefs{
+		InfoRefsUploadPackFunc: func(r *gitalypb.InfoRefsRequest, s gitalypb.SmartHTTPService_InfoRefsUploadPackServer) error {
+			return grpcstatus.Error(grpccodes.Unavailable, "error")
+		},
+	})
+
+	w := httptest.NewRecorder()
+	r := httptest.NewRequest("GET", "/?service=git-upload-pack", nil)
+	a := &api.Response{GitalyServer: gitaly.Server{Address: addr}}
+
+	handleGetInfoRefs(NewHttpResponseWriter(w), r, a)
+	require.Equal(t, 503, w.Code)
+
+	msg := "The git server, Gitaly, is not available at this time. Please contact your administrator.\n"
+	require.Equal(t, msg, w.Body.String())
+}

workhorse/internal/git/upload-pack_test.go

@@ -45,14 +45,13 @@ func TestUploadPackTimesOut(t *testing.T) {
 	uploadPackTimeout = time.Millisecond
 	defer func() { uploadPackTimeout = originalUploadPackTimeout }()
 
-	addr, cleanUp := startSmartHTTPServer(t, &smartHTTPServiceServer{
+	addr := startSmartHTTPServer(t, &smartHTTPServiceServer{
 		PostUploadPackFunc: func(stream gitalypb.SmartHTTPService_PostUploadPackServer) error {
 			_, err := stream.Recv() // trigger a read on the client request body
 			require.NoError(t, err)
 			return nil
 		},
 	})
-	defer cleanUp()
 
 	body := &fakeReader{n: 0, err: nil}
 
@@ -64,7 +63,9 @@ func TestUploadPackTimesOut(t *testing.T) {
 	require.EqualError(t, err, "smarthttp.UploadPack: busyReader: context deadline exceeded")
 }
 
-func startSmartHTTPServer(t testing.TB, s gitalypb.SmartHTTPServiceServer) (string, func()) {
+func startSmartHTTPServer(t testing.TB, s gitalypb.SmartHTTPServiceServer) string {
+	t.Helper()
+
 	tmp, err := ioutil.TempDir("", "")
 	require.NoError(t, err)
 
@@ -78,8 +79,10 @@ func startSmartHTTPServer(t testing.TB, s gitalypb.SmartHTTPServiceServer) (stri
 		require.NoError(t, srv.Serve(ln))
 	}()
 
-	return fmt.Sprintf("%s://%s", ln.Addr().Network(), ln.Addr().String()), func() {
+	t.Cleanup(func() {
 		srv.GracefulStop()
 		require.NoError(t, os.RemoveAll(tmp), "error removing temp dir %q", tmp)
-	}
+	})
+
+	return fmt.Sprintf("%s://%s", ln.Addr().Network(), ln.Addr().String())
 }