-
Notifications
You must be signed in to change notification settings - Fork 165
fix(api): resolve OCI conformance issues and refactor blob upload handling #3642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
4053037 to
8a9bf72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR resolves multiple OCI Distribution Spec conformance issues discovered through conformance testing. The changes ensure proper handling of empty blobs, correct error responses for invalid digests, and proper blob isolation between repositories.
Key changes:
- Fixed empty blob upload/retrieval to allow Content-Length: 0 and return proper status codes
- Separated blob existence checks between repositories (CheckBlob) vs mount operations (CheckBlobForMount) to ensure deleted blobs return 404 even if cached elsewhere
- Implemented "from" parameter support for cross-repository blob mounting with permission validation
- Made Content-Length and Content-Range optional for finishing chunked uploads per OCI spec
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 |
|---|---|
| pkg/test/mocks/image_store_mock.go | Added CheckBlobForMountFn mock to support new mount-specific blob checking |
| pkg/storage/types/types.go | Added CheckBlobForMount interface method for mount operations |
| pkg/storage/imagestore/imagestore.go | Implemented empty digest caching, separated CheckBlob/CheckBlobForMount logic, refactored blob verification to distinguish repository-specific vs cache-based checks |
| pkg/storage/cache/dynamodb.go | Added sorting to GetAllBlobs for consistent ordering |
| pkg/compliance/v1_0_0/check.go | Added comprehensive test cases for empty blob uploads, bad digest handling, blob deletion after mount, and chunked uploads without Content-Length |
| pkg/api/routes_test.go | Updated test digests to use valid format and corrected mock function usage |
| pkg/api/routes.go | Refactored CheckBlob to not check cache, implemented "from" parameter handling, extracted error handling helpers, made Content-Length/Content-Range optional for PUT |
| pkg/api/controller_test.go | Updated tests to expect 404 for cross-repo HEAD requests and added "from" parameter test coverage |
| .github/workflows/oci-conformance-action.yaml | Added new CI job for running pr-conformance-v2 tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dling This commit fixes several OCI Distribution Spec conformance issues found while running the tests in: https://github.com/sudo-bmitch/distribution-spec/tree/pr-conformance-v2 Also add a new CI job running the conformance actions Conformance Fixes: 1 Fix empty blob upload: allow Content-Length: 0 for POST/PUT requests instead of returning 400 2 Fix empty blob retrieval: return 404 when blob not found in repository instead of returning 400 3 Fix bad digest handling: return 400 Bad Request with DIGEST_INVALID for invalid digest formats and mismatched digests 4 Fix blob delete after mount: return 404 after blob was deleted from repository, even if it exists in cache (other repositories) 5 Fix chunked upload PUT: make Content-Length and Content-Range optional when finishing chunked uploads Other changes: - Given point 4, we can't auto-mount blobs from cache on HEAD requests, this code was removed. Since the tests uses HEAD after DELETE, auto-mounting from another repository would results in 200 instead of 404 - Implement the "from" parameter in case of "mount" and check user permissions on that repository. Except for the permission check this is more of a cosmetic update, there is no significant change, as in case of mounts all permission on all repos would have been taken into considentation and the "from" was ignored. Refactoring: - Add CheckBlobForMount method for mount operations that check cache - Extract handleBlobUploadError helper to consolidate duplicate error handling logic - Extract checkEmptyBlobUpload helper to reduce UpdateBlobUpload complexity - Remove goto statement from UpdateBlobUpload Signed-off-by: Andrei Aaron <[email protected]>
68c6f88 to
efc5036
Compare
Signed-off-by: Andrei Aaron <[email protected]>
This commit fixes several OCI Distribution Spec conformance issues found while running the tests in: https://github.com/sudo-bmitch/distribution-spec/tree/pr-conformance-v2
Add a new CI job running the conformance actions.
Conformance Fixes:
Other changes:
Refactoring:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.