Add image compatibility checks #3021

rnorth · 2020-07-26T11:06:56Z

Background to this change: the majority of modules make assumptions about the container image being used - for example, port numbers, expected log lines, etc. When asking users to provide their own images with modules, it is potentially confusing if the provided image diverges from the original 'vendor-provided' image that the module was built to support.

This change is intended to ensure that, if the user provides their own image that is not the same as the vendor-provided one, they are given adequate warning and forced to signal that this is intentional.

For example:

new KafkaContainer(DockerImageName.parse("confluentinc/cp-kafka:any")) will just work, because confluentinc/cp-kafka matches the image name that KafkaContainer was designed to work with
but new KafkaContainer(DockerImageName.parse("some-other-kafka")) will not work immediately, because some-other-kafka may be an entirely divergent image from confluentinc/cp-kafka. In this case, the user would be prompted to add .asCompatibleSubstituteFor("confluentinc/cp-kafka") which tells Testcontainers that this is a conscious decision

This PR:

Adds to DockerImageName:
- asCompatibleSubstituteFor(DockerImageName) and asCompatibleSubstituteFor(String) methods which may be used to claim compatibility with a vendor-provided image
- isCompatibleWith(DockerImageName) and assertCompatibleWith(DockerImageName) methods which can be used by Testcontainers to check that the provided image is compatible with the expected vendor-provided image
Refactors all modules to use this new mechanism
Amends the TestcontainersConfiguration class so that any file-based overrides automatically claim compatibility

core/src/main/java/org/testcontainers/utility/DockerImageName.java

So that compatibility assurances can be made in code rather than just being assumed.

rnorth · 2020-08-14T14:55:24Z

...lserver/src/main/java/org/testcontainers/containers/MSSQLR2DBCDatabaseContainerProvider.java

@@ -21,6 +21,7 @@ public boolean supports(ConnectionFactoryOptions options) {

    @Override
    public R2DBCDatabaseContainer createContainer(ConnectionFactoryOptions options) {
+        // TODO work out how best to do this if these constants become private


A TODO for a point in the near future. This has a lot to do with mandatory bring-your-own-image in R2DBC and JDBC URLs as discussed in Slack (@bsideup)

I think I missed the Slack discussion, but just being pragmatic and make the constants packacke-private?

Yeah, that's probably going to be the answer. This isn't something to worry about too much for now, anyway.

kiview

PR looks good overall, just had some questions and suggestion.

One copy-paste error with KafkaContainer it seems ;)

kiview · 2020-08-14T18:51:14Z

modules/mongodb/src/main/java/org/testcontainers/containers/MongoDBContainer.java

-    /**
-     * @deprecated use {@link MongoDBContainer(DockerImageName)} instead
-     */
-    @Deprecated


Do we really un-deprecate the String constructors?

I missed this - will look for others.

kiview · 2020-08-14T18:53:50Z

core/src/main/java/org/testcontainers/utility/DockerImageName.java

+     * @param other the other image that we are trying to check compatibility with
+     * @throws IllegalStateException if {@link DockerImageName#isCompatibleWith(DockerImageName)} returns false
+     */
+    public void assertCompatibleWith(DockerImageName other) {


If we would return DockerImageName, we could use this method in super constructor arguments.

I think I'd push back against this - it feels a bit strange to have a value be passed through a method that does assertion. I think I like the assertion being a distinct line in each constructor (after the call to super), as it feels more visible.

The JDK likes to object this argument of feeling strange 🙂
https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-

But this is not a hill I need to die on. I like it in super constructor though, because this means it gets evaluated before the super constructor is called.

kiview · 2020-08-14T18:54:43Z

modules/mongodb/src/main/java/org/testcontainers/containers/MongoDBContainer.java

-    /**
-     * @deprecated use {@link MongoDBContainer(DockerImageName)} instead
-     */
-    @Deprecated
    public MongoDBContainer(@NonNull final String dockerImageName) {
        this(DockerImageName.parse(dockerImageName));
    }

    public MongoDBContainer(final DockerImageName dockerImageName) {
        super(dockerImageName);


If assertCompatibleWith would return DockerImageName, we coould use it as argument for the super constructor.

kiview · 2020-08-14T18:58:56Z

modules/cassandra/src/main/java/org/testcontainers/containers/CassandraContainer.java

-    /**
-     * @deprecated use {@link #CassandraContainer(DockerImageName)} instead
-     */
-    @Deprecated


By which logic are the deprecations of constructors removed now? Seems kind of inconsistent between classes.

We were missing the deprecated annotation on CassandraContainer's no-arg constructor 🤦

The logic should be:

No-arg constructors: always deprecated

String, image name constructors: not deprecated

String, version constructors: always deprecated

kiview · 2020-08-14T19:12:58Z

modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java

    }

    /**
     * @deprecated use {@link KafkaContainer(DockerImageName)} instead
     */
    @Deprecated
    public KafkaContainer(String confluentPlatformVersion) {
-        this(DockerImageName.parse(TestcontainersConfiguration.getInstance().getKafkaImage() + ":" + confluentPlatformVersion));
+        this(TestcontainersConfiguration.getInstance().getPulsarDockerImageName().withTag(confluentPlatformVersion));


Suggested change

this(TestcontainersConfiguration.getInstance().getPulsarDockerImageName().withTag(confluentPlatformVersion));

this(TestcontainersConfiguration.getInstance().getKafkaDockerImageName().withTag(confluentPlatformVersion));

Is this constructor missing a test therefore?

Good spot - silly c&p error 😬

Yes, this is missing test coverage. Will add.

kiview · 2020-08-14T20:11:53Z

core/src/main/java/org/testcontainers/utility/Versioning.java

+
+    String getSeparator();
+
+    @Data


@Value instead of @Data? Or @EqualsAndHashcode? Or we don't use lombok in the first plance, since we already define toString() and the constructor.

@EqualsAndHashCode would do the trick - good suggestion.

kiview · 2020-08-14T20:14:47Z

core/src/test/java/org/testcontainers/utility/DockerImageNameCompatibilityTest.java

+    public void testLatestTreatedAsWildcard() {
+        final DockerImageName subject = DockerImageName.parse("foo:4.5.6");
+
+        assertTrue("foo:4.5.6 ~= foo:latest", subject.isCompatibleWith(DockerImageName.parse("foo:1.2.3").withTag("latest")));


Suggested change

assertTrue("foo:4.5.6 ~= foo:latest", subject.isCompatibleWith(DockerImageName.parse("foo:1.2.3").withTag("latest")));

assertTrue("foo:4.5.6 ~= foo:latest", subject.isCompatibleWith(DockerImageName.parse("foo")));

Since latest is default?

Ah, the intent here is to make sure that setting latest tag doesn't mess things up. I'll add a clarifying comment:

foo:1.2.3 != foo:4.5.6 foo:1.2.3 ~= foo foo:1.2.3 ~= foo:latest The test is effectively making sure that no tag and `latest` tag are equivalent

kiview · 2020-08-14T20:16:33Z

core/src/test/java/org/testcontainers/utility/DockerImageNameCompatibilityTest.java

+    }
+
+    @Test
+    public void testImageWithAutomaticCompatibility() {


after my suggestion, this would be the same test as testLatestTreatedAsWildcard

core/src/test/java/org/testcontainers/utility/DockerImageNameCompatibilityTest.java

kiview · 2020-08-14T20:18:53Z

core/src/test/java/org/testcontainers/utility/DockerImageNameCompatibilityTest.java

+    }
+
+    @Test
+    public void testCheckMethodRejectsIncompatible() {


Suggested change

public void testCheckMethodRejectsIncompatible() {

public void testAssertMethodRejectsIncompatible() {

Fix gap in testing and docs

…ompatibilityTest.java Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>

Adapt trim() usage to new code structure

…tespace in property files

…rides

bsideup · 2020-08-20T07:59:05Z

core/src/main/java/org/testcontainers/utility/DockerImageName.java

@@ -63,14 +62,16 @@ public DockerImageName(String fullImageName) {

        if (remoteName.contains("@sha256:")) {
            repo = remoteName.split("@sha256:")[0];
-            versioning = new Sha256Versioning(remoteName.split("@sha256:")[1]);
+            versioning = new Versioning.Sha256Versioning(remoteName.split("@sha256:")[1]);


tiny nit: if we import Versioning.Sha256Versioning and other Versioning.* classes, the changelog should be smaller :)

@rnorth is it? 😅

Oops, must have reverted it during moving the interface. Done again.

bsideup · 2020-08-20T07:59:54Z

core/src/main/java/org/testcontainers/utility/DockerImageName.java

+    private DockerImageName(String rawName,
+                            String registry,
+                            String repo,
+                            @Nullable Versioning versioning,


marked as @Nullable while the field isn't

bsideup · 2020-08-20T08:03:20Z

core/src/main/java/org/testcontainers/utility/DockerImageName.java

@@ -125,14 +141,14 @@ public String getUnversionedPart() {
     * @return the versioned part of this name (tag or sha256)
     */
    public String getVersionPart() {
-        return versioning.toString();
+        return versioning == null ? "latest" : versioning.toString();


can we make versioning @NonNull and use Versioning.TagVersioning.LATEST if null is passed to @Nullable methods?

So, this is one that requires some discussion, and potentially a change or just clear docs!

I'd expect most usage of this feature to be like foo.asCompatibleSubstituteFor("bar") meaning bar with any tag is accepted.

I wanted to leave the possibility open to specify an exact tag match, i.e. foo.asCompatibleSubstituteFor("bar:1.2.3").

So that this works:

an absent tag is recorded as null

the compatitility check code treats this null as a wildcard

conversion to a string treats this null as an implicit latest

It doesn't have to be this way, though. I reckon we could:

ignore tags altogether for compatibility checks

OR be more explicit about wildcards, e.g. foo.asCompatibleSubstituteFor("bar:*") in the API, and/or have a Versioning.Wildcard type internally instead of null.

WDYT?

have a Versioning.Wildcard type internally instead of null

I like this one! My main concern was the Nullable field that we can avoid and I think Versioning.Wildcard solves the problem very well 👍

👍 cool, I'll go with that then.

Done (called it AnyVersion)

bsideup · 2020-08-20T08:03:34Z

core/src/main/java/org/testcontainers/utility/DockerImageName.java

    }

    /**
     * @return canonical name for the image
     */
    public String asCanonicalNameString() {
-        return getUnversionedPart() + versioning.getSeparator() + versioning.toString();
+        return getUnversionedPart() + (versioning == null ? ":" : versioning.getSeparator()) + getVersionPart();


ditto re null vs Versioning.TagVersioning.LATEST

bsideup · 2020-08-20T08:06:14Z

core/src/main/java/org/testcontainers/utility/DockerImageName.java

+     * @return an immutable copy of this {@link DockerImageName} with the compatibility declaration attached.
+     */
+    public DockerImageName asCompatibleSubstituteFor(DockerImageName otherImageName) {
+        return new DockerImageName(rawName, registry, repo, versioning, otherImageName);


Consider adding @With(AccessLevel.PRIVATE) to otherImageName, so that we can do:

Suggested change

return new DockerImageName(rawName, registry, repo, versioning, otherImageName);

return withOtherImageName(otherImageName);

Nice, this works well (same for withTag).

N.B. I've upgraded the Lombok dependency so that we can use modern @With rather than deprecated @Wither

core/src/main/java/org/testcontainers/utility/DockerImageName.java

…rides

… equality)

All comments responded to (either changed or I think I've explained!)

…image

bsideup

🎉 🚀

rnorth · 2020-09-28T16:14:54Z

/azp run

azure-pipelines · 2020-09-28T16:15:06Z

Azure Pipelines successfully started running 2 pipeline(s).

dadoonet · 2020-10-16T08:37:53Z

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java

     */
-    private static final String ELASTICSEARCH_DEFAULT_IMAGE = "docker.elastic.co/elasticsearch/elasticsearch";
+    private static final DockerImageName DEFAULT_IMAGE_NAME = DockerImageName.parse("docker.elastic.co/elasticsearch/elasticsearch");


I'm wondering if we could make this static constant public so people can simply do something like:

new ElasticsearchContainer(ElasticsearchContainer.DEFAULT_IMAGE_NAME.withTag("7.9.2"));

Builds upon #3021: * adds a pluggable image substitution mechanism using ServiceLoader, enabling users to perform custom substitution/auditing of images being used by their tests * provides a default implementation that behaves similarly to legacy `TestcontainersConfiguration` approach (`testcontainers.properties`), but also... * For many orgs, sticking a prefix on the front of image names might be enough to use a private registry. I've added a default behaviour whereby, if a particular environment variable is present, image names are automatically substituted. e.g. `TESTCONTAINERS_IMAGE_NAME_PREFIX=my.registry.com/` would transform `redis` to `my.registry.com/redis` etc. Notes: * behaviour is similar but not quite identical to `TestcontainersConfiguration`: use of a configured custom image for, e.g. Kafka/Pulsar that does not have a tag specified causes the substitution to take effect for all usages. It seems very unlikely that people would be using a mix of the config file image overrides in some places _and_ specific images specified in code in others. * Duplication of default image names in modules vs `TestcontainersConfiguration` class is intentional: specifying image overrides in `testcontainers.properties` should be removed in the future. * ~Add log deprecation warnings when `testcontainers.properties` image overrides are used.~ Defer to a future release?

Builds upon #3021 and #3411: * adds a pluggable image substitution mechanism using ServiceLoader, enabling users to perform custom substitution/auditing of images being used by their tests * provides a default implementation that behaves similarly to legacy `TestcontainersConfiguration` approach (`testcontainers.properties`) Notes: * behaviour is similar but not quite identical to `TestcontainersConfiguration`: use of a configured custom image for, e.g. Kafka/Pulsar that does not have a tag specified causes the substitution to take effect for all usages. It seems very unlikely that people would be using a mix of the config file image overrides in some places _and_ specific images specified in code in others. * Duplication of default image names in modules vs `TestcontainersConfiguration` class is intentional: specifying image overrides in `testcontainers.properties` should be removed in the future. * ~Add log deprecation warnings when `testcontainers.properties` image overrides are used.~ Defer to a future release?

* Refactor Testcontainers configuration to allow config by env var * Add Image substitution mechanism Builds upon #3021 and #3411: * adds a pluggable image substitution mechanism using ServiceLoader, enabling users to perform custom substitution/auditing of images being used by their tests * provides a default implementation that behaves similarly to legacy `TestcontainersConfiguration` approach (`testcontainers.properties`) Notes: * behaviour is similar but not quite identical to `TestcontainersConfiguration`: use of a configured custom image for, e.g. Kafka/Pulsar that does not have a tag specified causes the substitution to take effect for all usages. It seems very unlikely that people would be using a mix of the config file image overrides in some places _and_ specific images specified in code in others. * Duplication of default image names in modules vs `TestcontainersConfiguration` class is intentional: specifying image overrides in `testcontainers.properties` should be removed in the future. * ~Add log deprecation warnings when `testcontainers.properties` image overrides are used.~ Defer to a future release? * Remove extraneous change * Un-ignore docs example test by implementing a 'reversing' image name substitutor * Use configuration, not service loader, to select an ImageNameSubstitutor * Add check for order of config setting precedence * Extract classpath scanner and support finding of multiple resources * Introduce deterministic merging of classpath properties files * Update docs * Update docs * Remove service loader reference * Chain substitution through default and configured implementations * Small tweaks following review * Fix test compile error * Add UnstableAPI annotation * Move TestSpecificImageNameSubstitutor back to original package and remove duplicate use of default substitutor

rnorth commented Jul 26, 2020

View reviewed changes

core/src/main/java/org/testcontainers/utility/DockerImageName.java Show resolved Hide resolved

Implement image compatibility checks

484f3cd

So that compatibility assurances can be made in code rather than just being assumed.

rnorth force-pushed the image-overrides branch from 71fe1b7 to 484f3cd Compare August 14, 2020 14:38

rnorth marked this pull request as ready for review August 14, 2020 14:51

rnorth requested review from bsideup and kiview as code owners August 14, 2020 14:51

rnorth commented Aug 14, 2020

View reviewed changes

Revert default image name selection in Kafka/Pulsar containers

49c2360

kiview previously requested changes Aug 14, 2020

View reviewed changes

rnorth added 9 commits August 15, 2020 16:30

Undeprecate remaining String constructors

a0ef1fe

Add comment re validation of HostAndPort

5be1a79

Add missing annotation

ddf8a28

Fix Kafka version string constructor

c5c1e74

Fix gap in testing and docs

Use @EqualsAndHashCode for Versioning

7d82db9

Clarify and expand test

de6dec6

Remove duplicate test

d87bcee

Rename test

6b63e88

Add continue-on-error for cache step, and upgrade GH cache action

202a163

rnorth requested a review from kiview August 15, 2020 19:10

rnorth mentioned this pull request Aug 16, 2020

Image substitution #3102

Merged

rnorth and others added 4 commits August 20, 2020 08:28

Update core/src/test/java/org/testcontainers/utility/DockerImageNameC…

b6e5191

…ompatibilityTest.java Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into image-overrides

33c7ae5

Adapt trim() usage to new code structure

Merge from origin/master and incorporate trim() to fix accidental whi…

ee5eae3

…tespace in property files

Merge remote-tracking branch 'origin/image-overrides' into image-over…

502c647

…rides

bsideup reviewed Aug 20, 2020

View reviewed changes

core/src/main/java/org/testcontainers/utility/DockerImageName.java Show resolved Hide resolved

rnorth added 6 commits September 1, 2020 12:21

Merge branch 'master' into image-overrides

3624787

Merge remote-tracking branch 'origin/master' into image-overrides

89a6b73

Add an 'AnyVersion' for images

dfa1e63

Merge remote-tracking branch 'origin/image-overrides' into image-over…

61c5fbc

…rides

Add import

a5b63ff

Adapt test for mocking compatibility (avoid testing latest vs any for…

3d43019

… equality)

rnorth added 3 commits September 24, 2020 09:12

Restore previous behaviour using a fixed version of Neo4j Enterprise …

022c493

…image

Reinstate standard image check

6cc5c5f

Merge branch 'master' into image-overrides

ef9633f

bsideup approved these changes Sep 25, 2020

View reviewed changes

rnorth added 2 commits September 25, 2020 08:46

Merge branch 'master' into image-overrides

db4dabf

Merge branch 'master' into image-overrides

80e6971

rnorth added 2 commits September 28, 2020 19:12

Merge branch 'master' into image-overrides

7794be6

Merge branch 'master' into image-overrides

a1b1f48

rnorth merged commit 1e597cc into master Sep 29, 2020

rnorth deleted the image-overrides branch September 29, 2020 10:02

bsideup mentioned this pull request Oct 7, 2020

Add GCloud module for Google Cloud Datastore, Firestore, PubSub, and Spanner emulators #2690

Merged

dadoonet reviewed Oct 16, 2020

View reviewed changes

rnorth mentioned this pull request Oct 24, 2020

Developer experience: recommending a default container image #3381

Open

rnorth mentioned this pull request Oct 29, 2020

Default prefixing image substitutor #3413

Merged

fruch mentioned this pull request Jan 11, 2021

testcontainers-java pervert running the test suite with scylladb/scylla docker image JanusGraph/janusgraph#2351

Closed

eddumelendez mentioned this pull request Jul 9, 2022

Replace deprecated clickhouse driver #4925

Closed

	this(TestcontainersConfiguration.getInstance().getPulsarDockerImageName().withTag(confluentPlatformVersion));
	this(TestcontainersConfiguration.getInstance().getKafkaDockerImageName().withTag(confluentPlatformVersion));

	assertTrue("foo:4.5.6 ~= foo:latest", subject.isCompatibleWith(DockerImageName.parse("foo:1.2.3").withTag("latest")));
	assertTrue("foo:4.5.6 ~= foo:latest", subject.isCompatibleWith(DockerImageName.parse("foo")));

	public void testCheckMethodRejectsIncompatible() {
	public void testAssertMethodRejectsIncompatible() {

	return new DockerImageName(rawName, registry, repo, versioning, otherImageName);
	return withOtherImageName(otherImageName);

Add image compatibility checks #3021

Add image compatibility checks #3021

Conversation

rnorth commented Jul 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiview left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsideup left a comment

Choose a reason for hiding this comment

rnorth commented Sep 28, 2020

azure-pipelines bot commented Sep 28, 2020

dadoonet Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

rnorth commented Jul 26, 2020 •

edited

Loading

rnorth Aug 15, 2020 •

edited

Loading

dadoonet Oct 16, 2020 •

edited

Loading