diff --git a/CLAUDE.md b/CLAUDE.md index 906a30e..bf4e34b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -22,7 +22,7 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/ ## Architecture (one-line per package) -- `cmd/master` — wiring only: config → store → hub → jobs → web, two listeners (UI, agents over pinned TLS) +- `cmd/master` — wiring only: config → store → hub → jobs → web + webhook, two listeners (UI, agents over pinned TLS) - `cmd/agent` — wiring only: config → docker client → inventory → job runner → master connection - `internal/protocol` — the WebSocket messages both sides speak (`Version`, `JobStart`, `JobResult`, ...) - `internal/pin` — master key pair, certificate and fingerprint pinning @@ -63,8 +63,8 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/ - **The webhook never creates services; new services always start `manual`.** The upsert does not touch `update_mode`. - **Auth**: agents by token over pinned TLS; people by OIDC; CI by the `UPDATER_API_KEY` - bearer token on `POST /update` only. Every state-changing UI - request is a POST with a CSRF token. + bearer token on `POST /update` only. Every state-changing UI request is a POST with a + CSRF token. ## Gotchas @@ -101,8 +101,10 @@ DATABASE_URL=postgres://... go test -tags integration ./internal/master/store/ - **A webhook job's target is `:`**, built from the service's own repo, never from the request's image string. With an explicit target the "follows no tag" refusal does not apply; removed, self, built and container still do. -- **`available_*` is cleared by `FinishJob`** on success when `available_at <= job.created_at` - (a job created after the suggestion deployed the newest version). +- **`available_*` is cleared by `FinishJob`** on success only when the job's `to_ref` + (as created, without `@digest`) equals `available_ref` and `available_at <= job.created_at` + (that version is now running). A manual Update of the followed tag does not clear a + badge for a different tag. - **Job output is capped at `protocol.MaxJobOutput` (128 KiB, tail).** NUL bytes are stripped by the hub because Postgres text rejects them. diff --git a/README.md b/README.md index 7f8c45f..dc79ae5 100644 --- a/README.md +++ b/README.md @@ -18,9 +18,9 @@ runs is exactly what was pulled. The log streams to the browser while it runs; j history is kept in Postgres. A service without a tag (`repo@digest`), a plain container, and the updater's own services cannot be updated, and the UI says why. A bare image (`image: postgres`, no tag and no digest) counts as `postgres:latest`; only a reference -with a digest and no tag (`repo@sha256:...`) "follows no tag". Compose services with a `build:` section are shown but never updated -("built from source (build:)"). One update at a -time per service, and one job at a time per agent. +with a digest and no tag (`repo@sha256:...`) "follows no tag". Compose services with a +`build:` section are shown but never updated ("built from source (build:)"). One update at +a time per service, and one job at a time per agent. ## Deploying the master @@ -54,7 +54,9 @@ Content-Type: application/json `image` is matched against the image repo of every service on every agent (tag and digest in the request are ignored for matching; matching is case-sensitive). `tag` is optional. For every match the master records the version as *available* (the overview shows an -"update available" badge). Services in **manual** mode stop there. Services in **auto** +"update available" badge), including services that cannot be updated automatically +(updater's own, `build:` and plain containers), so the badge shows for them too. Services +in **manual** mode stop there. Services in **auto** mode also get an update job, and the request waits for those jobs to finish, at most `UPDATE_TIMEOUT`. @@ -63,7 +65,8 @@ from the service's own repo and never from the request's `image` string. The tag one that has been pushed. Compose services get pinned to it through `compose.override.yaml`, as in any update. Omit `tag` to re-pull the tag the service already follows (a bare image counts as `:latest`); a service that follows no tag -(`repo@digest`) is then not touched, and an auto service is reported as `refused`. +(`repo@digest`) is then not touched and is reported as `refused` ("no tag given and the +service follows no tag"), manual or auto. With a `tag`, the "follows no tag" refusal does not apply; the other refusals (the updater's own services, `build:` services, plain containers) still do. @@ -86,7 +89,7 @@ Response (`200` or `500`): | `timeout` | Job still running when `UPDATE_TIMEOUT` passed. The master leaves it running. | yes | | `marked` | Manual service; the version was recorded only. | no | | `busy` | The service already has a queued or running job; `job_id` is that job, which is not waited for. | no | -| `refused` | The service may not be updated (`error` has the reason). | no | +| `refused` | The service may not be updated, or there is no target (no `tag` given and the service follows no tag). `error` has the reason. | no | | Code | When | |---|---| diff --git a/deploy/master.stack.yml b/deploy/master.stack.yml index 2061ada..3c0e413 100644 --- a/deploy/master.stack.yml +++ b/deploy/master.stack.yml @@ -1,4 +1,4 @@ -# Example Swarm stack for the package-updater master (phase 3: inventory, updates and the CI webhook). +# Example Swarm stack for the package-updater master (inventory, updates and the CI webhook). # Deploy on the manager: docker stack deploy -c master.stack.yml package-updater-master # # Before deploying: @@ -6,8 +6,9 @@ # 2. In Authentik: create an OAuth2/OpenID provider + application with redirect URI # https://updater.example.com/auth/callback, and a group "updater-admins". # 3. docker secret create pu_oidc_client_secret - ; docker secret create pu_database_url - -# 4. Optional, enables POST /update (without it the endpoint answers 503): -# docker secret create pu_updater_api_key - +# 4. docker secret create pu_updater_api_key - (required by this example; it enables POST /update) +# To run without the webhook, remove the pu_updater_api_key lines (the UPDATER_API_KEY +# part of the entrypoint, the service secret and the secrets entry); the endpoint then answers 503. # The reverse proxy must allow a read timeout longer than UPDATE_TIMEOUT (default 5m). services: master: diff --git a/docs/superpowers/plans/2026-10-12-master-agent-phase-3-webhook.md b/docs/superpowers/plans/2026-10-12-master-agent-phase-3-webhook.md index 1dd6dc0..5bf335a 100644 --- a/docs/superpowers/plans/2026-10-12-master-agent-phase-3-webhook.md +++ b/docs/superpowers/plans/2026-10-12-master-agent-phase-3-webhook.md @@ -21,10 +21,10 @@ ## Deliberate deviations from the spec in this phase -- **No `image_versions` table and no `available_digest` column.** Both belong to phase 4 (version history and the version picker). Phase 3 stores only `available_ref`, `available_source` (`webhook`; `poll` is reserved for phase 4) and `available_at` on `services`. +- **No `image_versions` table and no `available_digest` column.** Both belong to phase 4 (version history and the version picker). Phase 3 stores only `available_ref`, `available_source` (`webhook`) and `available_at` on `services`. - **The old `207` is gone.** A request with both a working and a failed service answers `500` as a whole; there is no partial-success code. The Gitea action fails on any 4xx/5xx, so CI surfaces it. - **`busy` and `refused` are not failures.** A service that already has an active job (`busy`, with that job's ID, not waited for) or may not be updated (`refused`, with the reason) does not make the request fail. Only `failed` and `timeout` do. A manual match is `marked`, also not a failure. -- **A webhook with a `tag` bypasses the "follows no tag" refusal.** The target is then explicit (`:`), so a `repo@digest` service can be moved to a tag. Removed, self, built and container refusals still apply. Without a `tag`, the target is the followed tag (bare image = `:latest`); a service that follows no tag is left alone (`available_*` untouched) and an auto one is `refused` with the no-tag reason. Because of this, the UI allows `auto` for a service that follows no tag (`jobs.AutoAllowed`), unlike the Update button (`jobs.Updatable`). +- **A webhook with a `tag` bypasses the "follows no tag" refusal.** The target is then explicit (`:`), so a `repo@digest` service can be moved to a tag. Removed, self, built and container refusals still apply. Without a `tag`, the target is the followed tag (bare image = `:latest`); a service that follows no tag is left alone (`available_*` untouched) and is reported `refused` ("no tag given and the service follows no tag"), manual or auto. Because of this, the UI allows `auto` for a service that follows no tag (`jobs.AutoAllowed`), unlike the Update button (`jobs.Updatable`). - **The target ref is built from the service's own repo.** `imageref.Parse(req.Image).Repo` only selects services; it never reaches the job, so a request cannot point a service at another registry. - **Token is optional on the master.** Unset `UPDATER_API_KEY` means `POST /update` answers `503` and startup logs `webhook_disabled`; the master still runs. - **`tag` is validated** against Docker's tag grammar (`400 invalid tag`). The spec does not say; without it a tag such as `v1@sha256:x` or one with spaces would end up in the override file and fail on the agent. @@ -45,7 +45,7 @@ | A timed-out job is reported `timeout` and left untouched. | webhook `TestTimeout` | | A client that disconnects does not abort marking or starting. | webhook `TestClientCancelStopsWaitingButNotStarting` | | The webhook waits for the real agent's result and answers 200 / 500. | e2e `TestWebhookWaitsForAutoJob`, `TestWebhookFailedAutoJobIs500` | -| The available ref is cleared only by a successful job created at or after the suggestion; a newer suggestion and a failed job keep it. | store `TestFinishJob_ClearsAvailableOnlyWhenJobIsNewer` | +| The available ref is cleared only by a successful job whose `to_ref` equals it and that was created at or after the suggestion; a different ref, a newer suggestion and a failed job keep it. | store `TestFinishJob_ClearsAvailableOnlyWhenThatVersionWasDeployed` | | Re-reporting inventory never resets a service's update mode; new services are manual. | store `TestUpsertKeepsUpdateMode` | | `auto` is refused for removed, self, built and container services; `manual` is always allowed. | jobs `TestAutoAllowed`; web `TestSetMode_AutoRefusedShows409`, `TestSetMode_ManualAlwaysAllowed` | @@ -56,7 +56,7 @@ ```sql ALTER TABLE services ADD COLUMN available_ref text NOT NULL DEFAULT ''; ALTER TABLE services ADD COLUMN available_source text NOT NULL DEFAULT '' - CHECK (available_source IN ('', 'webhook', 'poll')); + CHECK (available_source IN ('', 'webhook')); ALTER TABLE services ADD COLUMN available_at timestamptz; CREATE INDEX services_by_image_repo ON services (image_repo) WHERE removed_at IS NULL; @@ -64,7 +64,7 @@ CREATE INDEX services_by_image_repo ON services (image_repo) WHERE removed_at IS ### C2. `internal/master/model` additions -`Service` gains `AvailableRef string`, `AvailableSource string`, `AvailableAt *time.Time`. Constants `ModeManual`, `ModeAuto`, `SourceWebhook`, `SourcePoll`. `type Available struct { Ref, Source string; At time.Time }`. +`Service` gains `AvailableRef string`, `AvailableSource string`, `AvailableAt *time.Time`. Constants `ModeManual`, `ModeAuto`, `SourceWebhook`. `type Available struct { Ref, Source string; At time.Time }`. ### C3. Store methods @@ -74,7 +74,7 @@ SetAvailable(ctx, id int64, a model.Available) error SetUpdateMode(ctx, id int64, mode string) error // model.ErrNotFound for unknown or removed ``` -`FinishJob` additionally reads the job's `created_at` and, on success, clears `available_*` in the same transaction when `available_at <= created_at`. +`FinishJob` additionally reads the job's `created_at` and, on success, clears `available_*` in the same transaction when the job's `to_ref` (as created, without `@digest`) equals `available_ref` and `available_at <= created_at`. ### C4. `internal/master/jobs` @@ -113,7 +113,7 @@ Request `{"image","tag"}`; response `{"request_id","image","tag","matched","resu ### Task 1: Store and model -Migration C1, model C2, store C3. Integration tests (`//go:build integration`): `ServicesByRepo` excludes removed rows and other repos; `SetAvailable` round trip; `SetUpdateMode` including `ErrNotFound`; new service is manual and upsert/snapshot keep the mode; `FinishJob` clears, keeps newer, keeps on failure. +Migration C1, model C2, store C3. Integration tests (`//go:build integration`): `ServicesByRepo` excludes removed rows and other repos; `SetAvailable` round trip; `SetUpdateMode` including `ErrNotFound`; new service is manual and upsert/snapshot keep the mode; `FinishJob` clears the same ref, keeps a different ref, keeps newer, keeps on failure. ### Task 2: Jobs service @@ -125,7 +125,7 @@ Config C6 with tests. Handler C5 with fakes and the real `events.Broker`. `cmd/m ### Task 4: Web -Store gains `SetUpdateMode`, Jobs gains `AutoAllowed`. Overview: mode column with a toggle form (hidden `csrf_token`, `mode`, `return=overview`; disabled with the reason as title when auto is not allowed) and an "update available" badge. Service page: Available row, Mode row, toggle form, 409 error. The forms are inside the `services` template that the SSE fragment re-renders, so they survive a refresh. +Store gains `SetUpdateMode`, Jobs gains `AutoAllowed`. Overview: mode column with a toggle form (hidden `csrf_token`, `mode`, `return=overview`; disabled with the reason as visible text when auto is not allowed) and an "update available" badge. Service page: Available row, Mode row, toggle form, 409 error. The forms are inside the `services` template that the SSE fragment re-renders, so they survive a refresh. ### Task 5: End-to-end diff --git a/gitea-action/README.md b/gitea-action/README.md index f0f09a1..ef8475c 100644 --- a/gitea-action/README.md +++ b/gitea-action/README.md @@ -37,7 +37,7 @@ The step exits non-zero on any HTTP 4xx or 5xx. This is intentional: the workflo | Code | Meaning | |---|---| -| `200` | Every match was marked, updated, busy or refused. Manual services are only marked, which is not an error. | +| `200` | Every match was marked, updated, busy or refused. Manual services are only marked, which is not an error. A service that follows no tag is `refused` when the request has no `tag`. | | `401` | Wrong or missing token. | | `404` | No service uses the image. Check `image` for typos; matching is case-sensitive. | | `500` | At least one auto job failed or timed out (`UPDATE_TIMEOUT` on the master). | diff --git a/internal/e2e/agent_master_test.go b/internal/e2e/agent_master_test.go index 74cb2c2..9036eb1 100644 --- a/internal/e2e/agent_master_test.go +++ b/internal/e2e/agent_master_test.go @@ -174,6 +174,7 @@ func (m *memStore) FinishJob(_ context.Context, agentID, jobID int64, f model.Jo return false, nil } j.Status, j.Error, j.Output, j.OutputTruncated, j.FinishedAt = f.Status, f.Error, f.Output, f.OutputTruncated, &f.FinishedAt + requestedRef := j.ToRef if f.Ref != "" { j.ToRef = f.Ref } @@ -183,8 +184,9 @@ func (m *memStore) FinishJob(_ context.Context, agentID, jobID int64, f model.Jo if f.Status == model.JobSucceeded { svc := m.svcRows[j.ServiceID] svc.DesiredRef = f.Ref - // Same rule as the SQL: a job created after the suggestion deployed it. - if svc.AvailableAt != nil && !svc.AvailableAt.After(j.CreatedAt) { + // Same rule as the SQL: the job was created for the suggested ref after it was reported. + requested, _, _ := strings.Cut(requestedRef, "@") + if svc.AvailableAt != nil && svc.AvailableRef == requested && !svc.AvailableAt.After(j.CreatedAt) { svc.AvailableRef, svc.AvailableSource, svc.AvailableAt = "", "", nil } m.svcRows[j.ServiceID] = svc diff --git a/internal/master/model/model.go b/internal/master/model/model.go index b17e810..3b4a8ff 100644 --- a/internal/master/model/model.go +++ b/internal/master/model/model.go @@ -56,7 +56,7 @@ type Service struct { // AvailableRef is the newest ref a webhook or poll reported for this // service; "" if none, or if a later job already deployed it. AvailableRef string - AvailableSource string // SourceWebhook or SourcePoll; "" when AvailableRef is empty + AvailableSource string // SourceWebhook; "" when AvailableRef is empty AvailableAt *time.Time FirstSeenAt time.Time UpdatedAt time.Time @@ -72,13 +72,12 @@ const ( // Sources of an available ref. const ( SourceWebhook = "webhook" - SourcePoll = "poll" ) // Available is a newer ref reported for a service. type Available struct { Ref string - Source string // SourceWebhook or SourcePoll + Source string // SourceWebhook At time.Time } diff --git a/internal/master/store/jobs.go b/internal/master/store/jobs.go index 7933ed8..31a9da9 100644 --- a/internal/master/store/jobs.go +++ b/internal/master/store/jobs.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "time" "github.com/jackc/pgx/v5" @@ -187,9 +188,10 @@ func (s *Store) JobProgress(ctx context.Context, agentID, jobID int64, chunk str // agentID: status, error, output (replacing what progress assembled), // started_at (the agent's, or the existing one if the agent sent none), // finished_at; to_ref = f.Ref when set. On success it also sets the -// service's desired_ref = f.Ref and clears its available ref if that was -// reported no later than the job was created (the job then deployed the -// newest version), in the same transaction. Returns false if no +// service's desired_ref = f.Ref and clears its available ref when that is +// the ref the job was created for (ignoring any @digest) and was reported no +// later than the job was created (that version is now running), in the same +// transaction. Returns false if no // such active job belongs to agentID. func (s *Store) FinishJob(ctx context.Context, agentID, jobID int64, f model.JobFinish) (bool, error) { tx, err := s.pool.Begin(ctx) @@ -206,14 +208,17 @@ func (s *Store) FinishJob(ctx context.Context, agentID, jobID int64, f model.Job } var serviceID int64 var createdAt time.Time + var requestedRef string + // requested is the job's to_ref as it was created, before f.Ref overwrites it. err = tx.QueryRow(ctx, ` - UPDATE update_jobs SET + UPDATE update_jobs j SET status = $3, error = $4, output = $5, output_truncated = $6, - to_ref = CASE WHEN $7::text <> '' THEN $7::text ELSE to_ref END, - started_at = COALESCE($8, started_at), finished_at = $9 - WHERE id = $1 AND agent_id = $2 AND status IN ('queued', 'running') - RETURNING service_id, created_at`, - jobID, agentID, string(f.Status), f.Error, f.Output, f.OutputTruncated, f.Ref, startedAt, f.FinishedAt).Scan(&serviceID, &createdAt) + to_ref = CASE WHEN $7::text <> '' THEN $7::text ELSE j.to_ref END, + started_at = COALESCE($8, j.started_at), finished_at = $9 + FROM (SELECT id, to_ref FROM update_jobs WHERE id = $1) requested + WHERE j.id = requested.id AND j.agent_id = $2 AND j.status IN ('queued', 'running') + RETURNING j.service_id, j.created_at, requested.to_ref`, + jobID, agentID, string(f.Status), f.Error, f.Output, f.OutputTruncated, f.Ref, startedAt, f.FinishedAt).Scan(&serviceID, &createdAt, &requestedRef) if errors.Is(err, pgx.ErrNoRows) { return false, nil } @@ -221,12 +226,13 @@ func (s *Store) FinishJob(ctx context.Context, agentID, jobID int64, f model.Job return false, fmt.Errorf("finish job: %w", err) } if f.Status == model.JobSucceeded { + requestedRef, _, _ = strings.Cut(requestedRef, "@") if _, err := tx.Exec(ctx, ` UPDATE services SET desired_ref = $2, - available_ref = CASE WHEN available_at <= $3 THEN '' ELSE available_ref END, - available_source = CASE WHEN available_at <= $3 THEN '' ELSE available_source END, - available_at = CASE WHEN available_at <= $3 THEN NULL ELSE available_at END - WHERE id = $1`, serviceID, f.Ref, createdAt); err != nil { + available_ref = CASE WHEN available_ref = $4 AND available_at <= $3 THEN '' ELSE available_ref END, + available_source = CASE WHEN available_ref = $4 AND available_at <= $3 THEN '' ELSE available_source END, + available_at = CASE WHEN available_ref = $4 AND available_at <= $3 THEN NULL ELSE available_at END + WHERE id = $1`, serviceID, f.Ref, createdAt, requestedRef); err != nil { return false, fmt.Errorf("finish job: desired ref: %w", err) } } diff --git a/internal/master/store/migrations/0003_webhook.sql b/internal/master/store/migrations/0003_webhook.sql index ed7114b..0ea2bc2 100644 --- a/internal/master/store/migrations/0003_webhook.sql +++ b/internal/master/store/migrations/0003_webhook.sql @@ -1,6 +1,6 @@ ALTER TABLE services ADD COLUMN available_ref text NOT NULL DEFAULT ''; ALTER TABLE services ADD COLUMN available_source text NOT NULL DEFAULT '' - CHECK (available_source IN ('', 'webhook', 'poll')); + CHECK (available_source IN ('', 'webhook')); ALTER TABLE services ADD COLUMN available_at timestamptz; CREATE INDEX services_by_image_repo ON services (image_repo) WHERE removed_at IS NULL; diff --git a/internal/master/store/webhook_integration_test.go b/internal/master/store/webhook_integration_test.go index a8b4891..e047cec 100644 --- a/internal/master/store/webhook_integration_test.go +++ b/internal/master/store/webhook_integration_test.go @@ -88,18 +88,20 @@ func TestUpsertKeepsUpdateMode(t *testing.T) { assert.Equal(t, model.ModeAuto, got.UpdateMode) } -func TestFinishJob_ClearsAvailableOnlyWhenJobIsNewer(t *testing.T) { +func TestFinishJob_ClearsAvailableOnlyWhenThatVersionWasDeployed(t *testing.T) { succeed := model.JobFinish{Status: model.JobSucceeded, Ref: "r:2@sha256:d", StartedAt: t0, FinishedAt: t0} tests := []struct { - name string - availableAt time.Time // job is created at t0 - finish model.JobFinish - wantCleared bool + name string + availableRef string + availableAt time.Time // job is created at t0 for r:2 + finish model.JobFinish + wantCleared bool }{ - {"suggestion before the job", t0.Add(-time.Minute), succeed, true}, - {"suggestion at the same instant", t0, succeed, true}, - {"suggestion after the job was created", t0.Add(time.Minute), succeed, false}, - {"failed job keeps the suggestion", t0.Add(-time.Minute), + {"same ref, suggestion before the job", "r:2", t0.Add(-time.Minute), succeed, true}, + {"same ref, suggestion at the same instant", "r:2", t0, succeed, true}, + {"same ref, suggestion after the job was created", "r:2", t0.Add(time.Minute), succeed, false}, + {"different ref keeps the suggestion", "r:3", t0.Add(-time.Minute), succeed, false}, + {"failed job keeps the suggestion", "r:2", t0.Add(-time.Minute), model.JobFinish{Status: model.JobFailed, Error: "boom", StartedAt: t0, FinishedAt: t0}, false}, } for _, tt := range tests { @@ -107,8 +109,10 @@ func TestFinishJob_ClearsAvailableOnlyWhenJobIsNewer(t *testing.T) { s := openTestStore(t) ctx := context.Background() agent, svc := jobFixture(t, s, "a") - require.NoError(t, s.SetAvailable(ctx, svc, model.Available{Ref: "r:2", Source: model.SourceWebhook, At: tt.availableAt})) - j, err := s.CreateJob(ctx, newJob(svc, agent)) + require.NoError(t, s.SetAvailable(ctx, svc, model.Available{Ref: tt.availableRef, Source: model.SourceWebhook, At: tt.availableAt})) + nj := newJob(svc, agent) + nj.ToRef = "r:2" + j, err := s.CreateJob(ctx, nj) require.NoError(t, err) ok, err := s.FinishJob(ctx, agent, j.ID, tt.finish) @@ -122,7 +126,7 @@ func TestFinishJob_ClearsAvailableOnlyWhenJobIsNewer(t *testing.T) { assert.Empty(t, got.AvailableSource) assert.Nil(t, got.AvailableAt) } else { - assert.Equal(t, "r:2", got.AvailableRef) + assert.Equal(t, tt.availableRef, got.AvailableRef) assert.Equal(t, model.SourceWebhook, got.AvailableSource) assert.NotNil(t, got.AvailableAt) } diff --git a/internal/master/web/server_test.go b/internal/master/web/server_test.go index 4341f5c..52e40b4 100644 --- a/internal/master/web/server_test.go +++ b/internal/master/web/server_test.go @@ -616,7 +616,7 @@ func TestOverview_ShowsModeToggleAndAvailableBadge(t *testing.T) { assert.Contains(t, body, `name="return" value="overview"`, path) assert.Contains(t, body, `class="badge badge-update" title="gitea.shcizo.se/shcizo/slusk:1.5.0"`, path) assert.Contains(t, body, ``, "auto services toggle to manual: "+path) - assert.Contains(t, body, `disabled title="`+model.ReasonSelf+`"`, "blocked manual services get a disabled button: "+path) + assert.Contains(t, body, `disabled>Make auto `+model.ReasonSelf+``, "blocked manual services get a disabled button: "+path) } } diff --git a/internal/master/web/templates/overview.html b/internal/master/web/templates/overview.html index ed03317..3ff03b4 100644 --- a/internal/master/web/templates/overview.html +++ b/internal/master/web/templates/overview.html @@ -24,7 +24,7 @@ {{if eq .UpdateMode "auto"}} {{else}} - {{end}} + {{if .AutoBlocked}} {{.AutoBlocked}}{{end}}{{end}} diff --git a/internal/master/web/views.go b/internal/master/web/views.go index e811856..361d7e1 100644 --- a/internal/master/web/views.go +++ b/internal/master/web/views.go @@ -22,7 +22,8 @@ type serviceRow struct { // groupServices groups machine-bound services per agent and Swarm services // in one group, keeping the store's order (machines first, Swarm last). -// autoBlocked returns the reason a service cannot be set to auto, or "". +// autoBlocked is called for every service and returns the reason it cannot be +// set to auto, or "" when it can. func groupServices(services []model.Service, autoBlocked func(model.Service) string) []serviceGroup { var groups []serviceGroup index := map[string]int{} diff --git a/internal/master/webhook/webhook.go b/internal/master/webhook/webhook.go index f061374..36ebfbf 100644 --- a/internal/master/webhook/webhook.go +++ b/internal/master/webhook/webhook.go @@ -33,10 +33,17 @@ const maxBodyBytes = 64 << 10 // no broker signal arrives (signals to a slow subscriber can be dropped). const defaultPollEvery = 5 * time.Second +// finalReadTimeout bounds the last read of pending jobs after the wait ends. +const finalReadTimeout = 2 * time.Second + // tagPattern is Docker's tag grammar. It keeps spaces, colons and "@" out of // the ref the agent is asked to deploy. var tagPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9_.-]{0,127}$`) +// requestIDPattern limits a caller-supplied X-Request-ID to what is safe to +// echo in a header and write to the log. +var requestIDPattern = regexp.MustCompile(`^[A-Za-z0-9._-]{1,128}$`) + // Result statuses. const ( statusSucceeded = "succeeded" @@ -121,7 +128,7 @@ type response struct { func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { started := h.now() requestID := r.Header.Get("X-Request-ID") - if requestID == "" { + if !requestIDPattern.MatchString(requestID) { requestID = newRequestID() } w.Header().Set("X-Request-ID", requestID) @@ -175,7 +182,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { resp := response{RequestID: requestID, Image: req.Image, Tag: req.Tag, Matched: len(services)} pending := map[int]int64{} // index into resp.Results -> job ID for _, svc := range services { - res, job := h.handle(work, svc, req.Tag, started) + res, job := h.handle(work, svc, req.Tag, started, log) resp.Results = append(resp.Results, res) if job != nil { pending[len(resp.Results)-1] = job.ID @@ -201,7 +208,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // handle records the available ref on svc and, in auto mode, starts an update. // It returns the result and, if the job is still active, that job. -func (h *Handler) handle(ctx context.Context, svc model.Service, tag string, started time.Time) (result, *model.Job) { +func (h *Handler) handle(ctx context.Context, svc model.Service, tag string, started time.Time, log *slog.Logger) (result, *model.Job) { res := result{ServiceID: svc.ID, Service: svc.DisplayName(), Kind: string(svc.Kind), Mode: svc.UpdateMode} // The ref is built from the service's own repo, never from the request @@ -212,13 +219,14 @@ func (h *Handler) handle(ctx context.Context, svc model.Service, tag string, sta } else { ref = jobs.FollowedRef(svc.ActualRef) } + if ref == "" { + return h.finish(res, statusRefused, "no tag given and the service follows no tag", started), nil + } res.ToRef = ref - if ref != "" { - err := h.store.SetAvailable(ctx, svc.ID, model.Available{Ref: ref, Source: model.SourceWebhook, At: h.now()}) - if err != nil { - h.log.Error("webhook_set_available_failed", "service_id", svc.ID, "error", err) - return h.finish(res, statusFailed, "could not record available version: "+err.Error(), started), nil - } + err := h.store.SetAvailable(ctx, svc.ID, model.Available{Ref: ref, Source: model.SourceWebhook, At: h.now()}) + if err != nil { + log.Error("webhook_set_available_failed", "service_id", svc.ID, "error", err) + return h.finish(res, statusFailed, "could not record available version", started), nil } if svc.UpdateMode != model.ModeAuto { return h.finish(res, statusMarked, "", started), nil @@ -233,8 +241,8 @@ func (h *Handler) handle(ctx context.Context, svc model.Service, tag string, sta h.setJob(&res, job) return h.finish(res, statusBusy, err.Error(), started), nil case err != nil: - h.log.Error("webhook_start_failed", "service_id", svc.ID, "error", err) - return h.finish(res, statusFailed, err.Error(), started), nil + log.Error("webhook_start_failed", "service_id", svc.ID, "error", err) + return h.finish(res, statusFailed, "could not start job", started), nil } h.setJob(&res, job) if job.Active() { @@ -280,6 +288,11 @@ func (h *Handler) wait(parent context.Context, sub <-chan events.Topic, resp *re for len(pending) > 0 { select { case <-ctx.Done(): + // A job may have finished while the timer fired: look once more, + // on a context that outlives the request. + final, stop := context.WithTimeout(context.WithoutCancel(parent), finalReadTimeout) + h.readPending(final, resp, pending, started, log) + stop() for i := range pending { resp.Results[i] = h.finish(resp.Results[i], statusTimeout, "still running when the request timed out", started) } @@ -290,16 +303,22 @@ func (h *Handler) wait(parent context.Context, sub <-chan events.Topic, resp *re } case <-ticker.C: } - for i, id := range pending { - job, err := h.store.GetJob(ctx, id) - if err != nil { - log.Warn("webhook_get_job_failed", "job_id", id, "error", err) - continue - } - if !job.Active() { - h.resolveJob(&resp.Results[i], job, started) - delete(pending, i) - } + h.readPending(ctx, resp, pending, started, log) + } +} + +// readPending re-reads every pending job and resolves the ones that are done. +func (h *Handler) readPending(ctx context.Context, resp *response, pending map[int]int64, + started time.Time, log *slog.Logger) { + for i, id := range pending { + job, err := h.store.GetJob(ctx, id) + if err != nil { + log.Warn("webhook_get_job_failed", "job_id", id, "error", err) + continue + } + if !job.Active() { + h.resolveJob(&resp.Results[i], job, started) + delete(pending, i) } } } diff --git a/internal/master/webhook/webhook_test.go b/internal/master/webhook/webhook_test.go index b2b5890..57fde56 100644 --- a/internal/master/webhook/webhook_test.go +++ b/internal/master/webhook/webhook_test.go @@ -289,10 +289,14 @@ func TestAvailableRefWithoutTag(t *testing.T) { e := newEnv(t, compose(1, model.ModeManual, tt.actualRef)) rec := e.post(`{"image":"` + slusk + `"}`) assert.Equal(t, 200, rec.Code) + r := decode(t, rec).Results[0] if tt.wantRef == "" { assert.Empty(t, e.store.avail) + assert.Equal(t, statusRefused, r.Status) + assert.Equal(t, "no tag given and the service follows no tag", r.Error) return } + assert.Equal(t, statusMarked, r.Status) require.Len(t, e.store.avail, 1) assert.Equal(t, tt.wantRef, e.store.avail[0].a.Ref) }) @@ -302,18 +306,56 @@ func TestAvailableRefWithoutTag(t *testing.T) { func TestAutoDigestOnlyServiceWithoutTagIsRefused(t *testing.T) { svc := compose(1, model.ModeAuto, slusk+"@sha256:old") e := newEnv(t, svc) - e.jobs.errFor[1] = &model.NotUpdatableError{Reason: model.ReasonNoTag} rec := e.post(`{"image":"` + slusk + `"}`) assert.Equal(t, 200, rec.Code) assert.Empty(t, e.store.avail) - calls := e.jobs.startCalls() - require.Len(t, calls, 1) - assert.Empty(t, calls[0].req.Ref, "no request tag and nothing followed: the job layer decides") + assert.Empty(t, e.jobs.startCalls(), "no target ref: nothing to start") r := decode(t, rec).Results[0] assert.Equal(t, statusRefused, r.Status) - assert.Equal(t, model.ReasonNoTag, r.Error) + assert.Equal(t, "no tag given and the service follows no tag", r.Error) +} + +func TestValidTags(t *testing.T) { + for _, tag := range []string{"latest", "1.2.3-rc.1", "v1_2", strings.Repeat("a", 128)} { + e := newEnv(t, compose(1, model.ModeManual, slusk+":1")) + rec := e.post(`{"image":"` + slusk + `","tag":"` + tag + `"}`) + assert.Equal(t, 200, rec.Code, tag) + require.Len(t, e.store.avail, 1, tag) + assert.Equal(t, slusk+":"+tag, e.store.avail[0].a.Ref) + } + e := newEnv(t, compose(1, model.ModeManual, slusk+":1")) + rec := e.post(`{"image":"` + slusk + `","tag":"` + strings.Repeat("a", 129) + `"}`) + assert.Equal(t, 400, rec.Code) +} + +func TestUnsafeRequestIDIsReplaced(t *testing.T) { + for _, id := range []string{"has space", "bad/slash", strings.Repeat("a", 129)} { + e := newEnv(t, compose(1, model.ModeManual, slusk+":1")) + rec := e.post(`{"image":"`+slusk+`"}`, "X-Request-ID", id) + got := rec.Header().Get("X-Request-ID") + assert.NotEqual(t, id, got) + assert.Regexp(t, `^[0-9a-f]{32}$`, got) + } + e := newEnv(t, compose(1, model.ModeManual, slusk+":1")) + rec := e.post(`{"image":"`+slusk+`"}`, "X-Request-ID", strings.Repeat("a", 128)) + assert.Equal(t, strings.Repeat("a", 128), rec.Header().Get("X-Request-ID")) +} + +func TestJobFinishedJustBeforeTimeoutIsNotReportedAsTimeout(t *testing.T) { + e := newEnv(t, compose(1, model.ModeAuto, slusk+":1.4.2")) + e.h.cfg.Timeout = 200 * time.Millisecond + e.h.pollEvery = time.Hour // only the final read after the deadline can see the result + go func() { + assert.Eventually(t, func() bool { return len(e.jobs.startCalls()) == 1 }, time.Second, time.Millisecond) + e.store.setJob(model.Job{ID: 1, Status: model.JobSucceeded}) // no Publish + }() + + rec := e.post(`{"image":"` + slusk + `"}`) + + assert.Equal(t, 200, rec.Code) + assert.Equal(t, statusSucceeded, decode(t, rec).Results[0].Status) } func TestAutoJobTargetIsBuiltFromServiceRepo(t *testing.T) {