refactor: Extract Index Child Classification Behind A Registry - #524
Merged
Merged
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces BuildKit-specific attestation handling with a generic in-toto manifest classifier. It integrates the classifier into artifact abstraction, generalizes attestation tests, and separates accessory-linking errors from artifact conflict recovery. ChangesManifest Accessory Classification
Sequence Diagram(s)sequenceDiagram
participant Abstractor
participant InTotoAttestationClassifier
participant RegistryClient
participant ArtifactManager
Abstractor->>InTotoAttestationClassifier: classify index descriptors
InTotoAttestationClassifier->>RegistryClient: load attestation manifest and statement blob
InTotoAttestationClassifier->>ArtifactManager: retrieve attestation and target artifacts
InTotoAttestationClassifier-->>Abstractor: return references and accessory candidate
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors OCI index child handling by introducing a ChildClassifier registry/chain in src/controller/artifact/manifest, moving in-toto attestation classification out of the abstractor and into a dedicated, registerable classifier.
Changes:
- Added a
ChildClassifierinterface + global classifier chain (ChildClassifiers) withRegisterChildClassifier/ClassifyChild. - Implemented and auto-registered an
InTotoAttestationClassifierto classify in-toto attestation manifests as accessories. - Updated index abstraction to delegate child classification to the classifier chain and removed the previous BuildKit-specific implementation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/controller/artifact/manifest/classifier.go | Introduces the classifier interface, global chain, and chain evaluation function. |
| src/controller/artifact/manifest/classifier_test.go | Adds tests for default registration and classifier chain behavior. |
| src/controller/artifact/manifest/attestation.go | Adds the in-toto attestation classifier and registers it in init(). |
| src/controller/artifact/manifest/attestation_test.go | Updates/ports helper tests to the new manifest package and renamed identifiers. |
| src/controller/artifact/buildkit_attestation.go | Removes the old BuildKit-specific attestation implementation previously tied to abstractor. |
| src/controller/artifact/abstractor.go | Switches index child handling to call manifest.ClassifyChild and avoids manifest identifier shadowing. |
| src/controller/artifact/abstractor_test.go | Updates fixtures/names and wires tests through the new classifier chain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controller/artifact/abstractor_test.go`:
- Around line 410-415: Update the test setup around manifest.ChildClassifiers to
save the existing classifier chain before replacing it, then register an
a.T().Cleanup callback that restores the saved chain after the test. Keep the
mock-backed classifier assignment unchanged during the test.
In `@src/controller/artifact/manifest/attestation.go`:
- Around line 188-194: Update the subject-digest matching logic around
subjectDigests and digestInIndex to collect unique matching digest references
rather than returning the first match; proceed only when exactly one sibling
target is found, and reject or return no result for ambiguous matches. Add a
regression test covering two matching digest algorithms that resolve to
different siblings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dff5ed6-2690-4dd5-80f7-2d6f684d90ef
📒 Files selected for processing (7)
src/controller/artifact/abstractor.gosrc/controller/artifact/abstractor_test.gosrc/controller/artifact/buildkit_attestation.gosrc/controller/artifact/manifest/attestation.gosrc/controller/artifact/manifest/attestation_test.gosrc/controller/artifact/manifest/classifier.gosrc/controller/artifact/manifest/classifier_test.go
💤 Files with no reviewable changes (1)
- src/controller/artifact/buildkit_attestation.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
Preview images for this PR are available in
Verify a preview image: Verify SBOM attestation: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controller/artifact/manifest/classifier.go`:
- Around line 41-49: The RegisterChildClassifier guard must reject typed-nil
ChildClassifier implementations, not only a nil interface; detect nil underlying
pointers before appending, while preserving the existing log-and-return
behavior. Add coverage in
src/controller/artifact/manifest/classifier_test.go:127-137 by registering a
typed-nil ChildClassifier and asserting it is not appended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94abfb25-2689-487b-8693-bd81b9fcf51d
📒 Files selected for processing (5)
src/controller/artifact/abstractor_test.gosrc/controller/artifact/manifest/attestation.gosrc/controller/artifact/manifest/attestation_test.gosrc/controller/artifact/manifest/classifier.gosrc/controller/artifact/manifest/classifier_test.go
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The attestation handling added in #85 was implemented as methods on the abstractor struct and called inline from abstractIndexMetadata, which welds it to a struct upstream is restructuring in goharbor/harbor#23647. It becomes a standalone InTotoAttestationClassifier in a new manifest package, carrying its own artifact.Manager and registry.Client through an exported constructor. The abstractor holds one and asks it about each index child. Attestation manifests are not a BuildKit invention - Buildah and Podman emit the same layout - so the identifiers move onto the spec axis, matching the attestation.intoto accessory type that was already generic. Resolve the reference annotation before loading the in-toto payload. Every normally annotated attestation was pulling a manifest and a blob it did not need. Treat a statement naming several index children as ambiguous rather than taking the first match, and reject ambiguous digests within a single subject, whose map iterates in unspecified order. Reject the in-toto payload on its advertised size before pulling the blob, and stop deferring the blob Close inside a loop. Keep an accessory failure from being retried as a lost parent-artifact race: the recovery path re-reads the artifact by digest, which cannot succeed when the transaction that would have created it has just rolled back. Signed-off-by: Vad1mo <vadim@8gears.com>
Vad1mo
force-pushed
the
refactor/index-child-classifier
branch
from
July 30, 2026 23:40
87deacc to
cfb7f76
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/controller/artifact/abstractor_test.go (1)
599-630: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest doesn't exercise the scenario it claims to test.
The doc comment says this verifies "annotation-based subject resolution works when in-toto subject loading fails," but
attestationIndexAnnotationOnly'svnd.docker.reference.digest(cad250bb...) matches the platform child's digest directly. InClassify,if !digestInIndex(children, targetDigest)is therefore false, soloadSubjects/PullManifestis never invoked - the mocked failure on Line 609 is dead code, and this test actually exercises the direct-annotation-hit fast path, not the load-failure fallback.Note also that the claimed scenario is unreachable as implemented: if the annotation digest isn't in the index and the subsequent load fails,
resolveSubjectFromStatementgets no subjects and returns"", yielding no candidate - not the "candidate found" result this test asserts.Suggest renaming the test/comment to reflect the actual fast-path scenario being tested, and dropping the now-misleading
PullManifestfailure mock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controller/artifact/abstractor_test.go` around lines 599 - 630, The test TestAbstractMetadataOfIndexResolvesFromAnnotation actually covers direct annotation-based resolution, not subject loading failure. Rename the test and its comment to describe the direct annotation fast path, and remove the unused PullManifest failure mock for the attestation digest while preserving the existing candidate assertions.
🧹 Nitpick comments (1)
src/controller/artifact/abstractor.go (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
manifestimport shadowed by a local variable of the same name.This import's default identifier is
manifest, butAbstractMetadata(Line 63, unchanged) already declares a localmanifest, _, err := a.regCli.PullManifest(...)that shadows it for that function's scope. It compiles today since the package isn't referenced there, but it's a latent trap for future edits within that function.♻️ Suggested fix: alias the import
- "github.com/goharbor/harbor/src/controller/artifact/manifest" + childmanifest "github.com/goharbor/harbor/src/controller/artifact/manifest"(and update the two usages accordingly:
childmanifest.NewInTotoAttestationClassifier,*childmanifest.InTotoAttestationClassifier)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controller/artifact/abstractor.go` at line 27, Alias the manifest package import to avoid collision with the local manifest variable in AbstractMetadata, then update the childmanifest.NewInTotoAttestationClassifier and *childmanifest.InTotoAttestationClassifier references to use the alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/controller/artifact/abstractor_test.go`:
- Around line 599-630: The test
TestAbstractMetadataOfIndexResolvesFromAnnotation actually covers direct
annotation-based resolution, not subject loading failure. Rename the test and
its comment to describe the direct annotation fast path, and remove the unused
PullManifest failure mock for the attestation digest while preserving the
existing candidate assertions.
---
Nitpick comments:
In `@src/controller/artifact/abstractor.go`:
- Line 27: Alias the manifest package import to avoid collision with the local
manifest variable in AbstractMetadata, then update the
childmanifest.NewInTotoAttestationClassifier and
*childmanifest.InTotoAttestationClassifier references to use the alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c90fae7-3cc5-4729-8fd1-32b57c6ec18d
📒 Files selected for processing (6)
src/controller/artifact/abstractor.gosrc/controller/artifact/abstractor_test.gosrc/controller/artifact/buildkit_attestation.gosrc/controller/artifact/controller.gosrc/controller/artifact/manifest/attestation.gosrc/controller/artifact/manifest/attestation_test.go
💤 Files with no reviewable changes (1)
- src/controller/artifact/buildkit_attestation.go
…Child The classifier was called for every descriptor and handed the full sibling slice each time, so platformChildren rebuilt that slice on every call. A 4MiB index body holds thousands of descriptors, which made the walk quadratic in both time and allocations for a request an attacker fully controls and does not have to back with any upload. Each attestation whose annotation named no child also pulled its manifest and up to 4MiB of in-toto payload, with a cap per statement but none per index, so the same index could force thousands of synchronous registry round trips and gigabytes of reads during one push. Turn Classify into a single call that partitions the children, so the sibling slice is built once and the payload lookups share one budget of min(len(children), 32) - a well-formed index carries at most one attestation per platform child. Attestations beyond the budget stay children of the index rather than being dropped. Measured upstream on an index of 1 platform child and 512 attestations, abstraction goes from 12.5ms and 34.1MB to 1.76ms and 0.74MB. A realistic two-platform build is unchanged at one extra allocation. Classify no longer returns (nil, nil), so the three //nolint:nilnil suppressions are gone and attestation.go is byte-identical to the upstream copy in goharbor/harbor#23648. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controller/artifact/manifest/attestation.go`:
- Around line 44-49: The maxSubjectLookups limit currently bounds only payload
lookups, while annotation-resolved candidates can still be created repeatedly
for the same subject. Update Classify and its candidate handling to track
claimed sub-artifact digests, allow only the first candidate for each target,
and classify later duplicate annotation targets as references instead of issuing
additional GetByDigest lookups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f8f49ed-2db7-431e-af80-37487a0121b0
📒 Files selected for processing (3)
src/controller/artifact/abstractor.gosrc/controller/artifact/manifest/attestation.gosrc/controller/artifact/manifest/attestation_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/controller/artifact/abstractor.go
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/controller/artifact/manifest/attestation.go">
<violation number="1" location="src/controller/artifact/manifest/attestation.go:88">
P2: Payload-resolved attestations after this budget are recorded as ordinary index references instead of accessories. A one-platform index with multiple missing/invalid subject annotations already triggers this after the first descriptor; preserve their classification or fail abstraction explicitly rather than silently changing stored metadata based on descriptor order.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Take the upstream wording for the AccessoryCandidates field and the AccessoryCandidate type verbatim, so pkg/artifact/model.go is byte-identical to the copy in goharbor/harbor#23648 and later merges have nothing to reconcile here. The upstream text is the better of the two: it says why the abstractor cannot persist the accessory itself, which is the constraint the whole candidate indirection exists for. The struct re-indents because a comment at field indentation splits gofmt's alignment group; there is no code change. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
…ream goharbor/harbor#23648 grew these two cases after review pointed out that nothing exercised the accessory write from the controller side. The fork had the controller.go hunk but not the tests, so the branch that carries the seam had no coverage of it. TestEnsurePersistsAccessoryCandidates asserts the candidates the abstractor emits reach accessoryMgr.Ensure inside the artifact-create transaction. TestEnsureReturnsAccessoryError asserts an accessory conflict surfaces as itself instead of being mistaken for the parent-artifact race the recovery path below it handles. The bodies are upstream's verbatim; each gains the setupXxx calls this suite requires, since SetupTest here nils every mock and each test opts into the ones it needs. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
…ubjects Two follow-ups from bot review of the previous commit. Deriving the budget from len(children) assumed one attestation per platform child. Tools are free to publish SBOM and provenance as separate manifests against the same image, and a single-platform index of those would have resolved only the first, silently turning the rest into ordinary children based on their order in the index. Since min(len(children), 32) never exceeded 32 anyway, dropping the first term leaves the worst case identical and stops penalising legitimate layouts. Resolving the subject went through artMgr on every attestation, so an index attaching several attestations to one image repeated the same query. Cache the resolved subjects for the duration of one index, taking a repository with n attestations on a shared subject from 2n lookups to n+1. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
Classify built the platform child list before it could tell whether the index contained any attestation at all, so every ordinary multi-arch push paid an allocation of one descriptor slice for nothing. Almost no index carries an attestation, so rule that out with an allocation-free scan first. An eight-platform index abstracts with 1024 fewer bytes and one fewer allocation; a two-platform index with 240 fewer. Indexes that do carry attestations are unchanged, having simply moved the same scan earlier. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
Review found that classifying an attestation as an accessory removed the artifact_reference row whose foreign-key guard used to refuse deleting an index child directly. DELETE on the attestation digest then succeeded, and the next GC removed a manifest the still-present index references. Refuse deleting an artifact that is an in-toto attestation of a subject that still exists, with the same PRECONDITION error the reference row used to produce. The check is scoped to the attestation type: an attestation is by construction listed in an index, while other hard accessories such as cosign signatures are not, and deleting those alone stays legitimate. Orphaned accessory rows are let through so their artifacts stay deletable. Also pin two behaviours the review leaned on. The accessory dedupe key is the accessory side alone, which is what keeps the copy path from re-attaching an already-linked accessory under the wrong subject. And degenerate index layouts - attestations naming themselves, each other, or appearing twice - stay ordinary children instead of looping or attaching wrongly. Matches goharbor/harbor#23648 commit for commit; attestation_test.go stays byte-identical. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/controller/artifact/controller.go">
<violation number="1" location="src/controller/artifact/controller.go:406">
P2: Deleting an index whose platform child is tagged leaves its attestation behind, but that attestation can then never be deleted independently: this guard only checks whether its subject exists, not whether any index still references it. Preserve/query the index-to-attestation relationship (or otherwise allow deletion once no such index remains) so removing the index does not permanently retain the accessory.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| return err | ||
| } | ||
| return errors.New(nil).WithCode(errors.ViolateForeignKeyConstraintCode). |
There was a problem hiding this comment.
P2: Deleting an index whose platform child is tagged leaves its attestation behind, but that attestation can then never be deleted independently: this guard only checks whether its subject exists, not whether any index still references it. Preserve/query the index-to-attestation relationship (or otherwise allow deletion once no such index remains) so removing the index does not permanently retain the accessory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/controller/artifact/controller.go, line 406:
<comment>Deleting an index whose platform child is tagged leaves its attestation behind, but that attestation can then never be deleted independently: this guard only checks whether its subject exists, not whether any index still references it. Preserve/query the index-to-attestation relationship (or otherwise allow deletion once no such index remains) so removing the index does not permanently retain the accessory.</comment>
<file context>
@@ -388,6 +388,24 @@ func (c *controller) Delete(ctx context.Context, id int64) error {
+ }
+ return err
+ }
+ return errors.New(nil).WithCode(errors.ViolateForeignKeyConstraintCode).
+ WithMessage("the artifact is an in-toto attestation referenced by an OCI index, delete its subject image or the index instead")
+ }
</file context>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
src/controller/artifact/manifest/attestation.go:110
- Each attestation performs a linear scan of every platform child here; subject resolution repeats the same pattern through
digestInIndexand name matching. An index split roughly evenly between platform and attestation descriptors therefore remains O(n²), despite the PR's claim that children are walked once. Build digest/platform lookup indexes once perClassifycall and use constant-time membership checks in the loop and resolution helpers.
targetDigest := descriptor.Annotations[referenceDigestAnnotation]
if !digestInIndex(children, targetDigest) {
src/controller/artifact/manifest/attestation.go:117
- This message says resolution falls back to the annotation, but this branch is entered only after the annotation failed to identify a child and
targetDigestwas cleared. On this error the descriptor remains an index child; report that outcome instead of a fallback that cannot occur.
log.G(ctx).Debugf("could not load attestation subjects for %s@%s, falling back to annotation-based resolution: %v", repository, descriptor.Digest.String(), err)
src/controller/artifact/manifest/attestation.go:240
uniqueDigestInIndexreturns the same empty value for “no digest matched” and “multiple children matched,” so an ambiguous digest set falls through to name matching. If conflicting digests identify two children but the names identify one, this attaches the attestation despite the stated requirement to reject ambiguous payloads. Preserve the distinction and return immediately when more than one unique digest matches.
if digestRef := uniqueDigestInIndex(children, byDigest); digestRef != "" {
return digestRef
}
src/controller/artifact/abstractor.go:58
- The advertised
ChildClassifierregistry/chain is not present:abstractordepends directly on the concrete in-toto classifier, and the onlyClassifyimplementation in the tree is called directly. Adding the SBOM/signature classifiers described by the PR would still require editing this struct, constructor, and index abstraction path rather than registering a classifier. Introduce aChildClassifierinterface plus registry/chain and inject that abstraction here, or align the PR title/description if extensibility is not part of this change.
attestation *manifest.InTotoAttestationClassifier
src/controller/artifact/manifest/attestation.go:92
- This applies the absolute ceiling even when the index has fewer platform children. For the documented one-platform/14,000-attestation case, the code performs 32 payload lookups rather than
min(len(children), 32) == 1, contradicting the stated resource bound. Derive the lookup limit from both values, update the warning to report that effective limit, and correct the tests that currently expect the absolute ceiling.
This issue also appears in the following locations of the same file:
- line 109
- line 117
- line 238
lookups := maxSubjectLookups
Comment on lines
+292
to
+293
| if accessoryErr != nil { | ||
| return false, nil, accessoryErr |
Member
|
/backport v2.15 |
Contributor
|
Backport to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The in-toto attestation handling from #85 was called inline from
abstractIndexMetadataand implemented as two methods on theabstractorstruct. This moves it behind aChildClassifierseam in a newsrc/controller/artifact/manifestpackage.The index loop now asks the classifier chain whether a child descriptor is a platform child of the index or an accessory of one of its siblings. No attestation-specific branch remains in the loop.
No behaviour change — the same accessories are produced from the same inputs.
Before / after
A two-platform image built with
--provenance --sbom, viewed as the children of the index.Before — four children, two of them
unknown/unknown:After — two platform children, each with its attestation nested as an
attestation.intotoaccessory:What this opens up
ifin the index loop.artifact.Managerandregistry.Clientvia an exported constructor rather than being welded toabstractor, so it can be driven directly with mocks.abstractIndexMetadataplus two struct-bound methods to a single registered file, which survives the twice-daily upstream cherry-pick job far better. It is also the piece we can offer upstream against BuildKit attestation manifests shown as unknown/unknown in multi-arch artifact list goharbor/harbor#22848 on its own merits.Why this ordering
goharbor/harbor#22848 is listed as depending on the manifest-abstractor refactor (goharbor/harbor#22847, now goharbor/harbor#23647). It does not: attestation manifests are ordinary OCI image manifests appearing as index children, identified by the
vnd.docker.reference.typeannotation. They introduce no new manifest media type, so a media-type registry does not unlock them. Descriptor-level classification — this change — does.Resource bounds
Classification is one call per index rather than one per child, so the sibling list is walked once instead of rebuilt per descriptor. Without that, a 4 MiB index body (
common.MaxManifestBodySize) holds thousands of descriptors and makes the walk quadratic in time and allocations, for a request that needs no uploads to construct.Reading an in-toto payload to resolve a subject is bounded per index at
min(len(children), 32)lookups. A well-formed index carries at most one attestation per platform child, so an index with one platform child and 14,000 attestation descriptors gets exactly one lookup. Worst case per index is 64 registry round trips and 128 MiB of reads. Attestations past the budget stay children of the index rather than being dropped.Measured upstream on 1 platform child + 512 attestations: 12.5 ms / 34.1 MB / 8,230 allocs before, 1.76 ms / 0.74 MB / 7,726 allocs after. A realistic two-platform build is unchanged at one extra allocation.
Raised by Copilot on the upstream PR: goharbor/harbor#23648 (comment).
Deletion and the API contract
Because the attestation is no longer an
artifact_referencechild, the FK guard that refused deleting an index child directly no longer covers it.controller.Deletetherefore rejects deleting anattestation.intotoaccessory while its subject exists, with the same 412 the reference row used to produce; deleting the subject or the index cascades it away as before, and orphaned accessory rows stay deletable. Scoped to the attestation type: cosign/notation deletion is unchanged, since those are not index children.API contract note: tooling that enumerates an index's children via Harbor's API (
references[]) no longer sees attestation manifests there — they appear asaccessorieson the platform child they attest. The registry API is unaffected.Verified live on a dev instance: attestation DELETE → 412, index delete → full cascade, GC
delete_untagged=trueleaves attestations of tagged indexes untouched, repository copy attaches accessories to the correct subjects, plain multi-arch indexes unaffected, and the duplicate-attestation index that previously 404ed on push now pushes cleanly.Naming
Buildah and Podman emit the same attestation layout, so
buildKit*identifiers are renamed onto the spec axis, matching theattestation.intotoaccessory type that was already generic. Thevnd.docker.reference.*constants and all behaviour are unchanged.Fixed in passing
defer blob.Close()is no longer inside aforloop. It was safe only because the loop returned on the first in-toto layer.Testing
go test ./controller/artifact/ -run TestAbstractorTestSuitegreen, both attestation cases still exercised end to end.go test ./controller/artifact/manifest/...green, covering the ported helpers, the digest-ambiguity regression, andTestClassifyBoundsSubjectLookups, which pins both the child-count budget and the ceiling.golangci-lint,gofmtandgo vetclean.manifest/attestation.gois byte-identical to the copy in goharbor/harbor#23648, with no//nolint:nilnilon either side: partitioning per index removed the(nil, nil)returns that needed suppressing.