Add support for registry table prefix and location rewrite (without *, tag or digest yet)#542
Conversation
36b59d6 to
d6f83ca
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #542 +/- ##
============================================
+ Coverage 87.53% 88.17% +0.63%
- Complexity 753 763 +10
============================================
Files 42 42
Lines 2207 2241 +34
Branches 266 273 +7
============================================
+ Hits 1932 1976 +44
+ Misses 168 156 -12
- Partials 107 109 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2542931 to
8e22a03
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for registry table prefix and location rewrite functionality to the ORAS Java SDK. The changes enable the SDK to match container references against configured registry prefixes (including wildcard patterns like *.example.com) and rewrite them to different registry locations. This is useful for scenarios like redirecting Docker Hub references to an internal mirror or AWS Public ECR.
Changes:
- Added
prefixfield toRegistryConfigrecord to support prefix-based matching in addition to location-based matching - Implemented registry rewrite logic that replaces matched prefixes with configured locations in container references
- Refactored
isBlocked()andisInsecure()methods to acceptContainerRefinstead ofStringfor more precise matching - Added comprehensive test coverage for prefix matching, wildcard patterns, and rewrite functionality
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/land/oras/auth/RegistriesConf.java | Core implementation: added prefix field, ParsedPrefix record, rewrite logic, and enhanced matching with wildcard support |
| src/main/java/land/oras/ContainerRef.java | Updated to use registry rewrite in getEffectiveRegistry and isBlocked/isInsecure methods |
| src/main/java/land/oras/Registry.java | Updated getRepositories to pass ContainerRef instead of String to isInsecure check |
| src/test/java/land/oras/auth/RegistryConfTest.java | Added comprehensive tests for prefix matching, wildcards, rewrite logic, and multiple configuration scenarios |
| src/test/java/land/oras/RegistryWireMockTest.java | Added test for prefix configuration in repository listing |
| src/test/java/land/oras/RegistryTest.java | Moved unqualified registry test from ContainerRefTest (better location) |
| src/test/java/land/oras/ContainerRefTest.java | Removed test that was moved to RegistryTest |
| src/test/java/land/oras/PublicECRITCase.java | Added integration test for docker.io to public ECR rewrite scenario |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // With blocked false | ||
| registry = new RegistriesConf.RegistryConfig("test", false, null); | ||
| registry = new RegistriesConf.RegistryConfig("localhost:5000", null, false, null); | ||
| assertFalse(registry.isBlocked(), "Registry should be blocked when blocked is false"); |
There was a problem hiding this comment.
Incorrect assertion message. When blocked is false, the message should say "Registry should NOT be blocked when blocked is false" instead of "Registry should be blocked when blocked is false".
| assertFalse(registry.isBlocked(), "Registry should be blocked when blocked is false"); | |
| assertFalse(registry.isBlocked(), "Registry should NOT be blocked when blocked is false"); |
| } | ||
|
|
||
| // Path prefix match (namespace/repo) | ||
| String refPath = String.join("/", ref.getNamespace()) + "/" + ref.getRepository(); |
There was a problem hiding this comment.
This line has a compilation error. String.join("/", ref.getNamespace()) is incorrect because ref.getNamespace() returns a @Nullable String, not a collection. String.join() expects either varargs of CharSequence or an Iterable.
The correct implementation should build the path by concatenating namespace and repository. Since ContainerRef.getFullRepository() already provides this functionality, this line should be simplified to:
String refPath = ref.getFullRepository();| String refPath = String.join("/", ref.getNamespace()) + "/" + ref.getRepository(); | |
| String refPath = ref.getFullRepository(); |
| return ref; | ||
| } | ||
| String currentRefString = ref.toString(); | ||
| String rewrittenRefString = currentRefString.replaceFirst(prefix, registry); |
There was a problem hiding this comment.
The replaceFirst() method treats its first argument as a regex pattern, which could cause unexpected behavior if the prefix contains regex special characters like ., *, [, etc. For example, a prefix like example.com would match exampleXcom because . matches any character in regex.
Use replace() instead, which performs literal string replacement, or escape the prefix using Pattern.quote() if regex matching is needed.
| : determineFirstUnqualifiedSearchRegistry(target); | ||
| } | ||
| return registry; | ||
| // The effective registry can we rewrotten by the registry configuration. |
There was a problem hiding this comment.
Typo in comment: "rewrotten" should be "rewritten".
| // The effective registry can we rewrotten by the registry configuration. | |
| // The effective registry can be rewritten by the registry configuration. |
| public Repositories getRepositories() { | ||
| if (registry != null && getRegistriesConf().isInsecure(registry) && !this.isInsecure()) { | ||
| if (registry != null | ||
| && getRegistriesConf().isInsecure(ContainerRef.parse(registry).forRegistry(registry)) |
There was a problem hiding this comment.
This logic is incorrect. ContainerRef.parse(registry) where registry is a string like "localhost:5000" will be parsed as an unqualified ContainerRef with repository="localhost:5000", not as a registry. The NAME_REGEX pattern requires a / after the registry part, so a plain registry string won't match the registry group.
Then calling .forRegistry(registry) sets the registry field to "localhost:5000", creating a confusing and incorrect ContainerRef like "localhost:5000/localhost:5000".
A better approach would be to create a dummy ContainerRef with the registry properly set, such as:
ContainerRef.parse(registry + "/dummy:latest")or refactor to avoid this altogether.
| && getRegistriesConf().isInsecure(ContainerRef.parse(registry).forRegistry(registry)) | |
| && getRegistriesConf() | |
| .isInsecure(ContainerRef.parse("dummy:latest").forRegistry(registry)) |
9d43db2 to
edc0395
Compare
Signed-off-by: Valentin Delaye <jonesbusy@users.noreply.github.com>
edc0395 to
3e234f0
Compare
Description
Testing done
Submitter checklist
mvn license:update-file-header,mvn spotless:apply,pre-commit run -a,mvn clean installbefore opening the PR