Skip to content

Commit 652fa4e

Browse files
committed
fix(gc): use accurate audit reason for unknown-media-type prune
removeUnknownMediaTypeManifestEntries reused syncStaleManifestRemoval, which emitted reason="staleManifestPrune" / "pruned stale manifest entry from index" for entries pruned because their media type is unsupported - misleading for log/audit consumers keying off reason. Parametrize the helper (now syncManifestRemoval) so the unknown-media-type prune emits reason="unknownMediaTypePrune" with a matching message, while the stale-prune callsites keep their existing output. Add audit-capture tests asserting both reasons. Addresses a PR #4236 review comment on gc.go:245. Signed-off-by: Sebastian Thees <thees@users.noreply.github.com>
1 parent 26015d4 commit 652fa4e

2 files changed

Lines changed: 208 additions & 12 deletions

File tree

pkg/storage/gc/gc.go

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,8 @@ func (gc GarbageCollect) removeUnknownMediaTypeManifestEntries(repo string, inde
239239
continue
240240
}
241241

242-
if err := gc.syncStaleManifestRemoval(repo, desc); err != nil {
242+
if err := gc.syncManifestRemoval(repo, desc, "unknownMediaTypePrune",
243+
"pruned unknown media type manifest entry from index"); err != nil {
243244
return err
244245
}
245246
}
@@ -283,7 +284,8 @@ func (gc GarbageCollect) removeStaleManifestEntries(repo string, index *ispec.In
283284

284285
for _, desc := range index.Manifests {
285286
if !existingBlobs[desc.Digest.String()] {
286-
if err := gc.syncStaleManifestRemoval(repo, desc); err != nil {
287+
if err := gc.syncManifestRemoval(repo, desc, "staleManifestPrune",
288+
"pruned stale manifest entry from index"); err != nil {
287289
return err
288290
}
289291

@@ -297,7 +299,8 @@ func (gc GarbageCollect) removeStaleManifestEntries(repo string, index *ispec.In
297299
}
298300

299301
if stale {
300-
if err := gc.syncStaleManifestRemoval(repo, desc); err != nil {
302+
if err := gc.syncManifestRemoval(repo, desc, "staleManifestPrune",
303+
"pruned stale manifest entry from index"); err != nil {
301304
return err
302305
}
303306

@@ -319,11 +322,12 @@ func (gc GarbageCollect) removeStaleManifestEntries(repo string, index *ispec.In
319322
return nil
320323
}
321324

322-
// syncStaleManifestRemoval best-effort syncs metaDB before a stale descriptor is removed from
323-
// index.Manifests (index.json is persisted later in cleanRepo). metaDB failures are logged but
324-
// do not abort stale pruning — storage repair must not depend on secondary index consistency.
325-
// The blob is typically already absent from storage; cosign signatures are cleaned up via tag annotation.
326-
func (gc GarbageCollect) syncStaleManifestRemoval(repo string, desc ispec.Descriptor) error {
325+
// syncManifestRemoval best-effort syncs metaDB before a descriptor is removed from index.Manifests
326+
// (index.json is persisted later in cleanRepo). metaDB failures are logged but do not abort pruning —
327+
// storage repair must not depend on secondary index consistency. The blob is typically already absent
328+
// from storage; cosign signatures are cleaned up via tag annotation. reason/message identify which
329+
// prune path (stale vs. unknown media type) triggered the removal, for log/audit consumers.
330+
func (gc GarbageCollect) syncManifestRemoval(repo string, desc ispec.Descriptor, reason, message string) error {
327331
tag, hasTag := getDescriptorTag(desc)
328332
ref := tag
329333
if ref == "" {
@@ -367,27 +371,27 @@ func (gc GarbageCollect) syncStaleManifestRemoval(repo string, desc ispec.Descri
367371
Str("reference", ref).
368372
Str("digest", desc.Digest.String()).
369373
Str("decision", "delete").
370-
Str("reason", "staleManifestPrune")
374+
Str("reason", reason)
371375

372376
if subjectDigest != "" {
373377
logEvent = logEvent.Str("subject", subjectDigest.String())
374378
}
375379

376-
logEvent.Msg("pruned stale manifest entry from index")
380+
logEvent.Msg(message)
377381

378382
if gc.auditLog != nil {
379383
auditEvent := gc.auditLog.Info().Str("module", "gc").
380384
Str("repository", repo).
381385
Str("reference", ref).
382386
Str("digest", desc.Digest.String()).
383387
Str("decision", "delete").
384-
Str("reason", "staleManifestPrune")
388+
Str("reason", reason)
385389

386390
if subjectDigest != "" {
387391
auditEvent = auditEvent.Str("subject", subjectDigest.String())
388392
}
389393

390-
auditEvent.Msg("pruned stale manifest entry from index")
394+
auditEvent.Msg(message)
391395
}
392396

393397
return nil

pkg/storage/gc/gc_test.go

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3047,6 +3047,198 @@ func TestGCUnknownMediaTypeManifestPruned(t *testing.T) {
30473047
})
30483048
}
30493049

3050+
// auditLine is the subset of audit-log JSON fields relevant to prune-reason assertions.
3051+
type auditLine struct {
3052+
Digest string `json:"digest"`
3053+
Reason string `json:"reason"`
3054+
Message string `json:"message"`
3055+
}
3056+
3057+
// readAuditLines parses a temp audit-log file's newline-delimited JSON entries.
3058+
func readAuditLines(t *testing.T, path string) []auditLine {
3059+
t.Helper()
3060+
3061+
buf, err := os.ReadFile(path)
3062+
So(err, ShouldBeNil)
3063+
3064+
var lines []auditLine
3065+
3066+
for _, raw := range bytes.Split(bytes.TrimSpace(buf), []byte("\n")) {
3067+
if len(raw) == 0 {
3068+
continue
3069+
}
3070+
3071+
var line auditLine
3072+
3073+
err := json.Unmarshal(raw, &line)
3074+
So(err, ShouldBeNil)
3075+
3076+
lines = append(lines, line)
3077+
}
3078+
3079+
return lines
3080+
}
3081+
3082+
// TestGCUnknownMediaTypePruneReason guards that the unknown-media-type index prune emits its own
3083+
// accurate audit reason/message, distinct from the shared stale-prune helper's output, so audit
3084+
// consumers can tell the two prune paths apart (SPEC.md §2).
3085+
func TestGCUnknownMediaTypePruneReason(t *testing.T) {
3086+
Convey("index.json entry with unsupported manifest media type", t, func() {
3087+
log := zlog.NewTestLogger()
3088+
3089+
auditPath := path.Join(t.TempDir(), "audit.log")
3090+
audit := zlog.NewAuditLogger("debug", auditPath)
3091+
3092+
metrics := newTestMetricsServer(t, log)
3093+
3094+
rootDir := t.TempDir()
3095+
imgStore := local.NewImageStore(rootDir, false, false, log, metrics, nil, nil, nil, nil)
3096+
3097+
storeController := storage.StoreController{}
3098+
storeController.DefaultStore = imgStore
3099+
3100+
ctx := context.Background()
3101+
repoName := "gc-unknown-media-type-reason"
3102+
3103+
unsupportedMediaType := "application/vnd.oci.artifact.manifest.v1+json"
3104+
3105+
unknown := CreateRandomImage()
3106+
err := WriteImageToFileSystem(unknown, repoName, "unknown", storeController)
3107+
So(err, ShouldBeNil)
3108+
3109+
// rewrite the unknown image's manifest with an unsupported media type, re-hash it, write the
3110+
// new blob, and re-point/re-type its index.json descriptor - mirroring
3111+
// TestGCUnknownMediaTypeManifestPruned's fixture construction.
3112+
unknownBuf, err := os.ReadFile(path.Join(rootDir, repoName, "blobs",
3113+
unknown.ManifestDescriptor.Digest.Algorithm().String(), unknown.ManifestDescriptor.Digest.Encoded()))
3114+
So(err, ShouldBeNil)
3115+
3116+
var unknownManifest ispec.Manifest
3117+
3118+
err = json.Unmarshal(unknownBuf, &unknownManifest)
3119+
So(err, ShouldBeNil)
3120+
3121+
unknownManifest.MediaType = unsupportedMediaType
3122+
3123+
unknownBuf, err = json.Marshal(unknownManifest)
3124+
So(err, ShouldBeNil)
3125+
3126+
unknownDigest := godigest.FromBytes(unknownBuf)
3127+
3128+
err = os.WriteFile(path.Join(rootDir, repoName, "blobs", unknownDigest.Algorithm().String(), unknownDigest.Encoded()),
3129+
unknownBuf, storageConstants.DefaultFilePerms)
3130+
So(err, ShouldBeNil)
3131+
3132+
indexJSONBuf, err := os.ReadFile(path.Join(rootDir, repoName, "index.json"))
3133+
So(err, ShouldBeNil)
3134+
3135+
var indexJSON ispec.Index
3136+
3137+
err = json.Unmarshal(indexJSONBuf, &indexJSON)
3138+
So(err, ShouldBeNil)
3139+
3140+
for idx, desc := range indexJSON.Manifests {
3141+
if desc.Digest == unknown.ManifestDescriptor.Digest {
3142+
indexJSON.Manifests[idx].Digest = unknownDigest
3143+
indexJSON.Manifests[idx].MediaType = unsupportedMediaType
3144+
}
3145+
}
3146+
3147+
indexJSONBuf, err = json.Marshal(indexJSON)
3148+
So(err, ShouldBeNil)
3149+
3150+
err = os.WriteFile(path.Join(rootDir, repoName, "index.json"), indexJSONBuf, storageConstants.DefaultFilePerms)
3151+
So(err, ShouldBeNil)
3152+
3153+
time.Sleep(1 * time.Second)
3154+
3155+
gcInstance := gc.NewGarbageCollect(imgStore, nil, gc.Options{
3156+
Delay: 1 * time.Second,
3157+
ImageRetention: config.ImageRetention{
3158+
Delay: 1 * time.Second,
3159+
},
3160+
}, audit, log, metrics)
3161+
3162+
err = gcInstance.CleanRepo(ctx, repoName)
3163+
So(err, ShouldBeNil)
3164+
3165+
lines := readAuditLines(t, auditPath)
3166+
3167+
var found *auditLine
3168+
3169+
for idx := range lines {
3170+
if lines[idx].Digest == unknownDigest.String() {
3171+
found = &lines[idx]
3172+
3173+
break
3174+
}
3175+
}
3176+
3177+
So(found, ShouldNotBeNil)
3178+
So(found.Reason, ShouldEqual, "unknownMediaTypePrune")
3179+
So(found.Message, ShouldEqual, "pruned unknown media type manifest entry from index")
3180+
})
3181+
}
3182+
3183+
// TestGCStaleManifestPruneReason is the AC-2 regression guard: a stale-blob prune (missing blob path,
3184+
// removeStaleManifestEntries) must keep emitting the unchanged "staleManifestPrune" reason/message after
3185+
// syncManifestRemoval was parametrized for the unknown-media-type prune reason.
3186+
func TestGCStaleManifestPruneReason(t *testing.T) {
3187+
Convey("index.json entry pointing at a missing blob", t, func() {
3188+
log := zlog.NewTestLogger()
3189+
3190+
auditPath := path.Join(t.TempDir(), "audit.log")
3191+
audit := zlog.NewAuditLogger("debug", auditPath)
3192+
3193+
metrics := newTestMetricsServer(t, log)
3194+
3195+
rootDir := t.TempDir()
3196+
imgStore := local.NewImageStore(rootDir, false, false, log, metrics, nil, nil, nil, nil)
3197+
3198+
storeController := storage.StoreController{}
3199+
storeController.DefaultStore = imgStore
3200+
3201+
ctx := context.Background()
3202+
repoName := "gc-stale-manifest-reason"
3203+
3204+
stale := CreateRandomImage()
3205+
err := WriteImageToFileSystem(stale, repoName, "stale", storeController)
3206+
So(err, ShouldBeNil)
3207+
3208+
// delete the manifest blob itself so the descriptor points at a missing blob and is pruned by
3209+
// removeStaleManifestEntries's missing-blob path (gc.go's "!existingBlobs[...]" branch).
3210+
err = os.Remove(path.Join(rootDir, repoName, "blobs",
3211+
stale.ManifestDescriptor.Digest.Algorithm().String(), stale.ManifestDescriptor.Digest.Encoded()))
3212+
So(err, ShouldBeNil)
3213+
3214+
gcInstance := gc.NewGarbageCollect(imgStore, nil, gc.Options{
3215+
Delay: 1 * time.Second,
3216+
ImageRetention: config.ImageRetention{
3217+
Delay: 1 * time.Second,
3218+
},
3219+
}, audit, log, metrics)
3220+
3221+
err = gcInstance.CleanRepo(ctx, repoName)
3222+
So(err, ShouldBeNil)
3223+
3224+
lines := readAuditLines(t, auditPath)
3225+
3226+
var found *auditLine
3227+
3228+
for idx := range lines {
3229+
if lines[idx].Digest == stale.ManifestDescriptor.Digest.String() {
3230+
found = &lines[idx]
3231+
3232+
break
3233+
}
3234+
}
3235+
3236+
So(found, ShouldNotBeNil)
3237+
So(found.Reason, ShouldEqual, "staleManifestPrune")
3238+
So(found.Message, ShouldEqual, "pruned stale manifest entry from index")
3239+
})
3240+
}
3241+
30503242
// TestGCDryRunDeletesNothing guards DryRun's non-destructive-simulation contract: index.json pruning is
30513243
// already DryRun-gated, but blob GC re-reads the on-disk index.json independently and used to run
30523244
// unconditionally, so it could delete orphan blobs for real - including the config/layers of an

0 commit comments

Comments
 (0)