-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 8 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 |
---|---|---|
|
@@ -75,6 +75,12 @@ public class GenericContainer<SELF extends GenericContainer<SELF>> | |
@NonNull | ||
private String networkMode; | ||
|
||
@NonNull | ||
private Network network; | ||
|
||
@NonNull | ||
private List<String> networkAliases = new ArrayList<>(); | ||
|
||
@NonNull | ||
private Future<String> image; | ||
|
||
|
@@ -412,7 +418,14 @@ private void applyConfiguration(CreateContainerCmd createCommand) { | |
.toArray(String[]::new); | ||
createCommand.withExtraHosts(extraHostsArray); | ||
|
||
if (networkMode != null) { | ||
if (network != null) { | ||
if (!network.isCreated()) { | ||
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. Is this likely to happen? If the network hasn't been created, could we infer that the user wants it to be created and trigger creation? |
||
throw new ContainerLaunchException("Aborting attempt to use non-existing network " + network.getName()); | ||
} | ||
|
||
createCommand.withNetworkMode(network.getName()); | ||
createCommand.withAliases(this.networkAliases); | ||
} else if (networkMode != null) { | ||
createCommand.withNetworkMode(networkMode); | ||
} | ||
|
||
|
@@ -615,6 +628,18 @@ public SELF withNetworkMode(String networkMode) { | |
return self(); | ||
} | ||
|
||
@Override | ||
public SELF withNetwork(Network network) { | ||
this.network = network; | ||
return self(); | ||
} | ||
|
||
@Override | ||
public SELF withNetworkAliases(String... aliases) { | ||
Collections.addAll(this.networkAliases, aliases); | ||
return self(); | ||
} | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
package org.testcontainers.containers; | ||
|
||
import com.github.dockerjava.api.command.CreateNetworkCmd; | ||
import lombok.*; | ||
import lombok.experimental.Delegate; | ||
import org.junit.rules.ExternalResource; | ||
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.AtomicBoolean; | ||
import java.util.function.Consumer; | ||
|
||
public interface Network extends AutoCloseable { | ||
|
||
String getName(); | ||
|
||
Boolean getEnableIpv6(); | ||
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. Do you really want a |
||
|
||
String getDriver(); | ||
|
||
boolean isCreated(); | ||
|
||
@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 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<>(); | ||
|
||
@Override | ||
public boolean isCreated() { | ||
return false; | ||
} | ||
} | ||
|
||
@RequiredArgsConstructor | ||
class Runnable implements Network, java.lang.Runnable { | ||
|
||
@Delegate(excludes = Excludes.class) | ||
protected final Network network; | ||
|
||
private final AtomicBoolean created = new AtomicBoolean(false); | ||
|
||
@Override | ||
public void run() { | ||
if (!created.getAndSet(true)) { | ||
ResourceReaper.instance().registerNetworkForCleanup(getName()); | ||
|
||
CreateNetworkCmd createNetworkCmd = DockerClientFactory.instance().client().createNetworkCmd(); | ||
|
||
createNetworkCmd.withName(getName()); | ||
createNetworkCmd.withCheckDuplicate(true); | ||
|
||
if (getEnableIpv6() != null) { | ||
createNetworkCmd.withEnableIpv6(getEnableIpv6()); | ||
} | ||
|
||
if (getDriver() != null) { | ||
createNetworkCmd.withDriver(getDriver()); | ||
} | ||
|
||
if (network instanceof NetworkImpl) { | ||
for (Consumer<CreateNetworkCmd> consumer : ((NetworkImpl) network).getCreateNetworkCmdModifiers()) { | ||
consumer.accept(createNetworkCmd); | ||
} | ||
} | ||
|
||
createNetworkCmd.exec(); | ||
} | ||
} | ||
|
||
@Override | ||
public boolean isCreated() { | ||
return created.get(); | ||
} | ||
} | ||
|
||
class AutoCreated implements Network { | ||
|
||
@Delegate(types = Network.class) | ||
protected final Network.Runnable network; | ||
|
||
public AutoCreated(Network network) { | ||
this.network = network.as(Runnable.class); | ||
this.network.run(); | ||
} | ||
} | ||
|
||
class JUnitRule extends ExternalResource implements Network { | ||
|
||
@Delegate(types = Network.class) | ||
protected final Network.Runnable network; | ||
|
||
public JUnitRule(Network network) { | ||
this.network = network.as(Runnable.class); | ||
} | ||
|
||
@Override | ||
protected void before() throws Throwable { | ||
network.run(); | ||
} | ||
|
||
@Override | ||
protected void after() { | ||
network.close(); | ||
} | ||
} | ||
|
||
interface Excludes { | ||
boolean isCreated(); | ||
} | ||
} |
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); | ||
} | ||
} | ||
} |
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.
Trivial grammar note -
the
is unnecessary hereThere 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.
Thanks! 👍 One day I will learn how to use them properly :D