Merge branch 'an-fix-gitaly-ruby-proxy-issues' into 'master'

Bring back a custom dialer for Gitaly Ruby

Closes gitlab-ce#53473 and #1447

See merge request gitlab-org/gitaly!1072

7 files changed, 38 insertions, 3 deletions

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c84e7445d..fee394b7c 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -105,6 +105,12 @@ test:go1.10-git2.18-ruby-2.4:
   <<: *test_definition
   image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.4-golang-1.10-git-2.18
 
+test:proxy:
+  <<: *test_definition
+  image: registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.5-golang-1.10-git-2.18
+  script:
+    - make test-with-proxies
+
 race:
   <<: *go_test_definition
   script:
diff --git a/Makefile b/Makefile
index 5e79ab055..3772a585b 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,10 @@ prepare-tests: prepare-build
 test: prepare-build
 	cd $(BUILD_DIR) && $(MAKE) $@
 	
+.PHONY: test-with-proxies
+test-with-proxies: prepare-build
+	cd $(BUILD_DIR) && $(MAKE) $@
+
 .PHONY: rspec
 rspec: prepare-build
 	cd $(BUILD_DIR) && $(MAKE) $@
diff --git a/_support/makegen.go b/_support/makegen.go
index c381b149b..d5555acdd 100644
--- a/_support/makegen.go
+++ b/_support/makegen.go
@@ -317,6 +317,10 @@ test: test-go rspec rspec-gitlab-shell
 test-go: prepare-tests
 	@go test -tags "$(BUILD_TAGS)" -count=1 {{ join .AllPackages " " }} # count=1 bypasses go 1.10 test caching
 
+.PHONY: test-with-proxies
+test-with-proxies: prepare-tests
+	@http_proxy=http://invalid https_proxy=https://invalid go test -tags "$(BUILD_TAGS)" -count=1  {{ .Pkg }}/internal/rubyserver/
+
 .PHONY: race-go
 race-go: prepare-tests
 	@go test -tags "$(BUILD_TAGS)" -race {{ join .AllPackages " " }}
diff --git a/changelogs/unreleased/an-fix-gitaly-ruby-proxy-issues.yml b/changelogs/unreleased/an-fix-gitaly-ruby-proxy-issues.yml
new file mode 100644
index 000000000..0ea487a21
--- /dev/null
+++ b/changelogs/unreleased/an-fix-gitaly-ruby-proxy-issues.yml
@@ -0,0 +1,5 @@
+---
+title: Bring back a custom dialer for Gitaly Ruby
+merge_request: 1072
+author:
+type: fixed
diff --git a/client/dial.go b/client/dial.go
index fe4a3e683..1874cc6e1 100644
--- a/client/dial.go
+++ b/client/dial.go
@@ -59,6 +59,9 @@ func Dial(rawAddress string, connOpts []grpc.DialOption) (*grpc.ClientConn, erro
 		connOpts = append(
 			connOpts,
 			grpc.WithInsecure(),
+			// Use a custom dialer to ensure that we don't experience
+			// issues in environments that have proxy configurations
+			// https://gitlab.com/gitlab-org/gitaly/merge_requests/1072#note_140408512
 			grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
 				path, err := extractPathFromSocketURL(addr)
 				if err != nil {
diff --git a/internal/rubyserver/health.go b/internal/rubyserver/health.go
index cc6005f89..ffab0c779 100644
--- a/internal/rubyserver/health.go
+++ b/internal/rubyserver/health.go
@@ -2,6 +2,7 @@ package rubyserver
 
 import (
 	"context"
+	"net"
 	"time"
 
 	"google.golang.org/grpc"
@@ -12,6 +13,12 @@ func ping(address string) error {
 	conn, err := grpc.Dial(
 		address,
 		grpc.WithInsecure(),
+		// Use a custom dialer to ensure that we don't experience
+		// issues in environments that have proxy configurations
+		// https://gitlab.com/gitlab-org/gitaly/merge_requests/1072#note_140408512
+		grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
+			return net.DialTimeout("unix", addr, timeout)
+		}),
 	)
 	if err != nil {
 		return err
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 8eab03025..615444287 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
+	"net"
 	"os"
 	"path"
 	"path/filepath"
@@ -128,7 +129,6 @@ func Start() (*Server, error) {
 	for i := 0; i < numWorkers; i++ {
 		name := fmt.Sprintf("gitaly-ruby.%d", i)
 		socketPath := socketPath(i)
-		address := "unix://" + socketPath
 
 		// Use 'ruby-cd' to make sure gitaly-ruby has the same working directory
 		// as the current process. This is a hack to sort-of support relative
@@ -136,13 +136,13 @@ func Start() (*Server, error) {
 		args := []string{"bundle", "exec", "bin/ruby-cd", wd, gitalyRuby, strconv.Itoa(os.Getpid()), socketPath}
 
 		events := make(chan supervisor.Event)
-		check := func() error { return ping(address) }
+		check := func() error { return ping(socketPath) }
 		p, err := supervisor.New(name, env, args, cfg.Ruby.Dir, cfg.Ruby.MaxRSS, events, check)
 		if err != nil {
 			return nil, err
 		}
 
-		s.workers = append(s.workers, newWorker(p, address, events, false))
+		s.workers = append(s.workers, newWorker(p, socketPath, events, false))
 	}
 
 	return s, nil
@@ -256,6 +256,12 @@ func dialOptions() []grpc.DialOption {
 	return []grpc.DialOption{
 		grpc.WithBlock(), // With this we get retries. Without, connections fail fast.
 		grpc.WithInsecure(),
+		// Use a custom dialer to ensure that we don't experience
+		// issues in environments that have proxy configurations
+		// https://gitlab.com/gitlab-org/gitaly/merge_requests/1072#note_140408512
+		grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
+			return net.DialTimeout("unix", addr, timeout)
+		}),
 		grpc.WithUnaryInterceptor(
 			grpc_middleware.ChainUnaryClient(
 				grpc_prometheus.UnaryClientInterceptor,