test the code host connection for Perforce (fixes #43855) (#49069)

# changes - add p4 to the repo-updater Dockerfile - bump the version of p4 from 21.2 to 22.2 for all Dockerfiles, and add hash checking of the download - enhance the Perforce VCSSyncer so that it captures the output of `p4 depots` and matches it to the given depot, returning errors if the given depot doen't match any of the depots returned by `p4 depots` - fill out `ListRepos` in the Perforce Source so that it calls the Perforce VCSSyncer's IsCloneable, which checks the given depot agains the depots from the Perforce server # before and after videos https://www.loom.com/share/73099aa7f0ec4427a564f07b80a19a46 https://www.loom.com/share/b7d38469baab496ea50ab22bdfa256da ## Test plan Add or edit a code host with purposefully wrong connection info; either the host/port, the username, or the password. A warning message will display (sometimes a timeout message displays instead) in the Manage Code Host screen, and an error message will display in the list of code hosts (sometimes have to wait for the next sync before that appears). Add or modify a depot so that it is invalid. The repository count will not increase, and an error message will display in the list of code hosts. There's no error message in the Manage Code Hose screen, which would be nice - fodder for iteration.  --------- Co-authored-by: Thorsten Ball <mrnugget@gmail.com> Co-authored-by: Indradhanush Gupta <indradhanush.gupta@gmail.com>
2026-02-06 16:31:47 +00:00 · 2023-03-10 13:16:22 -08:00 · 2023-03-10 13:16:22 -08:00 · 3d9b72e43c
commit 3d9b72e43c
parent 4286ccc609
8 changed files with 221 additions and 20 deletions
--- a/cmd/gitserver/Dockerfile
+++ b/cmd/gitserver/Dockerfile
@ -6,10 +6,12 @@
 # Install p4 CLI (keep this up to date with cmd/server/Dockerfile)
 FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS p4cli

-# hadolint ignore=DL3003
-RUN wget http://cdist2.perforce.com/perforce/r21.2/bin.linux26x86_64/p4 && \
-    mv p4 /usr/local/bin/p4 && \
-    chmod +x /usr/local/bin/p4
+# hash provided in http://filehost.perforce.com/perforce/r22.2/bin.linux26x86_64/SHA256SUMS
+# if the hash is not provided, calculate it by downloading the file and running `sha256sum` on it in Terminal
+RUN echo "8bc10fca1c5a26262b4072deec76150a668581a9749d0504cd443084773d4fd0  /usr/local/bin/p4" >expected_hash && \
+    wget http://cdist2.perforce.com/perforce/r22.2/bin.linux26x86_64/p4 -O /usr/local/bin/p4 && \
+    chmod +x /usr/local/bin/p4 && \
+    sha256sum -c expected_hash

 FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS p4-fusion

--- a/cmd/gitserver/server/vcs_syncer_perforce.go
+++ b/cmd/gitserver/server/vcs_syncer_perforce.go
@ -1,7 +1,10 @@
 package server

 import (
+	"bufio"
+	"bytes"
 	"context"
+	"encoding/json"
 	"fmt"
 	"os"
 	"os/exec"
@ -11,11 +14,37 @@ import (
 	"time"

 	"github.com/sourcegraph/log"
+
 	"github.com/sourcegraph/sourcegraph/internal/vcs"
 	"github.com/sourcegraph/sourcegraph/internal/wrexec"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )

+type PerforceDepotType string
+
+const (
+	Local   PerforceDepotType = "local"
+	Remote  PerforceDepotType = "remote"
+	Stream  PerforceDepotType = "stream"
+	Spec    PerforceDepotType = "spec"
+	Unload  PerforceDepotType = "unload"
+	Archive PerforceDepotType = "archive"
+	Tangent PerforceDepotType = "tangent"
+	Graph   PerforceDepotType = "graph"
+)
+
+// PerforceDepot is a definiton of a depot that matches the format
+// returned from `p4 -Mj -ztag depots`
+type PerforceDepot struct {
+	Desc string `json:"desc,omitempty"`
+	Map  string `json:"map,omitempty"`
+	Name string `json:"name,omitempty"`
+	// Time is seconds since the Epoch, but p4 quotes it in the output, so it's a string
+	Time string `json:"time,omitempty"`
+	// Type is local, remote, stream, spec, unload, archive, tangent, graph
+	Type PerforceDepotType `json:"type,omitempty"`
+}
+
 // PerforceDepotSyncer is a syncer for Perforce depots.
 type PerforceDepotSyncer struct {
 	// MaxChanges indicates to only import at most n changes when possible.
@ -39,13 +68,39 @@ func (s *PerforceDepotSyncer) Type() string {

 // IsCloneable checks to see if the Perforce remote URL is cloneable.
 func (s *PerforceDepotSyncer) IsCloneable(ctx context.Context, remoteURL *vcs.URL) error {
-	username, password, host, _, err := decomposePerforceRemoteURL(remoteURL)
+	username, password, host, path, err := decomposePerforceRemoteURL(remoteURL)
 	if err != nil {
 		return errors.Wrap(err, "decompose")
 	}

-	// FIXME: Need to find a way to determine if depot exists instead of a general test of the Perforce server.
-	return p4testWithTrust(ctx, host, username, password)
+	// start with a test and set up trust if necessary
+	if err := p4testWithTrust(ctx, host, username, password); err != nil {
+		return err
+	}
+
+	// the path could be a path into a depot, or it could be just a depot
+	// expect it to start with at least one slash
+	// (the config defines it as starting with two, but converting it to a URL may change that)
+	// the first path part will be the depot - subsequent parts define a directory path into a depot
+	// ignore the directory parts for now, and only test for access to the depot
+	// TODO: revisit if we want to also test for access to the directories, if any are included
+	depot := strings.Split(strings.TrimLeft(path, "/"), "/")[0]
+
+	// get a list of depots that match the supplied depot (if it's defined)
+	if depots, err := p4depots(ctx, host, username, password, depot); err != nil {
+		return err
+	} else if len(depots) == 0 {
+		// this user doesn't have access to any depots,
+		// or to the given depot
+		if depot != "" {
+			return errors.Newf("the user %s does not have access to the depot %s on the server %s", username, depot, host)
+		} else {
+			return errors.Newf("the user %s does not have access to any depots on the server %s", username, host)
+		}
+	}
+
+	// no overt errors, so this depot is cloneable
+	return nil
 }

 // CloneCommand returns the command to be executed for cloning a Perforce depot as a Git repository.
@ -215,7 +270,7 @@ func p4test(ctx context.Context, host, username, password string) error {
 	ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
 	defer cancel()

-	// `p4 ping` requires extra-special access, so it would be good to avoid using it
+	// `p4 ping` requires extra-special access, so we want to avoid using it
 	//
 	// p4 login -s checks the connection and the credentials,
 	// so it seems like the perfect alternative to `p4 ping`.
@ -239,6 +294,57 @@ func p4test(ctx context.Context, host, username, password string) error {
 	return nil
 }

+// p4depots returns all of the depots to which the user has access on the host
+// and whose names match the given nameFilter, which can contain asterisks (*) for wildcards
+// if nameFilter is blank, return all depots
+func p4depots(ctx context.Context, host, username, password, nameFilter string) ([]PerforceDepot, error) {
+	ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
+	defer cancel()
+
+	var cmd *exec.Cmd
+	if nameFilter == "" {
+		cmd = exec.CommandContext(ctx, "p4", "-Mj", "-ztag", "depots")
+	} else {
+		cmd = exec.CommandContext(ctx, "p4", "-Mj", "-ztag", "depots", "-e", nameFilter)
+	}
+	cmd.Env = append(os.Environ(),
+		"P4PORT="+host,
+		"P4USER="+username,
+		"P4PASSWD="+password,
+	)
+
+	out, err := runWith(ctx, wrexec.Wrap(ctx, log.NoOp(), cmd), false, nil)
+	if err != nil {
+		if ctxerr := ctx.Err(); ctxerr != nil {
+			err = ctxerr
+		}
+		if len(out) > 0 {
+			err = errors.Wrapf(err, `failed to run command "p4 depots" (output follows)\n\n%s`, specifyCommandInErrorMessage(string(out), cmd))
+		}
+		return nil, err
+	}
+	depots := make([]PerforceDepot, 0)
+	if len(out) > 0 {
+		// the output of `p4 -Mj -ztag depots` is a series of JSON-formatted depot definitions, one per line
+		buf := bufio.NewScanner(bytes.NewBuffer(out))
+		for buf.Scan() {
+			depot := PerforceDepot{}
+			err := json.Unmarshal(buf.Bytes(), &depot)
+			if err != nil {
+				return nil, errors.Wrap(err, "malformed output from p4 depots")
+			}
+			depots = append(depots, depot)
+		}
+		if err := buf.Err(); err != nil {
+			return nil, errors.Wrap(err, "malformed output from p4 depots")
+		}
+		return depots, nil
+	}
+
+	// no error, but also no depots. Maybe the user doesn't have access to any depots?
+	return depots, nil
+}
+
 func specifyCommandInErrorMessage(errorMsg string, command *exec.Cmd) string {
 	if !strings.Contains(errorMsg, "this operation") {
 		return errorMsg
@ -254,7 +360,7 @@ func p4testWithTrust(ctx context.Context, host, username, password string) error
 	// Attempt to check connectivity, may be prompted to trust.
 	err := p4test(ctx, host, username, password)
 	if err == nil {
-		return nil // The test worked, session still validate for the user
+		return nil // The test worked, session still valid for the user
 	}

 	if strings.Contains(err.Error(), "To allow connection use the 'p4 trust' command.") {
--- a/cmd/repo-updater/Dockerfile
+++ b/cmd/repo-updater/Dockerfile
@ -3,6 +3,16 @@
 # file, please don't be scared to make it more pleasant / remove hadolint
 # ignores.

+# # Install p4 CLI (keep this up to date with cmd/gitserver/Dockerfile and cmd/server/Dockerfile)
+FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS p4cli
+
+# hash provided in http://filehost.perforce.com/perforce/r22.2/bin.linux26x86_64/SHA256SUMS
+# if the hash is not provided, calculate it by downloading the file and running `sha256sum` on it in Terminal
+RUN echo "8bc10fca1c5a26262b4072deec76150a668581a9749d0504cd443084773d4fd0  /usr/local/bin/p4" >expected_hash && \
+    wget http://cdist2.perforce.com/perforce/r22.2/bin.linux26x86_64/p4 -O /usr/local/bin/p4 && \
+    chmod +x /usr/local/bin/p4 && \
+    sha256sum -c expected_hash
+
 FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS coursier

 RUN wget -O coursier.gz https://github.com/coursier/coursier/releases/download/v2.1.0-RC4/cs-x86_64-pc-linux-static.gz && \
@ -21,8 +31,15 @@ LABEL org.opencontainers.image.created=${DATE}
 LABEL org.opencontainers.image.version=${VERSION}
 LABEL com.sourcegraph.github.url=https://github.com/sourcegraph/sourcegraph/commit/${COMMIT_SHA}

+COPY --from=p4cli /usr/local/bin/p4 /usr/local/bin/p4
+
 COPY --from=coursier /usr/local/bin/coursier /usr/local/bin/coursier

+# This is a trick to include libraries required by p4,
+# please refer to https://blog.tilander.org/docker-perforce/
+# hadolint ignore=DL4006
+RUN wget -O - https://github.com/jtilander/p4d/raw/4600d741720f85d77852dcca7c182e96ad613358/lib/lib-x64.tgz | tar zx --directory /
+
 USER sourcegraph
 ENTRYPOINT ["/sbin/tini", "--", "/usr/local/bin/repo-updater"]
 COPY repo-updater /usr/local/bin/
--- a/cmd/server/Dockerfile
+++ b/cmd/server/Dockerfile
@ -1,10 +1,12 @@
 # Install p4 CLI (keep this up to date with cmd/gitserver/Dockerfile)
 FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS p4cli

-# hadolint ignore=DL3003
-RUN wget http://cdist2.perforce.com/perforce/r21.2/bin.linux26x86_64/p4 && \
-    mv p4 /usr/local/bin/p4 && \
-    chmod +x /usr/local/bin/p4
+# hash provided in http://filehost.perforce.com/perforce/r22.2/bin.linux26x86_64/SHA256SUMS
+# if the hash is not provided, calculate it by downloading the file and running `sha256sum` on it in Terminal
+RUN echo "8bc10fca1c5a26262b4072deec76150a668581a9749d0504cd443084773d4fd0  /usr/local/bin/p4" >expected_hash && \
+    wget http://cdist2.perforce.com/perforce/r22.2/bin.linux26x86_64/p4 -O /usr/local/bin/p4 && \
+    chmod +x /usr/local/bin/p4 && \
+    sha256sum -c expected_hash

 # Install p4-fusion (keep this up to date with cmd/gitserver/Dockerfile)
 FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS p4-fusion
--- a/enterprise/cmd/gitserver/Dockerfile
+++ b/enterprise/cmd/gitserver/Dockerfile
@ -6,10 +6,12 @@
 # Install p4 CLI (keep this up to date with cmd/server/Dockerfile)
 FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS p4cli

-# hadolint ignore=DL3003
-RUN wget http://cdist2.perforce.com/perforce/r21.2/bin.linux26x86_64/p4 && \
-    mv p4 /usr/local/bin/p4 && \
-    chmod +x /usr/local/bin/p4
+# hash provided in http://filehost.perforce.com/perforce/r22.2/bin.linux26x86_64/SHA256SUMS
+# if the hash is not provided, calculate it by downloading the file and running `sha256sum` on it in Terminal
+RUN echo "8bc10fca1c5a26262b4072deec76150a668581a9749d0504cd443084773d4fd0  /usr/local/bin/p4" >expected_hash && \
+    wget http://cdist2.perforce.com/perforce/r22.2/bin.linux26x86_64/p4 -O /usr/local/bin/p4 && \
+    chmod +x /usr/local/bin/p4 && \
+    sha256sum -c expected_hash

 FROM sourcegraph/alpine-3.14:201280_2023-02-23_4.5-1071f8b97a60@sha256:c4970b21169db155c1b497740e622adb23007ac11a87ec571d9ecef8aba0adc5 AS p4-fusion

--- a/internal/repos/perforce.go
+++ b/internal/repos/perforce.go
@ -5,12 +5,14 @@ import (
 	"net/url"
 	"strings"

+	"github.com/sourcegraph/sourcegraph/cmd/gitserver/server"
 	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/conf/reposource"
 	"github.com/sourcegraph/sourcegraph/internal/extsvc"
 	"github.com/sourcegraph/sourcegraph/internal/extsvc/perforce"
 	"github.com/sourcegraph/sourcegraph/internal/jsonc"
 	"github.com/sourcegraph/sourcegraph/internal/types"
+	"github.com/sourcegraph/sourcegraph/internal/vcs"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"github.com/sourcegraph/sourcegraph/schema"
 )
@ -60,7 +62,23 @@ func (s PerforceSource) ListRepos(ctx context.Context, results chan SourceResult
 			return
 		}

-		results <- SourceResult{Source: s, Repo: s.makeRepo(depot)}
+		u := url.URL{
+			Scheme: "perforce",
+			Host:   s.config.P4Port,
+			Path:   depot,
+			User:   url.UserPassword(s.config.P4User, s.config.P4Passwd),
+		}
+		p4Url, err := vcs.ParseURL(u.String())
+		if err != nil {
+			results <- SourceResult{Source: s, Err: err}
+			continue
+		}
+		syncer := server.PerforceDepotSyncer{}
+		if err := syncer.IsCloneable(ctx, p4Url); err == nil {
+			results <- SourceResult{Source: s, Repo: s.makeRepo(depot)}
+		} else {
+			results <- SourceResult{Source: s, Err: err}
+		}
 	}
 }

--- a/internal/repos/perforce_test.go
+++ b/internal/repos/perforce_test.go
@ -3,6 +3,8 @@ package repos
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
 	"sort"
 	"testing"

@ -12,10 +14,62 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/testutil"
 	"github.com/sourcegraph/sourcegraph/internal/types"
 	"github.com/sourcegraph/sourcegraph/internal/types/typestest"
+	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"github.com/sourcegraph/sourcegraph/schema"
 )

+func setupMockP4Executable() (string, error) {
+	// an inline shell script is pretty ugly,
+	// but it has the advantage of keeping all the pieces in one place
+	mockP4Script := `#!/usr/bin/env bash
+
+### mock for p4 connection tests
+
+### debug output in case more arguments are added so it's easy to see what is expected
+DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+echo "$@" >>${DIR}/p4.log
+
+### handle "login -s" by outputting something that conforms to the expected output and exiting with success
+[[ "${1}" = "login" ]] && [[ "${2}" = "-s" ]] && {
+	echo "User ${P4USER:-admin} ticket expires in 130279 hours 20 minutes."
+	exit 0
+}
+
+### handle "p4 depots" by returns some hard-coded depots. If the tests change, these need to change also
+[[ "${3}" == "depots" ]] && {
+	sg_depot='{"desc":"Created by admin.\n","map":"Sourcegraph/...","name":"Sourcegraph","time":"1628879609","type":"local"}'
+	eng_depot='{"desc":"Created by admin.\n","map":"Engineering/...","name":"Engineering","time":"1628542108","type":"local"}'
+	case "${5}" in
+	Engineering) echo "${eng_depot}" ;;
+	Sourcegraph) echo "${sg_depot}" ;;
+	*)
+		echo echo "${eng_depot}"
+		echo "${sg_depot}"
+		;;
+	esac
+	exit 0
+}
+	`
+	tempdir, err := os.MkdirTemp("", "")
+	if err != nil {
+		return "", errors.Wrap(err, "setupMockP4Executable")
+	}
+	if err := os.WriteFile(filepath.Join(tempdir, "p4"), []byte(mockP4Script), 0755); err != nil {
+		return "", errors.Wrap(err, "setupMockP4Executable")
+	}
+	os.Setenv("PATH", fmt.Sprintf("%s%c%s", tempdir, os.PathListSeparator, os.Getenv("PATH")))
+	// return the temp directory so that it can be cleaned up later
+	// becasue `defer` doesn't cross function call boundaries
+	return tempdir, nil
+}
+
 func TestPerforceSource_ListRepos(t *testing.T) {
+	// set up a mock p4 executable before running the tests
+	tempdir, err := setupMockP4Executable()
+	if err != nil {
+		t.Errorf("error while setting up: %s", err)
+	}
+	defer os.RemoveAll(tempdir)
 	assertAllReposListed := func(want []string) typestest.ReposAssertion {
 		return func(t testing.TB, rs types.Repos) {
 			t.Helper()
--- a/wolfi-packages/p4cli.yaml
+++ b/wolfi-packages/p4cli.yaml
@ -1,6 +1,6 @@
 package:
  name: p4cli
-  version: 21.2
+  version: 22.2
  epoch: 0
  description: "Command line interface for Perforce"
  target-architecture:
@ -21,7 +21,7 @@ pipeline:
  - uses: fetch
    with:
      uri: http://cdist2.perforce.com/perforce/r${{package.version}}/bin.linux26x86_64/p4
-      expected-sha256: 3462491b61ba9cf0c5d70511b7a3280cd7c154b49bfd8c669d5761953968c159
+      expected-sha256: 8bc10fca1c5a26262b4072deec76150a668581a9749d0504cd443084773d4fd0
      extract: false
  - runs: |
      chmod +x p4