Fix false positive in DockerImageName.isCompatibleWith for library/-prefixed images - #11922
Conversation
aad5fed to
4d2c5e6
Compare
kdelay
left a comment
There was a problem hiding this comment.
I went through this locally against upstream/main (2ac3c97) and can confirm the diagnosis. A couple of measured notes below.
What reproduces. The degenerate branch behaves exactly as you describe — parse(this.repository).equals(this) puts AnyVersion on the left-hand side of equals, so other never participates:
library/mysql:8 ~ postgres -> true
library/foo:1.2.3 ~ foo:4.5.6 -> true
library/foo@sha256:1234567890abcdef1234567890abcdef ~ bar -> true
foo:1.2.3 ~ bar -> false (control)
So it swallows sha256-pinned names too, not only tags. With the patch all three become false. The three new tests fail on main and pass with the change; DockerImageNameCompatibilityTest, DockerImageNameTest, spotlessJavaCheck and checkstyleMain are all green on the branch.
The claim about testAssertMethodAcceptsCompatibleLibraryPrefix also checks out: if I delete only the new normalization block from your branch and leave everything else in place, that pre-existing test fails. So it does guard the behaviour after this change, whereas on main it was satisfied by the bug.
One thing I would tighten. withLibraryPrefix() prefixes any repository that has no registry, including multi-segment Docker Hub user namespaces. library/ is the single-segment official-image namespace, so library/myuser/myimage is a different repository from myuser/myimage. Measured:
| main | this PR | |
|---|---|---|
foo/bar ~ library/foo/bar |
false | true |
myuser/myimage:1 ~ library/myuser/myimage |
false | true |
library/foo/bar ~ foo/bar |
true (via the bug) | true |
The first two are a new false positive of the same family as the one being removed, and the third is not fixed, just re-derived from the new code path. Restricting the normalization to single-segment names makes all three false:
private static DockerImageName withLibraryPrefix(DockerImageName image) {
if (!image.registry.isEmpty() || image.repository.startsWith(LIBRARY_PREFIX)) {
return image;
}
if (image.repository.indexOf('/') >= 0) {
// Not a Docker Hub official image; the library/ namespace does not apply.
return image;
}
return image.withRepository(LIBRARY_PREFIX + image.repository);
}With that guard your three new tests plus DockerImageNameTest still pass and spotless/checkstyle stay clean.
Two smaller notes.
testLibraryPrefixedImageIsNotCompatibleWithDifferentImageandtestLibraryPrefixedImageWithClaimedCompatibilityassert negatives only, so they also pass if the library handling is removed outright; onlytestLibraryPrefixIsInterchangeablepins the normalization itself. Together they do cover the intended behaviour — it is just worth knowing which test holds which half.- The scope note looks accurate:
docker.io/library/foo ~ foo,docker.io/library/foo ~ docker.io/fooandlocalhost/foo ~ library/fooare allfalseboth before and after, so the registry-qualified half of #9958 is untouched. Since that issue asks for canonicalization todocker.io/library/<name>, the single-segment restriction above is the same rule a full canonicalization would need anyway.
I did not run the full core suite (it needs Docker); the above is the three org.testcontainers.utility test classes plus the checkstyle/spotless tasks. For blast radius, nothing under core/ or modules/ uses a library/-prefixed image name, so tightening the guard has no in-repo fallout — the behaviour change only lands on user-supplied names.
Broken behaviour
DockerImageName.isCompatibleWith()returnstruefor any image whose repository starts withlibrary/, regardless of what it is compared against:The library-prefix handling introduced in #6174 derives
imageWithLibraryPrefixfromthisonly and then compares it back againstthis:othernever takes part in that comparison, so for any registry-lesslibrary/…name the check degenerates toparse(this.repository).equals(this), which is always true (parsedrops the tag andAnyVersionequals everything). SinceassertCompatibleWith(...)guards the constructors of essentially every module (~96 call sites), it silently accepts wrong images instead of failing fast with theasCompatibleSubstituteForguidance. The existing testtestAssertMethodAcceptsCompatibleLibraryPrefixkept passing for this same wrong reason.Fix
Normalize both sides to the
library/-prefixed form before comparing, and only for names without a registry (Docker Hub official images):library/foo:1.2.3 ~ bar→false(wastrue)library/foo ~ foo→true(as before, now for the right reason)foo ~ library/foo→true(previouslyfalse; same image, now symmetric)otherstays on the left-hand side ofequals, so a tag present inothermust still match exactly and an untaggedotherstill acts as a wildcard (library/foo:1.2.3 ~ foo:4.5.6→false).some.registry/foo≠library/foo).Images named
library/…that previously passed the check only because of the false positive now fail with the standard compatibility error — that is the intended behaviour of the guard.Related to #9958: the
library/postgrescase reported there is what #6174 targeted; thedocker.io/…registry-qualified cases from that issue are out of scope for this PR.Testing
Added regression tests to
DockerImageNameCompatibilityTestcovering the false positive, prefix symmetry in both directions (with and without tags), registry exclusion, andasCompatibleSubstituteForinteraction. The new tests fail onmainand pass with this change; all pre-existing tests pass,checkstyleandspotlessare clean.