Address review findings on the initial service
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/pre-commit Pipeline was successful

Four issues from the review of the initial repospawner service, none of
which change the shape of a request or the file terraform-git receives.

- Encode status checks as one --check flag per context on the
  server-to-job hop, so a separator inside a context can no longer turn
  one context into several; ban commas (and cap lengths) in Validate as
  well, since a real context never holds one.
- Fail a merged request that has waited five minutes for a Woodpecker
  token that vanished after acceptance, surfacing "woodpecker token
  unavailable" through the API, instead of warning in the log forever
  from enabling-ci. Advance now leaves a terminal request alone so the
  failure sticks.
- Hold a per-name lock from the duplicate checks through the store write,
  so two concurrent submissions of one name cannot both be accepted.
- Cap the description at 500 characters and the status checks at 20
  contexts of 100 characters each, and mirror the first two caps in the
  form.
This commit is contained in:
2026-08-30 14:53:27 +10:00
parent f1bcb8cd3a
commit 8786636f7c
12 changed files with 404 additions and 34 deletions
+11 -2
View File
@@ -105,7 +105,7 @@ $ curl -sS https://repospawner.k8s.syd1.au.unkin.net/api/requests
| Route | Meaning |
| --- | --- |
| `POST /api/requests` | Submit a request. `202` with `{id, status_url, state}`; `400` with `{error, fields}` on validation failure; `409` if the name is taken (in terraform-git or by an in-flight request); `503` if `woodpecker:true` but no Woodpecker token is mounted. |
| `POST /api/requests` | Submit a request. `202` with `{id, status_url, state}`; `400` with `{error, fields}` on validation failure; `409` if the name is taken (in terraform-git, by an in-flight request, or by a request submitted concurrently); `503` if `woodpecker:true` but no Woodpecker token is mounted. |
| `GET /api/requests` | Every request, most recent first. |
| `GET /api/requests/{id}` | One request: `state`, `pr_url`, `error`. |
| `GET /api/capabilities` | Whether Woodpecker enablement is available. |
@@ -116,7 +116,16 @@ $ curl -sS https://repospawner.k8s.syd1.au.unkin.net/api/requests
`opening-pr` -> `pr-open` -> `merged` -> (`enabling-ci` ->) `ready`, with
`closed` (PR closed unmerged) and `failed` (a Job failed; `error` says why) as
the other terminal states.
the other terminal states. A request stuck in `enabling-ci` because the
Woodpecker token was unmounted after it was accepted fails with
`woodpecker token unavailable` after five minutes of waiting, rather than
waiting forever.
### Limits
`name` is at most 40 characters of `[a-z0-9-]`, `description` at most 500, and a
request carries at most 20 status check contexts of at most 100 characters each.
A context may not contain a quote, a newline or a comma.
### Generated config
+28 -7
View File
@@ -32,7 +32,7 @@ const usage = `repospawner - open terraform-git pull requests for new repositori
usage:
repospawner [serve] run the API and UI
repospawner job pr --request ID --name NAME \
--description TEXT --checks A,B open the terraform-git PR
--description TEXT --check A [--check B] open the terraform-git PR
repospawner job watch --repo OWNER/NAME --pr N follow that PR to its end
repospawner job woodpecker-enable --name NAME activate the repo in CI
repospawner version print the version
@@ -139,21 +139,42 @@ func runJob(ctx context.Context, log *slog.Logger, cfg *config.Config, args []st
}
}
func runPRJob(ctx context.Context, log *slog.Logger, cfg *config.Config, args []string) error {
// stringList collects a flag given once per value, so a value may hold any
// character without a separator changing what was meant.
type stringList []string
func (l *stringList) String() string { return strings.Join(*l, " ") }
func (l *stringList) Set(v string) error {
*l = append(*l, v)
return nil
}
// parsePRArgs turns the argv the server built into the pr job's inputs.
func parsePRArgs(args []string) (jobrun.PROptions, error) {
fs := flag.NewFlagSet("job pr", flag.ContinueOnError)
requestID := fs.String("request", "", "request id this job serves")
name := fs.String("name", "", "repository name")
description := fs.String("description", "", "repository description")
checks := fs.String("checks", "", "comma-separated required status check contexts")
var checks stringList
fs.Var(&checks, "check", "a required status check context; repeat once per context")
if err := fs.Parse(args); err != nil {
return err
return jobrun.PROptions{}, err
}
res, err := jobrun.PR(ctx, log, cfg, jobrun.PROptions{
return jobrun.PROptions{
RequestID: *requestID,
Name: *name,
Description: *description,
StatusChecks: strings.Split(*checks, ","),
})
StatusChecks: checks,
}, nil
}
func runPRJob(ctx context.Context, log *slog.Logger, cfg *config.Config, args []string) error {
opts, err := parsePRArgs(args)
if err != nil {
return err
}
res, err := jobrun.PR(ctx, log, cfg, opts)
jobrun.Report(log, jobrun.ReportPath(), res)
return err
}
+56
View File
@@ -0,0 +1,56 @@
package main
import (
"slices"
"testing"
"git.unkin.net/unkin/repospawner/internal/config"
"git.unkin.net/unkin/repospawner/internal/jobs"
"git.unkin.net/unkin/repospawner/internal/store"
)
// TestPRArgsRoundTrip pins the server-to-job hop: whatever the server put in a
// request must come back out of the Job's argv unchanged, one context per
// context, whatever characters a context happens to hold.
func TestPRArgsRoundTrip(t *testing.T) {
cases := [][]string{
{"ci/woodpecker/pr/build"},
{"ci/woodpecker/pr/build", "ci/woodpecker/pr/test", "ci/woodpecker/pr/pre-commit"},
{"ci/build, with a comma", "plain"},
{"has spaces and --dashes"},
}
cfg := &config.Config{Namespace: "repospawner", Image: "repospawner:test", VaultSATokenPath: "/var/run/secrets/vault/token"}
for _, checks := range cases {
req := store.Request{
ID: "abc123",
Name: "widget",
Description: "does widgets, comprehensively",
StatusChecks: checks,
}
argv := jobs.PR(cfg, req).Spec.Template.Spec.Containers[0].Args
if len(argv) < 2 || argv[0] != "job" || argv[1] != "pr" {
t.Fatalf("argv = %q", argv)
}
opts, err := parsePRArgs(argv[2:])
if err != nil {
t.Fatalf("parsePRArgs(%q): %v", argv, err)
}
if opts.RequestID != req.ID || opts.Name != req.Name || opts.Description != req.Description {
t.Errorf("opts = %+v, want the request's fields %+v", opts, req)
}
if !slices.Equal(opts.StatusChecks, checks) {
t.Errorf("StatusChecks = %q, want %q", opts.StatusChecks, checks)
}
}
}
func TestParsePRArgsWithoutChecks(t *testing.T) {
opts, err := parsePRArgs([]string{"--request", "abc", "--name", "widget", "--description", "d"})
if err != nil {
t.Fatalf("parsePRArgs: %v", err)
}
if len(opts.StatusChecks) != 0 {
t.Errorf("StatusChecks = %q, want none", opts.StatusChecks)
}
}
+5 -1
View File
@@ -87,7 +87,11 @@ func PR(cfg *config.Config, r store.Request) *batchv1.Job {
"--request", r.ID,
"--name", r.Name,
"--description", r.Description,
"--checks", strings.Join(r.StatusChecks, ","),
}
// One flag per context: a separator inside a check would otherwise turn one
// context into several on the way back out.
for _, c := range r.StatusChecks {
args = append(args, "--check", c)
}
return base(cfg, r, TypePR, args, shortDeadline)
}
+1 -1
View File
@@ -64,7 +64,7 @@ func TestPRJobSpec(t *testing.T) {
}
args := strings.Join(pod.Containers[0].Args, " ")
want := "job pr --request abc123 --name widget --description does widgets " +
"--checks ci/woodpecker/pr/build,ci/woodpecker/pr/test"
"--check ci/woodpecker/pr/build --check ci/woodpecker/pr/test"
if args != want {
t.Errorf("args = %q, want %q", args, want)
}
+5
View File
@@ -20,6 +20,11 @@ const (
// Job that should be created next. It is deliberately pure: the reconciler
// supplies the observations and performs the action.
func Advance(r store.Request, views map[Type]View) (store.Request, Action) {
// A request that already ended badly stays ended: later observations of the
// jobs that got it there must not walk it back out of a terminal state.
if r.State == store.StateFailed || r.State == store.StateClosed {
return r, ActionNone
}
if pr, ok := views[TypePR]; ok {
r = applyPR(r, pr)
}
+28 -5
View File
@@ -20,6 +20,16 @@ var nameRE = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`)
// character limit k8s applies to object names.
const maxNameLen = 40
const (
// maxDescriptionLen bounds the description, which becomes a YAML scalar and
// a Job annotation.
maxDescriptionLen = 500
// maxCheckLen bounds a single status check context.
maxCheckLen = 100
// maxChecks bounds how many contexts one branch protection rule carries.
maxChecks = 20
)
// Request is a submitted new-repo request.
type Request struct {
Name string `json:"name"`
@@ -75,17 +85,30 @@ func (r Request) Validate() error {
case !nameRE.MatchString(r.Name):
errs["name"] = "must be lowercase letters, digits and dashes, starting and ending alphanumeric"
}
if r.Description == "" {
switch {
case r.Description == "":
errs["description"] = "required"
case len(r.Description) > maxDescriptionLen:
errs["description"] = fmt.Sprintf("must be at most %d characters", maxDescriptionLen)
}
if len(r.StatusChecks) == 0 {
switch {
case len(r.StatusChecks) == 0:
errs["status_checks"] = "at least one status check context is required"
}
case len(r.StatusChecks) > maxChecks:
errs["status_checks"] = fmt.Sprintf("at most %d status check contexts are allowed", maxChecks)
default:
for _, c := range r.StatusChecks {
if strings.ContainsAny(c, "\n\"") {
errs["status_checks"] = "must not contain quotes or newlines"
// A comma would split into two contexts on the server-to-job hop, and
// a real context never contains one.
if strings.ContainsAny(c, "\n\",") {
errs["status_checks"] = "must not contain quotes, commas or newlines"
break
}
if len(c) > maxCheckLen {
errs["status_checks"] = fmt.Sprintf("each context must be at most %d characters", maxCheckLen)
break
}
}
}
if len(errs) == 0 {
return nil
+48
View File
@@ -2,6 +2,7 @@ package repospec
import (
"errors"
"strconv"
"strings"
"testing"
)
@@ -26,6 +27,45 @@ func TestValidate(t *testing.T) {
{name: "over long", req: with(base, func(r *Request) { r.Name = strings.Repeat("a", maxNameLen+1) }), fields: []string{"name"}, wantErr: true},
{name: "missing description", req: with(base, func(r *Request) { r.Description = "" }), fields: []string{"description"}, wantErr: true},
{name: "no checks", req: with(base, func(r *Request) { r.StatusChecks = nil }), fields: []string{"status_checks"}, wantErr: true},
{
name: "over long description",
req: with(base, func(r *Request) { r.Description = strings.Repeat("d", maxDescriptionLen+1) }),
fields: []string{"description"},
wantErr: true,
},
{
name: "description at the cap",
req: with(base, func(r *Request) { r.Description = strings.Repeat("d", maxDescriptionLen) }),
},
{
name: "check containing a comma",
req: with(base, func(r *Request) { r.StatusChecks = []string{"ci/woodpecker/pr/test,ci/woodpecker/pr/build"} }),
fields: []string{"status_checks"},
wantErr: true,
},
{
name: "check containing a quote",
req: with(base, func(r *Request) { r.StatusChecks = []string{`ci/"test"`} }),
fields: []string{"status_checks"},
wantErr: true,
},
{
name: "over long check",
req: with(base, func(r *Request) { r.StatusChecks = []string{strings.Repeat("c", maxCheckLen+1)} }),
fields: []string{"status_checks"},
wantErr: true,
},
{
name: "check at the cap",
req: with(base, func(r *Request) { r.StatusChecks = []string{strings.Repeat("c", maxCheckLen)} }),
},
{
name: "too many checks",
req: with(base, func(r *Request) { r.StatusChecks = manyChecks(maxChecks + 1) }),
fields: []string{"status_checks"},
wantErr: true,
},
{name: "checks at the cap", req: with(base, func(r *Request) { r.StatusChecks = manyChecks(maxChecks) })},
{
name: "every field bad at once",
req: Request{},
@@ -144,3 +184,11 @@ func with(r Request, f func(*Request)) Request {
f(&r)
return r
}
func manyChecks(n int) []string {
out := make([]string, 0, n)
for i := range n {
out = append(out, "ci/woodpecker/pr/check"+strconv.Itoa(i))
}
return out
}
+39 -1
View File
@@ -14,6 +14,16 @@ import (
// UI polls on the same cadence, so a change surfaces within two ticks.
const reconcileInterval = 10 * time.Second
// maxWoodpeckerTokenWaits bounds how many reconcile passes a merged request
// waits for a Woodpecker token that vanished after the request was accepted.
// Past it the request fails with the reason, rather than sitting in
// enabling-ci forever with the trouble visible only in the server's logs.
const maxWoodpeckerTokenWaits = 30
// woodpeckerTokenUnavailable is the error a request carries when the token
// never came back.
const woodpeckerTokenUnavailable = "woodpecker token unavailable"
// Run reconciles until ctx is cancelled, starting with an immediate pass so a
// restarted server rebuilds its state before serving its first request.
func (s *Server) Run(ctx context.Context) {
@@ -94,9 +104,10 @@ func (s *Server) act(ctx context.Context, r store.Request, action jobs.Action) e
return s.cluster.CreateJob(ctx, jobs.Watch(s.cfg, r))
case jobs.ActionCreateWoodpecker:
if !s.woodpeckerAvailable() {
s.log.Warn("woodpecker enablement requested but no token is mounted", "request", r.ID)
s.awaitWoodpeckerToken(r)
return nil
}
s.forgetWoodpeckerWait(r.ID)
s.log.Info("enabling repository in woodpecker", "request", r.ID, "name", r.Name)
return s.cluster.CreateJob(ctx, jobs.Woodpecker(s.cfg, r))
case jobs.ActionNone:
@@ -106,6 +117,33 @@ func (s *Server) act(ctx context.Context, r store.Request, action jobs.Action) e
}
}
// awaitWoodpeckerToken counts a pass spent waiting for a token that was there
// when the request was accepted, and fails the request once the wait is over.
func (s *Server) awaitWoodpeckerToken(r store.Request) {
s.waitMu.Lock()
s.woodpeckerWaits[r.ID]++
waits := s.woodpeckerWaits[r.ID]
s.waitMu.Unlock()
if waits < maxWoodpeckerTokenWaits {
s.log.Warn("woodpecker enablement requested but no token is mounted",
"request", r.ID, "name", r.Name, "waits", waits)
return
}
s.log.Error("failing request: woodpecker token never became available",
"request", r.ID, "name", r.Name, "waits", waits)
r.State = store.StateFailed
r.Error = woodpeckerTokenUnavailable
s.store.Put(r)
s.forgetWoodpeckerWait(r.ID)
}
func (s *Server) forgetWoodpeckerWait(id string) {
s.waitMu.Lock()
defer s.waitMu.Unlock()
delete(s.woodpeckerWaits, id)
}
type resultKey struct {
request string
jobType jobs.Type
+53
View File
@@ -13,6 +13,7 @@ import (
"net/http"
"os"
"strings"
"sync"
"time"
"git.unkin.net/unkin/repospawner/internal/auth"
@@ -35,6 +36,15 @@ type Server struct {
gate *auth.Middleware
assets fs.FS
log *slog.Logger
// names serialises the name claim so two concurrent submissions of one name
// cannot both pass the duplicate checks.
names keyedMutex
waitMu sync.Mutex
// woodpeckerWaits counts, per request id, the reconcile passes spent
// waiting for a Woodpecker token that vanished after acceptance.
woodpeckerWaits map[string]int
}
// New constructs a Server.
@@ -50,6 +60,44 @@ func New(cfg *config.Config, st *store.Store, forge *gitea.Client, cluster Clust
gate: auth.New(cfg.GroupsHeader, cfg.AllowedGroups),
assets: assets,
log: log,
woodpeckerWaits: map[string]int{},
}
}
// keyedMutex serialises work per key and forgets a key once nothing holds it.
type keyedMutex struct {
mu sync.Mutex
held map[string]*keyedEntry
}
type keyedEntry struct {
mu sync.Mutex
refs int
}
// lock blocks until key is free and returns the function that releases it.
func (k *keyedMutex) lock(key string) func() {
k.mu.Lock()
if k.held == nil {
k.held = map[string]*keyedEntry{}
}
e, ok := k.held[key]
if !ok {
e = &keyedEntry{}
k.held[key] = e
}
e.refs++
k.mu.Unlock()
e.mu.Lock()
return func() {
e.mu.Unlock()
k.mu.Lock()
defer k.mu.Unlock()
e.refs--
if e.refs == 0 {
delete(k.held, key)
}
}
}
@@ -142,6 +190,11 @@ func (s *Server) handleCreate(w http.ResponseWriter, r *http.Request) {
"woodpecker enablement is unavailable: no woodpecker API token is mounted; resubmit with woodpecker disabled")
return
}
// Everything from here to the store write claims the name; holding it per
// name keeps two concurrent submissions from both finding it free.
unlock := s.names.lock(spec.Name)
defer unlock()
if s.store.HasActiveName(spec.Name) {
writeErr(w, http.StatusConflict, "a request for that repository name is already in flight")
return
+119 -6
View File
@@ -485,7 +485,9 @@ func TestReconcileRebuildsStateFromJobs(t *testing.T) {
}
}
func TestReconcileSkipsWoodpeckerWithoutToken(t *testing.T) {
// mergedWoodpeckerCluster holds a request whose terraform-git PR merged and
// which asked for Woodpecker enablement.
func mergedWoodpeckerCluster() *fakeCluster {
anno := map[string]string{
jobs.AnnoName: "widget",
jobs.AnnoWoodpecker: "true",
@@ -494,18 +496,129 @@ func TestReconcileSkipsWoodpeckerWithoutToken(t *testing.T) {
jobs.AnnoPullRequestNo: "42",
}
watchJob, watchPod := jobFor("abc123", jobs.TypeWatch, anno, true, `{"merged":true}`)
cluster := &fakeCluster{jobs: []batchv1.Job{watchJob}, pods: []corev1.Pod{watchPod}}
return &fakeCluster{jobs: []batchv1.Job{watchJob}, pods: []corev1.Pod{watchPod}}
}
func TestReconcileWaitsForWoodpeckerToken(t *testing.T) {
cluster := mergedWoodpeckerCluster()
srv, st := newTestServer(t, testCfg(t), cluster)
// Short of the bound the request keeps waiting; the token may still arrive.
for i := range maxWoodpeckerTokenWaits - 1 {
if err := srv.Reconcile(context.Background()); err != nil {
t.Fatalf("Reconcile %d: %v", i, err)
}
got, _ := st.Get("abc123")
if got.State != store.StateEnablingCI {
t.Fatalf("state after %d passes = %q, want enabling-ci", i+1, got.State)
}
}
if len(cluster.createdNames()) != 0 {
t.Errorf("no woodpecker job may be created without a token: %v", cluster.createdNames())
}
}
func TestReconcileFailsRequestWhenWoodpeckerTokenNeverArrives(t *testing.T) {
cluster := mergedWoodpeckerCluster()
srv, st := newTestServer(t, testCfg(t), cluster)
for i := range maxWoodpeckerTokenWaits {
if err := srv.Reconcile(context.Background()); err != nil {
t.Fatalf("Reconcile %d: %v", i, err)
}
}
got, _ := st.Get("abc123")
if got.State != store.StateFailed {
t.Fatalf("state = %q, want failed", got.State)
}
if got.Error != woodpeckerTokenUnavailable {
t.Errorf("error = %q, want %q", got.Error, woodpeckerTokenUnavailable)
}
if len(cluster.createdNames()) != 0 {
t.Errorf("no woodpecker job may be created without a token: %v", cluster.createdNames())
}
// The failure sticks across later passes, and the API surfaces the reason.
if err := srv.Reconcile(context.Background()); err != nil {
t.Fatalf("Reconcile: %v", err)
}
r := httptest.NewRequest(http.MethodGet, "/api/requests/abc123", nil)
r.Header.Set("X-Forwarded-Groups", allowed)
w := httptest.NewRecorder()
srv.Handler().ServeHTTP(w, r)
if w.Code != http.StatusOK {
t.Fatalf("GET = %d", w.Code)
}
var served store.Request
if err := json.Unmarshal(w.Body.Bytes(), &served); err != nil {
t.Fatalf("decode: %v", err)
}
if served.State != store.StateFailed || served.Error != woodpeckerTokenUnavailable {
t.Errorf("served = %+v, want a failed request naming the missing token", served)
}
}
func TestReconcileEnablesWoodpeckerOnceTheTokenReturns(t *testing.T) {
cfg := testCfg(t)
cluster := mergedWoodpeckerCluster()
srv, st := newTestServer(t, cfg, cluster)
if err := srv.Reconcile(context.Background()); err != nil {
t.Fatalf("Reconcile: %v", err)
}
got, _ := st.Get("abc123")
if got.State != store.StateEnablingCI {
if err := os.WriteFile(cfg.WoodpeckerTokenFile, []byte("wp\n"), 0o600); err != nil {
t.Fatalf("write token: %v", err)
}
if err := srv.Reconcile(context.Background()); err != nil {
t.Fatalf("Reconcile: %v", err)
}
if names := cluster.createdNames(); len(names) != 1 || names[0] != "repospawner-woodpecker-abc123" {
t.Fatalf("created jobs = %v, want the woodpecker job", names)
}
if got, _ := st.Get("abc123"); got.State != store.StateEnablingCI {
t.Errorf("state = %q, want enabling-ci", got.State)
}
if len(cluster.createdNames()) != 0 {
t.Errorf("no woodpecker job may be created without a token: %v", cluster.createdNames())
}
func TestCreateSerialisesConcurrentSubmissionsOfOneName(t *testing.T) {
srv, st := newTestServer(t, testCfg(t), &fakeCluster{})
h := srv.Handler()
body := `{"name":"widget","description":"d","status_checks":["x"]}`
const submissions = 8
codes := make([]int, submissions)
start := make(chan struct{})
var wg sync.WaitGroup
for i := range submissions {
wg.Add(1)
go func() {
defer wg.Done()
<-start
r := httptest.NewRequest(http.MethodPost, "/api/requests", strings.NewReader(body))
r.Header.Set("Content-Type", "application/json")
r.Header.Set("X-Forwarded-Groups", allowed)
w := httptest.NewRecorder()
h.ServeHTTP(w, r)
codes[i] = w.Code
}()
}
close(start)
wg.Wait()
accepted, conflicted := 0, 0
for _, c := range codes {
switch c {
case http.StatusAccepted:
accepted++
case http.StatusConflict:
conflicted++
}
}
if accepted != 1 || conflicted != submissions-1 {
t.Fatalf("codes = %v, want exactly one 202 and %d 409s", codes, submissions-1)
}
if got := len(st.List()); got != 1 {
t.Errorf("stored requests = %d, want 1", got)
}
}
+2 -2
View File
@@ -23,7 +23,7 @@
<div class="form-group">
<label for="f-name">Name</label>
<input class="form-control" id="f-name" name="name" autocomplete="off"
spellcheck="false" placeholder="my-service">
spellcheck="false" maxlength="40" placeholder="my-service">
<p class="help">Lowercase letters, digits and dashes.</p>
<p class="field-error hidden" id="e-name"></p>
</div>
@@ -31,7 +31,7 @@
<div class="form-group">
<label for="f-description">Description</label>
<input class="form-control" id="f-description" name="description" autocomplete="off"
placeholder="What the repository is for">
maxlength="500" placeholder="What the repository is for">
<p class="field-error hidden" id="e-description"></p>
</div>