Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Only publish exposed ports #4122

Merged
merged 11 commits into from
May 26, 2021
Merged

Only publish exposed ports #4122

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0e0e8c8
WIP
rnorth May 24, 2021
cc133cd
Revert accidental change
rnorth May 24, 2021
c40be26
Make exposed ports configuration distinct, in case accidental duplica…
rnorth May 24, 2021
1c0cded
Explicitly add exposed ports in CouchbaseContainer
rnorth May 24, 2021
1373b0e
Tidy and add comments
rnorth May 24, 2021
006533e
Merge branch 'master' into only-publish-exposed-ports
rnorth May 24, 2021
c0dcddc
Change port binding assertion to work cross-platform, and add check t…
rnorth May 25, 2021
d931379
Ensure that port bindings are unique
rnorth May 25, 2021
5583d00
Inline vars
rnorth May 25, 2021
b80f00f
Change assertion
rnorth May 25, 2021
a9272c6
Merge branch 'master' into only-publish-exposed-ports
rnorth May 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 62 additions & 51 deletions core/src/main/java/org/testcontainers/containers/GenericContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
package org.testcontainers.containers;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding imports, there are some changes in ordering, but AFAICT these only match the IntelliJ defaults (with our .editorconfig tweaks layered on top for star imports).

TBH I'd prefer to go with the changed (hopefully stable) import order in this PR, rather than continue to constantly have the burden of maintaining the order (I don't know why the current order is as it is).

I think if we find ourselves continuing to run up against this then we should introduce a code formatter to our build (e.g. spotless gradle plugin + google formatter with AOSP style (4 space indent)).


import static com.google.common.collect.Lists.newArrayList;
import static org.testcontainers.utility.CommandLine.runShellCommand;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.SerializationFeature;
Expand All @@ -16,46 +13,14 @@
import com.github.dockerjava.api.model.HostConfig;
import com.github.dockerjava.api.model.Link;
import com.github.dockerjava.api.model.PortBinding;
import com.github.dockerjava.api.model.Ports;
import com.github.dockerjava.api.model.Volume;
import com.github.dockerjava.api.model.VolumesFrom;
import com.github.dockerjava.core.DefaultDockerClientConfig;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.hash.Hashing;
import java.io.File;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.UndeclaredThrowableException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.zip.Adler32;
import java.util.zip.Checksum;
import lombok.AccessLevel;
import lombok.Data;
import lombok.NonNull;
Expand Down Expand Up @@ -97,6 +62,43 @@
import org.testcontainers.utility.ResourceReaper;
import org.testcontainers.utility.TestcontainersConfiguration;

import java.io.File;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.UndeclaredThrowableException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.zip.Adler32;
import java.util.zip.Checksum;

import static com.google.common.collect.Lists.newArrayList;
import static org.testcontainers.utility.CommandLine.runShellCommand;

/**
* Base class for that allows a container to be launched and controlled.
*/
Expand Down Expand Up @@ -718,21 +720,32 @@ public Set<Integer> getLivenessCheckPortNumbers() {

private void applyConfiguration(CreateContainerCmd createCommand) {
HostConfig hostConfig = buildHostConfig();
createCommand.withHostConfig(hostConfig);

// Set up exposed ports (where there are no host port bindings defined)
ExposedPort[] portArray = exposedPorts.stream()
.map(ExposedPort::new)
.toArray(ExposedPort[]::new);
// PortBindings must contain:
// * all exposed ports with a randomized host port (equivalent to -p CONTAINER_PORT)
// * all exposed ports with a fixed host port (equivalent to -p HOST_PORT:CONTAINER_PORT)
List<PortBinding> allPortBindings = new ArrayList<>();
// First collect all the randomized host ports from our 'exposedPorts' field
exposedPorts.stream()
.map(ExposedPort::new)
.map(p -> new PortBinding(Ports.Binding.empty(), p))
.forEachOrdered(allPortBindings::add);
// Next collect all the fixed host ports from our 'portBindings' field
portBindings.stream()
.map(PortBinding::parse)
.forEachOrdered(allPortBindings::add);
bsideup marked this conversation as resolved.
Show resolved Hide resolved

hostConfig.withPortBindings(allPortBindings);

// Next, ExposedPorts must be set up to publish all of the above ports, randomized and fixed.
// Collect all of the exposed ports for publication
final List<ExposedPort> exposedPorts = allPortBindings.stream()
.map(PortBinding::getExposedPort)
.distinct()
.collect(Collectors.toList());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the thing that is substantially different from the previous implementation; changes above are mainly to support this change and to make it clearer what we're doing.

Of course, createCommand.withPublishAllPorts(true); on line 801 is also nuked.

createCommand.withExposedPorts(exposedPorts);

createCommand.withExposedPorts(portArray);

// Set up exposed ports that need host port bindings
PortBinding[] portBindingArray = portBindings.stream()
.map(PortBinding::parse)
.toArray(PortBinding[]::new);

createCommand.withPortBindings(portBindingArray);
createCommand.withHostConfig(hostConfig);

if (commandParts != null) {
createCommand.withCmd(commandParts);
Expand Down Expand Up @@ -798,8 +811,6 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
createCommand.withNetworkMode(networkForLinks.get());
}

createCommand.withPublishAllPorts(true);

PortForwardingContainer.INSTANCE.getNetwork().ifPresent(it -> {
withExtraHost(INTERNAL_HOST_HOSTNAME, it.getIpAddress());
});
Expand Down
61 changes: 60 additions & 1 deletion core/src/test/java/org/testcontainers/containers/GenericContainerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package org.testcontainers.containers;

import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.command.InspectContainerResponse;
import com.github.dockerjava.api.command.InspectContainerResponse.ContainerState;
import com.github.dockerjava.api.model.ExposedPort;
import com.github.dockerjava.api.model.Info;
import com.github.dockerjava.api.model.Ports;
import com.google.common.primitives.Ints;
import lombok.RequiredArgsConstructor;
import lombok.SneakyThrows;
import lombok.experimental.FieldDefaults;
Expand All @@ -11,15 +15,23 @@
import org.assertj.core.api.Assumptions;
import org.junit.Test;
import org.rnorth.ducttape.unreliables.Unreliables;
import org.testcontainers.TestImages;
import org.testcontainers.DockerClientFactory;
import org.testcontainers.TestImages;
import org.testcontainers.containers.startupcheck.StartupCheckStrategy;
import org.testcontainers.containers.wait.strategy.AbstractWaitStrategy;
import org.testcontainers.images.builder.ImageFromDockerfile;

import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals;
import static org.rnorth.visibleassertions.VisibleAssertions.assertThrows;
import static org.rnorth.visibleassertions.VisibleAssertions.assertTrue;

public class GenericContainerTest {

Expand Down Expand Up @@ -60,6 +72,53 @@ public void shouldReportErrorAfterWait() {
}
}

@Test
public void shouldOnlyPublishExposedPorts() {
ImageFromDockerfile image = new ImageFromDockerfile("publish-multiple")
.withDockerfileFromBuilder(builder ->
builder
.from("testcontainers/helloworld:1.1.0")
.expose(8080, 8081)
.build()
);
try (GenericContainer<?> container = new GenericContainer<>(image).withExposedPorts(8080)) {
container.start();

InspectContainerResponse inspectedContainer = container.getContainerInfo();

List<Integer> exposedPorts = Arrays.stream(inspectedContainer.getConfig().getExposedPorts())
.map(ExposedPort::getPort)
.collect(Collectors.toList());

assertEquals(
"the exposed ports are all of those EXPOSEd by the image",
Ints.asList(8080, 8081),
exposedPorts
);

Map<ExposedPort, Ports.Binding[]> hostBindings = inspectedContainer.getHostConfig().getPortBindings().getBindings();
assertEquals(
"only 1 port is bound on the host (published)",
1,
hostBindings.size()
);

Integer mappedPort = container.getMappedPort(8080);
assertTrue(
"port 8080 is bound to an ephemeral port on the host",
mappedPort >= 32768
bsideup marked this conversation as resolved.
Show resolved Hide resolved
);

assertThrows(
"trying to get a non-bound port mapping fails",
IllegalArgumentException.class,
() -> {
container.getMappedPort(8081);
}
);
}
}

static class NoopStartupCheckStrategy extends StartupCheckStrategy {

@Override
Expand Down
6 changes: 4 additions & 2 deletions modules/clickhouse/src/main/java/org/testcontainers/containers/ClickHouseContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package org.testcontainers.containers;

import com.google.common.collect.Sets;
import org.testcontainers.containers.wait.strategy.HttpWaitStrategy;
import org.testcontainers.utility.DockerImageName;

import java.time.Duration;
import java.util.Set;

public class ClickHouseContainer extends JdbcDatabaseContainer {
public static final String NAME = "clickhouse";
Expand Down Expand Up @@ -54,8 +56,8 @@ public ClickHouseContainer(final DockerImageName dockerImageName) {
}

@Override
protected Integer getLivenessCheckPort() {
return getMappedPort(HTTP_PORT);
public Set<Integer> getLivenessCheckPortNumbers() {
Copy link
Member Author

@rnorth rnorth May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly required for this PR, but during experimenting with approaches I found that a handful of our modules are using the getLivenessCheckPort method, which has been deprecated for 4 years. I'd like to take this chance to clean up.

return Sets.newHashSet(HTTP_PORT);
bsideup marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand Down
13 changes: 13 additions & 0 deletions modules/couchbase/src/main/java/org/testcontainers/couchbase/CouchbaseContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,19 @@ public String getConnectionString() {
protected void configure() {
super.configure();

addExposedPorts(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly the Couchbase module seems to have always relied on an EXPOSE statement + all-ports publication.

MGMT_PORT,
MGMT_SSL_PORT,
VIEW_PORT,
VIEW_SSL_PORT,
QUERY_PORT,
QUERY_SSL_PORT,
SEARCH_PORT,
SEARCH_SSL_PORT,
KV_PORT,
KV_SSL_PORT
);

WaitAllStrategy waitStrategy = new WaitAllStrategy();

// Makes sure that all nodes in the cluster are healthy.
Expand Down
7 changes: 5 additions & 2 deletions modules/mariadb/src/main/java/org/testcontainers/containers/MariaDBContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package org.testcontainers.containers;

import com.google.common.collect.Sets;
import org.testcontainers.utility.DockerImageName;

import java.util.Set;

/**
* Container implementation for the MariaDB project.
*
Expand Down Expand Up @@ -51,8 +54,8 @@ public MariaDBContainer(final DockerImageName dockerImageName) {
}

@Override
protected Integer getLivenessCheckPort() {
return getMappedPort(MARIADB_PORT);
public Set<Integer> getLivenessCheckPortNumbers() {
return Sets.newHashSet(MARIADB_PORT);
}

@Override
Expand Down
6 changes: 4 additions & 2 deletions modules/mssqlserver/src/main/java/org/testcontainers/containers/MSSQLServerContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package org.testcontainers.containers;

import com.google.common.collect.Sets;
import org.testcontainers.utility.DockerImageName;
import org.testcontainers.utility.LicenseAcceptance;

import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Stream;

Expand Down Expand Up @@ -60,8 +62,8 @@ public MSSQLServerContainer(final DockerImageName dockerImageName) {
}

@Override
protected Integer getLivenessCheckPort() {
return getMappedPort(MS_SQL_SERVER_PORT);
public Set<Integer> getLivenessCheckPortNumbers() {
return Sets.newHashSet(MS_SQL_SERVER_PORT);
}

@Override
Expand Down
6 changes: 4 additions & 2 deletions modules/oracle-xe/src/main/java/org/testcontainers/containers/OracleContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package org.testcontainers.containers;

import com.google.common.collect.Sets;
import org.testcontainers.utility.DockerImageName;
import org.testcontainers.utility.TestcontainersConfiguration;

import java.util.Set;
import java.util.concurrent.Future;

/**
Expand Down Expand Up @@ -61,8 +63,8 @@ private void preconfigure() {
}

@Override
protected Integer getLivenessCheckPort() {
return getMappedPort(ORACLE_PORT);
public Set<Integer> getLivenessCheckPortNumbers() {
return Sets.newHashSet(ORACLE_PORT);
}

@Override
Expand Down