-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
use ssh port forwarding to expose host's ports to the containers #806
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
Conversation
4f30096 to
f71c2bc
Compare
rnorth
left a comment
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.
This is pretty awesome, nice work!
I assume we are OK regarding security - this will not be accessible on any real external (LAN) network, will it?
We can update the docs after releasing. I think this will warrant some major examples updates too :)
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to generate a random password
kiview
left a comment
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.
Really love this feature, already thinking about how to refactor some of our tests at work, which are hardcoded interacting with the Linux host network.
| import java.util.UUID; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| public enum PortForwardingContainer { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the easiest way to achieve singleton in JVM, used by Spring and others.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method of the ssh lib is so cool 😎
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better, to use host.testcontainers here?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K, makes sense to be similar to Docker.
|
|
||
| containerId = createCommand.exec().getId(); | ||
|
|
||
| PortForwardingContainer.INSTANCE.getNetwork().map(ContainerNetwork::getNetworkID).ifPresent(networkId -> { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 yes, I think that would help the readability.
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.
@kiview extracted
| } | ||
|
|
||
| Optional<ContainerNetwork> getNetwork() { | ||
| return Optional.ofNullable(container) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;