From caa6c81716857b6454a39b73392f151d0b4a7bc6 Mon Sep 17 00:00:00 2001 From: Samuel Enocsson Date: Mon, 12 Oct 2026 19:14:59 +0200 Subject: [PATCH] fix(master): treat a bare image as :latest and handle a revoked agent in Trigger (#7) --- internal/master/jobs/jobs.go | 32 +++++++++++++++++++++---- internal/master/jobs/jobs_test.go | 40 +++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/internal/master/jobs/jobs.go b/internal/master/jobs/jobs.go index f32e1ee..c1bc25b 100644 --- a/internal/master/jobs/jobs.go +++ b/internal/master/jobs/jobs.go @@ -4,6 +4,7 @@ package jobs import ( "context" + "errors" "log/slog" "time" @@ -53,6 +54,20 @@ func New(store Store, hub Hub, pub Publisher, agentImage string, log *slog.Logge } } +// target splits a service's actual ref into the repo and tag to deploy. A bare +// image ("postgres", no tag and no digest) means "latest", as it does for +// Docker. A ref with a digest but no tag follows no tag (ok is false). +func target(actualRef string) (repo, tag string, ok bool) { + ref := imageref.Parse(actualRef) + switch { + case ref.Tag != "": + return ref.Repo, ref.Tag, true + case ref.Digest == "": + return ref.Repo, "latest", true + } + return ref.Repo, "", false +} + // Updatable returns nil or a *model.NotUpdatableError. It looks only at the // service row, so the UI can call it for every page render. func (s *Service) Updatable(svc model.Service) error { @@ -63,7 +78,8 @@ func (s *Service) Updatable(svc model.Service) error { return &model.NotUpdatableError{Reason: model.ReasonSelf} case svc.Kind == protocol.KindContainer: return &model.NotUpdatableError{Reason: model.ReasonContainer} - case imageref.Parse(svc.ActualRef).Tag == "": + } + if _, _, ok := target(svc.ActualRef); !ok { return &model.NotUpdatableError{Reason: model.ReasonNoTag} } return nil @@ -82,25 +98,31 @@ func (s *Service) Trigger(ctx context.Context, serviceID int64, requestedBy stri if err := s.Updatable(svc); err != nil { return model.Job{}, err } - ref := imageref.Parse(svc.ActualRef) - target := ref.Repo + ":" + ref.Tag + repo, tag, _ := target(svc.ActualRef) // Updatable checked the tag + ref := repo + ":" + tag r, routeErr := s.hub.Route(svc) nj := model.NewJob{ - ServiceID: svc.ID, FromRef: svc.ActualRef, ToRef: target, + ServiceID: svc.ID, FromRef: svc.ActualRef, ToRef: ref, Trigger: model.TriggerManual, RequestedBy: requestedBy, CreatedAt: s.now(), } if routeErr == nil { nj.AgentID = &r.AgentID } job, err := s.store.CreateJob(ctx, nj) + if errors.Is(err, model.ErrAgentOffline) { + // The routed agent was revoked after Route. Record the attempt as a + // failed job that was never sent anywhere. + routeErr, nj.AgentID = err, nil + job, err = s.store.CreateJob(ctx, nj) + } if err != nil { return job, err } failure := routeErr if failure == nil { - start := protocol.JobStart{Kind: svc.Kind, Ref: target} + start := protocol.JobStart{Kind: svc.Kind, Ref: ref} if svc.Kind == protocol.KindSwarm { start.SwarmClusterID, start.SwarmService = svc.SwarmClusterID, svc.SwarmService } else { diff --git a/internal/master/jobs/jobs_test.go b/internal/master/jobs/jobs_test.go index c996276..dbd5412 100644 --- a/internal/master/jobs/jobs_test.go +++ b/internal/master/jobs/jobs_test.go @@ -21,6 +21,8 @@ type fakeStore struct { jobs map[int64]model.Job created []model.NewJob active *model.Job // returned with ErrJobActive when set + + revokedAgents map[int64]bool // CreateJob returns ErrAgentOffline for these } func (f *fakeStore) GetService(_ context.Context, id int64) (model.Service, error) { @@ -35,6 +37,9 @@ func (f *fakeStore) CreateJob(_ context.Context, nj model.NewJob) (model.Job, er if f.active != nil { return *f.active, model.ErrJobActive } + if nj.AgentID != nil && f.revokedAgents[*nj.AgentID] { + return model.Job{}, model.ErrAgentOffline + } f.created = append(f.created, nj) j := model.Job{ID: int64(len(f.created)), ServiceID: nj.ServiceID, AgentID: nj.AgentID, FromRef: nj.FromRef, ToRef: nj.ToRef, Trigger: nj.Trigger, RequestedBy: nj.RequestedBy, Status: model.JobQueued, CreatedAt: nj.CreatedAt} @@ -80,6 +85,8 @@ const selfImage = "gitea.shcizo.se/shcizo/package-updater:latest" var ( composeSvc = model.Service{ID: 1, Kind: protocol.KindCompose, ComposeProject: "slusk", ComposeService: "web", AgentID: ptr(int64(7)), ActualRef: "gitea.shcizo.se/shcizo/slusk:1.4.2@sha256:d1"} + bareSvc = model.Service{ID: 3, Kind: protocol.KindCompose, ComposeProject: "db", ComposeService: "pg", + AgentID: ptr(int64(7)), ActualRef: "postgres"} swarmSvc = model.Service{ID: 2, Kind: protocol.KindSwarm, SwarmClusterID: "c1", SwarmService: "gitea_gitea", ActualRef: "gitea/gitea:1.27.3@sha256:87a6"} ) @@ -102,9 +109,12 @@ func TestUpdatable(t *testing.T) { {"same repo as the agent image", model.Service{Kind: protocol.KindCompose, ActualRef: "gitea.shcizo.se/shcizo/package-updater:1.2.0@sha256:abc"}, model.ReasonSelf}, {"container", model.Service{Kind: protocol.KindContainer, ActualRef: "redis:7"}, model.ReasonContainer}, - {"no tag", model.Service{Kind: protocol.KindCompose, ActualRef: "redis@sha256:abc"}, model.ReasonNoTag}, - {"plain", model.Service{Kind: protocol.KindCompose, ActualRef: "postgres:17.10"}, ""}, - {"registry with port", model.Service{Kind: protocol.KindSwarm, ActualRef: "localhost:5000/foo:v1"}, ""}, + {"digest without tag", model.Service{Kind: protocol.KindCompose, ActualRef: "redis@sha256:abc"}, model.ReasonNoTag}, + {"bare image means latest", model.Service{Kind: protocol.KindCompose, ActualRef: "postgres"}, ""}, + {"bare image on a registry with port", model.Service{Kind: protocol.KindCompose, ActualRef: "localhost:5000/foo"}, ""}, + {"tag", model.Service{Kind: protocol.KindCompose, ActualRef: "postgres:16"}, ""}, + {"tag and digest", model.Service{Kind: protocol.KindCompose, ActualRef: "postgres:17.10@sha256:abc"}, ""}, + {"registry with port and tag", model.Service{Kind: protocol.KindSwarm, ActualRef: "localhost:5000/foo:v1"}, ""}, } s, _ := newService(&fakeStore{}, &fakeHub{}) for _, tt := range tests { @@ -140,6 +150,8 @@ func TestTrigger(t *testing.T) { wantStart: &protocol.JobStart{Kind: protocol.KindCompose, ComposeProject: "slusk", ComposeService: "web", Ref: "gitea.shcizo.se/shcizo/slusk:1.4.2"}}, {name: "swarm", serviceID: 2, hub: &fakeHub{route: ok}, wantStatus: model.JobQueued, wantCreate: true, wantStart: &protocol.JobStart{Kind: protocol.KindSwarm, SwarmClusterID: "c1", SwarmService: "gitea_gitea", Ref: "gitea/gitea:1.27.3"}}, + {name: "bare image deploys latest", serviceID: 3, hub: &fakeHub{route: ok}, wantStatus: model.JobQueued, wantCreate: true, + wantStart: &protocol.JobStart{Kind: protocol.KindCompose, ComposeProject: "db", ComposeService: "pg", Ref: "postgres:latest"}}, {name: "active job", serviceID: 1, hub: &fakeHub{route: ok}, active: &activeJob, wantErr: model.ErrJobActive, wantStatus: model.JobRunning}, {name: "agent offline", serviceID: 1, hub: &fakeHub{routeErr: model.ErrAgentOffline}, wantStatus: model.JobFailed, wantError: "agent offline", wantCreate: true}, @@ -153,7 +165,7 @@ func TestTrigger(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { st := &fakeStore{ - services: map[int64]model.Service{1: composeSvc, 2: swarmSvc}, + services: map[int64]model.Service{1: composeSvc, 2: swarmSvc, 3: bareSvc}, jobs: map[int64]model.Job{99: activeJob}, active: tt.active, } @@ -193,6 +205,26 @@ func TestTrigger(t *testing.T) { } } +// The routed agent can be revoked between Route and CreateJob; the store then +// refuses the job. Trigger records a failed job that was never sent. +func TestTrigger_AgentRevokedAfterRoute(t *testing.T) { + st := &fakeStore{ + services: map[int64]model.Service{1: composeSvc}, + jobs: map[int64]model.Job{}, + revokedAgents: map[int64]bool{7: true}, + } + h := &fakeHub{route: model.Route{AgentID: 7, ConnID: 70}} + s, _ := newService(st, h) + + job, err := s.Trigger(context.Background(), 1, "sub-1") + + require.NoError(t, err) + assert.Equal(t, model.JobFailed, job.Status) + assert.Equal(t, "agent offline", job.Error) + assert.Nil(t, job.AgentID) + assert.Empty(t, h.starts) +} + func TestTrigger_NotUpdatableCreatesNoJob(t *testing.T) { st := &fakeStore{ services: map[int64]model.Service{1: {ID: 1, Kind: protocol.KindContainer, ActualRef: "redis:7"}},