refactor: Extract Manifest Abstraction Into A Registry - #528
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
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/abstractor.go">
<violation number="1" location="src/controller/artifact/abstractor.go:59">
P2: A nil abstractor registration makes artifact handling panic instead of returning a registration or unsupported-format error, because this calls `abs.Abstract` unconditionally. Reject nil abstractors in `manifest.Register` (including typed-nil implementations where practical) so a bad extension fails at registration time.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| index := &v1.Index{} | ||
| if err := json.Unmarshal(content, index); err != nil { | ||
| if err = abs.Abstract(ctx, art, content); err != nil { |
There was a problem hiding this comment.
P2: A nil abstractor registration makes artifact handling panic instead of returning a registration or unsupported-format error, because this calls abs.Abstract unconditionally. Reject nil abstractors in manifest.Register (including typed-nil implementations where practical) so a bad extension fails at registration time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/controller/artifact/abstractor.go, line 59:
<comment>A nil abstractor registration makes artifact handling panic instead of returning a registration or unsupported-format error, because this calls `abs.Abstract` unconditionally. Reject nil abstractors in `manifest.Register` (including typed-nil implementations where practical) so a bad extension fails at registration time.</comment>
<file context>
@@ -44,174 +32,33 @@ type Abstractor interface {
-
- index := &v1.Index{}
- if err := json.Unmarshal(content, index); err != nil {
+ if err = abs.Abstract(ctx, art, content); err != nil {
return err
}
</file context>
There was a problem hiding this comment.
Already handled in this PR: Register rejects a nil abstractor (manifest.go:40-42), so a bad registration fails at registration time rather than panicking at abs.Abstract.
On the parenthetical: abstractor == nil does not catch a typed nil (var a *fooAbstractor; Register(a, ...)) since the interface itself is non-nil. Left deliberately — registration happens in package init(), so a typed nil panics on the first artifact of that media type in any test environment, and reflect-based detection is more machinery than the risk warrants.
10f74ef to
d654ded
Compare
There was a problem hiding this comment.
Pull request overview
Refactors manifest metadata abstraction in src/controller/artifact/manifest/ from a hardcoded media-type switch in the artifact abstractor into a registry-driven manifest.Abstractor interface keyed by manifest media type, aligning the fork with the upstream shape and improving modularity/testability.
Changes:
- Introduces a manifest abstractor registry (
Register/Get) and registers per-format abstractors viainit(). - Extracts v1, v2, and index manifest abstraction logic into dedicated
manifestpackage implementations with exported constructors. - Moves/expands tests to validate default registrations, registry behavior, and attestation index handling alongside the
manifestcode.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/controller/artifact/manifest/manifest.go | Adds the manifest abstractor interface plus a global registry with Register/Get. |
| src/controller/artifact/manifest/v1.go | Implements and registers the schema1 (v1) manifest abstractor with injected blob manager. |
| src/controller/artifact/manifest/v2.go | Implements and registers the OCI/Docker v2 manifest abstractor, including wasm media-type override handling. |
| src/controller/artifact/manifest/index.go | Implements and registers the OCI index/manifest list abstractor, integrating attestation classification. |
| src/controller/artifact/manifest/manifest_test.go | Adds unit tests for default registrations and registry semantics (duplicates, rejected batches, unsupported types). |
| src/controller/artifact/manifest/abstractor_test.go | Adds direct unit tests for each abstractor via exported constructors and mocks. |
| src/controller/artifact/manifest/attestation_index_test.go | Adds tests for index attestation classification behavior and fallback paths. |
| src/controller/artifact/abstractor.go | Simplifies controller artifact abstractor to dispatch via manifest.Get() and the registry. |
| src/controller/artifact/abstractor_test.go | Updates the controller artifact abstractor test suite to use registry-registered abstractors backed by suite mocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func Register(abstractor Abstractor, mediaTypes ...string) error { | ||
| // Get treats any present entry as usable, so a nil stored here would panic on | ||
| // the next artifact of that media type instead of failing at registration. | ||
| if abstractor == nil { | ||
| return errors.New("refusing to register a nil manifest abstractor") | ||
| } | ||
|
|
||
| seen := make(map[string]struct{}, len(mediaTypes)) |
There was a problem hiding this comment.
Good catch — fixed in 69804a51. Register now rejects an empty media-type list, with TestRegisterRequiresMediaTypes covering it. Mirrored upstream in goharbor/harbor#23648.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Preview images for this PR are available in
Verify a preview image: Verify SBOM attestation: |
3d2863b to
a55ee09
Compare
Adopt the shape proposed upstream in goharbor/harbor#23647 so the fork stops diverging in this file. AbstractMetadata dispatched on manifest media type through a hardcoded switch, and the three abstraction bodies lived as private methods on the abstractor struct, which carried the union of every dependency. The three formats move behind the Abstractor interface in the manifest package, keyed by media type, each carrying only what it needs. The index abstractor owns the attestation classifier, so attestations are still resolved as accessories. Get returns an error rather than falling back to a default: guessing at an unknown manifest format would corrupt the artifact model. Register validates the whole batch before writing, since callers only log registration errors and a half-applied batch would make dispatch depend on the order of the media types, and rejects a nil abstractor that would otherwise panic on first use. The attestation index tests move into the manifest package, next to the code they cover. Signed-off-by: Vad1mo <vadim@8gears.com>
Register returned nil when called with no media types, so a mistaken Register(NewX()) succeeded while leaving the registry unchanged and only surfaced later as an unsupported media type at dispatch. Signed-off-by: Vad1mo <vadim@8gears.com>
Carry the per-index classification into the index abstractor that this branch extracts from abstractor.go, so the sibling slice is built once and the in-toto payload lookups share one budget instead of being unbounded per descriptor. Add the benchmark that backs the numbers: an index of 1 platform child and 512 attestations drops from 12.5ms and 34.1MB to 1.76ms and 0.74MB per abstraction, while a realistic two-platform build costs one extra allocation. The manifest package is now byte-identical to the copy in goharbor/harbor#23648. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
Upstream review pointed out that logging a failed registration and continuing means every push and pull of that manifest media type fails later at runtime with "unsupported manifest media type", far from the real cause. Unlike the processor registry this one has no default fallback, so a missed registration is not a degraded mode but a broken instance. panic rather than log.Fatalf: init runs before main configures logging, and log.Fatalf exits without the stack trace that points at the offending registration. No init in this codebase calls log.Fatalf either. Signed-off-by: Vadim Bauer <vb@8gears.com> Signed-off-by: Vad1mo <vadim@8gears.com>
a55ee09 to
4698e36
Compare
Summary
Adopts the shape proposed upstream in goharbor/harbor#23647, so the fork stops diverging in this file.
AbstractMetadata()dispatched on manifest media type through a hardcodedswitch, with the three abstraction bodies as private methods onabstractor. The struct carried the union of every dependency:blob.Managerfor schema1,artifact.Managerfor indexes, neither for v2. Those three formats now sit behind theAbstractorinterface in themanifestpackage, keyed by media type, each carrying only what it needs.No behaviour change — same media types accepted, same metadata produced, same accessories.
Why this matters for the fork
src/controller/artifact/manifest/is now byte-identical to the upstream branch (verified withdiff -rq). Together with #524 this removes the whole area as a source of cherry-pick conflicts againstupstream-cherry-pick.yml, which runs twice daily. Our remaining delta in artifact abstraction drops to the accessory plumbing that upstream hasn't taken yet.The upstream counterparts are goharbor/harbor#23647 (this registry) and goharbor/harbor#23648 (the attestation classifier, stacked on it). If both land, this area of the fork converges to zero.
What this opens up
AbstractMetadatapath.Notes
Getreturns an error rather than falling back to a default.processor.Getdefaults, which is right for artifact types; guessing at an unknown manifest format would corrupt the artifact model. The error carrieserrors.UNSUPPORTED.Registervalidates the whole batch before writing, so a rejected call leaves the registry untouched — callers only log registration errors, and a half-applied batch would make dispatch depend on the order of the media types.schema1.MediaTypeManifestis left unregistered, matching the previous switch exactly.manifestpackage, next to the code they cover.min(len(children), 32)per index.BenchmarkIndexAbstract, 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 Classify in-toto attestation manifests as accessories goharbor/harbor#23648 (comment).Testing
go build ./...clean.go test ./controller/artifact/manifest/...green at 91.9% statement coverage;go test ./controller/artifact/ -run TestAbstractorTestSuitegreen.golangci-lint,gofmt,go vetclean.TestClassifyBoundsSubjectLookupspins the payload-lookup budget and its ceiling.TestDefaultRegistrationspins the bootstrap — it fails ifinit()ever stops populating the registry. An empty registry would break every artifact push, so it is worth a test rather than a convention.