diff --git a/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx b/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx index 6d907a73..64112471 100644 --- a/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx +++ b/docs/src/content/docs/integrations/deploy-approvals/circleci.mdx @@ -240,8 +240,11 @@ sequenceDiagram CircleCI-->>Gateway: 202 Accepted ``` -The approval runs as a background job a moment after the admin approves. If the workflow was rerun, the gateway -approves the hold job in the newest workflow with the same name in that pipeline. +The approval runs as a background job a moment after the admin approves. It approves the hold job in the workflow +that requested the approval while that job is on hold. If it isn't, and the workflow was rerun, the gateway +approves the hold job in the newest rerun: the newest workflow in that pipeline with the same name, created after +the original. The gateway only looks at the pipeline's workflow list to find a rerun, because CircleCI sometimes +leaves workflows out of that list for a while. The `ci_metadata` comes from the CI job that requested the approval, so the gateway doesn't trust it on its own: the hold job is only approved when the workflow's pipeline is building the approved commit of the app's diff --git a/internal/gateway/circleci/client.go b/internal/gateway/circleci/client.go index fb1f6262..c529b107 100644 --- a/internal/gateway/circleci/client.go +++ b/internal/gateway/circleci/client.go @@ -62,10 +62,10 @@ type ApprovalMetadata struct { ApprovalJobName string `json:"approval_job_name"` } -// ApproveJob approves a pending approval job in a CircleCI workflow. +// ApproveJob approves the hold job named jobName in a CircleCI workflow. // The workflow's pipeline must be building the approved revision of the expected repository; // otherwise ErrPipelineMismatch is returned and nothing is approved. -// It handles workflow reruns by finding the latest workflow with the same name in the pipeline. +// If the workflow was rerun, the hold job in the newest rerun is approved instead (see approvalTarget). func (c *Client) ApproveJob(workflowID, pipelineNumber, jobName string, expect ApprovalExpectation) error { if err := ValidateWorkflowID(workflowID); err != nil { return err @@ -77,13 +77,12 @@ func (c *Client) ApproveJob(workflowID, pipelineNumber, jobName string, expect A return fmt.Errorf("job_name is required") } - // Get the workflow details to find its name and pipeline ID - originalWorkflow, err := c.getWorkflowDetails(workflowID) + original, err := c.getWorkflowDetails(workflowID) if err != nil { return fmt.Errorf("failed to get workflow details: %w", err) } - pipeline, err := c.getPipeline(originalWorkflow.PipelineID) + pipeline, err := c.getPipeline(original.PipelineID) if err != nil { return fmt.Errorf("failed to get pipeline: %w", err) } @@ -91,102 +90,84 @@ func (c *Client) ApproveJob(workflowID, pipelineNumber, jobName string, expect A return err } - // Find the latest workflow with the same name in the pipeline - // Use the pipeline ID (UUID) from the workflow, not the pipeline number - latestWorkflowID, err := c.findLatestWorkflowByName(originalWorkflow.PipelineID, originalWorkflow.Name) + targetWorkflowID, jobID, err := c.approvalTarget(original, jobName) if err != nil { - return fmt.Errorf("failed to find latest workflow: %w", err) + return err } - // Get the workflow jobs to find the approval job ID - workflow, err := c.getWorkflow(latestWorkflowID) - if err != nil { - return fmt.Errorf("failed to get workflow: %w", err) + url := fmt.Sprintf("%s/workflow/%s/approve/%s", c.BaseURL, targetWorkflowID, jobID) + if _, err := c.doRequest(http.MethodPost, url); err != nil { + return err } - // Find the approval job by name - jobID, err := findApprovalJobID(workflow, jobName) + return nil +} + +// approvalTarget returns the workflow and hold job to approve. While the original workflow's hold job is on hold, +// that's the one: CircleCI's pipeline workflow listing sometimes leaves a pipeline's workflows out for a while, so +// the usual case doesn't depend on it. Otherwise the listing is checked for a rerun (a newer workflow with the same +// name), and without one the original's hold job is approved anyway, leaving CircleCI to say why it can't be (for +// example, because it's already approved). +func (c *Client) approvalTarget(original *workflowDetails, jobName string) (workflowID, jobID string, err error) { + job, err := c.findApprovalJob(original.ID, jobName) if err != nil { - return fmt.Errorf("failed to find approval job: %w", err) + return "", "", err } - - // Approve the job - url := fmt.Sprintf("%s/workflow/%s/approve/%s", c.BaseURL, latestWorkflowID, jobID) - if _, err := c.doRequest(http.MethodPost, url); err != nil { - return err + if job.Status == jobStatusOnHold { + return original.ID, job.ID, nil } - return nil + rerunID, err := c.findRerun(original) + if err != nil { + return "", "", fmt.Errorf("failed to look for a rerun of workflow %s: %w", original.ID, err) + } + if rerunID == "" { + return original.ID, job.ID, nil + } + rerunJob, err := c.findApprovalJob(rerunID, jobName) + if err != nil { + return "", "", err + } + return rerunID, rerunJob.ID, nil } -type workflowResponse struct { - ID string `json:"id"` - Jobs []struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - } `json:"jobs"` +// jobStatusOnHold is the status of an approval job that is waiting to be approved. +const jobStatusOnHold = "on_hold" + +type workflowJob struct { + ID string `json:"id"` + Name string `json:"name"` + Type string `json:"type"` + Status string `json:"status"` } -func (c *Client) getWorkflow(workflowID string) (*workflowResponse, error) { +// findApprovalJob returns the approval job named jobName in a workflow. +func (c *Client) findApprovalJob(workflowID, jobName string) (*workflowJob, error) { url := fmt.Sprintf("%s/workflow/%s/job", c.BaseURL, workflowID) body, err := c.doRequest(http.MethodGet, url) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get workflow jobs: %w", err) } var result struct { - Items []struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - } `json:"items"` + Items []workflowJob `json:"items"` } - if err := json.Unmarshal(body, &result); err != nil { - return nil, fmt.Errorf("failed to parse workflow response: %w", err) - } - - workflow := &workflowResponse{ - ID: workflowID, - Jobs: make([]struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - }, len(result.Items)), - } - - for i, item := range result.Items { - workflow.Jobs[i] = struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - ApprovalRequest *string `json:"approval_request_id,omitempty"` - Status string `json:"status"` - }{ - ID: item.ID, - Name: item.Name, - Type: item.Type, - ApprovalRequest: item.ApprovalRequest, - Status: item.Status, - } + return nil, fmt.Errorf("failed to parse workflow jobs: %w", err) } - return workflow, nil + for i := range result.Items { + if result.Items[i].Name == jobName && result.Items[i].Type == "approval" { + return &result.Items[i], nil + } + } + return nil, fmt.Errorf("approval job %q not found in workflow %s", jobName, workflowID) } type workflowDetails struct { ID string `json:"id"` Name string `json:"name"` - ProjectID string `json:"project_id"` PipelineID string `json:"pipeline_id"` - Status string `json:"status"` CreatedAt string `json:"created_at"` } @@ -206,46 +187,40 @@ func (c *Client) getWorkflowDetails(workflowID string) (*workflowDetails, error) return &details, nil } -type pipelineWorkflow struct { - ID string `json:"id"` - Name string `json:"name"` - Status string `json:"status"` - CreatedAt string `json:"created_at"` -} +// findRerun returns the newest workflow in the original's pipeline that has its name and was created after it, or +// "" when there is none. +func (c *Client) findRerun(original *workflowDetails) (string, error) { + originalCreated, err := time.Parse(time.RFC3339, original.CreatedAt) + if err != nil { + return "", fmt.Errorf("failed to parse the workflow's created_at %q: %w", original.CreatedAt, err) + } -// findLatestWorkflowByName finds the most recent workflow with the given name in a pipeline. -// pipelineID must be the UUID pipeline ID, not the pipeline number. -func (c *Client) findLatestWorkflowByName(pipelineID, workflowName string) (string, error) { - url := fmt.Sprintf("%s/pipeline/%s/workflow", c.BaseURL, pipelineID) + url := fmt.Sprintf("%s/pipeline/%s/workflow", c.BaseURL, original.PipelineID) body, err := c.doRequest(http.MethodGet, url) if err != nil { return "", err } var result struct { - Items []pipelineWorkflow `json:"items"` + Items []struct { + ID string `json:"id"` + Name string `json:"name"` + CreatedAt string `json:"created_at"` + } `json:"items"` } if err := json.Unmarshal(body, &result); err != nil { return "", fmt.Errorf("failed to parse pipeline workflows: %w", err) } - // Find the latest workflow with matching name (workflows are ordered by created_at descending) + rerunID, newest := "", originalCreated for _, wf := range result.Items { - if wf.Name == workflowName { - return wf.ID, nil - } - } - - return "", fmt.Errorf("no workflow found with name %q in pipeline %s", workflowName, pipelineID) -} - -func findApprovalJobID(workflow *workflowResponse, jobName string) (string, error) { - for _, job := range workflow.Jobs { - if job.Name == jobName && job.Type == "approval" { - return job.ID, nil + created, err := time.Parse(time.RFC3339, wf.CreatedAt) + if wf.Name != original.Name || wf.ID == original.ID || err != nil || !created.After(newest) { + continue } + rerunID, newest = wf.ID, created } - return "", fmt.Errorf("approval job %q not found in workflow", jobName) + return rerunID, nil } // ValidateMetadata validates that the metadata contains required CircleCI fields. diff --git a/internal/gateway/circleci/client_test.go b/internal/gateway/circleci/client_test.go new file mode 100644 index 00000000..49a40894 --- /dev/null +++ b/internal/gateway/circleci/client_test.go @@ -0,0 +1,188 @@ +package circleci + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +const ( + rerunWorkflowID = "22222222-3333-4444-5555-666666666666" + setupWorkflowID = "33333333-4444-5555-6666-777777777777" + olderWorkflowID = "44444444-5555-6666-7777-888888888888" +) + +// fakeWorkflow is a workflow in the fake pipeline with one approval job, approve_deploy_us, whose ID is +// "hold-" and whose status is hold. +type fakeWorkflow struct { + id, name, createdAt, hold string +} + +// fakeCircleCI serves the workflow, pipeline and job endpoints ApproveJob uses, and records what it approves. +type fakeCircleCI struct { + pipeline fakePipeline + workflows []fakeWorkflow + listing []string // the workflow IDs GET /pipeline/{id}/workflow returns, newest first + + mu sync.Mutex + approvals []string // "/" + listings int +} + +func (f *fakeCircleCI) workflow(id string) (fakeWorkflow, bool) { + for _, wf := range f.workflows { + if wf.id == id { + return wf, true + } + } + return fakeWorkflow{}, false +} + +func (f *fakeCircleCI) approved() []string { + f.mu.Lock() + defer f.mu.Unlock() + return append([]string(nil), f.approvals...) +} + +func (f *fakeCircleCI) listingCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.listings +} + +func (f *fakeCircleCI) serveWorkflow(w http.ResponseWriter, path string) { + id, jobs := strings.CutSuffix(strings.TrimPrefix(path, "/workflow/"), "/job") + wf, ok := f.workflow(id) + switch { + case !ok: + w.WriteHeader(http.StatusNotFound) + case jobs: + writeJSON(w, map[string]interface{}{"items": []map[string]string{ + {"id": "hold-" + wf.id, "name": "approve_deploy_us", "type": "approval", "status": wf.hold}, + }}) + default: + writeJSON(w, map[string]string{ + "id": wf.id, "name": wf.name, "pipeline_id": testPipelineID, "created_at": wf.createdAt, + }) + } +} + +func (f *fakeCircleCI) servePipeline(w http.ResponseWriter, path string) { + switch path { + case "/pipeline/" + testPipelineID: + vcs := map[string]string{ + "revision": f.pipeline.revision, + "target_repository_url": f.pipeline.targetRepo, + "origin_repository_url": f.pipeline.originRepo, + } + writeJSON(w, map[string]interface{}{"id": testPipelineID, "project_slug": f.pipeline.slug, "vcs": vcs}) + case "/pipeline/" + testPipelineID + "/workflow": + f.mu.Lock() + f.listings++ + f.mu.Unlock() + items := make([]map[string]string, 0, len(f.listing)) + for _, id := range f.listing { + wf, _ := f.workflow(id) + items = append(items, map[string]string{"id": wf.id, "name": wf.name, "created_at": wf.createdAt}) + } + writeJSON(w, map[string]interface{}{"items": items}) + default: + w.WriteHeader(http.StatusNotFound) + } +} + +func (f *fakeCircleCI) client(t *testing.T) *Client { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/approve/"): + approval := strings.Replace(strings.TrimPrefix(r.URL.Path, "/workflow/"), "/approve", "", 1) + f.mu.Lock() + f.approvals = append(f.approvals, approval) + f.mu.Unlock() + writeJSON(w, map[string]string{"message": "Accepted."}) + case strings.HasPrefix(r.URL.Path, "/workflow/"): + f.serveWorkflow(w, r.URL.Path) + default: + f.servePipeline(w, r.URL.Path) + } + })) + t.Cleanup(server.Close) + client := NewClient("token") + client.BaseURL = server.URL + return client +} + +func writeJSON(w http.ResponseWriter, v interface{}) { _ = json.NewEncoder(w).Encode(v) } + +func approveForTest(client *Client) error { + return client.ApproveJob(testWorkflowID, "100", "approve_deploy_us", + ApprovalExpectation{Revision: approvedSHA, Repo: "DocSpring/docspring"}) +} + +var matchingPipeline = fakePipeline{revision: approvedSHA, targetRepo: docspringRepo, originRepo: docspringRepo} + +// CircleCI's pipeline workflow listing can leave out a workflow for a while (listing only setup-workflow). The +// stored workflow's hold job is approved without the listing. +func TestApproveJobApprovesStoredWorkflowMissingFromListing(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: matchingPipeline, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "on_hold"}, + {id: setupWorkflowID, name: "setup-workflow", createdAt: "2026-10-13T03:00:00Z", hold: "success"}, + }, + listing: []string{setupWorkflowID}, + } + require.NoError(t, approveForTest(fake.client(t))) + require.Equal(t, []string{testWorkflowID + "/hold-" + testWorkflowID}, fake.approved()) + require.Zero(t, fake.listingCount(), "a hold job that's on hold is approved without listing the pipeline") +} + +// A rerun is a newer workflow with the same name in the same pipeline. Its hold job is approved. +func TestApproveJobApprovesTheNewestRerun(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: matchingPipeline, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "canceled"}, + {id: rerunWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T04:00:00.123Z", hold: "on_hold"}, + {id: setupWorkflowID, name: "setup-workflow", createdAt: "2026-10-13T03:00:00Z", hold: "success"}, + }, + listing: []string{rerunWorkflowID, testWorkflowID, setupWorkflowID}, + } + require.NoError(t, approveForTest(fake.client(t))) + require.Equal(t, []string{rerunWorkflowID + "/hold-" + rerunWorkflowID}, fake.approved()) +} + +// Without a rerun, the stored workflow's hold job is approved even when it isn't on hold, so CircleCI's response +// says why it can't be. An older workflow with the same name isn't a rerun. +func TestApproveJobIgnoresOlderWorkflowsWithTheSameName(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: matchingPipeline, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "success"}, + {id: olderWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T02:00:00Z", hold: "on_hold"}, + }, + listing: []string{olderWorkflowID}, + } + require.NoError(t, approveForTest(fake.client(t))) + require.Equal(t, []string{testWorkflowID + "/hold-" + testWorkflowID}, fake.approved()) +} + +// A rerun's pipeline is the verified pipeline, so a mismatch stops the approval before any rerun is considered. +func TestApproveJobChecksThePipelineBeforeFollowingARerun(t *testing.T) { + fake := &fakeCircleCI{ + pipeline: fakePipeline{revision: strings.Repeat("f", 40), targetRepo: docspringRepo, originRepo: docspringRepo}, + workflows: []fakeWorkflow{ + {id: testWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T03:00:01Z", hold: "canceled"}, + {id: rerunWorkflowID, name: "test_build_deploy", createdAt: "2026-10-13T04:00:00Z", hold: "on_hold"}, + }, + listing: []string{rerunWorkflowID, testWorkflowID}, + } + require.ErrorIs(t, approveForTest(fake.client(t)), ErrPipelineMismatch) + require.Empty(t, fake.approved()) +} diff --git a/internal/gateway/circleci/pipeline_verification_test.go b/internal/gateway/circleci/pipeline_verification_test.go index 06b185c6..632f8b48 100644 --- a/internal/gateway/circleci/pipeline_verification_test.go +++ b/internal/gateway/circleci/pipeline_verification_test.go @@ -1,11 +1,7 @@ package circleci import ( - "encoding/json" - "net/http" - "net/http/httptest" "strings" - "sync/atomic" "testing" "github.com/stretchr/testify/require" @@ -22,49 +18,23 @@ type fakePipeline struct { revision, targetRepo, originRepo, slug string } -// newFakeCircleCI serves the workflow/pipeline/job endpoints ApproveJob uses and counts approvals. -func newFakeCircleCI(t *testing.T, pipeline fakePipeline) (*Client, *int32) { +// newFakeCircleCI serves one workflow, testWorkflowID, whose hold job is on hold and which the pipeline's workflow +// listing includes. +func newFakeCircleCI(t *testing.T, pipeline fakePipeline) (*Client, *fakeCircleCI) { t.Helper() - var approvals int32 - write := func(w http.ResponseWriter, v interface{}) { _ = json.NewEncoder(w).Encode(v) } - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/approve/"): - atomic.AddInt32(&approvals, 1) - write(w, map[string]string{"message": "Accepted."}) - case r.URL.Path == "/workflow/"+testWorkflowID: - write(w, map[string]string{"id": testWorkflowID, "name": "deploy", "pipeline_id": testPipelineID}) - case r.URL.Path == "/pipeline/"+testPipelineID: - vcs := map[string]string{ - "revision": pipeline.revision, - "target_repository_url": pipeline.targetRepo, - "origin_repository_url": pipeline.originRepo, - } - write(w, map[string]interface{}{"id": testPipelineID, "project_slug": pipeline.slug, "vcs": vcs}) - case r.URL.Path == "/pipeline/"+testPipelineID+"/workflow": - write(w, map[string]interface{}{"items": []map[string]string{{"id": testWorkflowID, "name": "deploy"}}}) - case r.URL.Path == "/workflow/"+testWorkflowID+"/job": - write(w, map[string]interface{}{"items": []map[string]string{ - {"id": "job-1", "name": "approve_deploy_us", "type": "approval"}, - }}) - default: - w.WriteHeader(http.StatusNotFound) - } - })) - t.Cleanup(server.Close) - client := NewClient("token") - client.BaseURL = server.URL - return client, &approvals + original := fakeWorkflow{id: testWorkflowID, name: "deploy", createdAt: "2026-10-13T03:00:00Z", hold: "on_hold"} + fake := &fakeCircleCI{pipeline: pipeline, workflows: []fakeWorkflow{original}, listing: []string{testWorkflowID}} + return fake.client(t), fake } func TestApproveJobApprovesMatchingPipeline(t *testing.T) { - client, approvals := newFakeCircleCI(t, fakePipeline{ + client, fake := newFakeCircleCI(t, fakePipeline{ revision: approvedSHA, targetRepo: docspringRepo, originRepo: docspringRepo, slug: "gh/DocSpring/docspring", }) err := client.ApproveJob(testWorkflowID, "100", "approve_deploy_us", ApprovalExpectation{Revision: approvedSHA, Repo: "DocSpring/docspring"}) require.NoError(t, err) - require.EqualValues(t, 1, *approvals) + require.Equal(t, []string{testWorkflowID + "/hold-" + testWorkflowID}, fake.approved()) } func TestApproveJobRefusesMismatchedPipelines(t *testing.T) { @@ -86,22 +56,22 @@ func TestApproveJobRefusesMismatchedPipelines(t *testing.T) { } for name, pipeline := range cases { t.Run(name, func(t *testing.T) { - client, approvals := newFakeCircleCI(t, pipeline) + client, fake := newFakeCircleCI(t, pipeline) err := client.ApproveJob(testWorkflowID, "100", "approve_deploy_us", ApprovalExpectation{Revision: approvedSHA, Repo: "DocSpring/docspring"}) require.ErrorIs(t, err, ErrPipelineMismatch) - require.EqualValues(t, 0, *approvals, "nothing may be approved on a mismatch") + require.Empty(t, fake.approved(), "nothing may be approved on a mismatch") }) } } func TestApproveJobRejectsNonUUIDWorkflowID(t *testing.T) { - client, approvals := newFakeCircleCI(t, fakePipeline{revision: approvedSHA}) + client, fake := newFakeCircleCI(t, fakePipeline{revision: approvedSHA}) for _, id := range []string{"test-workflow-1", "../pipeline/x", ""} { err := client.ApproveJob(id, "100", "approve_deploy_us", ApprovalExpectation{Revision: approvedSHA}) require.ErrorIs(t, err, ErrInvalidWorkflowID) } - require.EqualValues(t, 0, *approvals) + require.Empty(t, fake.approved()) } func TestParseMetadataRequiresUUIDWorkflowID(t *testing.T) {