-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add support for Docker networks. #372
Changes from 11 commits
a265dc0
0de27b9
93f97ed
7fd6802
12a777e
9e690d5
9e5a3f6
06d6e1d
4ebffb0
c8c5530
a2e6caa
14e698d
83368e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
package org.testcontainers.containers; | ||
|
||
import com.github.dockerjava.api.command.CreateNetworkCmd; | ||
import lombok.*; | ||
import org.junit.rules.ExternalResource; | ||
import org.junit.rules.TestRule; | ||
import org.testcontainers.DockerClientFactory; | ||
import org.testcontainers.utility.ResourceReaper; | ||
|
||
import java.util.LinkedHashSet; | ||
import java.util.Set; | ||
import java.util.UUID; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import java.util.function.Consumer; | ||
import java.util.function.Supplier; | ||
|
||
public interface Network extends AutoCloseable, TestRule { | ||
|
||
String getId(); | ||
|
||
String getName(); | ||
|
||
Boolean getEnableIpv6(); | ||
|
||
String getDriver(); | ||
|
||
boolean isCreated(); | ||
|
||
default boolean create() { | ||
return getId() != null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would leave out the default implementation- it's too magic: you need to guess that getId is doing the actual create. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. default implementation here assumes that there is kinda no need to create a network. If id is known then what else to "create"? The method returns boolean instead of an exception because it's up to the consumer how to react on that. However, I agree that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I prefer the previous approach with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the impl you placed the logic of creating the network in getId() - that was my point. In the default implementation you assumed getId will be called this create the network if needed. Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think @asafm point is, that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Example: class ExistingNetwork implements Network {
String getId() {
return "some-predefined-id";
}
} |
||
} | ||
|
||
@Override | ||
default void close() { | ||
if (isCreated()) { | ||
ResourceReaper.instance().removeNetworks(getName()); | ||
} | ||
} | ||
|
||
@SneakyThrows | ||
@SuppressWarnings("unchecked") | ||
default <T extends Network> T as(Class<T> type) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious to that purpose of this class, which is related to another question I had: why have an interface? Do we plan on users extending and supplying their own Network implementation? |
||
return type.getDeclaredConstructor(Network.class).newInstance(this); | ||
} | ||
|
||
static Network newNetwork() { | ||
return builder().build(); | ||
} | ||
|
||
static NetworkImpl.NetworkImplBuilder builder() { | ||
return NetworkImpl.builder(); | ||
} | ||
|
||
@Builder | ||
@Getter | ||
class NetworkImpl extends ExternalResource implements Network { | ||
|
||
private final String name = UUID.randomUUID().toString(); | ||
|
||
private Boolean enableIpv6; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. null here means "the value wasn't provided and |
||
|
||
private String driver; | ||
|
||
@Singular | ||
private Set<Consumer<CreateNetworkCmd>> createNetworkCmdModifiers = new LinkedHashSet<>(); | ||
|
||
@Getter(lazy = true) | ||
private final String id = ((Supplier<String>) () -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why a variable of type String receives a lambda function, casted to a Suplier of String and then executed? I mean - what's going on? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
ResourceReaper.instance().registerNetworkForCleanup(getName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to have this in a private method. |
||
|
||
CreateNetworkCmd createNetworkCmd = DockerClientFactory.instance().client().createNetworkCmd(); | ||
|
||
createNetworkCmd.withName(getName()); | ||
createNetworkCmd.withCheckDuplicate(true); | ||
|
||
if (getEnableIpv6() != null) { | ||
createNetworkCmd.withEnableIpv6(getEnableIpv6()); | ||
} | ||
|
||
if (getDriver() != null) { | ||
createNetworkCmd.withDriver(getDriver()); | ||
} | ||
|
||
for (Consumer<CreateNetworkCmd> consumer : createNetworkCmdModifiers) { | ||
consumer.accept(createNetworkCmd); | ||
} | ||
|
||
return createNetworkCmd.exec().getId(); | ||
}).get(); | ||
|
||
@Override | ||
public boolean isCreated() { | ||
// Lombok with @Getter(lazy = true) will use AtomicReference as a field type for id | ||
return ((AtomicReference<String>) (Object) id).get() != null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a personal opinion - that's too much magic for such a simple thing as having a field called Id of type String. Maybe it will more clear if for this field, Lombok won't be used. |
||
} | ||
|
||
@Override | ||
protected void before() throws Throwable { | ||
create(); | ||
} | ||
|
||
@Override | ||
protected void after() { | ||
close(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,15 +5,12 @@ | |
import com.github.dockerjava.api.exception.DockerException; | ||
import com.github.dockerjava.api.exception.InternalServerErrorException; | ||
import com.github.dockerjava.api.exception.NotFoundException; | ||
import com.github.dockerjava.api.model.Container; | ||
import com.github.dockerjava.api.model.Network; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.testcontainers.DockerClientFactory; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.*; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
|
||
/** | ||
|
@@ -25,7 +22,7 @@ public final class ResourceReaper { | |
private static ResourceReaper instance; | ||
private final DockerClient dockerClient; | ||
private Map<String, String> registeredContainers = new ConcurrentHashMap<>(); | ||
private List<String> registeredNetworks = new ArrayList<>(); | ||
private Set<String> registeredNetworks = Collections.newSetFromMap(new ConcurrentHashMap<>()); | ||
|
||
private ResourceReaper() { | ||
dockerClient = DockerClientFactory.instance().client(); | ||
|
@@ -145,22 +142,34 @@ public void removeNetworks(String identifier) { | |
} | ||
|
||
private void removeNetwork(String networkName) { | ||
List<Network> networks; | ||
try { | ||
networks = dockerClient.listNetworksCmd().withNameFilter(networkName).exec(); | ||
} catch (DockerException e) { | ||
LOGGER.trace("Error encountered when looking up network for removal (name: {}) - it may not have been removed", networkName); | ||
return; | ||
} | ||
try { | ||
// First try to remove by name | ||
dockerClient.removeNetworkCmd(networkName).exec(); | ||
} catch (Exception e) { | ||
LOGGER.trace("Error encountered removing network by name ({}) - it may not have been removed", networkName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two tests running with same network name will fail , at least the second create will. They will not understand why since the failure is hidden under trace level. I would keep this at error level or warn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. network's name is random (see NetworkImpl) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So no way of allowing me to specify a name? I'm Starting to build Testclusters, where I'm reusing containers across runs to shorten test duration. My assumption is same network name across runs. Would love to use the class created here for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If your use case would reuse the containers, it would reuse the network as well, I assume? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm reusing across builds, on same machine. Quite similar to docker compose. I can code it with the client my self, but I think it's harmless to leave the opening to set the name to who ever needs it. I'm ok with any direction you take. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's think about whether we could allow reuse between runs separately. It's probably quite doable, but for the sake of this PR let's resume later. |
||
} | ||
|
||
for (Network network : networks) { | ||
List<Network> networks; | ||
try { | ||
dockerClient.removeNetworkCmd(network.getId()).exec(); | ||
registeredNetworks.remove(network.getId()); | ||
LOGGER.debug("Removed network: {}", networkName); | ||
} catch (DockerException e) { | ||
LOGGER.trace("Error encountered removing network (name: {}) - it may not have been removed", network.getName()); | ||
// Then try to list all networks with the same name | ||
networks = dockerClient.listNetworksCmd().withNameFilter(networkName).exec(); | ||
} catch (Exception e) { | ||
LOGGER.trace("Error encountered when looking up network for removal (name: {}) - it may not have been removed", networkName); | ||
return; | ||
} | ||
|
||
for (Network network : networks) { | ||
try { | ||
dockerClient.removeNetworkCmd(network.getId()).exec(); | ||
registeredNetworks.remove(network.getId()); | ||
LOGGER.debug("Removed network: {}", networkName); | ||
} catch (Exception e) { | ||
LOGGER.trace("Error encountered removing network (name: {}) - it may not have been removed", network.getName()); | ||
} | ||
} | ||
} finally { | ||
registeredNetworks.remove(networkName); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
package org.testcontainers.containers; | ||
|
||
import org.junit.Rule; | ||
import org.junit.Test; | ||
import org.junit.experimental.runners.Enclosed; | ||
import org.junit.runner.RunWith; | ||
import org.testcontainers.DockerClientFactory; | ||
|
||
import static org.rnorth.visibleassertions.VisibleAssertions.*; | ||
import static org.testcontainers.containers.Network.newNetwork; | ||
|
||
@RunWith(Enclosed.class) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neat - I didn't know about this! |
||
public class NetworkTest { | ||
|
||
public static class WithRules { | ||
|
||
@Rule | ||
public Network network = newNetwork(); | ||
|
||
@Rule | ||
public GenericContainer foo = new GenericContainer() | ||
.withNetwork(network) | ||
.withNetworkAliases("foo") | ||
.withCommand("/bin/sh", "-c", "while true ; do printf 'HTTP/1.1 200 OK\\n\\nyay' | nc -l -p 8080; done"); | ||
|
||
@Rule | ||
public GenericContainer bar = new GenericContainer() | ||
.withNetwork(network) | ||
.withCommand("top"); | ||
|
||
@Test | ||
public void testNetworkSupport() throws Exception { | ||
String response = bar.execInContainer("wget", "-O", "-", "http://foo:8080").getStdout(); | ||
assertEquals("received response", "yay", response); | ||
} | ||
} | ||
|
||
public static class WithoutRules { | ||
|
||
@Test | ||
public void testNetworkSupport() throws Exception { | ||
try ( | ||
Network network = newNetwork(); | ||
|
||
GenericContainer foo = new GenericContainer() | ||
.withNetwork(network) | ||
.withNetworkAliases("foo") | ||
.withCommand("/bin/sh", "-c", "while true ; do printf 'HTTP/1.1 200 OK\\n\\nyay' | nc -l -p 8080; done"); | ||
|
||
GenericContainer bar = new GenericContainer() | ||
.withNetwork(network) | ||
.withCommand("top") | ||
) { | ||
foo.start(); | ||
bar.start(); | ||
|
||
String response = bar.execInContainer("wget", "-O", "-", "http://foo:8080").getStdout(); | ||
assertEquals("received response", "yay", response); | ||
} | ||
} | ||
|
||
@Test | ||
public void testBuilder() throws Exception { | ||
try ( | ||
Network network = Network.builder() | ||
.driver("macvlan") | ||
.build(); | ||
) { | ||
network.create(); | ||
assertEquals( | ||
"Flag is set", | ||
"macvlan", | ||
DockerClientFactory.instance().client().inspectNetworkCmd().withNetworkId(network.getName()).exec().getDriver() | ||
); | ||
} | ||
} | ||
|
||
@Test | ||
public void testModifiers() throws Exception { | ||
try ( | ||
Network network = Network.builder() | ||
.createNetworkCmdModifier(cmd -> cmd.withDriver("macvlan")) | ||
.build(); | ||
) { | ||
network.create(); | ||
assertEquals( | ||
"Flag is set", | ||
"macvlan", | ||
DockerClientFactory.instance().client().inspectNetworkCmd().withNetworkId(network.getName()).exec().getDriver() | ||
); | ||
} | ||
} | ||
|
||
@Test | ||
public void testLaziness() throws Exception { | ||
try ( | ||
Network network = newNetwork() | ||
) { | ||
assertFalse("Not created by default", network.isCreated()); | ||
assertNotNull("Returns an id", network.getId()); | ||
assertTrue("Is created after id request", network.isCreated()); | ||
} | ||
} | ||
} | ||
} |
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.
Do you really want a
Boolean
?