use ssh port forwarding to expose host's ports to the containers #806
Conversation
bsideup
force-pushed
the
expose_host_ports
branch
from
4f30096
to
f71c2bc
Compare
| @@ -245,6 +248,13 @@ private void tryStart(Profiler profiler) { | |||
| applyConfiguration(createCommand); | |||
|
|
|||
| containerId = createCommand.exec().getId(); | |||
|
|
|||
| PortForwardingContainer.INSTANCE.getNetwork().map(ContainerNetwork::getNetworkID).ifPresent(networkId -> { | |||
| if (!Arrays.asList(networkId, "none", "host").contains(createCommand.getNetworkMode())) { | |||
There was a problem hiding this comment.
Ah good, does this prevent the sshd container being connected to the host network?
Obviously it would be bad to let this thing be accessible on real networks 😬
There was a problem hiding this comment.
host & none networks are not connectable, this is why I added it :)
We could also use a random password to improve the security
There was a problem hiding this comment.
If adding a random password isn't a massive change, let's do it. I can't think of a practical attack via this container, but we may as well just in case we've missed something.
There was a problem hiding this comment.
changed to generate a random password
| import java.util.UUID; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| public enum PortForwardingContainer { |
There was a problem hiding this comment.
What's the reason to use an Enum here and not a Singleton? Maybe I'm missing something obvious, or do we use this approach elsewhere as well?
There was a problem hiding this comment.
that's the easiest way to achieve singleton in JVM, used by Spring and others.
There was a problem hiding this comment.
Okay, I didn't know this, nice I could learn something new 😄
| @SneakyThrows | ||
| public void exposeHostPort(int port) { | ||
| if (exposedPorts.add(port)) { | ||
| getSshConnection().requestRemotePortForwarding("", port, "localhost", port); |
There was a problem hiding this comment.
This method of the ssh lib is so cool 😎
| @@ -85,6 +86,8 @@ | |||
|
|
|||
| public static final int CONTAINER_RUNNING_TIMEOUT_SEC = 30; | |||
|
|
|||
| public static final String INTERNAL_HOST_HOSTNAME = "host.testcontainers.internal"; | |||
There was a problem hiding this comment.
Maybe better, to use host.testcontainers here?
There was a problem hiding this comment.
While the .internal TLD is technically 'wrong' to use, I can't actually find an official TLD that is properly reserved and semantically correct.
Given that Docker is host.docker.internal, I feel it's OK to follow in their tracks and keep host.testcontainers.internal.
There was a problem hiding this comment.
K, makes sense to be similar to Docker.
| @@ -245,6 +248,13 @@ private void tryStart(Profiler profiler) { | |||
| applyConfiguration(createCommand); | |||
|
|
|||
| containerId = createCommand.exec().getId(); | |||
|
|
|||
| PortForwardingContainer.INSTANCE.getNetwork().map(ContainerNetwork::getNetworkID).ifPresent(networkId -> { | |||
There was a problem hiding this comment.
WDYT about refactoring to private method for better readibility? Took me some time to get the high level view what's happening here (and I possibly even got it wrong):
[...]
String networkMode = createCommand.getNetworkMode();
connectToPortForwardingNetwork(networkMode);
[...]
private void connectToPortForwardingNetwork(String networkMode) {
PortForwardingContainer.INSTANCE.getNetwork().map(ContainerNetwork::getNetworkID).ifPresent(networkId -> {
if (!Arrays.asList(networkId, "none", "host").contains(networkMode)) {
dockerClient.connectToNetworkCmd().withContainerId(containerId).withNetworkId(networkId).exec();
}
});
}
There was a problem hiding this comment.
👍 yes, I think that would help the readability.
| } | ||
|
|
||
| Optional<ContainerNetwork> getNetwork() { | ||
| return Optional.ofNullable(container) |
There was a problem hiding this comment.
I think we have the same exact code multiple times in our code base, so we could add a utility method for this? (not in this PR).
Optional<ContainerNetwork> getFirstNetwork() in GenericContainer?
There was a problem hiding this comment.
this code doesn't have any logic in it (and just calls the getters), but exposing it as public API (in Testcontainers) makes me feel uneasy, I would rather not do that
|
@rnorth AFAIK the SSH server is only available to Docker networks, if I understood the implementation correctly? |
|
@kiview yep, that makes perfect sense. |
|
We have this out in a Release Candidate build (1.9.0-rc1) for anyone who is keen to try it! |
// tell Testcontainers which host ports to expose to your containers
Testcontainers.exposeHostPorts(someHostPortNumber);// later, just give your container this address to use to reach back to the host:
address = "http://host.testcontainers.internal:" + someHostPortNumber;