internal/relui: fold SecurityReleaseCoalesceTask into main workflow The SecurityReleaseCoalesceTask becomes a new prerequisite step to the minor release workflow; it keeps all of its prior semantics and execution flow with minor changes to support idempotent workflow runs in the larger minor release context. For golang/go#79034 For golang/go#78458 Change-Id: I76948fa0c8e4d46a0053e14030bd6b36a66e68af Reviewed-on: https://go-review.googlesource.com/c/build/+/801102 Reviewed-by: Nicholas Husin <nsh@golang.org> Reviewed-by: Nicholas Husin <husin@google.com> Reviewed-by: Neal Patel <nealpatel@google.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/cmd/relui/main.go b/cmd/relui/main.go index e5c6b29..264141e 100644 --- a/cmd/relui/main.go +++ b/cmd/relui/main.go
@@ -309,6 +309,7 @@ V3: github.NewClient(githubHTTPClient), V4: githubv4.NewClient(githubHTTPClient), } + buildTasks.GitHub = githubClient milestoneTasks := &task.MilestoneTasks{ Client: githubClient, RepoOwner: "golang", @@ -416,18 +417,14 @@ PublicRepoURL: func(repo string) string { return "https://go.googlesource.com/" + repo }, + GitHub: githubClient, ApproveAction: relui.ApproveActionDep(dbPool), SendMail: mailFunc, AnnounceMailHeader: annMail, + AwaitAnnounceMail: commTasks.AnnounceMailTasks.AwaitAnnounceMail, } dh.RegisterDefinition("Publish a private patch to a x/ repo", privateXPatchTask.NewDefinition(tagTasks)) - securityReleaseCoalesceTask := &task.SecurityReleaseCoalesceTask{ - PrivateGerrit: privateGerritClient, - Version: versionTasks, - } - dh.RegisterDefinition("Prepare internal security release branches", securityReleaseCoalesceTask.NewDefinition()) - var base *url.URL if *baseURL != "" { base, err = url.Parse(*baseURL)
diff --git a/internal/relui/buildrelease_test.go b/internal/relui/buildrelease_test.go index b93c1bd..f5f27e9 100644 --- a/internal/relui/buildrelease_test.go +++ b/internal/relui/buildrelease_test.go
@@ -20,6 +20,7 @@ "net/http/httptest" "os" "os/exec" + "path" "path/filepath" "runtime" "strings" @@ -37,6 +38,9 @@ "golang.org/x/build/internal/releasetargets" "golang.org/x/build/internal/task" "golang.org/x/build/internal/workflow" + "golang.org/x/build/relmeta" + "golang.org/x/vulndb/report" + yaml "gopkg.in/yaml.v3" ) func TestRelease(t *testing.T) { @@ -149,7 +153,7 @@ return nil } - var goDirectives = make(map[string]string) + goDirectives := make(map[string]string) goRepo := task.NewFakeRepo(t, "go") base := goRepo.Commit(goFiles) goRepo.Tag(previousTag, base) @@ -217,6 +221,7 @@ BuildBucketClient: buildBucket, CloudBuildClient: task.NewFakeCloudBuild(t, fakeGerrit, dockerProject, map[string]map[string]string{dockerTrigger: {"_GO_VERSION": wantVersion[2:]}}), SwarmingClient: task.NewFakeSwarmingClient(t, fakeGo), + GitHub: &task.FakeGitHub{}, ApproveAction: func(ctx *workflow.TaskContext) error { switch ctx.TaskName { case "Confirm PRIVATE-track security CLs", @@ -451,6 +456,9 @@ // on top of the fake public repo content. The workflow will upstream these // commits to the fake public repo. privateRepo := task.CloneFakeRepo(t, "go-private", deps.goRepo) + // The private Gerrit mirrors the public release branch at the public release + // branch head (the clone's current HEAD, which equals the base commit). + privateRepo.Branch("release-branch.go1.26", privateRepo.History()[0]) securityFix1 := map[string]string{"security.txt": "This file makes us secure"} securityFix2 := map[string]string{"security2.txt": "This file makes us more secure"} securityFix3 := map[string]string{"security3.txt": "This file makes us even more secure"} @@ -534,6 +542,498 @@ }) } +// newMinorCoalesceTestDeps sets up release test dependencies for exercising +// createMinorReleaseWorkflow with PRIVATE-track security patches present. +// +// It extends a base set of single-major deps with a second public release +// branch (so both minors can be released), a second GitHub milestone, and a +// fully-wired private coalesce Gerrit backed by a private clone of the public +// "go" repo and a security-metadata repo holding the milestone YAML. +// +// withPrivatePatches controls whether the milestone has any PRIVATE patches. +func newMinorCoalesceTestDeps(t *testing.T, withPrivatePatches bool) (*releaseTestDeps, *task.FakeGerrit) { + // currentMajor=26, prevMajor=25. newReleaseTestDeps sets up the 26 series; + // add the 25 series so GetNextMinorVersions([26,25]) returns the two minors. + deps := newReleaseTestDeps(t, "go1.26.0", 26, "go1.26.1") + + base, err := deps.gerrit.ReadBranchHead(deps.ctx, "go", "release-branch.go1.26") + if err != nil { + t.Fatal(err) + } + deps.goRepo.Branch("release-branch.go1.25", base) + deps.goRepo.Tag("go1.25.0", base) + + // FetchMilestones for go1.25.1 needs a "Go1.25.1" milestone to already exist. + fakeGitHub, ok := deps.milestoneTasks.Client.(*task.FakeGitHub) + if !ok { + t.Fatalf("milestone client is %T, want *task.FakeGitHub", deps.milestoneTasks.Client) + } + fakeGitHub.Milestones[2] = "Go1.25.1" + + // Private side: clone the public repo and create the branches the coalesce + // steps read: "public" and the major release branches. + privGoRepo := task.CloneFakeRepo(t, "go", deps.goRepo) + privGoRepo.Branch("public", base) + privGoRepo.Branch("release-branch.go1.26", base) + privGoRepo.Branch("release-branch.go1.25", base) + + // security-metadata holds the milestone that lists the security patches. + smRepo := task.NewFakeRepo(t, "security-metadata") + smHead := smRepo.History()[0] + smRepo.Branch("main", smHead) + var milestoneYAML string + if withPrivatePatches { + milestoneYAML = `id: 99915010 +security_patches: + - id: 40027190 + package: crypto/tls + track: PRIVATE + changelists: + - https://go-internal-review.git.corp.google.com/c/go/+/1234 + - https://go-internal-review.git.corp.google.com/c/go/+/5678 + target_releases: + - go1.26.1 + - go1.25.1` + } else { + // A milestone with only PUBLIC patches: the coalesce must short-circuit. + milestoneYAML = `id: 99915010 +security_patches: + - id: 20024001 + package: runtime + track: PUBLIC + changelists: + - https://go.dev/cl/123456 + target_releases: + - go1.26.1 + - go1.25.1` + } + smRepo.CommitOnBranch("main", map[string]string{ + filepath.Join("data", "milestones", "99915010.yaml"): milestoneYAML, + }) + + privGerrit := task.NewFakeGerrit(t, privGoRepo, smRepo) + if withPrivatePatches { + privGerrit.AddChange("go", "1234", &gerrit.ChangeInfo{ + ID: "1234", + ChangeID: "1234", + ChangeNumber: 1234, + Branch: "public", + Submittable: true, + Mergeable: true, + }, "crypto/tls: fix something\n\nFixes CVE-1985-0703\nFixes golang/go#1") + privGerrit.AddChange("go", "5678", &gerrit.ChangeInfo{ + ID: "5678", + ChangeID: "5678", + ChangeNumber: 5678, + Branch: "public", + Submittable: true, + Mergeable: true, + }, "cmd/compile: fix something else\n\nFixes CVE-1970-0001\nFixes #2") + } + + deps.buildTasks.PrivateGerritClient = privGerrit + deps.buildTasks.PrivateGerritProject = "go" + + return deps, privGerrit +} + +func TestMinorReleaseSecurityCoalesce(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + + // Approve the confirm step; fail any other approval request. + deps.buildTasks.ApproveAction = func(ctx *workflow.TaskContext) error { + if strings.Contains(ctx.TaskName, "Confirm PRIVATE-track security CLs") { + return nil + } + return fmt.Errorf("unexpected approval request for %q", ctx.TaskName) + } + + // Stop the workflow once both minors' confirm tasks have finished, so we + // don't have to drive the full build. Canceling on finish (not on approval) + // keeps the test robust: if the bug makes a confirm task error instead of + // reaching the approval, the test still unblocks rather than stalling. + runCtx, stop := context.WithCancel(deps.ctx) + t.Cleanup(stop) + listener := &verboseListener{t: t, onStall: stop} + + comm := task.CommunicationTasks{ + SecurityCommunicationTasks: task.SecurityCommunicationTasks{PrivateGerrit: privGerrit}, + } + + publicHeadBefore, err := privGerrit.ReadBranchHead(deps.ctx, "go", "public") + if err != nil { + t.Fatalf("reading public head before workflow: %v", err) + } + + wd, err := createMinorReleaseWorkflow(deps.buildTasks, deps.milestoneTasks, deps.versionTasks, comm, 25, 26) + if err != nil { + t.Fatal(err) + } + w, err := workflow.Start(wd, minorReleaseParams()) + if err != nil { + t.Fatal(err) + } + + if _, err := w.Run(runCtx, listener); err != nil && runCtx.Err() == nil { + t.Fatalf("workflow failed before confirming security CLs: %v", err) + } + + branches, err := privGerrit.ListBranches(deps.ctx, "go") + if err != nil { + t.Fatalf("listing branches: %v", err) + } + branchNames := make(map[string]bool) + for _, b := range branches { + name := strings.TrimPrefix(b.Ref, "refs/heads/") + branchNames[name] = true + } + for _, want := range []string{ + "internal-release-branch.go1.26.1", + "internal-release-branch.go1.25.1", + } { + if !branchNames[want] { + t.Errorf("internal release branch %q not found; branches: %v", want, branchNames) + } + } + + for _, ib := range []string{ + "internal-release-branch.go1.26.1", + "internal-release-branch.go1.25.1", + } { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", ib) + if err != nil { + t.Fatalf("reading head of %s: %v", ib, err) + } + if head == publicHeadBefore { + t.Errorf("internal branch %s head (%s) equals original public head; cherry-picks did not land", ib, head) + } + } + + var foundCheckpoint bool + for name := range branchNames { + if strings.HasPrefix(name, "go1.26.1-go1.25.1-checkpoint-") { + foundCheckpoint = true + break + } + } + if !foundCheckpoint { + t.Errorf("checkpoint branch matching go1.26.1-go1.25.1-checkpoint-* not found; branches: %v", branchNames) + } + + for _, clID := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, clID) + if err != nil { + t.Fatalf("GetChange(%s): %v", clID, err) + } + if ci.Status != gerrit.ChangeStatusMerged { + t.Errorf("CL %s status = %q, want %q", clID, ci.Status, gerrit.ChangeStatusMerged) + } + } +} + +func TestMinorReleaseCoalesceNoPrivatePatches(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, false) + + // There are no PRIVATE patches, so each release's confirm task takes the "no + // security fix" path. Allow those approvals; fail any other approval request. + deps.buildTasks.ApproveAction = func(ctx *workflow.TaskContext) error { + if strings.Contains(ctx.TaskName, "Confirm PRIVATE-track security CLs") { + return nil + } + return fmt.Errorf("unexpected approval request for %q", ctx.TaskName) + } + + // Stop the workflow once the coalesce reaches its terminal step, so we can + // check its side effects without driving the full build. By the time "Create + // cherry-picks" finishes, the checkpoint and internal release branches would + // have been created (if the coalesce didn't short-circuit). + runCtx, stop := context.WithCancel(deps.ctx) + t.Cleanup(stop) + listener := &verboseListener{t: t, onStall: stop} + + comm := task.CommunicationTasks{ + SecurityCommunicationTasks: task.SecurityCommunicationTasks{PrivateGerrit: privGerrit}, + } + wd, err := createMinorReleaseWorkflow(deps.buildTasks, deps.milestoneTasks, deps.versionTasks, comm, 25, 26) + if err != nil { + t.Fatal(err) + } + w, err := workflow.Start(wd, minorReleaseParams()) + if err != nil { + t.Fatal(err) + } + + if _, err := w.Run(runCtx, listener); err != nil && runCtx.Err() == nil { + t.Fatalf("workflow failed: %v", err) + } + + // The coalesce must not have created any security branches. + branches, err := privGerrit.ListBranches(deps.ctx, "go") + if err != nil { + t.Fatal(err) + } + for _, b := range branches { + name := strings.TrimPrefix(b.Ref, "refs/heads/") + if strings.Contains(name, "checkpoint") || strings.HasPrefix(name, "internal-") { + t.Errorf("coalesce created branch %q despite there being no PRIVATE-track patches", name) + } + } +} + +func TestMinorReleaseSecurityCoalesceRestart(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "coalesce"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + // First run: establish a prior-iteration checkpoint branch. + first, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("first createSecurityCheckpoint: %v", err) + } + if !strings.HasPrefix(first, bi.CheckpointName+"-") { + t.Errorf("checkpoint name %q is not prefixed with %q", first, bi.CheckpointName+"-") + } + firstHead, err := privGerrit.ReadBranchHead(deps.ctx, "go", first) + if err != nil { + t.Fatalf("reading first checkpoint head: %v", err) + } + + // Second run: a restart forks a new checkpoint. The branch name embeds a + // minute-resolution timestamp, so a same-minute restart collides on the + // branch name (real Gerrit 409). When the minute has rolled over, the + // restart succeeds with a distinct name; either way, the first run's + // checkpoint branch must remain exactly as it was. + second, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + var httpErr *gerrit.HTTPError + if !errors.As(err, &httpErr) || httpErr.Res.StatusCode != http.StatusConflict { + t.Fatalf("second createSecurityCheckpoint: %v", err) + } + t.Logf("same-minute restart collided on the timestamped checkpoint name (expected): %v", err) + } else if second == first { + t.Errorf("restart reused checkpoint name %q; want a distinct timestamped branch", second) + } + + // The first run's checkpoint branch is left untouched. + gotHead, err := privGerrit.ReadBranchHead(deps.ctx, "go", first) + if err != nil { + t.Fatalf("re-reading first checkpoint head: %v", err) + } + if gotHead != firstHead { + t.Errorf("first checkpoint head moved: was %q, now %q", firstHead, gotHead) + } +} + +func TestFetchSecurityMilestone(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + ctx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "milestone"}} + + t.Run("nil client", func(t *testing.T) { + b := *deps.buildTasks + b.PrivateGerritClient = nil + b.PrivateGerritProject = "" + rm, err := b.fetchSecurityMilestone(ctx, "99915010") + if err != nil { + t.Fatalf("fetchSecurityMilestone: %v", err) + } + if rm != nil { + t.Errorf("got %+v, want nil milestone", rm) + } + }) + + t.Run("empty milestone", func(t *testing.T) { + for _, num := range []string{"", "0"} { + rm, err := deps.buildTasks.fetchSecurityMilestone(ctx, num) + if err != nil { + t.Fatalf("fetchSecurityMilestone(%q): %v", num, err) + } + if rm != nil { + t.Errorf("fetchSecurityMilestone(%q): got %+v, want nil milestone", num, rm) + } + } + }) + + t.Run("happy", func(t *testing.T) { + rm, err := deps.buildTasks.fetchSecurityMilestone(ctx, "99915010") + if err != nil { + t.Fatalf("fetchSecurityMilestone: %v", err) + } + if len(rm.Patches) != 1 { + t.Fatalf("got %d patches, want 1", len(rm.Patches)) + } + if got := rm.Patches[0].Track; got != relmeta.Private { + t.Errorf("patch track = %q, want %q", got, relmeta.Private) + } + }) + + t.Run("read branch head error", func(t *testing.T) { + // A private Gerrit with no security-metadata repo makes ReadBranchHead fail. + b := *deps.buildTasks + b.PrivateGerritClient = task.NewFakeGerrit(t, deps.goRepo) + _, err := b.fetchSecurityMilestone(ctx, "99915010") + if err == nil { + t.Fatal("fetchSecurityMilestone with no security-metadata repo: got nil error") + } + }) + + t.Run("read file error", func(t *testing.T) { + // A milestone number with no corresponding YAML file makes ReadFile fail. + _, err := deps.buildTasks.fetchSecurityMilestone(ctx, "00000000") + if err == nil { + t.Fatal("fetchSecurityMilestone for a missing milestone file: got nil error") + } + }) + + t.Run("unmarshal error", func(t *testing.T) { + // Commit a milestone file with invalid YAML to drive the Unmarshal error. + if _, err := privGerrit.CreateAutoSubmitChange(ctx, gerrit.ChangeInput{ + Project: "security-metadata", + Branch: "main", + }, nil, map[string]string{ + filepath.Join("data", "milestones", "12345678.yaml"): "\tnot: [valid yaml", + }); err != nil { + t.Fatal(err) + } + _, err := deps.buildTasks.fetchSecurityMilestone(ctx, "12345678") + if err == nil { + t.Fatal("fetchSecurityMilestone for invalid YAML: got nil error") + } + if !strings.Contains(err.Error(), "YAML unmarshal") { + t.Errorf("error = %v, want a YAML unmarshal error", err) + } + }) +} + +func TestComputeSecurityBranchInfoWithRC(t *testing.T) { + deps, _ := newMinorCoalesceTestDeps(t, true) + ctx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "branchinfo"}} + + // The base deps set up go1.25 and go1.26 release branches but no go1.27. Add a + // go1.27 release branch on the public repo so ReadBranchHead succeeds and the + // RC path fires (currentMajor=26 -> looks for release-branch.go1.27). + base, err := deps.gerrit.ReadBranchHead(deps.ctx, "go", "release-branch.go1.26") + if err != nil { + t.Fatal(err) + } + deps.goRepo.Branch("release-branch.go1.27", base) + + bi, err := computeSecurityBranchInfo(ctx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + nextRC, err := deps.versionTasks.GetNextVersion(deps.ctx, 27, task.KindRC) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(bi.CheckpointName, nextRC+"-") { + t.Errorf("checkpoint name = %q, want it prefixed with %q", bi.CheckpointName, nextRC+"-") + } + wantRCBranch := "release-branch." + nextRC + if len(bi.PublicReleaseBranches) == 0 || bi.PublicReleaseBranches[0] != wantRCBranch { + t.Errorf("public release branches = %v, want %q first", bi.PublicReleaseBranches, wantRCBranch) + } +} + +func TestCheckPrivateChangesLint(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + ctx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "lint"}} + + // Replace the well-formed commit messages with ones missing both a CVE + // reference and a GitHub issue reference. + privGerrit.AddChange("go", "1234", nil, "crypto/tls: fix something\n\nNo references here.") + privGerrit.AddChange("go", "5678", nil, "cmd/compile: fix something else\n\nStill nothing.") + + rm := &relmeta.ReleaseMilestone{ + Patches: []*relmeta.SecurityPatch{{ + Track: relmeta.Private, + Package: "crypto/tls", + Changelists: []string{"https://go-internal-review.git.corp.google.com/c/go/+/1234"}, + }}, + } + _, err := deps.buildTasks.checkPrivateChanges(ctx, rm) + if err == nil { + t.Fatal("checkPrivateChanges with bad commit messages: got nil error") + } + for _, want := range []string{"missing CVE reference", "missing GitHub issue reference"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + + // A well-formed commit message produces no lint errors. + privGerrit.AddChange("go", "1234", nil, "crypto/tls: fix\n\nFixes CVE-1985-0703\nFixes golang/go#1") + if _, err := deps.buildTasks.checkPrivateChanges(ctx, rm); err != nil { + t.Errorf("checkPrivateChanges with a well-formed message: %v", err) + } +} + +func TestMinorReleaseSecurityCoalesceMetadata(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + + comm := task.CommunicationTasks{ + SecurityCommunicationTasks: task.SecurityCommunicationTasks{PrivateGerrit: privGerrit}, + } + + deps.buildTasks.ApproveAction = func(ctx *workflow.TaskContext) error { + if strings.Contains(ctx.TaskName, "Confirm PRIVATE-track security CLs") { + return nil + } + return fmt.Errorf("unexpected approval request for %q", ctx.TaskName) + } + + runCtx, stop := context.WithCancel(deps.ctx) + t.Cleanup(stop) + listener := &verboseListener{t: t, onStall: stop} + + wd, err := createMinorReleaseWorkflow(deps.buildTasks, deps.milestoneTasks, deps.versionTasks, comm, 25, 26) + if err != nil { + t.Fatal(err) + } + w, err := workflow.Start(wd, minorReleaseParams()) + if err != nil { + t.Fatal(err) + } + if _, err := w.Run(runCtx, listener); err != nil && runCtx.Err() == nil { + t.Fatalf("workflow failed before the metadata tasks finished: %v", err) + } +} + +// mustGetNextMinors returns the next minor versions for the 26 and 25 series. +func mustGetNextMinors(t *testing.T, deps *releaseTestDeps) []string { + t.Helper() + next, err := deps.versionTasks.GetNextMinorVersions(deps.ctx, []int{26, 25}) + if err != nil { + t.Fatal(err) + } + return next +} + +// minorReleaseParams returns the parameters needed to start the workflow built +// by createMinorReleaseWorkflow(.., 25, 26). Each minor's sub-workflow +// contributes its own prefixed "Targets to skip testing" parameter. +func minorReleaseParams() map[string]any { + return map[string]any{ + "Release Coordinator Usernames (optional)": []string(nil), + "Release Milestone": "99915010", + "Go 1.26: Targets to skip testing (or 'all') (optional)": []string{"all"}, + "Go 1.25: Targets to skip testing (or 'all') (optional)": []string{"all"}, + } +} + func TestAdvisoryTestsFail(t *testing.T) { deps := newReleaseTestDeps(t, "go1.26.0", 26, "go1.26.1") deps.buildBucket.FailBuilds = append(deps.buildBucket.FailBuilds, "linux-amd64-longtest") @@ -693,7 +1193,9 @@ check(t, body) }) } + func fetch(t *testing.T, url string) []byte { + t.Helper() resp, err := http.Get(url) if err != nil { t.Fatalf("getting %v: %v", url, err) @@ -904,3 +1406,559 @@ t.Fatal(err) } } + +func TestCreateInternalReleaseBranchesIdempotent(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "id8"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + // First run: creates internal release branches. + branches1, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatalf("first createInternalReleaseBranches: %v", err) + } + if len(branches1) == 0 { + t.Fatal("first run created no internal release branches") + } + + // Record the first run's branch heads. + firstHeads := map[string]string{} + for _, b := range branches1 { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("reading head of %s: %v", b, err) + } + firstHeads[b] = head + } + + // Second run (restart): must succeed, not 409. + branches2, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatalf("second createInternalReleaseBranches: %v (expected idempotent success)", err) + } + if len(branches2) != len(branches1) { + t.Fatalf("branch count mismatch: first=%d, second=%d", len(branches1), len(branches2)) + } + + // Verify the recreated branches point at the same public heads. + for _, b := range branches2 { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("reading head of %s after restart: %v", b, err) + } + if head != firstHeads[b] { + t.Errorf("branch %s head after restart = %q, want %q (same public head)", b, head, firstHeads[b]) + } + } +} + +func TestCreateSecurityCherryPicksDedup(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "id9"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + // Create internal release branches so cherry-picks have somewhere to land. + releaseBranches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + // (a) Fresh run: cherry-picks ALL CLs onto each internal branch. + freshCPs, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, releaseBranches, cls) + if err != nil { + t.Fatalf("fresh createSecurityCherryPicks: %v", err) + } + wantCount := len(cls) * len(releaseBranches) + if got := len(freshCPs); got != wantCount { + t.Fatalf("fresh cherry-picks: got %d, want %d (cls=%d * branches=%d)", got, wantCount, len(cls), len(releaseBranches)) + } + + // (b) Restart: all cherry-picks already exist. The function must skip + // duplicates and still return the same number of cherry-picks (the + // existing ones). + restartCPs, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, releaseBranches, cls) + if err != nil { + t.Fatalf("restart createSecurityCherryPicks: %v", err) + } + if got := len(restartCPs); got != wantCount { + t.Fatalf("restart cherry-picks: got %d, want %d", got, wantCount) + } + + // Verify the restart reused the existing CLs (same change numbers). + freshNums := map[int]bool{} + for _, cp := range freshCPs { + freshNums[cp.ChangeNumber] = true + } + for _, cp := range restartCPs { + if !freshNums[cp.ChangeNumber] { + t.Errorf("restart returned unknown cherry-pick CL %d; want an existing CL", cp.ChangeNumber) + } + } +} + +func TestCreateSecurityCherryPicksPartialDedup(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "id9-partial"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + releaseBranches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + // Pre-seed a cherry-pick for the first CL onto the first branch only. + // This simulates a partial prior run. + firstBranch := releaseBranches[0] + preseeded := &gerrit.ChangeInfo{ + ID: "pre-cp-1", + ChangeID: cls[0].ChangeID, // same Change-Id as original + ChangeNumber: 9999, + Branch: firstBranch, + Submittable: true, + Mergeable: true, + Status: "NEW", + } + privGerrit.AddChange("go", "pre-cp-1", preseeded, "preseeded cherry-pick") + + cps, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, releaseBranches, cls) + if err != nil { + t.Fatalf("partial createSecurityCherryPicks: %v", err) + } + wantCount := len(cls) * len(releaseBranches) + if got := len(cps); got != wantCount { + t.Fatalf("partial cherry-picks: got %d, want %d", got, wantCount) + } + + // The preseeded cherry-pick must be reused (its ChangeNumber is 9999). + found := false + for _, cp := range cps { + if cp.ChangeNumber == 9999 { + found = true + break + } + } + if !found { + t.Error("preseeded cherry-pick (CL 9999) was not reused") + } +} + +func TestMoveAndRebasePrivateChanges(t *testing.T) { + t.Run("fresh", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "move-fresh"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + moved, err := deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges: %v", err) + } + if len(moved) != len(cls) { + t.Fatalf("got %d CLs, want %d", len(moved), len(cls)) + } + for _, ci := range moved { + if ci.Branch != checkpoint { + t.Errorf("CL %d branch = %q, want %q", ci.ChangeNumber, ci.Branch, checkpoint) + } + } + }) + + t.Run("restart_already_moved", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "move-restart"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + // Simulate the CLs having already been moved to the checkpoint branch + // by a prior run, so moveAndRebasePrivateChanges sees them as already + // on the correct branch and tolerates the 409. + for _, ci := range cls { + ci.Branch = checkpoint + } + + moved, err := deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges on already-moved CLs: %v", err) + } + if len(moved) != len(cls) { + t.Fatalf("got %d CLs, want %d", len(moved), len(cls)) + } + }) + + t.Run("restart_already_merged", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "move-merged"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + // Simulate CL 1234 having already been merged by a prior run, + // so moveAndRebasePrivateChanges sees it as merged and tolerates the 409. + cls[0].Status = gerrit.ChangeStatusMerged + cls[0].Submittable = false + + moved, err := deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges with merged CL: %v", err) + } + if len(moved) != len(cls) { + t.Fatalf("got %d CLs, want %d", len(moved), len(cls)) + } + for _, ci := range moved { + if ci.ChangeNumber == 1234 && ci.Status != gerrit.ChangeStatusMerged { + t.Errorf("merged CL 1234 status = %q, want %q", ci.Status, gerrit.ChangeStatusMerged) + } + } + }) +} + +func TestSubmitPrivateChanges(t *testing.T) { + t.Run("happy", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "submit-happy"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + cls, err = deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges: %v", err) + } + + submitted, err := deps.buildTasks.submitPrivateChanges(taskCtx, cls) + if err != nil { + t.Fatalf("submitPrivateChanges: %v", err) + } + if len(submitted) != len(cls) { + t.Fatalf("got %d CLs, want %d", len(submitted), len(cls)) + } + for _, ci := range submitted { + if ci.Status != gerrit.ChangeStatusMerged { + t.Errorf("CL %d status = %q, want %q", ci.ChangeNumber, ci.Status, gerrit.ChangeStatusMerged) + } + } + }) + + t.Run("already_merged_skip", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "submit-skip"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + cls, err = deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges: %v", err) + } + + // Simulate CL 1234 having been merged by a prior run. Update both the + // canonical state (via GetChange's returned pointer) and the local slice + // so submitPrivateChanges sees the CL as already merged. + merged1234, err := privGerrit.GetChange(deps.ctx, "1234") + if err != nil { + t.Fatalf("GetChange(1234): %v", err) + } + merged1234.Status = gerrit.ChangeStatusMerged + merged1234.Submittable = false + cls[0].Status = gerrit.ChangeStatusMerged + cls[0].Submittable = false + + submitted, err := deps.buildTasks.submitPrivateChanges(taskCtx, cls) + if err != nil { + t.Fatalf("submitPrivateChanges with pre-merged CL: %v", err) + } + if len(submitted) != len(cls) { + t.Fatalf("got %d CLs, want %d", len(submitted), len(cls)) + } + for _, ci := range submitted { + if ci.Status != gerrit.ChangeStatusMerged { + t.Errorf("CL %d status = %q, want %q", ci.ChangeNumber, ci.Status, gerrit.ChangeStatusMerged) + } + } + }) +} + +func TestCreateVulnReportsStdCmd(t *testing.T) { + deps, _ := newMinorCoalesceTestDeps(t, true) + + vulndbRepo := task.NewFakeRepo(t, "vulndb") + vulndbRepo.CommitOnBranch("master", map[string]string{"README": "vulndb"}) + pubGerrit := task.NewFakeGerrit(t, vulndbRepo) + deps.buildTasks.GerritClient = pubGerrit + + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "vu1"}} + + const announceURL = "https://groups.google.com/g/golang-announce/c/test-minor" + + rm := &relmeta.ReleaseMilestone{ + Patches: []*relmeta.SecurityPatch{ + { + ID: 40027190, + Track: relmeta.Private, + Package: "crypto/tls", + Changelists: []string{"https://go-internal-review.git.corp.google.com/c/go/+/1234"}, + TargetReleases: []string{"1.25.1", "1.26.1"}, + ReleaseNote: "crypto/tls: bad handshake causes panic.\n\nA specially crafted ClientHello triggers a nil pointer dereference.", + GitHubIssueID: 99999, + VulnReportID: "GO-2026-9001", + CVE: "CVE-2026-9001", + Credits: []string{"Alice"}, + }, + { + ID: 40027191, + Track: relmeta.Private, + Package: "cmd/go", + Changelists: []string{"https://go-internal-review.git.corp.google.com/c/go/+/5678"}, + TargetReleases: []string{"1.26.1"}, + ReleaseNote: "cmd/go: module download executes arbitrary code.\n\nA crafted go.sum allows execution of untrusted binaries.", + GitHubIssueID: 99998, + VulnReportID: "GO-2026-9002", + CVE: "CVE-2026-9002", + Credits: []string{"Bob"}, + }, + }, + } + + changeID, err := deps.buildTasks.createVulnReports(taskCtx, rm, announceURL) + if err != nil { + t.Fatalf("createVulnReports: %v", err) + } + if changeID == "" { + t.Fatal("createVulnReports returned empty change ID") + } + + vulndbHead, err := pubGerrit.ReadBranchHead(deps.ctx, "vulndb", "master") + if err != nil { + t.Fatal(err) + } + + for _, p := range rm.Patches { + reportPath := path.Join("data", "reports", p.VulnReportID+".yaml") + b, err := pubGerrit.ReadFile(deps.ctx, "vulndb", vulndbHead, reportPath) + if err != nil { + t.Fatalf("reading %s: %v", reportPath, err) + } + + if !bytes.Contains(b, []byte(announceURL)) { + t.Errorf("report %s does not contain announcement URL %s", p.VulnReportID, announceURL) + } + + var vr report.Report + if err := yaml.Unmarshal(b, &vr); err != nil { + t.Fatalf("unmarshal %s: %v", reportPath, err) + } + + if len(vr.Modules) != 1 { + t.Errorf("%s: got %d modules, want 1", p.VulnReportID, len(vr.Modules)) + continue + } + wantModule := task.VulnModule(p.Package) + if vr.Modules[0].Module != wantModule { + t.Errorf("%s: module = %q, want %q", p.VulnReportID, vr.Modules[0].Module, wantModule) + } + + if vr.Modules[0].VulnerableAt == nil { + t.Errorf("%s: VulnerableAt is nil", p.VulnReportID) + } + } +} + +func TestCreateVulnReportsNilMilestone(t *testing.T) { + deps, _ := newMinorCoalesceTestDeps(t, false) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "vu1-noop"}} + + t.Run("nil milestone", func(t *testing.T) { + got, err := deps.buildTasks.createVulnReports(taskCtx, nil, "https://example.com") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "" { + t.Errorf("got change ID %q, want empty", got) + } + }) + + t.Run("empty patches", func(t *testing.T) { + got, err := deps.buildTasks.createVulnReports(taskCtx, &relmeta.ReleaseMilestone{}, "https://example.com") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "" { + t.Errorf("got change ID %q, want empty", got) + } + }) +} + +func TestMergedCLCherryPickedOntoInternalBranch(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "cp1"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + // Mark CL 1234 as already merged. + merged1234, err := privGerrit.GetChange(taskCtx, "1234") + if err != nil { + t.Fatalf("GetChange(1234): %v", err) + } + merged1234.Status = gerrit.ChangeStatusMerged + merged1234.Submittable = false + + var allCLs []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + allCLs = append(allCLs, ci) + } + + openCLs := []*gerrit.ChangeInfo{} + for _, ci := range allCLs { + if ci.Status != gerrit.ChangeStatusMerged { + openCLs = append(openCLs, ci) + } + } + _, err = deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, openCLs) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + // Create internal release branches from ALL cls (the full milestone). + releaseBranches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, allCLs) + if err != nil { + t.Fatalf("createInternalReleaseBranches: %v", err) + } + + cps, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, releaseBranches, allCLs) + if err != nil { + t.Fatalf("createSecurityCherryPicks: %v", err) + } + + wantCount := len(allCLs) * len(releaseBranches) + if got := len(cps); got != wantCount { + t.Errorf("cherry-picks: got %d, want %d", got, wantCount) + } +}
diff --git a/internal/relui/workflows.go b/internal/relui/workflows.go index de805d6..e672d1c 100644 --- a/internal/relui/workflows.go +++ b/internal/relui/workflows.go
@@ -43,8 +43,11 @@ "golang.org/x/build/internal/relui/sign" "golang.org/x/build/internal/task" wf "golang.org/x/build/internal/workflow" + "golang.org/x/build/relmeta" "golang.org/x/net/context/ctxhttp" + "golang.org/x/vulndb/report" "google.golang.org/protobuf/types/known/structpb" + yaml "gopkg.in/yaml.v3" ) // DefinitionHolder holds workflow definitions. @@ -121,7 +124,7 @@ Example: "CVE-2023-XXXX", Doc: "List of CVEs for PRIVATE track fixes contained in the release to be included in the pre-announcement.", Check: func(cves []string) error { - var m = make(map[string]bool) + m := make(map[string]bool) for _, c := range cves { switch { case !cveRE.MatchString(c): @@ -441,7 +444,9 @@ securityFixes = wf.Param(wd, securityFixesParameter) } } - addCommTasks(wd, build, comm, r.kind, wf.Slice(published), securitySummary, securityFixes, coordinators) + + rm := wf.Const[*relmeta.ReleaseMilestone](nil) + addCommTasks(wd, build, comm, r.kind, wf.Slice(published), securitySummary, securityFixes, coordinators, rm) if r.major >= currentMajor { wf.Action1(wd, "update-proxy-test", version.UpdateProxyTestRepo, published) } @@ -449,19 +454,11 @@ h.RegisterDefinition(fmt.Sprintf("Go 1.%d %s", r.major, r.suffix), wd) } - for _, v := range [...]struct { - UseMetadata bool - Description string - }{ - {false, "manually input security comms"}, - {true, "metadata-based security comms"}, - } { - wd, err := createMinorReleaseWorkflow(build, milestone, version, comm, currentMajor-1, currentMajor, v.UseMetadata) - if err != nil { - return err - } - h.RegisterDefinition(fmt.Sprintf("Minor releases for Go 1.%d and 1.%d (%s)", currentMajor-1, currentMajor, v.Description), wd) + wd, err := createMinorReleaseWorkflow(build, milestone, version, comm, currentMajor-1, currentMajor) + if err != nil { + return err } + h.RegisterDefinition(fmt.Sprintf("Minor releases for Go 1.%d and 1.%d", currentMajor-1, currentMajor), wd) return nil } @@ -488,27 +485,53 @@ h.RegisterDefinition(fmt.Sprintf("dry-run (build, test, and sign only): Go 1.%d next beta", major), wd) } -func createMinorReleaseWorkflow(build *BuildReleaseTasks, milestone *task.MilestoneTasks, version *task.VersionTasks, comm task.CommunicationTasks, prevMajor, currentMajor int, useMetadata bool) (*wf.Definition, error) { +func createMinorReleaseWorkflow(build *BuildReleaseTasks, milestone *task.MilestoneTasks, version *task.VersionTasks, comm task.CommunicationTasks, prevMajor, currentMajor int) (*wf.Definition, error) { wd := wf.New(wf.ACL{Groups: []string{groups.ReleaseTeam}}) - coordinators := wf.Param(wd, releaseCoordinators) - currPublished := addSingleReleaseWorkflow(build, milestone, version, wd.Sub(fmt.Sprintf("Go 1.%d", currentMajor)), currentMajor, task.KindMinor, coordinators) - prevPublished := addSingleReleaseWorkflow(build, milestone, version, wd.Sub(fmt.Sprintf("Go 1.%d", prevMajor)), prevMajor, task.KindMinor, coordinators) + milestoneNum := wf.Param(wd, task.SecurityMilestoneParameter) + + rm := wf.Task1(wd, "Fetch security milestone", build.fetchSecurityMilestone, milestoneNum) + + // cls are drafted by patch owners against `public` + // branch of sso://go-internal/go. Typically, no + // human should submit these patches; however, the + // workflow is hardened against accidental submission + // in order to provide idempotent checkpoint branches. + cls := wf.Task1(wd, "Check private changes", build.checkPrivateChanges, rm) var ( - securitySummary wf.Value[string] - securityFixes wf.Value[[]string] + nextMinors = wf.Task1(wd, "Get next minor versions", version.GetNextMinorVersions, wf.Const([]int{currentMajor, prevMajor})) + vt = wf.Const(version) + major = wf.Const(currentMajor) ) - if useMetadata { - milestoneNum := wf.Param(wd, task.SecurityMilestoneParameter) - securitySummary = wf.Task1(wd, "Get short security content summary from metadata", comm.GetSecuritySummary, milestoneNum) - securityFixes = wf.Task1(wd, "Get security release notes from metadata", comm.GetSecurityReleaseNotes, milestoneNum) - } else { - securitySummary = wf.Param(wd, securitySummaryParameter) - securityFixes = wf.Param(wd, securityFixesParameter) - } + branchInfo := wf.Task3(wd, "Compute security branch names", computeSecurityBranchInfo, vt, major, nextMinors, wf.After(cls)) - addCommTasks(wd, build, comm, task.KindMinor, wf.Slice(currPublished, prevPublished), securitySummary, securityFixes, coordinators) + // checkpoint is created with a timestamp trailer + // to ensure that workflow restarts are idempotent. + checkpoint := wf.Task2(wd, "Create checkpoint branch", build.createSecurityCheckpoint, branchInfo, cls) + cls = wf.Task2(wd, "Move and rebase private changes", build.moveAndRebasePrivateChanges, checkpoint, cls) + cls = wf.Task1(wd, "Submit private changes", build.submitPrivateChanges, cls) + + // internalBranches are NOT created with a timestamp + // trailer; instead, they are deleted lazily before + // creation to make workflow restarts idempotent. + internalBranches := wf.Task2(wd, "Create internal release branches", build.createInternalReleaseBranches, branchInfo, cls) + cherryPicks := wf.Task2(wd, "Create cherry-picks", build.createSecurityCherryPicks, internalBranches, cls) + coalesced := wf.Task1(wd, "Submit cherry-picks", build.submitCherryPicks, cherryPicks) + + // once all internal branches have their + // respective cherrypicked patches, the + // security release coalescing is done + // and any single-release workflows can + // proceed by reaching the branch state. + wf.Output(wd, "Cherry-picks", coalesced) + + currPublished := addSingleReleaseWorkflow(build, milestone, version, wd.Sub(fmt.Sprintf("Go 1.%d", currentMajor)), currentMajor, task.KindMinor, coordinators, coalesced) + prevPublished := addSingleReleaseWorkflow(build, milestone, version, wd.Sub(fmt.Sprintf("Go 1.%d", prevMajor)), prevMajor, task.KindMinor, coordinators, coalesced) + + securitySummary := wf.Task1(wd, "Get short security content summary from metadata", comm.GetSecuritySummary, milestoneNum) + securityFixes := wf.Task1(wd, "Get security release notes from metadata", comm.GetSecurityReleaseNotes, milestoneNum) + addCommTasks(wd, build, comm, task.KindMinor, wf.Slice(currPublished, prevPublished), securitySummary, securityFixes, coordinators, rm) wf.Action1(wd, "update-proxy-test", version.UpdateProxyTestRepo, currPublished) return wd, nil @@ -516,7 +539,8 @@ func addCommTasks( wd *wf.Definition, build *BuildReleaseTasks, comm task.CommunicationTasks, - kind task.ReleaseKind, published wf.Value[[]task.Published], securitySummary wf.Value[string], securityFixes, coordinators wf.Value[[]string], + kind task.ReleaseKind, published wf.Value[[]task.Published], securitySummary wf.Value[string], + securityFixes, coordinators wf.Value[[]string], rm wf.Value[*relmeta.ReleaseMilestone], ) { okayToAnnounce := wf.Action0(wd, "Wait to Announce", build.ApproveAction, wf.After(published)) @@ -527,10 +551,37 @@ mastodonURL := wf.Task4(wd, "post-mastodon", comm.TrumpetRelease, wf.Const(kind), published, securitySummary, announcementURL, wf.After(okayToAnnounce)) blueskyURL := wf.Task4(wd, "post-bluesky", comm.SkeetRelease, wf.Const(kind), published, securitySummary, announcementURL, wf.After(okayToAnnounce)) + vulndbChangeID := wf.Task2(wd, "file-vulndb-reports", build.createVulnReports, rm, announcementURL) + + wf.Action2(wd, "Update GitHub issues", task.UpdateGitHubIssues, wf.Const(build.GitHub), rm, wf.After(vulndbChangeID)) + wf.Output(wd, "Announcement URL", announcementURL) wf.Output(wd, "Tweet URL", tweetURL) wf.Output(wd, "Mastodon URL", mastodonURL) wf.Output(wd, "Bluesky URL", blueskyURL) + wf.Output(wd, "VulnDB Change ID", vulndbChangeID) +} + +// createVulnReports builds and submits vulndb reports for std/cmd +// security patches. It no-ops when rm is nil or has no patches +// (non-security minor release or major-release path). +func (b *BuildReleaseTasks) createVulnReports(ctx *wf.TaskContext, rm *relmeta.ReleaseMilestone, announceURL string) (string, error) { + if rm == nil || len(rm.Patches) == 0 { + return "", nil + } + var reports []*report.Report + for _, p := range rm.Patches { + mod, err := task.DeriveVulnModuleInfo(p) + if err != nil { + return "", err + } + r, err := task.VulnReport(p, mod, announceURL) + if err != nil { + return "", err + } + reports = append(reports, r) + } + return task.MailVulnReports(ctx, b.GerritClient, reports) } func now(_ context.Context) (time.Time, error) { @@ -540,6 +591,7 @@ func addSingleReleaseWorkflow( build *BuildReleaseTasks, milestone *task.MilestoneTasks, version *task.VersionTasks, wd *wf.Definition, major int, kind task.ReleaseKind, coordinators wf.Value[[]string], + securityPrereqs ...wf.Dependency, ) wf.Value[task.Published] { kindVal := wf.Const(kind) branch := fmt.Sprintf("release-branch.go1.%d", major) @@ -557,8 +609,9 @@ milestones := wf.Task2(wd, "Pick milestones", milestone.FetchMilestones, nextVersion, kindVal) checkedStartingBlockingIssues := wf.Action3(wd, "Check blocking issues", milestone.CheckBlockers, milestones, nextVersion, kindVal) - // Look up the prepared security commit for this release, if any. - securityCommit := wf.Task1(wd, "Read security ref", build.readSecurityRef, nextVersion) + // Read the security ref for internal branches. + securityCommit := wf.Task1(wd, "Read security ref", build.readSecurityRef, nextVersion, wf.After(securityPrereqs...)) + confirmPrivateSecurityFixes := wf.Action4(wd, "Confirm PRIVATE-track security CLs", func(ctx *wf.TaskContext, version, targetBranch, startingHead, securityCommit string, ) error { @@ -591,7 +644,12 @@ if bottomSecurityCL := commits[len(commits)-1]; len(bottomSecurityCL.Parents) != 1 { return fmt.Errorf("bottom-most security commit %q has %d parents, want 1 parent", bottomSecurityCL.Commit, len(bottomSecurityCL.Parents)) } else if bottomSecurityCL.Parents[0] != startingHead { - return fmt.Errorf("bottom-most security commit %q's parent is %q, want %q", bottomSecurityCL.Commit, bottomSecurityCL.Parents[0], startingHead) + // The security fixes were coalesced onto a public head that has since + // diverged from the current public release-branch head. Because each + // coalesce run now produces a fresh timestamped checkpoint and the old + // artifacts are left untouched, the cheap and safe remedy is to restart + // the workflow so the coalesce re-runs against the current head. + return fmt.Errorf("bottom-most security commit %q's parent is %q, but the current public %s head is %q; the coalesced security fixes are stale. Restart the release workflow to re-coalesce against the current head", bottomSecurityCL.Commit, bottomSecurityCL.Parents[0], targetBranch, startingHead) } var summary strings.Builder fmt.Fprintf(&summary, "Will build with %d security fix CL(s) on top of public %s:\n\n", len(commits), targetBranch) @@ -623,23 +681,23 @@ // Detect and handle the unexpected case of either the public or internal release branches // changing from the time the workflow was started. if releaseDayPublicHead, err := build.GerritClient.ReadBranchHead(ctx, build.GerritProject, targetBranch); err != nil { - return nil, err + return nil, fmt.Errorf("reading public branch head (safe to retry this step): %w", err) } else if releaseDayPublicHead != startingHead { // Something is unexpected if the public release branch now doesn't match what it was // when the workflow started. Whether or not it's possible to proceed depends on what // exactly happened. For now handle this by refusing to proceed, but if we learn that // it's worth handling this differently, we'll revisit this. - return nil, fmt.Errorf("head of public %q branch %q unexpectedly differs from head at workflow start %q", targetBranch, releaseDayPublicHead, startingHead) + return nil, fmt.Errorf("head of public %q branch is %q, but was %q when the workflow started; retrying this step alone will not help; restart the release workflow to re-coalesce against the current head", targetBranch, releaseDayPublicHead, startingHead) } internalBranch := fmt.Sprintf("internal-release-branch.%s", version) if releaseDayPrivateHead, err := build.PrivateGerritClient.ReadBranchHead(ctx, build.PrivateGerritProject, internalBranch); err != nil { - return nil, err + return nil, fmt.Errorf("reading private branch head (safe to retry this step): %w", err) } else if releaseDayPrivateHead != securityCommit { // Something is unexpected if the internal release branch now doesn't match what it was // when the workflow started. Whether or not it's possible to proceed depends on what // exactly happened. For now handle this by refusing to proceed, but if we learn that // it's worth handling this differently, we'll revisit this. - return nil, fmt.Errorf("head of private %q branch %q unexpectedly differs from head at workflow start %q", internalBranch, releaseDayPrivateHead, securityCommit) + return nil, fmt.Errorf("head of private %q branch is %q, but was %q when the workflow started; retrying this step alone will not help; restart the release workflow to re-coalesce against the current head", internalBranch, releaseDayPrivateHead, securityCommit) } /* @@ -664,7 +722,7 @@ publicOrigin := build.GerritClient.GitRepoURL(build.GerritProject) repo, err := build.Git.CloneBranch(ctx, publicOrigin, targetBranch) if err != nil { - return nil, err + return nil, fmt.Errorf("cloning public repo (safe to retry this step): %w", err) } defer repo.Close() ctx.Printf("cloned public repo") @@ -672,18 +730,18 @@ privateOrigin, privateRef := build.PrivateGerritClient.GitRepoURL(build.PrivateGerritProject), "refs/heads/"+internalBranch ctx.Printf("fetching %s from %s", privateRef, privateOrigin) if _, err := repo.RunCommand(ctx, "fetch", privateOrigin, privateRef); err != nil { - return nil, err + return nil, fmt.Errorf("fetching private branch (safe to retry this step): %w", err) } ctx.Printf("fetched") if _, err := repo.RunCommand(ctx, "cherry-pick", startingHead+".."+securityCommit); err != nil { - return nil, err + return nil, fmt.Errorf("cherry-picking security fixes (safe to retry this step): %w", err) } ctx.Printf("cherry-picked") var refspec strings.Builder fmt.Fprintf(&refspec, "HEAD:refs/for/%s%%l=Auto-Submit+1,l=TryBot-Bypass+1", targetBranch) reviewerEmails, err := task.CoordinatorEmails(reviewers) if err != nil { - return nil, err + return nil, fmt.Errorf("resolving coordinator emails (safe to retry this step): %w", err) } for _, r := range reviewerEmails { fmt.Fprintf(&refspec, ",r=%s", r) @@ -696,7 +754,7 @@ ctx.Printf("pushing %s to %s", refspec.String(), publicOrigin) gitPushOutput, err := repo.RunGitPush(ctx, publicOrigin, refspec.String()) if err != nil { - return nil, err + return nil, fmt.Errorf("pushing security CLs to public Gerrit (manual intervention required): %w", err) } ctx.Printf("git push output:\n%s\n", gitPushOutput) @@ -926,6 +984,7 @@ BuildBucketClient task.BuildBucketClient SwarmingClient task.SwarmingClient ApproveAction func(*wf.TaskContext) error + GitHub task.GitHubClientInterface } // readSecurityRef reads the head of the internal release branch that corresponds @@ -938,15 +997,9 @@ return "", nil } - // Read the internal release branch prepared by the 'Prepare internal security release branches' - // workflow for this version, if any. internalBranch := fmt.Sprintf("internal-release-branch.%s", version) commit, err := b.PrivateGerritClient.ReadBranchHead(ctx, b.PrivateGerritProject, internalBranch) if errors.Is(err, gerrit.ErrResourceNotExist) { - // The internal release branch doesn't exist. - // - // This is okay. It happens when there are no PRIVATE-track security fixes for this release, - // and the public release branch is used as the source. Proceed without a security commit. return "", nil } else if err != nil { return "", fmt.Errorf("error reading private Gerrit project's branch %q head: %v", internalBranch, err) @@ -954,6 +1007,326 @@ return commit, nil } +func (b *BuildReleaseTasks) fetchSecurityMilestone(ctx *wf.TaskContext, milestoneNum string) (*relmeta.ReleaseMilestone, error) { + if b.PrivateGerritClient == nil || b.PrivateGerritProject == "" { + ctx.Printf("Private Gerrit fields are unset, no security milestone to fetch.") + return nil, nil + } + if milestoneNum == "" || milestoneNum == "0" { + ctx.Printf("No security milestone specified, no security milestone to fetch.") + return nil, nil + } + const project = "security-metadata" + head, err := b.PrivateGerritClient.ReadBranchHead(ctx, project, "main") + if err != nil { + return nil, err + } + raw, err := b.PrivateGerritClient.ReadFile(ctx, project, head, path.Join("data", "milestones", milestoneNum+".yaml")) + if err != nil { + return nil, err + } + var rm relmeta.ReleaseMilestone + if err := yaml.Unmarshal(raw, &rm); err != nil { + return nil, fmt.Errorf("cannot YAML unmarshal the milestone: %v", err) + } + return &rm, nil +} + +type securityBranchInfo struct { + CheckpointName string + PublicReleaseBranches []string +} + +// computeSecurityBranchInfo derives the checkpoint branch name and the set of +// public release branches the PRIVATE-track security coalesce targets, given the +// next minor versions for the two release series. If the next major's release +// branch already exists, the next RC is included as well. +func computeSecurityBranchInfo(ctx *wf.TaskContext, version *task.VersionTasks, currentMajor int, nextMinors []string) (securityBranchInfo, error) { + bi := securityBranchInfo{ + CheckpointName: strings.Join(nextMinors, "-") + "-checkpoint", + } + for _, v := range nextMinors { + bi.PublicReleaseBranches = append(bi.PublicReleaseBranches, "release-branch."+v) + } + // If the next major's release branch already exists, include an RC. + switch _, err := version.Gerrit.ReadBranchHead(ctx, version.GoProject, fmt.Sprintf("release-branch.go1.%d", currentMajor+1)); { + case errors.Is(err, gerrit.ErrResourceNotExist): + // No RC branch; minors only. + case err == nil: + nextRC, err := version.GetNextVersion(ctx, currentMajor+1, task.KindRC) + if err != nil { + return securityBranchInfo{}, err + } + bi.CheckpointName = nextRC + "-" + bi.CheckpointName + bi.PublicReleaseBranches = append([]string{"release-branch." + nextRC}, bi.PublicReleaseBranches...) + default: + return securityBranchInfo{}, err + } + return bi, nil +} + +var ( + commitCVERE = regexp.MustCompile(`(?m)^Fixes CVE-\d{4}-\d+`) + commitGitHubIssueRE = regexp.MustCompile(`(?m)^Fixes (?:golang/go)?#(\d+)`) +) + +func (b *BuildReleaseTasks) checkPrivateChanges(ctx *wf.TaskContext, rm *relmeta.ReleaseMilestone) ([]*gerrit.ChangeInfo, error) { + if rm == nil { + return nil, nil + } + var ( + cls []*gerrit.ChangeInfo + lintErrs []error + ) + for _, patch := range rm.Patches { + if patch.Track == relmeta.Public { + continue + } + for _, clURL := range patch.Changelists { + _, num, _ := strings.Cut(clURL, "/+/") + ci, err := b.PrivateGerritClient.GetChange(ctx, num, gerrit.QueryChangesOpt{Fields: []string{"SUBMITTABLE"}}) + if err != nil { + return nil, err + } + if ci.Status == gerrit.ChangeStatusMerged { + cls = append(cls, ci) + continue + } + if !ci.Submittable { + return nil, fmt.Errorf("change %s is not submittable", privateChangeURL(num)) + } + ra, err := b.PrivateGerritClient.GetRevisionActions(ctx, num, "current") + if err != nil { + return nil, err + } + if ra["submit"] == nil || !ra["submit"].Enabled { + return nil, fmt.Errorf("change %s is not submittable", privateChangeURL(num)) + } + cm, err := b.PrivateGerritClient.GetCommitMessage(ctx, num) + if err != nil { + return nil, err + } + if !commitCVERE.MatchString(cm) { + lintErrs = append(lintErrs, fmt.Errorf("change %s is missing CVE reference", privateChangeURL(num))) + } + if !commitGitHubIssueRE.MatchString(cm) { + lintErrs = append(lintErrs, fmt.Errorf("change %s is missing GitHub issue reference", privateChangeURL(num))) + } + cls = append(cls, ci) + } + } + if len(cls) == 0 { + ctx.Printf("No non-PUBLIC security patches to prepare.") + } + return cls, errors.Join(lintErrs...) +} + +func (b *BuildReleaseTasks) createSecurityCheckpoint(ctx *wf.TaskContext, bi securityBranchInfo, cls []*gerrit.ChangeInfo) (string, error) { + if len(cls) == 0 { + ctx.Printf("No PRIVATE-track security patches; skipping checkpoint branch creation.") + return "", nil + } + publicHead, err := b.PrivateGerritClient.ReadBranchHead(ctx, b.PrivateGerritProject, "public") + if err != nil { + return "", err + } + + // Append the formatted timestamp to make any restarts idempotent. + checkpointName := bi.CheckpointName + "-" + time.Now().UTC().Format("20060102-150405") + if _, err := b.PrivateGerritClient.CreateBranch(ctx, b.PrivateGerritProject, checkpointName, gerrit.BranchInput{Revision: publicHead}); err != nil { + return "", err + } + return checkpointName, nil +} + +func (b *BuildReleaseTasks) moveAndRebasePrivateChanges(ctx *wf.TaskContext, checkpointBranch string, cls []*gerrit.ChangeInfo) ([]*gerrit.ChangeInfo, error) { + for i, ci := range cls { + // Idempotent. Changes can be in the MERGED (HTTP 409) state which means + // that they cannot be moved or rebased. Refetch it and if it is MERGED, + // skip it similarly to submitPrivateChanges. + fresh, err := b.PrivateGerritClient.GetChange(ctx, ci.ID) + if err != nil { + return nil, err + } + if fresh.Status == gerrit.ChangeStatusMerged { + cls[i] = fresh + continue + } + movedCI, err := b.PrivateGerritClient.MoveChange(ctx, ci.ID, checkpointBranch) + if err != nil { + var httpErr *gerrit.HTTPError + if !errors.As(err, &httpErr) || httpErr.Res.StatusCode != http.StatusConflict || string(httpErr.Body) != "Change is already destined for the specified branch\n" { + return nil, err + } + movedCI = *ci + } else { + cls[i] = &movedCI + } + rebasedCI, err := b.PrivateGerritClient.RebaseChange(ctx, movedCI.ID, "") + if err != nil { + var httpErr *gerrit.HTTPError + if !errors.As(err, &httpErr) || httpErr.Res.StatusCode != http.StatusConflict || string(httpErr.Body) != "Change is already up to date.\n" { + return nil, err + } + } else { + cls[i] = &rebasedCI + } + } + return cls, nil +} + +func (b *BuildReleaseTasks) submitPrivateChanges(ctx *wf.TaskContext, cls []*gerrit.ChangeInfo) ([]*gerrit.ChangeInfo, error) { + if _, err := task.AwaitCondition(ctx, time.Second*10, func() (string, bool, error) { + unsubmitted := len(cls) + for i, change := range cls { + if change.Status == gerrit.ChangeStatusMerged { + unsubmitted-- + continue + } + ci, err := b.PrivateGerritClient.GetChange(ctx, change.ID, gerrit.QueryChangesOpt{Fields: []string{"SUBMITTABLE"}}) + if err != nil { + return "", false, err + } + if !ci.Submittable { + continue + } + submitted, err := b.PrivateGerritClient.SubmitChange(ctx, ci.ID) + if err != nil { + return "", false, err + } + cls[i] = &submitted + unsubmitted-- + } + if unsubmitted == 0 { + return "", true, nil + } + return "", false, nil + }); err != nil { + return nil, err + } + return cls, nil +} + +func (b *BuildReleaseTasks) createInternalReleaseBranches(ctx *wf.TaskContext, bi securityBranchInfo, cls []*gerrit.ChangeInfo) ([]string, error) { + if len(cls) == 0 { + ctx.Printf("No PRIVATE-track security patches; skipping internal release branch creation.") + return nil, nil + } + var internalBranches []string + for _, next := range bi.PublicReleaseBranches { + publicHead, err := b.PrivateGerritClient.ReadBranchHead(ctx, b.PrivateGerritProject, majorFromMinor(next)) + if err != nil { + return nil, err + } + internalReleaseBranch := "internal-" + next + + // `internal-<relver>` branches are not timestamped; we must + // try to delete existing branches to preserve idempotency. + if err := b.PrivateGerritClient.DeleteBranch(ctx, b.PrivateGerritProject, internalReleaseBranch); err != nil && !errors.Is(err, gerrit.ErrResourceNotExist) { + return nil, err + } + if _, err := b.PrivateGerritClient.CreateBranch(ctx, b.PrivateGerritProject, internalReleaseBranch, gerrit.BranchInput{Revision: publicHead}); err != nil { + return nil, err + } + internalBranches = append(internalBranches, internalReleaseBranch) + } + return internalBranches, nil +} + +func (b *BuildReleaseTasks) createSecurityCherryPicks(ctx *wf.TaskContext, releaseBranches []string, cls []*gerrit.ChangeInfo) ([]*gerrit.ChangeInfo, error) { + var cherryPicks []*gerrit.ChangeInfo + for _, ci := range cls { + for _, releaseBranch := range releaseBranches { + // Check whether a non-abandoned cherry-pick of this + // change already exists on the target branch (e.g. from + // a prior run). The Change-Id footer is preserved in + // the cherry-pick commit message, so we query by it. + existing, err := b.PrivateGerritClient.QueryChanges(ctx, + fmt.Sprintf("project:%s branch:%s change:%s -is:abandoned", + b.PrivateGerritProject, releaseBranch, ci.ChangeID)) + if err != nil { + return nil, err + } + if len(existing) > 0 { + ctx.Printf("Skipping cherry-pick of %s to %s: existing CL %s (status %s)", + ci.ChangeID, releaseBranch, + privateChangeURL(existing[0].ChangeNumber), + existing[0].Status) + cherryPicks = append(cherryPicks, existing[0]) + continue + } + + commitMessage, err := b.PrivateGerritClient.GetCommitMessage(ctx, ci.ID) + if err != nil { + return nil, err + } + commitMessage = fmt.Sprintf("[%s] %s", majorFromMinor(strings.TrimPrefix(releaseBranch, "internal-")), commitMessage) + + cpCI, conflicts, err := b.PrivateGerritClient.CreateCherryPick(ctx, ci.ID, releaseBranch, commitMessage) + if err != nil { + return nil, err + } + if conflicts { + ctx.Printf("Cherry-pick of %s has merge conflicts against %s: %s", privateChangeURL(ci.ChangeNumber), releaseBranch, privateChangeURL(cpCI.ChangeNumber)) + } + cp := cpCI + cherryPicks = append(cherryPicks, &cp) + } + } + return cherryPicks, nil +} + +// submitCherryPicks waits for the cherry-pick CLs to become submittable and +// submits them, landing the security fixes on the internal release branches. +// It returns a display map of internal release branch to submitted CL URLs. +func (b *BuildReleaseTasks) submitCherryPicks(ctx *wf.TaskContext, cherryPicks []*gerrit.ChangeInfo) (map[string][]string, error) { + if _, err := task.AwaitCondition(ctx, time.Second*10, func() (string, bool, error) { + unsubmitted := len(cherryPicks) + for i, cp := range cherryPicks { + if cp.Status == gerrit.ChangeStatusMerged { + unsubmitted-- + continue + } + ci, err := b.PrivateGerritClient.GetChange(ctx, cp.ID, gerrit.QueryChangesOpt{Fields: []string{"SUBMITTABLE"}}) + if err != nil { + return "", false, err + } + if !ci.Submittable { + continue + } + submitted, err := b.PrivateGerritClient.SubmitChange(ctx, ci.ID) + if err != nil { + return "", false, err + } + cherryPicks[i] = &submitted + unsubmitted-- + } + if unsubmitted == 0 { + return "", true, nil + } + return "", false, nil + }); err != nil { + return nil, err + } + submitted := map[string][]string{} + for _, cp := range cherryPicks { + submitted[cp.Branch] = append(submitted[cp.Branch], privateChangeURL(cp.ChangeNumber)) + } + return submitted, nil +} + +// majorFromMinor converts a release branch name from its minor version form to +// its major version form (i.e., release-branch.go1.2.3 to release-branch.go1.2). +func majorFromMinor(branch string) string { + stripped := strings.TrimPrefix(branch, "release-branch.") + major := goversion.Lang(stripped) + return "release-branch." + major +} + +func privateChangeURL[T int | string](clNum T) string { + return fmt.Sprintf("https://go-internal-review.git.corp.google.com/c/go/+/%v", clNum) +} + // getGitSource selects a source spec from the provided inputs. // If securityCommit is a non-empty string, it takes precedence over the public commit. func (b *BuildReleaseTasks) getGitSource(ctx *wf.TaskContext, branch, commit, securityCommit, versionFile string) (sourceSpec, error) { @@ -1204,7 +1577,6 @@ _, err = io.Copy(w, distpack) return err }) - } func (b *BuildReleaseTasks) checkDistpacksMatch(ctx *wf.TaskContext, linux, windows artifact) error { @@ -1282,21 +1654,6 @@ return result, nil } -func (b *BuildReleaseTasks) modFilesFromBinary(ctx *wf.TaskContext, version string, t time.Time, tar artifact) (moduleArtifact, error) { - result := moduleArtifact{Target: tar.Target} - a, err := b.runBuildStep(ctx, nil, tar, "mod.zip", func(r io.Reader, w io.Writer) error { - ctx.DisableWatchdog() // The zipping process can be time consuming and is unlikely to hang. - var err error - result.Mod, result.Info, err = task.TarToModFiles(tar.Target, version, t, r, w) - return err - }) - if err != nil { - return moduleArtifact{}, err - } - result.ZipScratch = a.Scratch - return result, nil -} - func (b *BuildReleaseTasks) mergeSignedToTGZ(ctx *wf.TaskContext, unsigned, signed artifact) (artifact, error) { return b.runBuildStep(ctx, unsigned.Target, signed, "tar.gz", func(signed io.Reader, w io.Writer) error { signedBinaries, err := task.ReadBinariesFromPKG(signed) @@ -1524,7 +1881,7 @@ } // All done, we have our GPG signatures. // Put them in a base name → scratch path map. - var signatures = make(map[string]string) + signatures := make(map[string]string) for _, o := range out { signatures[path.Base(o)] = o } @@ -1727,7 +2084,6 @@ return testResult{name, false}, b.ApproveAction(ctx) } return testResult{name, true}, nil - } func (b *BuildReleaseTasks) checkTestResults(ctx *wf.TaskContext, results []testResult) error { @@ -1948,7 +2304,7 @@ // It returns the Go version and files that have been successfully published. func (tasks *BuildReleaseTasks) publishArtifacts(ctx *wf.TaskContext, version string, artifacts []artifact) (task.Published, error) { // Each release artifact corresponds to a single website file. - var files = make([]task.WebsiteFile, len(artifacts)) + files := make([]task.WebsiteFile, len(artifacts)) for i, a := range artifacts { // Define website file metadata. f := task.WebsiteFile{
diff --git a/internal/task/announce.go b/internal/task/announce.go index 998f5cb..e83fe10 100644 --- a/internal/task/announce.go +++ b/internal/task/announce.go
@@ -6,6 +6,7 @@ import ( "bytes" + "context" "embed" "errors" "fmt" @@ -15,6 +16,8 @@ "net/http" "net/mail" "net/url" + "path" + "regexp" "strings" "text/template" "time" @@ -32,7 +35,9 @@ "golang.org/x/build/internal/secret" "golang.org/x/build/internal/workflow" "golang.org/x/build/maintner/maintnerd/maintapi/version" + "golang.org/x/build/relmeta" "golang.org/x/net/html" + yaml "gopkg.in/yaml.v3" ) type releaseAnnouncement struct { @@ -126,6 +131,7 @@ func (d Date) Format(layout string) string { return time.Date(d.Year, d.Month, d.Day, 0, 0, 0, 0, time.UTC).Format(layout) } + func (d Date) After(year int, month time.Month, day int) bool { return time.Date(d.Year, d.Month, d.Day, 0, 0, 0, 0, time.UTC). After(time.Date(year, month, day, 0, 0, 0, 0, time.UTC)) @@ -634,7 +640,6 @@ return "", false, nil } return threadURL, threadURL != "", nil - } return AwaitCondition(ctx, 10*time.Second, check) } @@ -807,9 +812,8 @@ n.Dump(source, 0) } - var ( - markers []byte // Stack of list markers, from outermost to innermost. - ) + var markers []byte // Stack of list markers, from outermost to innermost. + walk := func(n ast.Node, entering bool) (ast.WalkStatus, error) { if entering { if n.Type() == ast.TypeBlock && n.PreviousSibling() != nil { @@ -977,3 +981,38 @@ } return releaseNotes, nil } + +var ( + SecurityMilestoneParameter = workflow.ParamDef[string]{ + Name: "Release Milestone", + ParamType: workflow.BasicString, + Doc: `Release Milestone is the security-metadata milestone for the security patch(es) being included in a Go release. + +You can check with the security release coordinator for this release to confirm this input.`, + Example: "123456", + Check: func(num string) error { + if !numOnlyRE.MatchString(num) { + return errors.New("milestone number must contain only numbers") + } + return nil + }, + } + numOnlyRE = regexp.MustCompile(`^\d+$`) +) + +func fetchReleaseMilestone(ctx context.Context, private GerritClient, milestoneNum string) (relmeta.ReleaseMilestone, error) { + const project = "security-metadata" + head, err := private.ReadBranchHead(ctx, project, "main") + if err != nil { + return relmeta.ReleaseMilestone{}, err + } + b, err := private.ReadFile(ctx, project, head, path.Join("data", "milestones", milestoneNum+".yaml")) + if err != nil { + return relmeta.ReleaseMilestone{}, err + } + var rm relmeta.ReleaseMilestone + if err := yaml.Unmarshal(b, &rm); err != nil { + return relmeta.ReleaseMilestone{}, fmt.Errorf("cannot YAML unmarshal the milestone: %v", err) + } + return rm, nil +}
diff --git a/internal/task/fakes.go b/internal/task/fakes.go index 0767cc1..be79585 100644 --- a/internal/task/fakes.go +++ b/internal/task/fakes.go
@@ -110,8 +110,11 @@ func NewFakeGerrit(t *testing.T, repos ...*FakeRepo) *FakeGerrit { result := &FakeGerrit{ - repos: make(map[string]*FakeRepo), - changes: make(map[string]string), + repos: make(map[string]*FakeRepo), + changes: make(map[string]string), + cls: make(map[string]*gerrit.ChangeInfo), + commitMessages: make(map[string]string), + clProjects: make(map[string]string), } mux := http.NewServeMux() mux.HandleFunc("GET /a/{repo}/+archive/{archive}", result.serveArchive) // Serve a revision tarball (.tar.gz) like Gerrit does. @@ -134,6 +137,12 @@ repos map[string]*FakeRepo // Repo name → repo. changesMu sync.Mutex changes map[string]string // Change ID → commit hash. + + // CL state tracking for security release tests. Populated via AddChange. + cls map[string]*gerrit.ChangeInfo // CL ID → state. + commitMessages map[string]string // CL ID → commit message. + clProjects map[string]string // CL ID → project name. + nextCL int // Counter for generated cherry-pick CL IDs. } type FakeRepo struct { @@ -494,6 +503,22 @@ g.changesMu.Unlock() } +// AddChange registers a CL for stateful tracking. Methods like GetChange, +// SubmitChange, MoveChange, etc. will use this state when the CL is found; +// otherwise they fall back to their default stub behavior. +// +// If ci is nil, only the commit message and project are updated (the existing +// ChangeInfo, if any, is preserved). +func (g *FakeGerrit) AddChange(project string, id string, ci *gerrit.ChangeInfo, commitMsg string) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + if ci != nil { + g.cls[id] = ci + } + g.commitMessages[id] = commitMsg + g.clProjects[id] = project +} + func (g *FakeGerrit) Submitted(ctx context.Context, changeID, baseCommit string) (string, bool, error) { g.changesMu.Lock() commit, ok := g.changes[changeID] @@ -702,40 +727,143 @@ } } -func (*FakeGerrit) QueryChanges(_ context.Context, query string) ([]*gerrit.ChangeInfo, error) { - return nil, nil +func (g *FakeGerrit) QueryChanges(_ context.Context, query string) ([]*gerrit.ChangeInfo, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + if len(g.cls) == 0 { + return nil, nil + } + var wantBranch, wantChangeID string + for _, tok := range strings.Fields(query) { + if rest, ok := strings.CutPrefix(tok, "branch:"); ok { + wantBranch = rest + } + if rest, ok := strings.CutPrefix(tok, "change:"); ok { + wantChangeID = rest + } + } + var results []*gerrit.ChangeInfo + for _, ci := range g.cls { + if ci.Status == "ABANDONED" { + continue + } + if wantBranch != "" && ci.Branch != wantBranch { + continue + } + if wantChangeID != "" && ci.ChangeID != wantChangeID { + continue + } + results = append(results, ci) + } + return results, nil } func (*FakeGerrit) SetHashtags(_ context.Context, changeID string, _ gerrit.HashtagsInput) error { return fmt.Errorf("pretend that SetHashtags failed") } -func (*FakeGerrit) GetChange(_ context.Context, _ string, _ ...gerrit.QueryChangesOpt) (*gerrit.ChangeInfo, error) { - return nil, nil +func (g *FakeGerrit) GetChange(_ context.Context, changeID string, _ ...gerrit.QueryChangesOpt) (*gerrit.ChangeInfo, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + ci, ok := g.cls[changeID] + if !ok { + return nil, nil + } + return ci, nil } -func (*FakeGerrit) SubmitChange(ctx context.Context, changeID string) (gerrit.ChangeInfo, error) { - return gerrit.ChangeInfo{}, nil +func (g *FakeGerrit) SubmitChange(_ context.Context, changeID string) (gerrit.ChangeInfo, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + ci, ok := g.cls[changeID] + if !ok { + return gerrit.ChangeInfo{}, nil + } + if ci.Status == gerrit.ChangeStatusMerged { + return gerrit.ChangeInfo{}, NewGerritHTTPError(http.StatusConflict, "change is merged\n") + } + project := g.clProjects[changeID] + repo, err := g.repo(project) + if err != nil { + return gerrit.ChangeInfo{}, err + } + repo.CommitOnBranchWithMessage(ci.Branch, g.commitMessages[changeID], map[string]string{"security-" + changeID + ".txt": "fix from " + changeID}) + ci.Status = gerrit.ChangeStatusMerged + ci.Submittable = false + return *ci, nil } -func (*FakeGerrit) CreateCherryPick(ctx context.Context, changeID string, branch string, message string) (gerrit.ChangeInfo, bool, error) { - return gerrit.ChangeInfo{}, false, nil +func (g *FakeGerrit) CreateCherryPick(_ context.Context, changeID string, branch string, message string) (gerrit.ChangeInfo, bool, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + orig, ok := g.cls[changeID] + if !ok { + return gerrit.ChangeInfo{}, false, nil + } + g.nextCL++ + cpID := fmt.Sprintf("cp-%d", g.nextCL) + cp := &gerrit.ChangeInfo{ + ID: cpID, + ChangeID: orig.ChangeID, + ChangeNumber: g.nextCL, + Branch: branch, + Status: "NEW", + Submittable: true, + Mergeable: true, + } + g.cls[cpID] = cp + g.commitMessages[cpID] = message + g.clProjects[cpID] = g.clProjects[changeID] + return *cp, false, nil } -func (*FakeGerrit) MoveChange(ctx context.Context, changeID string, branch string) (gerrit.ChangeInfo, error) { - return gerrit.ChangeInfo{}, nil +func (g *FakeGerrit) MoveChange(_ context.Context, changeID string, branch string) (gerrit.ChangeInfo, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + ci, ok := g.cls[changeID] + if !ok { + return gerrit.ChangeInfo{}, nil + } + if ci.Status == gerrit.ChangeStatusMerged { + return gerrit.ChangeInfo{}, NewGerritHTTPError(http.StatusConflict, "Change is merged\n") + } + if ci.Branch == branch { + return gerrit.ChangeInfo{}, NewGerritHTTPError(http.StatusConflict, "Change is already destined for the specified branch\n") + } + ci.Branch = branch + return *ci, nil } -func (*FakeGerrit) RebaseChange(ctx context.Context, changeID string, baseRev string) (gerrit.ChangeInfo, error) { - return gerrit.ChangeInfo{}, nil +func (g *FakeGerrit) RebaseChange(_ context.Context, changeID string, baseRev string) (gerrit.ChangeInfo, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + ci, ok := g.cls[changeID] + if !ok { + return gerrit.ChangeInfo{}, nil + } + if ci.Status == gerrit.ChangeStatusMerged { + return gerrit.ChangeInfo{}, NewGerritHTTPError(http.StatusConflict, fmt.Sprintf("Change %s is merged\n", changeID)) + } + return *ci, nil } -func (*FakeGerrit) GetRevisionActions(ctx context.Context, changeID, revision string) (map[string]*gerrit.ActionInfo, error) { - return map[string]*gerrit.ActionInfo{}, nil +func (g *FakeGerrit) GetRevisionActions(_ context.Context, changeID, revision string) (map[string]*gerrit.ActionInfo, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + ci, ok := g.cls[changeID] + if !ok { + return map[string]*gerrit.ActionInfo{}, nil + } + if ci.Status == gerrit.ChangeStatusMerged { + return map[string]*gerrit.ActionInfo{}, nil + } + return map[string]*gerrit.ActionInfo{"submit": {Enabled: true}}, nil } -func (*FakeGerrit) GetCommitMessage(ctx context.Context, changeID string) (string, error) { - return "", nil +func (g *FakeGerrit) GetCommitMessage(_ context.Context, changeID string) (string, error) { + g.changesMu.Lock() + defer g.changesMu.Unlock() + return g.commitMessages[changeID], nil } // NewFakeSignService returns a fake signing service that can sign PKGs, MSIs,
diff --git a/internal/task/privx.go b/internal/task/privx.go index b78485f..19f191a 100644 --- a/internal/task/privx.go +++ b/internal/task/privx.go
@@ -6,12 +6,10 @@ import ( "bytes" - "context" "errors" "fmt" "net/http" "net/mail" - "path" "regexp" "slices" "strings" @@ -26,45 +24,8 @@ "golang.org/x/mod/semver" "golang.org/x/sync/errgroup" "golang.org/x/vulndb/report" - yaml "gopkg.in/yaml.v3" ) -// Security release parameter definitions. -var ( - SecurityMilestoneParameter = wf.ParamDef[string]{ - Name: "Release Milestone", - ParamType: wf.BasicString, - Doc: `Release Milestone is the security-metadata milestone for the security patch(es) being included in a Go release. - -You can check with the security release coordinator for this release to confirm this input.`, - Example: "123456", - Check: func(num string) error { - if !numOnlyRE.MatchString(num) { - return errors.New("milestone number must contain only numbers") - } - return nil - }, - } - numOnlyRE = regexp.MustCompile(`^\d+$`) -) - -func fetchReleaseMilestone(ctx context.Context, private GerritClient, milestoneNum string) (relmeta.ReleaseMilestone, error) { - const project = "security-metadata" - head, err := private.ReadBranchHead(ctx, project, "main") - if err != nil { - return relmeta.ReleaseMilestone{}, err - } - b, err := private.ReadFile(ctx, project, head, path.Join("data", "milestones", milestoneNum+".yaml")) - if err != nil { - return relmeta.ReleaseMilestone{}, err - } - var rm relmeta.ReleaseMilestone - if err := yaml.Unmarshal(b, &rm); err != nil { - return relmeta.ReleaseMilestone{}, fmt.Errorf("cannot YAML unmarshal the milestone: %v", err) - } - return rm, nil -} - type PrivXPatch struct { Git *Git PublicGerrit GerritClient @@ -495,10 +456,19 @@ var vulndbReviewers = []string{"neal@golang.org", "nsh@golang.org"} func (x *PrivXPatch) UpdateGitHubIssues(ctx *wf.TaskContext, rm *relmeta.ReleaseMilestone) error { + return UpdateGitHubIssues(ctx, x.GitHub, rm) +} + +// UpdateGitHubIssues updates the body of each security issue in rm +// with a disclosure notice. It is a no-op when rm is nil or has no patches. +func UpdateGitHubIssues(ctx *wf.TaskContext, gh GitHubClientInterface, rm *relmeta.ReleaseMilestone) error { + if rm == nil { + return nil + } for _, p := range rm.Patches { body := fmt.Sprintf(disclosureBody, p.ReleaseNote, p.Track, p.ID) req := &github.IssueRequest{Body: &body} - if _, _, err := x.GitHub.EditIssue(ctx, "golang", "go", int(p.GitHubIssueID), req); err != nil { + if _, _, err := gh.EditIssue(ctx, "golang", "go", int(p.GitHubIssueID), req); err != nil { return err } ctx.Printf("Updated https://go.dev/issue/%d", p.GitHubIssueID)
diff --git a/internal/task/privx_test.go b/internal/task/privx_test.go index 8ee3b06..0f3ba39 100644 --- a/internal/task/privx_test.go +++ b/internal/task/privx_test.go
@@ -16,6 +16,7 @@ "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/google/go-github/v74/github" "golang.org/x/build/gerrit" wf "golang.org/x/build/internal/workflow" @@ -349,8 +350,8 @@ t.Fatal(err) } - if !reflect.DeepEqual(announcementHeader, p.AnnounceMailHeader) { - t.Errorf("announcement header: got %#v, want %#v", announcementHeader, p.AnnounceMailHeader) + if diff := cmp.Diff(p.AnnounceMailHeader, announcementHeader); diff != "" { + t.Errorf("announcement header mismatch (-want +got):\n%s", diff) } wantSubject := `[security] Vulnerabilities in golang.org/x/net` if announcementMessage.Subject != wantSubject { @@ -394,8 +395,8 @@ Cheers, Go Security team ` - if announcementMessage.BodyText != wantText { - t.Errorf("announcement text:\ngot:\n%s\nwant:\n%s", announcementMessage.BodyText, wantText) + if diff := cmp.Diff(wantText, announcementMessage.BodyText); diff != "" { + t.Errorf("announcement text mismatch (-want +got):\n%s", diff) } wantHTML := `<p>Hello gophers,</p> @@ -421,8 +422,8 @@ <p>Cheers,<br> Go Security team</p> ` - if announcementMessage.BodyHTML != wantHTML { - t.Errorf("announcement HTML:\ngot:\n%s\nwant:\n%s", announcementMessage.BodyHTML, wantHTML) + if diff := cmp.Diff(wantHTML, announcementMessage.BodyHTML); diff != "" { + t.Errorf("announcement HTML mismatch (-want +got):\n%s", diff) } // Verify that vuln reports were submitted to vulndb. @@ -463,7 +464,11 @@ if mod.Module != "golang.org/x/net" { t.Errorf("patch %d: module = %q, want %q", p.ID, mod.Module, "golang.org/x/net") } - if len(mod.Packages) != 1 || mod.Packages[0].Package != p.Package { + if len(mod.Packages) != 1 { + t.Errorf("patch %d: got %d packages, want 1", p.ID, len(mod.Packages)) + continue + } + if mod.Packages[0].Package != p.Package { t.Errorf("patch %d: package = %q, want %q", p.ID, mod.Packages[0].Package, p.Package) } @@ -477,8 +482,8 @@ } // Credits. - if !reflect.DeepEqual(vr.Credits, p.Credits) { - t.Errorf("patch %d: credits = %v, want %v", p.ID, vr.Credits, p.Credits) + if diff := cmp.Diff(p.Credits, vr.Credits); diff != "" { + t.Errorf("patch %d: credits mismatch (-want +got):\n%s", p.ID, diff) } // Description must be non-empty. @@ -628,6 +633,59 @@ } } +func TestUpdateGitHubIssues(t *testing.T) { + ctx := &wf.TaskContext{ + Context: context.Background(), + Logger: &testLogger{t: t}, + } + + t.Run("nil milestone", func(t *testing.T) { + if err := UpdateGitHubIssues(ctx, nil, nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("patches", func(t *testing.T) { + fakeGH := &FakeGitHub{} + rm := &relmeta.ReleaseMilestone{ + Patches: []*relmeta.SecurityPatch{ + { + ID: 100, + Track: relmeta.Private, + Package: "crypto/tls", + ReleaseNote: "crypto/tls: bad handshake.\n\nA crafted ClientHello causes a panic.", + GitHubIssueID: 11111, + }, + { + ID: 200, + Track: relmeta.Public, + Package: "net/http", + ReleaseNote: "net/http: request smuggling.\n\nMalformed headers bypass validation.", + GitHubIssueID: 22222, + }, + }, + } + if err := UpdateGitHubIssues(ctx, fakeGH, rm); err != nil { + t.Fatalf("UpdateGitHubIssues: %v", err) + } + for _, p := range rm.Patches { + issue, ok := fakeGH.Issues[int(p.GitHubIssueID)] + if !ok { + t.Errorf("patch %d: GitHub issue %d not found", p.ID, p.GitHubIssueID) + continue + } + body := issue.GetBody() + if !strings.Contains(body, p.ReleaseNote) { + t.Errorf("patch %d: issue body missing release note", p.ID) + } + wantTrailer := fmt.Sprintf("This was a %s issue originally tracked in http://b/%d.", p.Track, p.ID) + if !strings.Contains(body, wantTrailer) { + t.Errorf("patch %d: issue body missing trailer, got:\n%s", p.ID, body) + } + } + }) +} + func TestRepoName(t *testing.T) { tests := []struct { name string
diff --git a/internal/task/security_release_coalesce.go b/internal/task/security_release_coalesce.go deleted file mode 100644 index 9023877..0000000 --- a/internal/task/security_release_coalesce.go +++ /dev/null
@@ -1,309 +0,0 @@ -// Copyright 2024 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package task - -import ( - "errors" - "fmt" - goversion "go/version" - "net/http" - "regexp" - "strings" - "time" - - "golang.org/x/build/gerrit" - "golang.org/x/build/internal/relui/groups" - wf "golang.org/x/build/internal/workflow" - "golang.org/x/build/relmeta" -) - -// SecurityReleaseCoalesceTask is the workflow used to preparing patches for -// minor security releases. The workflow is described in detail in -// go/go-security-release-workflow. -// -// In short, this workflow: -// 1. Checks that all patches are ready, indicated by two Code-Review+2's labels -// and a Security-Patch-Ready+1 label (this is checked via submit requirements -// rather than directly inspecting the labels) and lack of merge conflicts -// 2. Creates a new branch from master HEAD -// 3. Moves all patches from master onto the new branch -// 4. Submits the rebased patches -// 5. Create internal release branches -// 6. Creates cherry-picks of the submitted patches onto the release branches, -// setting Commit-Queue+1 -type SecurityReleaseCoalesceTask struct { - PrivateGerrit GerritClient - Version *VersionTasks -} - -func (x *SecurityReleaseCoalesceTask) NewDefinition() *wf.Definition { - var ( - wd = wf.New(wf.ACL{Groups: []string{groups.SecurityTeam}}) - milestoneNum = wf.Param(wd, SecurityMilestoneParameter) - ) - // fetch all non-PUBLIC security patch changelists - clNums := wf.Task1(wd, "Get private changelists from security-metadata", x.GetPrivateChangelists, milestoneNum) - // check eligibility of specified changelists - cls := wf.Task1(wd, "Check changes", x.CheckChanges, clNums) - // look up branch names - branchInfo := wf.Task0(wd, "Get branch names", x.GetBranchNames, wf.After(cls)) - // create checkpoint branch - checkpointBranch := wf.Task1(wd, "Create checkpoint branch", x.CreateCheckpoint, branchInfo) - // rebase changes to checkpoint branch - cls = wf.Task2(wd, "Move changes onto checkpoint branch", x.MoveAndRebaseChanges, checkpointBranch, cls) - // wait for changes to be submittable, and then submit them - cls = wf.Task1(wd, "Await submissions", x.WaitAndSubmit, cls) - // create internal release branches - internalReleaseBranches := wf.Task1(wd, "Create internal release branches", x.CreateInternalReleaseBranches, branchInfo, wf.After(cls)) - // create cherry-picks to internal release branches - cherryPicks := wf.Task2(wd, "Create cherry-picks", x.CreateCherryPicks, internalReleaseBranches, cls) - wf.Output(wd, "Cherry-picks", cherryPicks) - - return wd -} - -type branchInfo struct { - CheckpointName string - PublicReleaseBranches []string -} - -func (x *SecurityReleaseCoalesceTask) GetBranchNames(ctx *wf.TaskContext) (branchInfo, error) { - // TODO: consider using the release milestone to derive - // the active version patch and backports? - currentMajor, _, err := x.Version.GetCurrentMajor(ctx) - if err != nil { - return branchInfo{}, err - } - nextMinors, err := x.Version.GetNextMinorVersions(ctx, []int{currentMajor, currentMajor - 1}) - if err != nil { - return branchInfo{}, err - } - switch _, err := x.Version.Gerrit.ReadBranchHead(ctx, "go", fmt.Sprintf("release-branch.go1.%d", currentMajor+1)); { - case errors.Is(err, gerrit.ErrResourceNotExist): - // The next release branch hasn't been cut yet. Include release branches for minors only. - return branchInfo{ - CheckpointName: fmt.Sprintf("%s-%s-checkpoint", nextMinors[0], nextMinors[1]), - PublicReleaseBranches: []string{ - fmt.Sprintf("release-branch.%s", nextMinors[0]), - fmt.Sprintf("release-branch.%s", nextMinors[1]), - }, - }, nil - case err == nil: - // Include release branches for the minors and the next release candidate. - nextRC, err := x.Version.GetNextVersion(ctx, currentMajor+1, KindRC) - if err != nil { - return branchInfo{}, err - } - return branchInfo{ - CheckpointName: fmt.Sprintf("%s-%s-%s-checkpoint", nextRC, nextMinors[0], nextMinors[1]), - PublicReleaseBranches: []string{ - fmt.Sprintf("release-branch.%s", nextRC), - fmt.Sprintf("release-branch.%s", nextMinors[0]), - fmt.Sprintf("release-branch.%s", nextMinors[1]), - }, - }, nil - default: - return branchInfo{}, err - } -} - -func (x *SecurityReleaseCoalesceTask) GetPrivateChangelists(ctx *wf.TaskContext, milestoneNum string) (clNums []string, _ error) { - rm, err := fetchReleaseMilestone(ctx, x.PrivateGerrit, milestoneNum) - if err != nil { - return nil, err - } - for _, patch := range rm.Patches { - if patch.Track == relmeta.Public { - continue - } - for _, url := range patch.Changelists { - _, num, _ := strings.Cut(url, "/+/") - clNums = append(clNums, num) - } - } - return clNums, nil -} - -func (x *SecurityReleaseCoalesceTask) CheckChanges(ctx *wf.TaskContext, clNums []string) ([]*gerrit.ChangeInfo, error) { - var ( - cls []*gerrit.ChangeInfo - lintErrs []error - ) - for _, num := range clNums { - ci, err := x.PrivateGerrit.GetChange(ctx, num, gerrit.QueryChangesOpt{Fields: []string{"SUBMITTABLE"}}) - if err != nil { - return nil, err - } - if !ci.Submittable { - return nil, fmt.Errorf("Change %s is not submittable", internalGerritChangeURL(num)) - } - ra, err := x.PrivateGerrit.GetRevisionActions(ctx, num, "current") - if err != nil { - return nil, err - } - if ra["submit"] == nil || !ra["submit"].Enabled { - return nil, fmt.Errorf("Change %s is not submittable", internalGerritChangeURL(num)) - } - cm, err := x.PrivateGerrit.GetCommitMessage(ctx, num) - if err != nil { - return nil, err - } - if !cveRE.MatchString(cm) { - lintErrs = append(lintErrs, fmt.Errorf("Change %s is missing CVE reference", internalGerritChangeURL(num))) - } - if !githubIssueRE.MatchString(cm) { - lintErrs = append(lintErrs, fmt.Errorf("Change %s is missing GitHub issue reference", internalGerritChangeURL(num))) - } - cls = append(cls, ci) - } - - return cls, errors.Join(lintErrs...) -} - -var ( - cveRE = regexp.MustCompile(`(?m)^Fixes CVE-\d{4}-\d+`) - githubIssueRE = regexp.MustCompile(`(?m)^Fixes (?:golang/go)?#(\d+)`) -) - -func (x *SecurityReleaseCoalesceTask) CreateCheckpoint(ctx *wf.TaskContext, bi branchInfo) (string, error) { - publicHead, err := x.PrivateGerrit.ReadBranchHead(ctx, "go", "public") - if err != nil { - return "", err - } - if _, err := x.PrivateGerrit.CreateBranch(ctx, "go", bi.CheckpointName, gerrit.BranchInput{Revision: publicHead}); err != nil { - return "", err - } - return bi.CheckpointName, nil -} - -func (x *SecurityReleaseCoalesceTask) MoveAndRebaseChanges(ctx *wf.TaskContext, checkpointBranch string, cls []*gerrit.ChangeInfo) ([]*gerrit.ChangeInfo, error) { - for i, ci := range cls { - movedCI, err := x.PrivateGerrit.MoveChange(ctx, ci.ID, checkpointBranch) - if err != nil { - // In case we need to re-run the Move step, tolerate the case where the change - // is already on the branch. - var httpErr *gerrit.HTTPError - if !errors.As(err, &httpErr) || httpErr.Res.StatusCode != http.StatusConflict || string(httpErr.Body) != "Change is already destined for the specified branch\n" { - return nil, err - } - } else { - cls[i] = &movedCI - } - rebasedCI, err := x.PrivateGerrit.RebaseChange(ctx, movedCI.ID, "") - if err != nil { - var httpErr *gerrit.HTTPError - if !errors.As(err, &httpErr) || httpErr.Res.StatusCode != http.StatusConflict || string(httpErr.Body) != "Change is already up to date.\n" { - return nil, err - } - } else { - cls[i] = &rebasedCI - } - } - return cls, nil -} - -func (x *SecurityReleaseCoalesceTask) WaitAndSubmit(ctx *wf.TaskContext, cls []*gerrit.ChangeInfo) ([]*gerrit.ChangeInfo, error) { - if _, err := AwaitCondition(ctx, time.Second*10, func() (string, bool, error) { - unsubmitted := len(cls) - - for i, change := range cls { - if change.Status == gerrit.ChangeStatusMerged { - unsubmitted-- - continue - } - - ci, err := x.PrivateGerrit.GetChange(ctx, change.ID, gerrit.QueryChangesOpt{Fields: []string{"SUBMITTABLE"}}) - if err != nil { - return "", false, err - } - - if !ci.Submittable { - continue - } - - submitted, err := x.PrivateGerrit.SubmitChange(ctx, ci.ID) - if err != nil { - return "", false, err - } - - cls[i] = &submitted - unsubmitted-- - } - - if unsubmitted == 0 { - return "", true, nil - } - return "", false, nil - }); err != nil { - return nil, err - } - - return cls, nil -} - -// majorFromMinor converts a release branch name from its minor version form to -// its major version form (i.e., release-branch.go1.2.3 to release-branch.go1.2). -func majorFromMinor(branch string) string { - stripped := strings.TrimPrefix(branch, "release-branch.") - major := goversion.Lang(stripped) - return "release-branch." + major -} - -var internalReleaseBranchPrefix = "internal-" - -func (x *SecurityReleaseCoalesceTask) CreateInternalReleaseBranches(ctx *wf.TaskContext, bi branchInfo) ([]string, error) { - // TODO: update step to commit the metadata - // about the submitted changes and their - // branch hashes to security-metadata. - var internalBranches []string - for _, next := range bi.PublicReleaseBranches { - publicHead, err := x.PrivateGerrit.ReadBranchHead(ctx, "go", majorFromMinor(next)) - if err != nil { - return nil, err - } - internalReleaseBranch := internalReleaseBranchPrefix + next - if _, err := x.PrivateGerrit.CreateBranch(ctx, "go", internalReleaseBranch, gerrit.BranchInput{Revision: publicHead}); err != nil { - return nil, err - } - internalBranches = append(internalBranches, internalReleaseBranch) - } - return internalBranches, nil -} - -func (x *SecurityReleaseCoalesceTask) CreateCherryPicks(ctx *wf.TaskContext, releaseBranches []string, cls []*gerrit.ChangeInfo) (map[string][]string, error) { - // TODO: this currently assumes we want to cherry-pick everything to all - // branches, which is _normally_ the case, but sometimes is not accurate. We - // can manually just abandon cherry-picks we don't care about, but probably - // we should have a way to indicate which branches we want each patch - // cherry-picked onto. - - cherryPicks := map[string][]string{} - for _, ci := range cls { - for _, releaseBranch := range releaseBranches { - commitMessage, err := x.PrivateGerrit.GetCommitMessage(ctx, ci.ID) - if err != nil { - return nil, err - } - // TODO: might be cleaner to just pass this information from CreateInternalReleaseBranches - commitMessage = fmt.Sprintf("[%s] %s", majorFromMinor(strings.TrimPrefix(releaseBranch, internalReleaseBranchPrefix)), commitMessage) - - cpCI, conflicts, err := x.PrivateGerrit.CreateCherryPick(ctx, ci.ID, releaseBranch, commitMessage) - if err != nil { - return nil, err - } - if conflicts { - ctx.Printf("Cherry-pick of %s has merge conflicts against %s: %s", internalGerritChangeURL(ci.ChangeNumber), releaseBranch, internalGerritChangeURL(cpCI.ChangeNumber)) - } - cherryPicks[releaseBranch] = append(cherryPicks[releaseBranch], internalGerritChangeURL(cpCI.ChangeNumber)) - } - } - return cherryPicks, nil -} - -// internalGerritChangeURL can take either a int or string and return the -// relevant CL URL for a change number. -func internalGerritChangeURL[T int | string](clNum T) string { - return fmt.Sprintf("https://go-internal-review.git.corp.google.com/c/go/+/%v", clNum) -}
diff --git a/internal/task/security_release_coalesce_test.go b/internal/task/security_release_coalesce_test.go deleted file mode 100644 index adada29..0000000 --- a/internal/task/security_release_coalesce_test.go +++ /dev/null
@@ -1,281 +0,0 @@ -// Copyright 2024 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package task - -import ( - "context" - "crypto/rand" - "errors" - "fmt" - "path" - "slices" - "strings" - "testing" - "time" - - "golang.org/x/build/gerrit" - wf "golang.org/x/build/internal/workflow" -) - -type fakeCoalesceGerrit struct { - *FakeGerrit - - changes map[string]*gerrit.ChangeInfo - commitMessages map[string]string - cherryPicks map[string][]cherryPickedCommit -} - -type cherryPickedCommit struct { - changeID string - message string -} - -func (g *fakeCoalesceGerrit) GetChange(_ context.Context, changeID string, _ ...gerrit.QueryChangesOpt) (*gerrit.ChangeInfo, error) { - ci, ok := g.changes[changeID] - if !ok { - return nil, errors.New("GetChange: not found") - } - return ci, nil -} - -func (g *fakeCoalesceGerrit) GetRevisionActions(_ context.Context, changeID string, revision string) (map[string]*gerrit.ActionInfo, error) { - if _, ok := g.changes[changeID]; !ok { - return nil, nil - } - action := &gerrit.ActionInfo{Enabled: true} - return map[string]*gerrit.ActionInfo{"submit": action}, nil -} - -func (g *fakeCoalesceGerrit) MoveChange(ctx context.Context, changeID string, branch string) (gerrit.ChangeInfo, error) { - ci, ok := g.changes[changeID] - if !ok { - return gerrit.ChangeInfo{}, errors.New("MoveChange: not found") - } - ci.Branch = branch - return *ci, nil -} - -func (g *fakeCoalesceGerrit) SubmitChange(ctx context.Context, changeID string) (gerrit.ChangeInfo, error) { - ci, ok := g.changes[changeID] - if !ok { - return gerrit.ChangeInfo{}, errors.New("SubmitChange: not found") - } - - r := make([]byte, 4) - rand.Read(r) - g.repos["go"].CommitOnBranch(ci.Branch, map[string]string{"patch": fmt.Sprintf("%x", r)}) - - ci.Status = gerrit.ChangeStatusMerged - - return *ci, nil -} - -func (g *fakeCoalesceGerrit) RebaseChange(ctx context.Context, changeID string, baseRev string) (gerrit.ChangeInfo, error) { - return *g.changes[changeID], nil -} - -func (g *fakeCoalesceGerrit) CreateCherryPick(ctx context.Context, changeID string, branch string, message string) (gerrit.ChangeInfo, bool, error) { - ci, ok := g.changes[changeID] - if !ok { - return gerrit.ChangeInfo{}, false, errors.New("CreateCherryPick: not found") - } - - g.cherryPicks[branch] = append(g.cherryPicks[branch], cherryPickedCommit{ci.ChangeID, message}) - return *ci, false, nil -} - -func (g *fakeCoalesceGerrit) GetCommitMessage(ctx context.Context, changeID string) (string, error) { - return g.commitMessages[changeID], nil -} - -type securityVersionClient struct { - GerritClient - tags, branches []string -} - -func (c *securityVersionClient) ListTags(_ context.Context, project string) ([]string, error) { - if project != "go" { - return nil, nil - } - return c.tags, nil -} - -func (c *securityVersionClient) GetTag(_ context.Context, project, tag string) (gerrit.TagInfo, error) { - if project != "go" { - return gerrit.TagInfo{}, gerrit.ErrResourceNotExist - } - if slices.Contains(c.tags, tag) { - return gerrit.TagInfo{Created: gerrit.TimeStamp(time.Now())}, nil - } - return gerrit.TagInfo{}, gerrit.ErrResourceNotExist -} - -func (c *securityVersionClient) ReadBranchHead(_ context.Context, project, branch string) (string, error) { - if project != "go" { - return "", gerrit.ErrResourceNotExist - } - if !slices.Contains(c.branches, branch) { - return "", gerrit.ErrResourceNotExist - } - return branch + "-head", nil -} - -func TestSecurityReleaseCoalesceTask(t *testing.T) { - t.Run("minors only", func(t *testing.T) { - testSecurityReleaseCoalesceTask(t, false) - }) - t.Run("minors with RC", func(t *testing.T) { - testSecurityReleaseCoalesceTask(t, true) - }) -} - -func testSecurityReleaseCoalesceTask(t *testing.T, withNextReleaseBranch bool) { - publicTags := []string{"go1.3", "go1.3.1", "go1.4", "go1.4.1"} - publicBranches := []string{"release-branch.go1.3", "release-branch.go1.4"} - if withNextReleaseBranch { - publicBranches = append(publicBranches, "release-branch.go1.5") - } - privGoRepo, smRepo := NewFakeRepo(t, "go"), NewFakeRepo(t, "security-metadata") - - // Commit a test generated milestone to security-metadata. - head := smRepo.History()[0] - smRepo.Branch("main", head) - smRepo.CommitOnBranch("main", map[string]string{ - path.Join("data", "milestones", "99915010.yaml"): `id: 99915010 -security_patches: - - id: 20024001 - package: runtime - track: PUBLIC - changelists: - - https://go.dev/cl/123456 - target_releases: - - go1.3.1 - - go1.4.1 - - id: 40027190 - package: runtime - track: PRIVATE - changelists: - - https://go-internal-review.git.corp.google.com/c/security-metadata/+/1234 - - https://go-internal-review.git.corp.google.com/c/security-metadata/+/5678 - target_releases: - - go1.3.1 - - go1.4.1`}) - - privGerrit := &fakeCoalesceGerrit{ - FakeGerrit: NewFakeGerrit(t, privGoRepo, smRepo), - cherryPicks: map[string][]cherryPickedCommit{}, - commitMessages: map[string]string{}, - } - task := &SecurityReleaseCoalesceTask{ - PrivateGerrit: privGerrit, - Version: &VersionTasks{ - Gerrit: &securityVersionClient{tags: publicTags, branches: publicBranches}, - GoProject: "go", - }, - } - - privGerrit.changes = map[string]*gerrit.ChangeInfo{ - "1234": { - ID: "1234", - ChangeID: "1234", - ChangeNumber: 1234, - Branch: "public", - Submittable: true, - Mergeable: true, - }, - "5678": { - ID: "5678", - ChangeID: "5678", - ChangeNumber: 5678, - Branch: "public", - Submittable: true, - Mergeable: true, - }, - } - - privGerrit.commitMessages = map[string]string{ - "1234": commitMsg1234, - "5678": commitMsg5678, - } - - head = privGoRepo.History()[0] - privGoRepo.Branch("public", head) - privGoRepo.Branch("release-branch.go1.3", head) - privGoRepo.Branch("release-branch.go1.4", head) - if withNextReleaseBranch { - privGoRepo.Branch("release-branch.go1.5", head) - } - - wd := task.NewDefinition() - params := map[string]any{"Release Milestone": "99915010"} - w, err := wf.Start(wd, params) - if err != nil { - t.Fatal(err) - } - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - _, err = w.Run(&wf.TaskContext{Context: ctx, Logger: &testLogger{t: t}}, &verboseListener{t: t}) - if err != nil { - t.Fatal(err) - } - - // Check checkpoint branch has the expected number of submitted changes - checkpointBranch := "go1.4.2-go1.3.2-checkpoint" - if withNextReleaseBranch { - checkpointBranch = "go1.5rc1-go1.4.2-go1.3.2-checkpoint" - } - commits := len(strings.Split(string(privGoRepo.runGit("log", checkpointBranch, "--format=%H")), "\n")) - 1 - if commits != 3 { - t.Errorf("unexpected number of commits on checkpoint branch: got %d, want 3", commits) - } - - // Check each internal release branch has the expected cherry-picks - expected := map[string][]cherryPickedCommit{ - "internal-release-branch.go1.4.2": { - { - changeID: "1234", - message: "[release-branch.go1.4] " + commitMsg1234, - }, - { - changeID: "5678", - message: "[release-branch.go1.4] " + commitMsg5678, - }, - }, - "internal-release-branch.go1.3.2": { - { - changeID: "1234", - message: "[release-branch.go1.3] " + commitMsg1234, - }, - { - changeID: "5678", - message: "[release-branch.go1.3] " + commitMsg5678, - }, - }, - } - - if withNextReleaseBranch { - expected["internal-release-branch.go1.5rc1"] = []cherryPickedCommit{ - { - changeID: "1234", - message: "[release-branch.go1.5] " + commitMsg1234, - }, - { - changeID: "5678", - message: "[release-branch.go1.5] " + commitMsg5678, - }, - } - } - - for branch, commits := range privGerrit.cherryPicks { - if !slices.Equal(commits, expected[branch]) { - t.Errorf("unexpected cherry-picks on %s: got %s, want %s", branch, commits, expected[branch]) - } - } -} - -const ( - commitMsg1234 = "go2/types: type confusion inverts flux capacitor\n\nFixes CVE-1985-0703\nFixes golang/go#1" - commitMsg5678 = "cmd/compile: import rustc to fix go\n\nFixes CVE-1970-0001\nFixes #4294967296" -)