allow CreateContainer to specify NetworkConfig#2556
allow CreateContainer to specify NetworkConfig#2556nbrugger-tgm wants to merge 1 commit intodocker-java:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for specifying Docker’s NetworkingConfig when creating a container, enabling attachment to multiple networks at creation time (per #2555).
Changes:
- Introduces a new public API model
com.github.dockerjava.api.model.NetworkingConfig. - Exposes
getNetworkingConfig()/withNetworkingConfig(...)onCreateContainerCmd. - Updates
CreateContainerCmdImpl#exec()to merge endpoint settings instead of overwriting them.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docker-java-core/src/main/java/com/github/dockerjava/core/command/CreateContainerCmdImpl.java | Adds networkingConfig accessors and changes exec() to merge endpoint configs rather than replace them. |
| docker-java-api/src/main/java/com/github/dockerjava/api/model/NetworkingConfig.java | Adds a new API model for Docker NetworkingConfig. |
| docker-java-api/src/main/java/com/github/dockerjava/api/command/CreateContainerCmd.java | Exposes networkingConfig getter/setter on the public command interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (containerNetwork != null && hostConfig.getNetworkMode() != null) { | ||
| networkingConfig = new NetworkingConfig() | ||
| .withEndpointsConfig(singletonMap(hostConfig.getNetworkMode(), containerNetwork)); | ||
| // If the user explicitly sets networkingConfig or endpointConfig to null, its reasonable to not overwrite it |
There was a problem hiding this comment.
Typo in comment: "its reasonable" should be "it's reasonable".
| // If the user explicitly sets networkingConfig or endpointConfig to null, its reasonable to not overwrite it | |
| // If the user explicitly sets networkingConfig or endpointConfig to null, it's reasonable to not overwrite it |
| if (containerNetwork != null && hostConfig.getNetworkMode() != null) { | ||
| networkingConfig = new NetworkingConfig() | ||
| .withEndpointsConfig(singletonMap(hostConfig.getNetworkMode(), containerNetwork)); | ||
| // If the user explicitly sets networkingConfig or endpointConfig to null, its reasonable to not overwrite it | ||
| if (networkingConfig != null | ||
| && networkingConfig.getEndpointsConfig() != null | ||
| && !networkingConfig.getEndpointsConfig().containsKey(hostConfig.getNetworkMode()) | ||
| ) { | ||
| networkingConfig.getEndpointsConfig().put(hostConfig.getNetworkMode(), containerNetwork); | ||
| } |
There was a problem hiding this comment.
singletonMap is no longer used in this class after the networkingConfig logic change. Because the build runs Checkstyle with failOnViolation=true, this unused static import will likely fail CI; remove the now-unused import static java.util.Collections.singletonMap;.
| // If the user explicitly sets networkingConfig or endpointConfig to null, its reasonable to not overwrite it | ||
| if (networkingConfig != null | ||
| && networkingConfig.getEndpointsConfig() != null | ||
| && !networkingConfig.getEndpointsConfig().containsKey(hostConfig.getNetworkMode()) | ||
| ) { |
There was a problem hiding this comment.
This change adds a new public API surface (withNetworkingConfig/getNetworkingConfig) and changes create-container networking behavior (merging endpoint configs). There are existing integration tests for container networking in CreateContainerCmdIT, but none that assert attaching to multiple networks at creation via NetworkingConfig. Adding a test that creates two networks and verifies the container is attached to both would help prevent regressions.
| // If the user explicitly sets networkingConfig or endpointConfig to null, its reasonable to not overwrite it | |
| if (networkingConfig != null | |
| && networkingConfig.getEndpointsConfig() != null | |
| && !networkingConfig.getEndpointsConfig().containsKey(hostConfig.getNetworkMode()) | |
| ) { | |
| // If the user already provided endpointsConfig, merge without overwriting existing entries. | |
| if (networkingConfig == null || networkingConfig.getEndpointsConfig() == null) { | |
| networkingConfig = new NetworkingConfig() | |
| .withEndpointsConfig(singletonMap(hostConfig.getNetworkMode(), containerNetwork)); | |
| } else if (!networkingConfig.getEndpointsConfig().containsKey(hostConfig.getNetworkMode())) { |
| public class NetworkingConfig { | ||
|
|
||
| public NetworkingConfig() { | ||
| this.endpointsConfig = new HashMap<>(); | ||
| } | ||
| public NetworkingConfig(Map<String, ContainerNetwork> endpointsConfig) { | ||
| this.endpointsConfig = endpointsConfig; | ||
| } |
There was a problem hiding this comment.
NetworkingConfig as added won’t compile and will fail existing architecture/serialization tests: it uses HashMap without importing it, and it does not extend DockerObject nor implement Serializable/serialVersionUID (see docker-java-api ArchUnit test and ModelsSerializableTest which enforce these conventions for com.github.dockerjava.api.model classes). Update the class to follow the existing model conventions (extend DockerObject, implement Serializable, add serialVersionUID, and fix missing imports).
| if (containerNetwork != null && hostConfig.getNetworkMode() != null) { | ||
| networkingConfig = new NetworkingConfig() | ||
| .withEndpointsConfig(singletonMap(hostConfig.getNetworkMode(), containerNetwork)); | ||
| // If the user explicitly sets networkingConfig or endpointConfig to null, its reasonable to not overwrite it | ||
| if (networkingConfig != null | ||
| && networkingConfig.getEndpointsConfig() != null | ||
| && !networkingConfig.getEndpointsConfig().containsKey(hostConfig.getNetworkMode()) | ||
| ) { | ||
| networkingConfig.getEndpointsConfig().put(hostConfig.getNetworkMode(), containerNetwork); | ||
| } |
There was a problem hiding this comment.
networkingConfig.getEndpointsConfig().put(...) mutates the caller-provided map. If the user supplied an immutable map (e.g., Map.of(...)/Collections.unmodifiableMap(...)), this will throw UnsupportedOperationException at runtime. Consider copying to a mutable map (and setting it back via withEndpointsConfig) before adding/merging the endpoint, and also handle the null endpointsConfig case by creating a new map when you need to add the default network endpoint.
Closes: #2555