Don't use stale mapped-ports info in the ContainerState

[kwart]

This PR improves the behavior of ContainerState.getMappedPort(int) method. Instead of the getContainerInfo() it uses getCurrentContainerInfo().

The problem with the original behavior comes when a container is disconnected from a network and then newly connected. The exposed port number will change, but the GenericContainer.getMappedPort() returns the old value.

The PR also contains a new test with a simple reproducer.

[kiview]

Hey @kwart, thanks for the PR. CI is failing because of spotless, you can fix it with:

./gradlew :testcontainers:spotlessApply

Although it makes sense for the issue you described, we are a bit concerned about this change, since it will add a roundtrip to the Docker API for every call to getMappedPort() and these tend to be slow.

Can you expand on your use case for disconnecting and reconnecting a network?

[kwart]

Our use case is split-brain testing in Hazelcast.

Sample scenario:

• start 5 Hazelcast containers and put data in once the members form a cluster;
• disconnect two members from the network (without shutting them down), and wait for the 3-member cluster stabilization;
• reconnect the disconnected members and wait for cluster stabilization
• check if all the data is present in the cluster.

We expose the Hazelcast default port (5701) from containers and use the mapped ports to call REST API (checking cluster state and size).

[kwart]

Another option would be providing a method to manually refresh the info provided by the ContainerState.getContainerInfo(). Or is there such an option already and I just missed it?

[eddumelendez]

@kwart thanks for sharing your use case! I would highly recommend to use ToxiProxy module. You can add proxies to the Hazelcast instances you want to cut down the connection and although the containers will be running the cluster should not take those into account.

[kwart]

@kwart thanks for sharing your use case! I would highly recommend to use ToxiProxy module. You can add proxies to the Hazelcast instances you want to cut down the connection and although the containers will be running the cluster should not take those into account.

We also have tests with toxiproxy used. These were just other kinds of tests. And using additional proxies can come with its own issues, as it proved to us in https://github.com/Shopify/toxiproxy/issues/254)

[bsideup]

Hey @kwart,

FYI since you only need it for a specific use case, there is a very easy workaround that you can apply "per container":

@Override
public InspectContainerResponse getContainerInfo() {
    return getCurrentContainerInfo();
}

Unfortunately, making the change for every container isn't viable because we sometimes rely on the same value, e.g. here in KafkaContainer where we set it as advertised port: https://github.com/testcontainers/testcontainers-java/blob/221a8dac14982b05b7823ef9b9ed13ba817911f4/modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java#L116

Also, note that the direct dockerClient usage is considered advanced/low-level, so it is expected that you need to take care of such things

[kwart]

Thanks for all the feedback. I've reverted the ContainerState code change, added a JavaDoc, and updated the new test to show the correct way. Could you re-check?

[bsideup]

The JavaDoc change looks good to me 👍 I am not sure we need a test for that tho. At the very least, I would make it an example, but even as an example it is too exotic. IMO your JavaDoc comment is self-explanatory already.

[kiview]

The Javadoc is a great addition, thanks @kwart.

We could add the test as example code for this use case under https://www.testcontainers.org/features/networking/#advanced-networking, with an additional link, suggesting the use of Toxiproxy as the easier choice for most users. WDYT @bsideup?

[vcvitaly]

@kwart What do you think about adding the additional assert that the port value really changed?

[kwart]

@kwart What do you think about adding the additional assert that the port value really changed?

I think the port change is an implementation detail here, and the behavior could change in the future. The test should verify that access to the container will be possible after it reconnects to a network.