diff --git a/api/containerautomation/autoupdate.go b/api/containerautomation/autoupdate.go index 782a67ea7..333af7302 100644 --- a/api/containerautomation/autoupdate.go +++ b/api/containerautomation/autoupdate.go @@ -177,7 +177,7 @@ func (s *Service) updateEndpoint(endpoint *portainer.Endpoint, scope string, opt continue } - candidates = append(candidates, UpdateCandidate{ID: c.ID, Name: containerName(c.Names), ImageID: c.ImageID, Labels: c.Labels}) + candidates = append(candidates, UpdateCandidate{ID: c.ID, Name: containerName(c.Names), ImageID: c.ImageID, Image: c.Image, Labels: c.Labels}) } // Route and de-duplicate: one redeploy per stack per tick. @@ -193,7 +193,7 @@ func (s *Service) updateEndpoint(endpoint *portainer.Endpoint, scope string, opt } for _, st := range grouped.Stacks { - s.updateStack(endpoint, st) + s.updateStack(cli, endpoint, st) } } @@ -506,7 +506,11 @@ func (s *Service) cleanupOldImage(cli *dockerclient.Client, endpoint *portainer. // change or via a manual "Update now", and we do not fetch git every tick. // - file stacks: the deployer is driven directly with forcePullImage=true, // applying the image update immediately. -func (s *Service) updateStack(endpoint *portainer.Endpoint, st StackUpdate) { +// +// On a successful file-stack redeploy it emits one EventUpdated per member +// container that triggered the update (not a single aggregate stack event), each +// carrying the stack name and a best-effort post-redeploy new image id. +func (s *Service) updateStack(cli *dockerclient.Client, endpoint *portainer.Endpoint, st StackUpdate) { if st.IsGit { // Detect-only: leave git bookkeeping to the git redeploy path. Logged at // debug so it does not repeat at info on every tick (it would otherwise @@ -553,8 +557,42 @@ func (s *Service) updateStack(endpoint *portainer.Endpoint, st StackUpdate) { log.Info().Int("stack_id", st.StackID).Int("endpoint_id", int(endpoint.ID)). Msg("auto-update: redeployed compose stack with updated images") - s.notifier.Notify(Event{ - Kind: EventUpdated, EndpointID: int(endpoint.ID), StackID: st.StackID, - Message: "redeployed compose stack with updated images", - }) + + // One notification PER updated container (the maintainer's requirement), each + // showing the container's stack name. The stack was redeployed as a whole, so the + // per-container new image id is not in hand; re-inspect each container by its + // (compose-stable) name to fill in the "new" digest best-effort. A failed inspect + // leaves NewDigest empty and the message falls back to "image updated" — never a + // blocked delivery. + for _, c := range st.Containers { + s.notifier.Notify(Event{ + Kind: EventUpdated, EndpointID: int(endpoint.ID), StackID: st.StackID, + StackName: c.Labels[composeProjectLabel], ContainerName: c.Name, + Image: c.Image, OldDigest: c.ImageID, NewDigest: s.inspectImageID(cli, c.Name), + Message: "updated stack container", + }) + } +} + +// inspectImageID re-inspects a container by its (compose-stable) name after a stack +// redeploy to recover the new local image id for the update notification. It is +// best-effort: any failure (or an empty name) yields "", and the caller degrades the +// message to "image updated" rather than blocking delivery. The inspect is bounded +// like every other engine call so a hung engine cannot stall the tick. +func (s *Service) inspectImageID(cli *dockerclient.Client, containerName string) string { + if containerName == "" { + return "" + } + + ctx, cancel := context.WithTimeout(s.baseCtx, endpointTimeout) + defer cancel() + + inspect, err := cli.ContainerInspect(ctx, containerName) + if err != nil { + log.Debug().Err(err).Str("container", containerName). + Msg("auto-update: unable to inspect stack container for its new image id, notifying without it") + return "" + } + + return inspect.Image } diff --git a/api/containerautomation/autoupdate_test.go b/api/containerautomation/autoupdate_test.go new file mode 100644 index 000000000..00e80ab1d --- /dev/null +++ b/api/containerautomation/autoupdate_test.go @@ -0,0 +1,148 @@ +package containerautomation + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/api/datastore" + "github.com/portainer/portainer/api/internal/testhelpers" + + "github.com/docker/docker/api/types/container" + dockerclient "github.com/docker/docker/client" + "github.com/stretchr/testify/require" +) + +// newStackInspectClient builds a Docker client wired to a test server that answers +// ContainerInspect by name, returning the given new image id. It is the seam the +// post-redeploy best-effort "new digest" re-inspect uses. +func newStackInspectClient(t *testing.T, newImageIDByName map[string]string) *dockerclient.Client { + t.Helper() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + for name, imageID := range newImageIDByName { + if strings.HasSuffix(r.URL.Path, "/containers/"+name+"/json") { + _ = json.NewEncoder(w).Encode(container.InspectResponse{ + ContainerJSONBase: &container.ContainerJSONBase{ID: name, Image: imageID}, + Config: &container.Config{}, + }) + return + } + } + + http.Error(w, "not found", http.StatusNotFound) + })) + t.Cleanup(srv.Close) + + cli, err := dockerclient.NewClientWithOpts( + dockerclient.WithHost(srv.URL), + dockerclient.WithHTTPClient(http.DefaultClient), + ) + require.NoError(t, err) + + return cli +} + +// TestUpdateStackEmitsPerContainerEvents proves the maintainer's requirement: a +// (file) stack redeploy emits one EventUpdated PER updated member container, each +// carrying the compose stack name (from the container's label, not a Stack().Read) +// and a best-effort post-redeploy new image id — never a single aggregate stack +// event. +func TestUpdateStackEmitsPerContainerEvents(t *testing.T) { + _, store := datastore.MustNewTestStore(t, true, false) + + // A stack author must exist for registry resolution; an admin resolves to the + // (empty) registry set without needing endpoint/team wiring. + require.NoError(t, store.User().Create(&portainer.User{ID: 1, Username: "auto", Role: portainer.AdministratorRole})) + + endpoint := &portainer.Endpoint{ID: 1, Name: "nebula.lc"} + require.NoError(t, store.Endpoint().Create(endpoint)) + + require.NoError(t, store.Stack().Create(&portainer.Stack{ + ID: 7, EndpointID: 1, Name: "cache-demo", Type: portainer.DockerComposeStack, CreatedBy: "auto", + })) + + const ( + oldEsphome = "sha256:59b94983c73a000000000000000000000000000000000000000000000000aaaa" + newEsphome = "sha256:2231ca5d676d000000000000000000000000000000000000000000000000bbbb" + oldOther = "sha256:1111111111110000000000000000000000000000000000000000000000000000" + newOther = "sha256:2222222222220000000000000000000000000000000000000000000000000000" + ) + + cli := newStackInspectClient(t, map[string]string{ + "esphome": newEsphome, + "other": newOther, + }) + + rec := &recordingNotifier{} + s := &Service{ + baseCtx: context.Background(), + dataStore: store, + stackDeployer: testhelpers.NewTestStackDeployer(), + notifier: rec, + } + + st := StackUpdate{ + StackID: 7, + IsGit: false, + Containers: []UpdateCandidate{ + {Name: "esphome", ImageID: oldEsphome, Image: "esphome/esphome:latest", Labels: map[string]string{composeProjectLabel: "cache-demo"}}, + {Name: "other", ImageID: oldOther, Image: "redis:7", Labels: map[string]string{composeProjectLabel: "cache-demo"}}, + }, + } + + s.updateStack(cli, endpoint, st) + + require.Len(t, rec.events, 2, "one EventUpdated per updated member container, not one aggregate stack event") + + byContainer := map[string]Event{} + for _, e := range rec.events { + require.Equal(t, EventUpdated, e.Kind) + require.Equal(t, "cache-demo", e.StackName, "each per-container event carries the compose stack name") + require.Equal(t, 7, e.StackID) + byContainer[e.ContainerName] = e + } + + esphome, ok := byContainer["esphome"] + require.True(t, ok, "expected a per-container event for esphome") + require.Equal(t, oldEsphome, esphome.OldDigest) + require.Equal(t, newEsphome, esphome.NewDigest, "the new image id is recovered by re-inspecting the container after redeploy") + + other, ok := byContainer["other"] + require.True(t, ok, "expected a per-container event for other") + require.Equal(t, oldOther, other.OldDigest) + require.Equal(t, newOther, other.NewDigest) +} + +// TestUpdateStackGitIsDetectOnly guards that a git stack stays detect-only: it is +// not redeployed and emits no notification (its image update lands on the next git +// change or a manual update). +func TestUpdateStackGitIsDetectOnly(t *testing.T) { + _, store := datastore.MustNewTestStore(t, true, false) + + endpoint := &portainer.Endpoint{ID: 1, Name: "nebula.lc"} + require.NoError(t, store.Endpoint().Create(endpoint)) + + deployer := testhelpers.NewTestStackDeployer() + rec := &recordingNotifier{} + s := &Service{ + baseCtx: context.Background(), + dataStore: store, + stackDeployer: deployer, + notifier: rec, + } + + cli := newStackInspectClient(t, nil) + + s.updateStack(cli, endpoint, StackUpdate{ + StackID: 9, IsGit: true, + Containers: []UpdateCandidate{{Name: "esphome", Labels: map[string]string{composeProjectLabel: "cache-demo"}}}, + }) + + require.Empty(t, rec.events, "a git stack is detect-only, no per-container notification") + require.Zero(t, deployer.DeployComposeCallCount, "a git stack must not be redeployed here") +} diff --git a/api/containerautomation/labels.go b/api/containerautomation/labels.go index dff4544bf..d59b48217 100644 --- a/api/containerautomation/labels.go +++ b/api/containerautomation/labels.go @@ -161,15 +161,26 @@ type UpdateCandidate struct { ID string // Name is the container's primary name (no leading slash). It is stable across // a recreate and keys the update->rollback loop guard. - Name string + Name string + // ImageID is the pre-update local image id ("sha256:..."), the "old" digest in a + // per-container update notification. ImageID string - Labels map[string]string + // Image is the container's image reference (e.g. "nginx:latest"), carried for the + // notification. + Image string + Labels map[string]string } -// StackUpdate identifies a Portainer stack to redeploy once. +// StackUpdate identifies a Portainer stack to redeploy once, together with the +// affected member containers so each updated container can emit its own +// notification (with the stack name) after the redeploy. type StackUpdate struct { StackID int IsGit bool + // Containers are the outdated member containers that triggered this stack + // redeploy, threaded through from detection so a per-container notification can + // be emitted for each (name + old image id + image + labels/stack name). + Containers []UpdateCandidate } // GroupedUpdates partitions candidates into their apply paths, de-duplicating @@ -186,7 +197,10 @@ type GroupedUpdates struct { // so a stack with several outdated containers is redeployed only once. func groupContainersForUpdate(candidates []UpdateCandidate, stackLookup func(project string) *StackMatch) GroupedUpdates { grouped := GroupedUpdates{} - seenStacks := make(map[int]bool) + // stackIndex maps a stack id to its slot in grouped.Stacks so a stack is + // redeployed once, while every member container is still collected for its own + // notification (rather than discarded at the collapse). + stackIndex := make(map[int]int) for _, c := range candidates { routing := resolveContainerUpdateRouting(c.Labels, stackLookup) @@ -196,12 +210,14 @@ func groupContainersForUpdate(candidates []UpdateCandidate, stackLookup func(pro case UpdateExternal: grouped.External = append(grouped.External, c) case UpdateStack: - if seenStacks[routing.StackID] { - continue + idx, ok := stackIndex[routing.StackID] + if !ok { + grouped.Stacks = append(grouped.Stacks, StackUpdate{StackID: routing.StackID, IsGit: routing.IsGit}) + idx = len(grouped.Stacks) - 1 + stackIndex[routing.StackID] = idx } - seenStacks[routing.StackID] = true - grouped.Stacks = append(grouped.Stacks, StackUpdate{StackID: routing.StackID, IsGit: routing.IsGit}) + grouped.Stacks[idx].Containers = append(grouped.Stacks[idx].Containers, c) } } diff --git a/api/containerautomation/labels_test.go b/api/containerautomation/labels_test.go index cf9759aec..9b130bebc 100644 --- a/api/containerautomation/labels_test.go +++ b/api/containerautomation/labels_test.go @@ -195,9 +195,9 @@ func TestGroupContainersForUpdate(t *testing.T) { candidates := []UpdateCandidate{ {ID: "standalone-1"}, - {ID: "web-a", Labels: map[string]string{composeProjectLabel: "web"}}, - {ID: "web-b", Labels: map[string]string{composeProjectLabel: "web"}}, // same stack -> deduped - {ID: "api-a", Labels: map[string]string{composeProjectLabel: "api"}}, + {ID: "web-a", Name: "web-a", Labels: map[string]string{composeProjectLabel: "web"}}, + {ID: "web-b", Name: "web-b", Labels: map[string]string{composeProjectLabel: "web"}}, // same stack -> deduped redeploy, both kept as members + {ID: "api-a", Name: "api-a", Labels: map[string]string{composeProjectLabel: "api"}}, {ID: "ext-1", Labels: map[string]string{composeProjectLabel: "unknown"}}, } @@ -228,4 +228,21 @@ func TestGroupContainersForUpdate(t *testing.T) { if isGit, ok := got[4]; !ok || !isGit { t.Errorf("stack 4 = (%v, present=%v), want present git stack", isGit, ok) } + + // The stack is redeployed once, but every member container is threaded through + // (not discarded at the collapse) so each can emit its own notification. + members := map[int][]string{} + for _, st := range grouped.Stacks { + for _, c := range st.Containers { + members[st.StackID] = append(members[st.StackID], c.Name) + } + } + + if got := members[3]; len(got) != 2 || got[0] != "web-a" || got[1] != "web-b" { + t.Errorf("stack 3 members = %v, want [web-a web-b]", got) + } + + if got := members[4]; len(got) != 1 || got[0] != "api-a" { + t.Errorf("stack 4 members = %v, want [api-a]", got) + } } diff --git a/api/containerautomation/notify.go b/api/containerautomation/notify.go index 231fde7f8..c49b568b8 100644 --- a/api/containerautomation/notify.go +++ b/api/containerautomation/notify.go @@ -29,7 +29,13 @@ type Event struct { // by the webhook message. It may be empty for events keyed only by ID. ContainerName string StackID int - Image string + // StackName is the compose project (stack) name a container belongs to, sourced + // from its com.docker.compose.project label at detection time. It is set on a + // per-container update event for a stack member so the webhook can print a + // "Stack [name]" line without a StackID/Stack().Read round-trip; empty for + // standalone containers. + StackName string + Image string // OldDigest and NewDigest carry the pre/post image identities for an update // (image IDs, e.g. "sha256:59b9..."). They are threaded from the update call // site where they are known and left empty otherwise; the webhook notifier diff --git a/api/containerautomation/webhook.go b/api/containerautomation/webhook.go index a8599c38c..9c36cf550 100644 --- a/api/containerautomation/webhook.go +++ b/api/containerautomation/webhook.go @@ -144,8 +144,13 @@ func (n webhookNotifier) deliver(webhookURL, message string) { func (n webhookNotifier) formatMessage(settings *portainer.Settings, event Event) string { lines := []string{"Environment | " + n.environmentName(event.EndpointID)} - // Context line: the stack for stack-scoped events, otherwise the container. + // Context line: the stack for stack-scoped events, otherwise the container. A + // per-container stack-member update carries StackName (from the compose label), + // preferred over a StackID/Stack().Read round-trip; the container itself still + // names the action line below. switch { + case event.StackName != "": + lines = append(lines, fmt.Sprintf("Stack [%s]", event.StackName)) case event.StackID != 0: lines = append(lines, fmt.Sprintf("Stack [%s]", n.stackName(event.StackID))) case event.ContainerName != "": diff --git a/api/containerautomation/webhook_test.go b/api/containerautomation/webhook_test.go index 4610da2dc..ba589863b 100644 --- a/api/containerautomation/webhook_test.go +++ b/api/containerautomation/webhook_test.go @@ -205,6 +205,49 @@ func TestFormatMessageStackUpdate(t *testing.T) { } } +// TestFormatMessageStackMemberUpdate covers the per-container update of a +// stack-member container: the context line is the compose stack name (from +// StackName, no Stack().Read), the action line names the container with its +// old->new digests. This is the maintainer's target output. +func TestFormatMessageStackMemberUpdate(t *testing.T) { + n, store := newTestWebhookNotifier(t, "unused") + createEndpoint(t, store, 1, "nebula.lc") + + settings, _ := store.Settings().Settings() + + msg := n.formatMessage(settings, Event{ + Kind: EventUpdated, EndpointID: 1, StackID: 7, StackName: "cache-demo", + ContainerName: "esphome", + OldDigest: "sha256:59b94983c73aabcd", NewDigest: "sha256:2231ca5d676dabcd", + }) + + want := "Environment | nebula.lc\nStack [cache-demo]\nUpdate [esphome]: 59b94983c73a → 2231ca5d676d" + if msg != want { + t.Errorf("got:\n%q\nwant:\n%q", msg, want) + } +} + +// TestFormatMessageStackMemberUpdateNoNewDigest covers the best-effort fallback: +// when the post-redeploy new image id could not be recovered, the message still +// carries the stack and container and degrades the action line to "image updated" +// rather than blocking delivery. +func TestFormatMessageStackMemberUpdateNoNewDigest(t *testing.T) { + n, store := newTestWebhookNotifier(t, "unused") + createEndpoint(t, store, 1, "nebula.lc") + + settings, _ := store.Settings().Settings() + + msg := n.formatMessage(settings, Event{ + Kind: EventUpdated, EndpointID: 1, StackID: 7, StackName: "cache-demo", + ContainerName: "esphome", OldDigest: "sha256:59b94983c73aabcd", + }) + + want := "Environment | nebula.lc\nStack [cache-demo]\nUpdate [esphome]: image updated" + if msg != want { + t.Errorf("got:\n%q\nwant:\n%q", msg, want) + } +} + // TestFormatMessageAutoHeal covers the auto-heal message design. func TestFormatMessageAutoHeal(t *testing.T) { n, store := newTestWebhookNotifier(t, "unused") diff --git a/api/docker/images/status.go b/api/docker/images/status.go index dabdaf58b..f486b90f1 100644 --- a/api/docker/images/status.go +++ b/api/docker/images/status.go @@ -9,6 +9,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/filters" + dockerclient "github.com/docker/docker/client" portainer "github.com/portainer/portainer/api" consts "github.com/portainer/portainer/api/docker/consts" @@ -17,6 +18,7 @@ import ( "github.com/pkg/errors" "github.com/rs/zerolog/log" "golang.org/x/sync/errgroup" + "golang.org/x/sync/singleflight" ) // Status constants @@ -42,12 +44,32 @@ const ( statusCacheTTL = 5 * time.Minute errorStatusCacheTTL = 5 * time.Minute maxConcurrentStatusChecks = 8 + + // forcedRecheckMinInterval bounds how often a forced re-check (?force=true) + // actually contacts the registry for the same local imageID. A manual re-check + // bypasses the read caches and issues an outbound registry HEAD; because the + // endpoint is available to any env-authorized user with no throttle, an + // unbounded loop of forced calls would burn the instance's shared registry + // pull-rate quota. Within this window a forced call reuses the just-computed + // fresh result instead of re-HEADing. It is tied to the remoteDigestCache TTL so + // a genuine manual re-check still gets a fresh answer the first time. + forcedRecheckMinInterval = 5 * time.Second ) var ( statusCache = cache.New(statusCacheTTL, statusCacheTTL) remoteDigestCache = cache.New(5*time.Second, 5*time.Second) swarmID2NameCache = cache.New(5*time.Second, 5*time.Second) + + // forcedRecheckGroup coalesces concurrent forced re-checks of the SAME local + // imageID so N simultaneous ?force=true calls collapse to ONE registry HEAD + // (singleflight), rather than each issuing its own outbound HEAD. + forcedRecheckGroup singleflight.Group + // forcedResultCache holds the last successful forced-recompute result per + // imageID for forcedRecheckMinInterval, so rapid successive forced calls reuse it + // instead of re-HEADing the registry. Only successful results are stored (errors + // are never cached, so a transient failure does not suppress a real re-check). + forcedResultCache = cache.New(forcedRecheckMinInterval, forcedRecheckMinInterval) ) // Status holds Docker image analysis @@ -172,16 +194,30 @@ func (c *DigestClient) containerImageStatus(ctx context.Context, containerID str // and Skipped are cached too (only the error paths return early without caching). // // A forced re-check (force=true) skips this cached read and recomputes against - // the registry, then writes the fresh result back into the cache below. + // the registry (see forcedImageStatus), then writes the fresh result back into + // the cache below. if !force { if s, err := CachedResourceImageStatus(imageID); err == nil { return s, nil } + + return c.computeImageStatus(ctx, cli, imageID, container.Config.Image, false) } + return c.forcedImageStatus(ctx, cli, imageID, container.Config.Image) +} + +// computeImageStatus performs the full, uncached image-status computation for a +// resolved local imageID: it parses the configured image reference, inspects the +// local image for its repo digests/tags, compares them against the remote registry +// digest (honoring force for the short-lived remoteDigestCache read in checkStatus) +// and writes the fresh result back into the statusCache. It is the shared body of +// both the default (cache-miss) and forced-recompute paths; the default path's +// behaviour is unchanged from the previous inline implementation. +func (c *DigestClient) computeImageStatus(ctx context.Context, cli *dockerclient.Client, imageID, configImage string, force bool) (Status, error) { digs := make([]digest.Digest, 0) images := make([]*Image, 0) - if i, err := ParseImage(ParseImageOptions{Name: container.Config.Image}); err == nil { + if i, err := ParseImage(ParseImageOptions{Name: configImage}); err == nil { images = append(images, &i) } @@ -201,13 +237,54 @@ func (c *DigestClient) containerImageStatus(ctx context.Context, containerID str s, err := c.checkStatus(ctx, images, digs, force) if err != nil { - log.Debug().Str("image", container.Image).Err(err).Msg("fetching a certain image status") + log.Debug().Str("image", configImage).Err(err).Msg("fetching a certain image status") return Error, err } statusCache.Set(imageID, s, 0) - return s, err + return s, nil +} + +// forcedImageStatus recomputes the image status against the registry for a manual +// re-check while bounding the registry load a ?force=true call can cause: +// +// - forcedResultCache serves the just-computed fresh result for +// forcedRecheckMinInterval, so rapid successive forced calls reuse it instead +// of re-HEADing the registry (min-interval throttle). +// - forcedRecheckGroup (singleflight) shares one in-flight computation per +// imageID, so N concurrent forced calls collapse to a single registry HEAD. +// +// A successful recompute still repopulates both the statusCache and (via +// checkStatus) the remoteDigestCache, so an immediately following default read is +// served from cache. Failures are not cached, so a transient error never suppresses +// a genuine re-check. +func (c *DigestClient) forcedImageStatus(ctx context.Context, cli *dockerclient.Client, imageID, configImage string) (Status, error) { + if s, ok := forcedResultCache.Get(imageID); ok { + return s.(Status), nil + } + + v, err, _ := forcedRecheckGroup.Do(imageID, func() (any, error) { + // A concurrent forced call may have populated the result between our read + // above and entering the flight; reuse it rather than HEAD the registry again. + if s, ok := forcedResultCache.Get(imageID); ok { + return s.(Status), nil + } + + s, err := c.computeImageStatus(ctx, cli, imageID, configImage, true) + if err != nil { + return Error, err + } + + forcedResultCache.Set(imageID, s, 0) + + return s, nil + }) + if err != nil { + return Error, err + } + + return v.(Status), nil } func (c *DigestClient) ServiceImageStatus(ctx context.Context, serviceID string, endpoint *portainer.Endpoint) (Status, error) { diff --git a/api/docker/images/status_test.go b/api/docker/images/status_test.go index 2aaa54a28..5b404c26c 100644 --- a/api/docker/images/status_test.go +++ b/api/docker/images/status_test.go @@ -6,15 +6,19 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" + "sync/atomic" "testing" "time" portainer "github.com/portainer/portainer/api" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/image" dockerclient "github.com/docker/docker/client" "github.com/opencontainers/go-digest" "github.com/stretchr/testify/require" + imagetypes "go.podman.io/image/v5/types" ) // fakeClientFactory hands the DigestClient a Docker client wired to a test server. @@ -131,6 +135,159 @@ func TestCheckStatusForcedBypassesRemoteDigestCache(t *testing.T) { require.Equal(t, Error, forced) } +// forcedStubs wires a DigestClient to a fake Docker engine (plain HTTP) and a fake +// registry (TLS, insecure-skipped) so a forced recompute succeeds end-to-end and +// resolves an "updated" status. It exposes hit counters for the two expensive calls +// a forced recompute makes exactly once: the local image inspect (Docker) and the +// remote manifest HEAD (registry). +type forcedStubs struct { + client *DigestClient + containerID string + imageID string + imageInspect *int64 // Docker ImageInspectWithRaw hits + registryHEAD *int64 // registry manifest HEAD hits (the pull-rate-costly call) +} + +// newForcedStubs builds the stubs so the local repo digest matches the remote HEAD +// digest, i.e. a successful recompute yields images.Updated. registryDelay is slept +// inside the manifest HEAD handler to widen the window in which concurrent forced +// calls overlap (used by the singleflight-collapse test). +func newForcedStubs(t *testing.T, containerID, imageID string, registryDelay time.Duration) forcedStubs { + t.Helper() + + const matchDigest = "sha256:" + "cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc" + + var imageInspect, registryHEAD int64 + + registry := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v2/" { + w.WriteHeader(http.StatusOK) + return + } + if strings.Contains(r.URL.Path, "/manifests/") { + atomic.AddInt64(®istryHEAD, 1) + if registryDelay > 0 { + time.Sleep(registryDelay) + } + w.Header().Set("Docker-Content-Digest", matchDigest) + w.Header().Set("Content-Type", "application/vnd.docker.distribution.manifest.v2+json") + w.WriteHeader(http.StatusOK) + return + } + http.NotFound(w, r) + })) + t.Cleanup(registry.Close) + + regHost := strings.TrimPrefix(registry.URL, "https://") + imageRef := regHost + "/repo/app:latest" + + docker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/containers/"+containerID+"/json"): + _ = json.NewEncoder(w).Encode(container.InspectResponse{ + ContainerJSONBase: &container.ContainerJSONBase{ID: containerID, Image: imageID}, + Config: &container.Config{Image: imageRef}, + }) + case strings.Contains(r.URL.Path, "/images/") && strings.HasSuffix(r.URL.Path, "/json"): + atomic.AddInt64(&imageInspect, 1) + _ = json.NewEncoder(w).Encode(image.InspectResponse{ + ID: imageID, + RepoTags: []string{imageRef}, + RepoDigests: []string{regHost + "/repo/app@" + matchDigest}, + }) + default: + http.Error(w, "not found", http.StatusNotFound) + } + })) + t.Cleanup(docker.Close) + + cli, err := dockerclient.NewClientWithOpts( + dockerclient.WithHost(docker.URL), + dockerclient.WithHTTPClient(http.DefaultClient), + ) + require.NoError(t, err) + + // registryClient nil so RemoteDigest keeps c.sysCtx (insecure skip) rather than + // replacing it with an auth-only context; the stub registry uses a self-signed cert. + c := &DigestClient{ + clientFactory: fakeClientFactory{cli: cli}, + sysCtx: &imagetypes.SystemContext{DockerInsecureSkipTLSVerify: imagetypes.OptionalBoolTrue}, + } + + return forcedStubs{ + client: c, + containerID: containerID, + imageID: imageID, + imageInspect: &imageInspect, + registryHEAD: ®istryHEAD, + } +} + +// TestContainerImageStatusForcedSuccessRepopulatesCache proves the other half of +// the forced-recheck contract (the error-path tests only cover the bypass): a +// forced recompute that SUCCEEDS writes the fresh value back into the caches, so a +// subsequent DEFAULT (non-force) read is served from cache without a second local +// image inspect or a second registry HEAD. +func TestContainerImageStatusForcedSuccessRepopulatesCache(t *testing.T) { + const containerID = "force-success-container" + const imageID = "sha256:0000000000000000000000000000000000000000000000000000000000000002" + + stubs := newForcedStubs(t, containerID, imageID, 0) + defer EvictImageStatus(imageID) + defer forcedResultCache.Delete(imageID) + + forced, err := stubs.client.ContainerImageStatusForced(context.Background(), containerID, &portainer.Endpoint{}, "") + require.NoError(t, err) + require.Equal(t, Updated, forced) + require.Equal(t, int64(1), atomic.LoadInt64(stubs.imageInspect), "forced recompute inspects the local image once") + require.Equal(t, int64(1), atomic.LoadInt64(stubs.registryHEAD), "forced recompute HEADs the registry once") + + // The forced recompute must have repopulated the statusCache under the imageID. + cachedStatus, err := CachedResourceImageStatus(imageID) + require.NoError(t, err) + require.Equal(t, Updated, cachedStatus) + + // A following default read is served from the statusCache: no extra image + // inspect and, crucially, no extra registry HEAD. + def, err := stubs.client.ContainerImageStatus(context.Background(), containerID, &portainer.Endpoint{}, "") + require.NoError(t, err) + require.Equal(t, Updated, def) + require.Equal(t, int64(1), atomic.LoadInt64(stubs.imageInspect), "default read after a forced success must not re-inspect") + require.Equal(t, int64(1), atomic.LoadInt64(stubs.registryHEAD), "default read after a forced success must not re-HEAD the registry") +} + +// TestForcedRechecksCollapseToSingleRegistryHead proves the abuse mitigation: many +// concurrent forced re-checks of the same imageID collapse (singleflight) to a +// single outbound registry HEAD instead of one HEAD per call. +func TestForcedRechecksCollapseToSingleRegistryHead(t *testing.T) { + const containerID = "force-collapse-container" + const imageID = "sha256:0000000000000000000000000000000000000000000000000000000000000003" + + // A small delay in the manifest HEAD widens the overlap window so the concurrent + // callers pile up on the shared in-flight computation. + stubs := newForcedStubs(t, containerID, imageID, 100*time.Millisecond) + defer EvictImageStatus(imageID) + defer forcedResultCache.Delete(imageID) + + const callers = 16 + var wg sync.WaitGroup + wg.Add(callers) + for range callers { + go func() { + defer wg.Done() + s, err := stubs.client.ContainerImageStatusForced(context.Background(), containerID, &portainer.Endpoint{}, "") + require.NoError(t, err) + require.Equal(t, Updated, s) + }() + } + wg.Wait() + + require.Equal(t, int64(1), atomic.LoadInt64(stubs.registryHEAD), + "concurrent forced re-checks of the same image must collapse to a single registry HEAD") + require.Equal(t, int64(1), atomic.LoadInt64(stubs.imageInspect), + "concurrent forced re-checks of the same image must share a single local inspect") +} + func TestAggregateImageStatus(t *testing.T) { t.Parallel() diff --git a/app/react/docker/containers/ItemView/ContainerActionsSection/SecondaryActions/UpdateNowButton.test.tsx b/app/react/docker/containers/ItemView/ContainerActionsSection/SecondaryActions/UpdateNowButton.test.tsx index 238e69841..9dc1589b6 100644 --- a/app/react/docker/containers/ItemView/ContainerActionsSection/SecondaryActions/UpdateNowButton.test.tsx +++ b/app/react/docker/containers/ItemView/ContainerActionsSection/SecondaryActions/UpdateNowButton.test.tsx @@ -1,4 +1,4 @@ -import { render, screen } from '@testing-library/react'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; import { vi } from 'vitest'; import { COMPOSE_STACK_NAME_LABEL } from '@/react/constants'; @@ -11,11 +11,28 @@ const mockUseImageStatus = vi.fn(); const mockUseStacks = vi.fn(); const mockUseAuthorizations = vi.fn(); const mockMutate = vi.fn(); +const mockConfirmContainerRecreation = vi.fn(); +const mockConfirmStackUpdate = vi.fn(); vi.mock('@uirouter/react', () => ({ useRouter: () => ({ stateService: { go: vi.fn() } }), })); +// The two confirm dialogs are the async gate before the mutation dispatches; +// stub them so a test can drive the resolved (confirmed) branch. +vi.mock('@/react/docker/containers/ItemView/ConfirmRecreationModal', () => ({ + confirmContainerRecreation: (...args: unknown[]) => + mockConfirmContainerRecreation(...args), +})); + +vi.mock('@/react/common/stacks/common/confirm-stack-update', () => ({ + confirmStackUpdate: (...args: unknown[]) => mockConfirmStackUpdate(...args), +})); + +vi.mock('@/portainer/services/notifications', () => ({ + notifySuccess: vi.fn(), +})); + vi.mock('@/react/docker/containers/queries/useContainerImageStatus', () => ({ useContainerImageStatus: () => mockUseImageStatus(), })); @@ -118,4 +135,60 @@ describe('UpdateNowButton', () => { }); expect(screen.getByTestId('update-now-button')).toBeDisabled(); }); + + // apply() dispatch: clicking confirms, then routes to the mutation exactly once + // with the resolved pullImage, via the standalone (recreate) vs stack (redeploy) + // confirm dialog. + it('applies a standalone update via the recreate confirm, mutating once with pullImage', async () => { + setStatus('outdated'); + mockConfirmContainerRecreation.mockResolvedValue({ pullLatest: true }); + + renderButton(); // no compose label -> standalone + + fireEvent.click(screen.getByTestId('update-now-button')); + + await waitFor(() => expect(mockMutate).toHaveBeenCalledTimes(1)); + expect(mockConfirmContainerRecreation).toHaveBeenCalledTimes(1); + expect(mockConfirmStackUpdate).not.toHaveBeenCalled(); + expect(mockMutate).toHaveBeenCalledWith( + expect.objectContaining({ pullImage: true }), + expect.anything() + ); + }); + + it('applies a stack update via the stack confirm, mutating once with pullImage', async () => { + setStatus('outdated'); + mockUseStacks.mockReturnValue({ data: [composeStack], isLoading: false }); + mockUseAuthorizations.mockReturnValue({ authorized: true }); + mockConfirmStackUpdate.mockResolvedValue({ repullImageAndRedeploy: false }); + + renderButton({ + environmentId: 3, + labels: { [COMPOSE_STACK_NAME_LABEL]: 'my-stack' }, + }); + + fireEvent.click(screen.getByTestId('update-now-button')); + + await waitFor(() => expect(mockMutate).toHaveBeenCalledTimes(1)); + expect(mockConfirmStackUpdate).toHaveBeenCalledTimes(1); + expect(mockConfirmContainerRecreation).not.toHaveBeenCalled(); + expect(mockMutate).toHaveBeenCalledWith( + expect.objectContaining({ pullImage: false }), + expect.anything() + ); + }); + + it('does not mutate when the confirm dialog is dismissed', async () => { + setStatus('outdated'); + mockConfirmContainerRecreation.mockResolvedValue(false); + + renderButton(); + + fireEvent.click(screen.getByTestId('update-now-button')); + + await waitFor(() => + expect(mockConfirmContainerRecreation).toHaveBeenCalledTimes(1) + ); + expect(mockMutate).not.toHaveBeenCalled(); + }); });