diff --git a/README.md b/README.md index e4d06ae..3285480 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/cmd/repospawner/main.go b/cmd/repospawner/main.go index b82ba30..945180c 100644 --- a/cmd/repospawner/main.go +++ b/cmd/repospawner/main.go @@ -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 } diff --git a/cmd/repospawner/main_test.go b/cmd/repospawner/main_test.go new file mode 100644 index 0000000..8dab827 --- /dev/null +++ b/cmd/repospawner/main_test.go @@ -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) + } +} diff --git a/internal/jobs/jobs.go b/internal/jobs/jobs.go index 8e05bc2..3af6bd4 100644 --- a/internal/jobs/jobs.go +++ b/internal/jobs/jobs.go @@ -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) } diff --git a/internal/jobs/jobs_test.go b/internal/jobs/jobs_test.go index f716df3..10f5379 100644 --- a/internal/jobs/jobs_test.go +++ b/internal/jobs/jobs_test.go @@ -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) } diff --git a/internal/jobs/state.go b/internal/jobs/state.go index 057f407..a8bcb5d 100644 --- a/internal/jobs/state.go +++ b/internal/jobs/state.go @@ -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) } diff --git a/internal/repospec/repospec.go b/internal/repospec/repospec.go index f1c3777..b98a052 100644 --- a/internal/repospec/repospec.go +++ b/internal/repospec/repospec.go @@ -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,16 +85,29 @@ 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" - } - for _, c := range r.StatusChecks { - if strings.ContainsAny(c, "\n\"") { - errs["status_checks"] = "must not contain quotes or newlines" - break + 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 { + // 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 { diff --git a/internal/repospec/repospec_test.go b/internal/repospec/repospec_test.go index a3c951b..3dfd174 100644 --- a/internal/repospec/repospec_test.go +++ b/internal/repospec/repospec_test.go @@ -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 +} diff --git a/internal/server/reconcile.go b/internal/server/reconcile.go index 4f94f82..a6c27fe 100644 --- a/internal/server/reconcile.go +++ b/internal/server/reconcile.go @@ -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 diff --git a/internal/server/server.go b/internal/server/server.go index 42ae529..3d0c09c 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -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. @@ -43,13 +53,51 @@ func New(cfg *config.Config, st *store.Store, forge *gitea.Client, cluster Clust log = slog.Default() } return &Server{ - cfg: cfg, - store: st, - forge: forge, - cluster: cluster, - gate: auth.New(cfg.GroupsHeader, cfg.AllowedGroups), - assets: assets, - log: log, + cfg: cfg, + store: st, + forge: forge, + cluster: cluster, + 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 diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 2b83c23..2607ae2 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -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) } } diff --git a/ui/static/index.html b/ui/static/index.html index 66e68d7..ae9f51b 100644 --- a/ui/static/index.html +++ b/ui/static/index.html @@ -23,7 +23,7 @@
Lowercase letters, digits and dashes.