containerd / containerd/containerd

CRI: pinned label never applied when pinned_images config value has no tag

Open
#13,314 1 comment 0 reactions 0 assignees View on GitHub
area/cri kind/bug
Dominant language
Go
Stars
21.3k
Forks
4.1k
Avg merge
2d 4h
Merged PRs (30d)
92

Description

## Description

When a `pinned_images` config value has no explicit tag (e.g. `localhost/kubernetes/pause`), the pinned label (`io.cri-containerd.pinned=pinned`) is never written onto the image. This means kubelet sees `Pinned: false` from `ImageStatus` and will garbage collect the image under disk pressure.

## Root cause

`getLabels` (`internal/cri/server/images/image_pull.go#L421-L428`) does a raw string comparison between the config value and the image ref:

\`\`\`go
for _, pinned := range c.config.PinnedImages {
if pinned == name { // exact string match, no normalization
labels[crilabels.PinnedImageLabelKey] = crilabels.PinnedImageLabelValue
}
}
\`\`\`

But `name` at the call site has already been normalized through `ParseDockerRef`, which appends `:latest` to any tagless ref:

\`\`\`go
namedRef, err := distribution.ParseDockerRef(name)
ref := namedRef.String() // "localhost/kubernetes/pause" → "localhost/kubernetes/pause:latest"
labels := c.getLabels(ctx, ref) // comparison always fails
\`\`\`

So `"localhost/kubernetes/pause"` (config) never equals `"localhost/kubernetes/pause:latest"` (normalized ref). The same mismatch affects the `UpdateImage` path (called from `ImageCreate`/`ImageUpdate` events and `CheckImages` at startup), so the label is never applied via any code path.

## Regression

This was introduced in commit ad4c9f8a9deaaafea37e4afce0b0b2c0d128e436, which refactored from a single `SandboxImage` string to the generic `PinnedImages` list. The **old code correctly normalized** the config value before comparing:

\`\`\`go
// Old code — worked correctly
sandboxNamedRef, err := distribution.ParseDockerRef(configSandboxImage)
sandboxRef := sandboxNamedRef.String() // normalizes tagless ref → :latest
if sandboxRef == name { // apples-to-apples
labels[crilabels.PinnedImageLabelKey] = crilabels.PinnedImageLabelValue
}
\`\`\`

The refactor dropped the `ParseDockerRef` call on the config side.

## The test that masked it

The same commit updated the "without tag" test case but worked around the regression instead of fixing it — by adding the already-normalized form to `pinnedImages` so the exact match would succeed:

https://github.com/containerd/containerd/blob/ad4c9f8a9deaaafea37e4afce0b0b2c0d128e436/pkg/cri/server/images/image_pull_test.go#L505-L508

\`\`\`go
// Test is named "without tag" but passes because :latest is explicitly listed
pinnedImages: []string{"k8s.gcr.io/pause", "k8s.gcr.io/pause:latest"},
pullImageName: "k8s.gcr.io/pause:latest",
\`\`\`

The tagless entry `"k8s.gcr.io/pause"` matches nothing and is dead code. The correct test would be:

\`\`\`go
pinnedImages: []string{"k8s.gcr.io/pause"}, // tagless only, as a user would configure
pullImageName: "k8s.gcr.io/pause:latest", // normalized form
// expected: still pinned — this FAILS with current code
\`\`\`

## Impact

1. Image exists in the store and is used as the sandbox image — everything appears healthy
2. Kubelet's image GC sees `Pinned: false` from `ImageStatus`
3. Under disk pressure, kubelet removes the pause/sandbox image
4. Next pod creation: `LocalResolve` fails, pull from local registry fails, pod creation fails across the entire node
5. Failure is silent until disk pressure is triggered

## Affected code

- Broken `getLabels`: https://github.com/containerd/containerd/blob/bc69a52680912da0c1ad9b876b21db5f7c0db99c/internal/cri/server/images/image_pull.go#L421-L428
- Regression commit: https://github.com/containerd/containerd/commit/ad4c9f8a9deaaafea37e4afce0b0b2c0d128e436

## Suggested fix

Normalize each pinned image config value through `ParseDockerRef` before comparing, restoring the behavior that existed before the refactor:

\`\`\`go
func (c *CRIImageService) getLabels(ctx context.Context, name string) map[string]string {
labels := map[string]string{crilabels.ImageLabelKey: crilabels.ImageLabelValue}
for _, pinned := range c.config.PinnedImages {
normalizedPinned, err := docker.ParseDockerRef(pinned)
if err == nil && normalizedPinned.String() == name {
labels[crilabels.PinnedImageLabelKey] = crilabels.PinnedImageLabelValue
}
}
return labels
}
\`\`\`

## Version

Reproducible on current `main` (bc69a52680912da0c1ad9b876b21db5f7c0db99c). Present since ad4c9f8a9deaaafea37e4afce0b0b2c0d128e436 (merged during containerd v2 development).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.