internal/relui: use a per-workflow directory for signing

We learned the hard way that using a well-known directory creates
confusion in the release process. Instead, create a "signing" directory
inside the per-workflow scratch dir and use that to communicate with the
internal signing tool.

In addition, create a sentinel file "ready" that can be used to signal
the internal tool to start working.

For golang/go#51797.

Change-Id: Id8236b932e22c445cc03ecec3e975d85583c914b
Reviewed-on: https://go-review.googlesource.com/c/build/+/411902
Run-TryBot: Heschi Kreinick <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Author: heschi

cmd/relui/deployment-prod.yaml

@@ -37,7 +37,6 @@ spec:
             - "--builder-master-key=secret:symbolic-datum-552/builder-master-key"
             - "--github-token=secret:symbolic-datum-552/maintner-github-token"
             - "--scratch-files-base=gs://golang-release-staging/relui-scratch"
-            - "--staging-files-base=gs://golang-release-staging"
             - "--serving-files-base=gs://golang"
             - "--edge-cache-url=https://dl.google.com/go"
             - "--website-upload-url=https://go.dev/dl/upload"

cmd/relui/main.go

@@ -46,7 +46,6 @@ var (
 	pgConnect   = flag.String("pg-connect", "", "Postgres connection string or URI. If empty, libpq connection defaults are used.")
 
 	scratchFilesBase = flag.String("scratch-files-base", "", "Storage for scratch files. gs://bucket/path or file:///path/to/scratch.")
-	stagingFilesBase = flag.String("staging-files-base", "", "Storage for staging files. gs://bucket/path or file:///path/to/staging.")
 	servingFilesBase = flag.String("serving-files-base", "", "Storage for serving files. gs://bucket/path or file:///path/to/serving.")
 	edgeCacheURL     = flag.String("edge-cache-url", "", "URL release files appear at when published to the CDN, e.g. https://dl.google.com/go.")
 	websiteUploadURL = flag.String("website-upload-url", "", "URL to POST website file data to, e.g. https://go.dev/dl/upload.")
@@ -130,7 +129,6 @@ func main() {
 		CreateBuildlet: coordinator.CreateBuildlet,
 		GCSClient:      gcsClient,
 		ScratchURL:     *scratchFilesBase,
-		StagingURL:     *stagingFilesBase,
 		ServingURL:     *servingFilesBase,
 		DownloadURL:    *edgeCacheURL,
 		PublishFile: func(f *relui.WebsiteFile) error {

internal/relui/buildrelease_test.go

@@ -20,6 +20,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strings"
 	"sync"
@@ -65,9 +66,23 @@ func testRelease(t *testing.T, wantVersion string, kind task.ReleaseKind) {
 	}
 
 	// Set up the fake signing process.
-	stagingDir := t.TempDir()
-	go fakeSign(ctx, t, filepath.Join(stagingDir, wantVersion))
+	scratchDir := t.TempDir()
 	signingPollDuration = 100 * time.Millisecond
+	argRe := regexp.MustCompile(`--relui_staging="(.*?)"`)
+	outputListener := func(taskName string, output interface{}) {
+		if taskName != "Start signing command" {
+			return
+		}
+		matches := argRe.FindStringSubmatch(output.(string))
+		if matches == nil {
+			return
+		}
+		u, err := url.Parse(matches[1])
+		if err != nil {
+			t.Fatal(err)
+		}
+		go fakeSign(ctx, t, u.Path)
+	}
 
 	// Set up the fake CDN publishing process.
 	servingDir := t.TempDir()
@@ -100,8 +115,7 @@ func testRelease(t *testing.T, wantVersion string, kind task.ReleaseKind) {
 	buildTasks := &BuildReleaseTasks{
 		GerritURL:      tarballServer.URL,
 		GCSClient:      nil,
-		ScratchURL:     "file://" + filepath.ToSlash(t.TempDir()),
-		StagingURL:     "file://" + filepath.ToSlash(stagingDir),
+		ScratchURL:     "file://" + filepath.ToSlash(scratchDir),
 		ServingURL:     "file://" + filepath.ToSlash(servingDir),
 		CreateBuildlet: fakeBuildlets.createBuildlet,
 		DownloadURL:    dlServer.URL,
@@ -122,7 +136,7 @@ func testRelease(t *testing.T, wantVersion string, kind task.ReleaseKind) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	_, err = w.Run(ctx, &verboseListener{t})
+	_, err = w.Run(ctx, &verboseListener{t, outputListener})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -596,7 +610,10 @@ func (b *fakeBuildlet) WorkDir(ctx context.Context) (string, error) {
 	return b.dir, nil
 }
 
-type verboseListener struct{ t *testing.T }
+type verboseListener struct {
+	t              *testing.T
+	outputListener func(string, interface{})
+}
 
 func (l *verboseListener) TaskStateChanged(_ uuid.UUID, _ string, st *workflow.TaskState) error {
 	switch {
@@ -606,6 +623,9 @@ func (l *verboseListener) TaskStateChanged(_ uuid.UUID, _ string, st *workflow.T
 		l.t.Logf("task %-10v: error: %v", st.Name, st.Error)
 	default:
 		l.t.Logf("task %-10v: done: %v", st.Name, st.Result)
+		if l.outputListener != nil {
+			l.outputListener(st.Name, st.Result)
+		}
 	}
 	return nil
 }
@@ -632,13 +652,17 @@ func fakeSign(ctx context.Context, t *testing.T, dir string) {
 }
 
 func fakeSignOnce(t *testing.T, dir string, seen map[string]bool) {
-	contents, err := os.ReadDir(dir)
+	_, err := os.Stat(filepath.Join(dir, "ready"))
 	if os.IsNotExist(err) {
 		return
 	}
 	if err != nil {
 		t.Fatal(err)
 	}
+	contents, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
 	for _, fi := range contents {
 		fn := fi.Name()
 		if fn == "signed" || seen[fn] {
@@ -759,6 +783,9 @@ func TestFakeSign(t *testing.T) {
 			t.Fatal(err)
 		}
 	}
+	if err := ioutil.WriteFile(filepath.Join(dir, "ready"), nil, 0777); err != nil {
+		t.Fatal(err)
+	}
 	fakeSignOnce(t, dir, map[string]bool{})
 	want := map[string]bool{}
 	for _, f := range strings.Split(strings.TrimSpace(outputs), "\n") {

internal/relui/workflows.go

@@ -307,6 +307,9 @@ func addSingleReleaseWorkflow(build *BuildReleaseTasks, milestone *task.Mileston
 	dlclCommit := wd.Task("Wait for DL CL", version.AwaitCL, dlcl, wd.Constant(""))
 	wd.Output("Download CL submitted", dlclCommit)
 
+	startSigner := wd.Task("Start signing command", build.startSigningCommand, nextVersion)
+	wd.Output("Signing command", startSigner)
+
 	// Build, test, and sign release.
 	signedAndTestedArtifacts, err := build.addBuildTasks(wd, "go1.19", nextVersion, releaseBase, skipTests, checked)
 	if err != nil {
@@ -382,13 +385,13 @@ func (tasks *BuildReleaseTasks) addBuildTasks(wd *workflow.Definition, majorVers
 
 // BuildReleaseTasks serves as an adapter to the various build tasks in the task package.
 type BuildReleaseTasks struct {
-	GerritURL                          string
-	GCSClient                          *storage.Client
-	ScratchURL, StagingURL, ServingURL string
-	DownloadURL                        string
-	PublishFile                        func(*WebsiteFile) error
-	CreateBuildlet                     func(string) (buildlet.Client, error)
-	ApproveActionFunc                  func(taskName string) func(*workflow.TaskContext, interface{}) error
+	GerritURL              string
+	GCSClient              *storage.Client
+	ScratchURL, ServingURL string
+	DownloadURL            string
+	PublishFile            func(*WebsiteFile) error
+	CreateBuildlet         func(string) (buildlet.Client, error)
+	ApproveActionFunc      func(taskName string) func(*workflow.TaskContext, interface{}) error
 }
 
 func (b *BuildReleaseTasks) buildSource(ctx *workflow.TaskContext, revision, version string) (artifact, error) {
@@ -530,15 +533,22 @@ func (b *BuildReleaseTasks) runBuildStep(
 	}, nil
 }
 
+// An artifact represents a file as it moves through the release process. Most
+// files will appear on go.dev/dl eventually.
 type artifact struct {
 	// The target platform of this artifact, or nil for source.
 	Target *releasetargets.Target
-	// The scratch path of this artifact.
+	// The scratch path of this artifact within the scratch directory.
+	// <workflow-id>/<filename>-<random-number>
 	ScratchPath string
-	// The path the artifact was staged to for the signing process.
+	// The path within the scratch directory the artifact was staged to for the
+	// signing process.
+	// <workflow-id>/signing/<go version>/<filename>
 	StagingPath string
-	// The path artifact can be found at after the signing process. It may be
-	// the same as the staging path for artifacts that are externally signed.
+	// The path within the scratch directory the artifact can be found at
+	// after the signing process. For files not modified by the signing
+	// process, the staging path, or for those that are
+	// <workflow-id>/signing/<go version>/signed/<filename>
 	SignedPath string
 	// The contents of the GPG signature for this artifact (.asc file).
 	GPGSignature string
@@ -560,15 +570,17 @@ func (w *sizeWriter) Write(p []byte) (n int, err error) {
 	return len(p), nil
 }
 
+func (tasks *BuildReleaseTasks) startSigningCommand(ctx *workflow.TaskContext, version string) (string, error) {
+	args := fmt.Sprintf("--relui_staging=%q", path.Join(tasks.ScratchURL, signingStagingDir(ctx, version)))
+	ctx.Printf("run signer with " + args)
+	return args, nil
+}
+
 func (tasks *BuildReleaseTasks) copyToStaging(ctx *workflow.TaskContext, version string, artifacts []artifact) ([]artifact, error) {
 	scratchFS, err := gcsfs.FromURL(ctx, tasks.GCSClient, tasks.ScratchURL)
 	if err != nil {
 		return nil, err
 	}
-	stagingFS, err := gcsfs.FromURL(ctx, tasks.GCSClient, tasks.StagingURL)
-	if err != nil {
-		return nil, err
-	}
 	var stagedArtifacts []artifact
 	for _, a := range artifacts {
 		staged := a
@@ -577,14 +589,14 @@ func (tasks *BuildReleaseTasks) copyToStaging(ctx *workflow.TaskContext, version
 		} else {
 			staged.Filename = version + "." + a.Suffix
 		}
-		staged.StagingPath = path.Join(version, staged.Filename)
+		staged.StagingPath = path.Join(signingStagingDir(ctx, version), staged.Filename)
 		stagedArtifacts = append(stagedArtifacts, staged)
 
 		in, err := scratchFS.Open(a.ScratchPath)
 		if err != nil {
 			return nil, err
 		}
-		out, err := gcsfs.Create(stagingFS, staged.StagingPath)
+		out, err := gcsfs.Create(scratchFS, staged.StagingPath)
 		if err != nil {
 			return nil, err
 		}
@@ -598,9 +610,20 @@ func (tasks *BuildReleaseTasks) copyToStaging(ctx *workflow.TaskContext, version
 			return nil, err
 		}
 	}
+	out, err := gcsfs.Create(scratchFS, path.Join(signingStagingDir(ctx, version), "ready"))
+	if err != nil {
+		return nil, err
+	}
+	if err := out.Close(); err != nil {
+		return nil, err
+	}
 	return stagedArtifacts, nil
 }
 
+func signingStagingDir(ctx *workflow.TaskContext, version string) string {
+	return path.Join(ctx.WorkflowID.String(), "signing", version)
+}
+
 var signingPollDuration = 30 * time.Second
 
 // awaitSigned waits for all of artifacts to be signed, plus the pkgs for
@@ -617,7 +640,7 @@ func (tasks *BuildReleaseTasks) awaitSigned(ctx *workflow.TaskContext, version s
 		})
 	}
 
-	stagingFS, err := gcsfs.FromURL(ctx, tasks.GCSClient, tasks.StagingURL)
+	scratchFS, err := gcsfs.FromURL(ctx, tasks.GCSClient, tasks.ScratchURL)
 	if err != nil {
 		return nil, err
 	}
@@ -629,7 +652,7 @@ func (tasks *BuildReleaseTasks) awaitSigned(ctx *workflow.TaskContext, version s
 	var signedArtifacts []artifact
 	for {
 		for a := range todo {
-			signed, ok, err := readSignedArtifact(stagingFS, version, a)
+			signed, ok, err := readSignedArtifact(ctx, scratchFS, version, a)
 			if err != nil {
 				return nil, err
 			}
@@ -653,7 +676,7 @@ func (tasks *BuildReleaseTasks) awaitSigned(ctx *workflow.TaskContext, version s
 	}
 }
 
-func readSignedArtifact(stagingFS fs.FS, version string, a artifact) (_ artifact, ok bool, _ error) {
+func readSignedArtifact(ctx *workflow.TaskContext, scratchFS fs.FS, version string, a artifact) (_ artifact, ok bool, _ error) {
 	// Our signing process has somewhat uneven behavior. In general, for things
 	// that contain their own signature, such as MSIs and .pkgs, we don't
 	// produce a GPG signature, just the new file. On macOS, tars can be signed
@@ -684,18 +707,19 @@ func readSignedArtifact(stagingFS fs.FS, version string, a artifact) (_ artifact
 		Filename: a.Filename,
 		Suffix:   a.Suffix,
 	}
+	stagingDir := signingStagingDir(ctx, version)
 	if modifiedBySigning {
-		signed.SignedPath = version + "/signed/" + a.Filename
+		signed.SignedPath = stagingDir + "/signed/" + a.Filename
 	} else {
-		signed.SignedPath = version + "/" + a.Filename
+		signed.SignedPath = stagingDir + "/" + a.Filename
 	}
 
-	fi, err := fs.Stat(stagingFS, signed.SignedPath)
+	fi, err := fs.Stat(scratchFS, signed.SignedPath)
 	if err != nil {
 		return artifact{}, false, nil
 	}
 	if modifiedBySigning {
-		hash, err := fs.ReadFile(stagingFS, version+"/signed/"+a.Filename+".sha256")
+		hash, err := fs.ReadFile(scratchFS, stagingDir+"/signed/"+a.Filename+".sha256")
 		if err != nil {
 			return artifact{}, false, nil
 		}
@@ -706,7 +730,7 @@ func readSignedArtifact(stagingFS fs.FS, version string, a artifact) (_ artifact
 		signed.Size = a.Size
 	}
 	if hasGPG {
-		sig, err := fs.ReadFile(stagingFS, version+"/signed/"+a.Filename+".asc")
+		sig, err := fs.ReadFile(scratchFS, stagingDir+"/signed/"+a.Filename+".asc")
 		if err != nil {
 			return artifact{}, false, nil
 		}
@@ -718,7 +742,7 @@ func readSignedArtifact(stagingFS fs.FS, version string, a artifact) (_ artifact
 var uploadPollDuration = 30 * time.Second
 
 func (tasks *BuildReleaseTasks) uploadArtifacts(ctx *workflow.TaskContext, artifacts []artifact) error {
-	stagingFS, err := gcsfs.FromURL(ctx, tasks.GCSClient, tasks.StagingURL)
+	scratchFS, err := gcsfs.FromURL(ctx, tasks.GCSClient, tasks.ScratchURL)
 	if err != nil {
 		return err
 	}
@@ -729,7 +753,7 @@ func (tasks *BuildReleaseTasks) uploadArtifacts(ctx *workflow.TaskContext, artif
 
 	todo := map[artifact]bool{}
 	for _, a := range artifacts {
-		if err := uploadArtifact(stagingFS, servingFS, a); err != nil {
+		if err := uploadArtifact(scratchFS, servingFS, a); err != nil {
 			return err
 		}
 		todo[a] = true
@@ -762,8 +786,8 @@ func (tasks *BuildReleaseTasks) uploadArtifacts(ctx *workflow.TaskContext, artif
 	}
 }
 
-func uploadArtifact(stagingFS, servingFS fs.FS, a artifact) error {
-	in, err := stagingFS.Open(a.SignedPath)
+func uploadArtifact(scratchFS, servingFS fs.FS, a artifact) error {
+	in, err := scratchFS.Open(a.SignedPath)
 	if err != nil {
 		return err
 	}

internal/relui/workflows_test.go

@@ -150,7 +150,7 @@ func runWorkflow(t *testing.T, ctx context.Context, w *workflow.Workflow, listen
 	defer cancel()
 	t.Helper()
 	if listener == nil {
-		listener = &verboseListener{t}
+		listener = &verboseListener{t, nil}
 	}
 	return w.Run(ctx, listener)
 }