Skip to content

Commit fe4ab2b

Browse files
VietND96VietND96
committed
Fix review comment
Signed-off-by: Viet Nguyen Duc <[email protected]>
1 parent ca10286 commit fe4ab2b

File tree

4 files changed

+202
-37
lines changed

4 files changed

+202
-37
lines changed

‎java/src/org/openqa/selenium/docker/client/V144Adapter.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,14 @@ public Map<String, Object> adaptContainerInspectResponse(Map<String, Object> res
115115
// v1.44+ includes DNSNames field
116116
// Ensure deprecated fields are handled if present
117117
@SuppressWarnings("unchecked")
118-
Map<String, Object> networkSettings = (Map<String, Object>) adapted.get("NetworkSettings");
118+
Map<String, Object> originalNetworkSettings =
119+
(Map<String, Object>) adapted.get("NetworkSettings");
120+
121+
if (originalNetworkSettings != null) {
122+
// Create defensive copy to avoid mutating the original response
123+
Map<String, Object> networkSettings = new HashMap<>(originalNetworkSettings);
124+
adapted.put("NetworkSettings", networkSettings);
119125

120-
if (networkSettings != null) {
121126
// Remove deprecated fields if present (they shouldn't be in v1.44+)
122127
String[] deprecatedFields = {
123128
"HairpinMode",

‎java/src/org/openqa/selenium/docker/client/V148Adapter.java‎

Lines changed: 133 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -55,19 +55,6 @@ public Map<String, Object> adaptImageResponse(Map<String, Object> response) {
5555

5656
Map<String, Object> adapted = new HashMap<>(response);
5757

58-
// v1.48+ includes ImageManifestDescriptor for multi-platform images
59-
if (adapted.containsKey("ImageManifestDescriptor")) {
60-
LOG.fine(
61-
"Image response includes ImageManifestDescriptor (multi-platform support in API v"
62-
+ apiVersion
63-
+ ")");
64-
}
65-
66-
// v1.48+ includes Descriptor field (OCI descriptor)
67-
if (adapted.containsKey("Descriptor")) {
68-
LOG.fine("Image response includes OCI Descriptor field (API v" + apiVersion + ")");
69-
}
70-
7158
// Ensure VirtualSize is not present (removed in v1.44)
7259
if (adapted.containsKey("VirtualSize")) {
7360
LOG.warning(
@@ -77,6 +64,46 @@ public Map<String, Object> adaptImageResponse(Map<String, Object> response) {
7764
adapted.remove("VirtualSize");
7865
}
7966

67+
// Ensure Size field is present (required in v1.44+)
68+
if (!adapted.containsKey("Size")) {
69+
LOG.warning("Size field missing from image response in API v" + apiVersion);
70+
}
71+
72+
// v1.48+ includes ImageManifestDescriptor for multi-platform images
73+
// Extract platform information for better observability
74+
if (adapted.containsKey("ImageManifestDescriptor")) {
75+
@SuppressWarnings("unchecked")
76+
Map<String, Object> descriptor = (Map<String, Object>) adapted.get("ImageManifestDescriptor");
77+
if (descriptor != null && descriptor.containsKey("platform")) {
78+
Object platformObj = descriptor.get("platform");
79+
if (platformObj instanceof Map) {
80+
@SuppressWarnings("unchecked")
81+
Map<String, Object> platform = (Map<String, Object>) platformObj;
82+
String arch = (String) platform.get("architecture");
83+
String os = (String) platform.get("os");
84+
if (arch != null && os != null) {
85+
LOG.fine(
86+
String.format(
87+
"Image is platform-specific: %s/%s (API v%s multi-platform support)",
88+
os, arch, apiVersion));
89+
}
90+
}
91+
}
92+
}
93+
94+
// v1.48+ includes Descriptor field (OCI descriptor)
95+
// Validate OCI descriptor structure
96+
if (adapted.containsKey("Descriptor")) {
97+
@SuppressWarnings("unchecked")
98+
Map<String, Object> descriptor = (Map<String, Object>) adapted.get("Descriptor");
99+
if (descriptor != null) {
100+
String mediaType = (String) descriptor.get("mediaType");
101+
if (mediaType != null) {
102+
LOG.fine("Image includes OCI descriptor with mediaType: " + mediaType);
103+
}
104+
}
105+
}
106+
80107
return adapted;
81108
}
82109

@@ -89,6 +116,7 @@ public Map<String, Object> adaptContainerCreateRequest(Map<String, Object> reque
89116
Map<String, Object> adapted = new HashMap<>(request);
90117

91118
// v1.48+ supports Mount type "image" for mounting images inside containers
119+
// Validate image mount configurations
92120
@SuppressWarnings("unchecked")
93121
Map<String, Object> hostConfig = (Map<String, Object>) adapted.get("HostConfig");
94122

@@ -104,17 +132,31 @@ public Map<String, Object> adaptContainerCreateRequest(Map<String, Object> reque
104132
String type = (String) mountMap.get("Type");
105133

106134
if ("image".equals(type)) {
107-
LOG.fine(
108-
"Container creation includes image mount type (supported in API v"
109-
+ apiVersion
110-
+ "+)");
135+
// Validate required fields for image mounts
136+
String source = (String) mountMap.get("Source");
137+
String target = (String) mountMap.get("Target");
138+
139+
if (source == null || source.isEmpty()) {
140+
LOG.warning("Image mount missing required 'Source' field");
141+
}
142+
if (target == null || target.isEmpty()) {
143+
LOG.warning("Image mount missing required 'Target' field");
144+
}
145+
146+
if (source != null && target != null) {
147+
LOG.fine(
148+
String.format(
149+
"Mounting image '%s' at '%s' (API v%s+ image mount support)",
150+
source, target, apiVersion));
151+
}
111152
}
112153
}
113154
}
114155
}
115156
}
116157

117158
// v1.48+ supports GwPriority in NetworkingConfig for gateway priority
159+
// Validate and log gateway priority configuration
118160
@SuppressWarnings("unchecked")
119161
Map<String, Object> networkingConfig = (Map<String, Object>) adapted.get("NetworkingConfig");
120162

@@ -123,22 +165,39 @@ public Map<String, Object> adaptContainerCreateRequest(Map<String, Object> reque
123165
Map<String, Object> endpointsConfig =
124166
(Map<String, Object>) networkingConfig.get("EndpointsConfig");
125167

126-
if (endpointsConfig != null) {
168+
if (endpointsConfig != null && endpointsConfig.size() > 1) {
169+
// Track gateway priorities for multi-network containers
170+
int highestPriority = Integer.MIN_VALUE;
171+
String defaultGatewayNetwork = null;
172+
127173
for (Map.Entry<String, Object> entry : endpointsConfig.entrySet()) {
128174
if (entry.getValue() instanceof Map) {
129175
@SuppressWarnings("unchecked")
130176
Map<String, Object> endpointConfig = (Map<String, Object>) entry.getValue();
131177

132178
if (endpointConfig.containsKey("GwPriority")) {
133-
LOG.fine(
134-
"Network endpoint '"
135-
+ entry.getKey()
136-
+ "' includes GwPriority (supported in API v"
137-
+ apiVersion
138-
+ "+)");
179+
Object priorityObj = endpointConfig.get("GwPriority");
180+
int priority = priorityObj instanceof Number ? ((Number) priorityObj).intValue() : 0;
181+
182+
if (priority > highestPriority) {
183+
highestPriority = priority;
184+
defaultGatewayNetwork = entry.getKey();
185+
}
139186
}
140187
}
141188
}
189+
190+
if (defaultGatewayNetwork != null) {
191+
LOG.fine(
192+
String.format(
193+
"Container will use '%s' as default gateway (priority: %d, API v%s+)",
194+
defaultGatewayNetwork, highestPriority, apiVersion));
195+
} else {
196+
LOG.fine(
197+
String.format(
198+
"Creating container with %d networks, no explicit gateway priority set",
199+
endpointsConfig.size()));
200+
}
142201
}
143202
}
144203

@@ -153,34 +212,74 @@ public Map<String, Object> adaptContainerInspectResponse(Map<String, Object> res
153212

154213
Map<String, Object> adapted = new HashMap<>(response);
155214

156-
// v1.48+ includes ImageManifestDescriptor
215+
// v1.48+ includes ImageManifestDescriptor with platform information
216+
// Extract and expose platform details for better observability
157217
if (adapted.containsKey("ImageManifestDescriptor")) {
158-
LOG.fine(
159-
"Container inspect includes ImageManifestDescriptor (multi-platform support in API v"
160-
+ apiVersion
161-
+ ")");
218+
@SuppressWarnings("unchecked")
219+
Map<String, Object> descriptor = (Map<String, Object>) adapted.get("ImageManifestDescriptor");
220+
if (descriptor != null && descriptor.containsKey("platform")) {
221+
Object platformObj = descriptor.get("platform");
222+
if (platformObj instanceof Map) {
223+
@SuppressWarnings("unchecked")
224+
Map<String, Object> platform = (Map<String, Object>) platformObj;
225+
String arch = (String) platform.get("architecture");
226+
String os = (String) platform.get("os");
227+
String digest = (String) descriptor.get("digest");
228+
229+
if (arch != null && os != null) {
230+
LOG.fine(
231+
String.format(
232+
"Container running on %s/%s platform (digest: %s, API v%s+)",
233+
os,
234+
arch,
235+
digest != null ? digest.substring(0, Math.min(12, digest.length())) : "unknown",
236+
apiVersion));
237+
}
238+
}
239+
}
162240
}
163241

164242
// v1.48+ includes GwPriority in NetworkSettings
243+
// Identify which network is providing the default gateway
165244
@SuppressWarnings("unchecked")
166-
Map<String, Object> networkSettings = (Map<String, Object>) adapted.get("NetworkSettings");
245+
Map<String, Object> originalNetworkSettings =
246+
(Map<String, Object>) adapted.get("NetworkSettings");
247+
248+
if (originalNetworkSettings != null) {
249+
// Create defensive copy to avoid mutating the original response
250+
Map<String, Object> networkSettings = new HashMap<>(originalNetworkSettings);
251+
adapted.put("NetworkSettings", networkSettings);
167252

168-
if (networkSettings != null) {
169253
@SuppressWarnings("unchecked")
170254
Map<String, Object> networks = (Map<String, Object>) networkSettings.get("Networks");
171255

172-
if (networks != null) {
256+
if (networks != null && networks.size() > 1) {
257+
int highestPriority = Integer.MIN_VALUE;
258+
String defaultGatewayNetwork = null;
259+
173260
for (Map.Entry<String, Object> entry : networks.entrySet()) {
174261
if (entry.getValue() instanceof Map) {
175262
@SuppressWarnings("unchecked")
176263
Map<String, Object> network = (Map<String, Object>) entry.getValue();
177264

178265
if (network.containsKey("GwPriority")) {
179-
LOG.fine(
180-
"Network '" + entry.getKey() + "' includes GwPriority (API v" + apiVersion + ")");
266+
Object priorityObj = network.get("GwPriority");
267+
int priority = priorityObj instanceof Number ? ((Number) priorityObj).intValue() : 0;
268+
269+
if (priority > highestPriority) {
270+
highestPriority = priority;
271+
defaultGatewayNetwork = entry.getKey();
272+
}
181273
}
182274
}
183275
}
276+
277+
if (defaultGatewayNetwork != null) {
278+
LOG.fine(
279+
String.format(
280+
"Container using '%s' as default gateway (priority: %d)",
281+
defaultGatewayNetwork, highestPriority));
282+
}
184283
}
185284

186285
// Remove deprecated fields (should not be present in v1.48+)

‎java/test/org/openqa/selenium/docker/client/V144AdapterTest.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,4 +141,33 @@ void shouldHandleContainerInspectResponseWithoutNetworkSettings() {
141141
assertThat(adapted.get("Id")).isEqualTo("abc123");
142142
assertThat(adapted.containsKey("NetworkSettings")).isFalse();
143143
}
144+
145+
@Test
146+
void shouldNotMutateOriginalResponseWhenRemovingDeprecatedFields() {
147+
// Create original response with deprecated fields
148+
Map<String, Object> originalNetworkSettings = new HashMap<>();
149+
originalNetworkSettings.put("IPAddress", "172.17.0.2");
150+
originalNetworkSettings.put("HairpinMode", false);
151+
originalNetworkSettings.put("LinkLocalIPv6Address", "fe80::1");
152+
153+
Map<String, Object> originalResponse = new HashMap<>();
154+
originalResponse.put("Id", "container123");
155+
originalResponse.put("NetworkSettings", originalNetworkSettings);
156+
157+
// Adapt the response
158+
Map<String, Object> adapted = adapter.adaptContainerInspectResponse(originalResponse);
159+
160+
// Verify original response is unchanged
161+
assertThat(originalResponse.get("NetworkSettings")).isEqualTo(originalNetworkSettings);
162+
assertThat(originalNetworkSettings).containsKey("HairpinMode");
163+
assertThat(originalNetworkSettings).containsKey("LinkLocalIPv6Address");
164+
165+
// Verify adapted response has deprecated fields removed
166+
@SuppressWarnings("unchecked")
167+
Map<String, Object> adaptedNetworkSettings =
168+
(Map<String, Object>) adapted.get("NetworkSettings");
169+
assertThat(adaptedNetworkSettings).doesNotContainKey("HairpinMode");
170+
assertThat(adaptedNetworkSettings).doesNotContainKey("LinkLocalIPv6Address");
171+
assertThat(adaptedNetworkSettings).containsKey("IPAddress");
172+
}
144173
}

‎java/test/org/openqa/selenium/docker/client/V148AdapterTest.java‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@ void shouldRemoveVirtualSizeFromImageResponse() {
8989
void shouldHandleImageManifestDescriptorInImageResponse() {
9090
V148Adapter adapter = new V148Adapter("1.48");
9191

92-
Map<String, Object> descriptor = Map.of("digest", "sha256:xyz789", "platform", "linux/amd64");
92+
Map<String, Object> platform = Map.of("architecture", "amd64", "os", "linux");
93+
Map<String, Object> descriptor = Map.of("digest", "sha256:xyz789", "platform", platform);
9394

9495
Map<String, Object> response = new HashMap<>();
9596
response.put("Id", "sha256:abc123");
@@ -242,4 +243,35 @@ void shouldHandleNullContainerInspectResponse() {
242243
V148Adapter adapter = new V148Adapter("1.48");
243244
assertThat(adapter.adaptContainerInspectResponse(null)).isNull();
244245
}
246+
247+
@Test
248+
void shouldNotMutateOriginalResponseWhenRemovingDeprecatedFields() {
249+
V148Adapter adapter = new V148Adapter("1.48");
250+
251+
// Create original response with deprecated fields
252+
Map<String, Object> originalNetworkSettings = new HashMap<>();
253+
originalNetworkSettings.put("IPAddress", "172.17.0.2");
254+
originalNetworkSettings.put("HairpinMode", false);
255+
originalNetworkSettings.put("Bridge", "docker0");
256+
257+
Map<String, Object> originalResponse = new HashMap<>();
258+
originalResponse.put("Id", "container123");
259+
originalResponse.put("NetworkSettings", originalNetworkSettings);
260+
261+
// Adapt the response
262+
Map<String, Object> adapted = adapter.adaptContainerInspectResponse(originalResponse);
263+
264+
// Verify original response is unchanged
265+
assertThat(originalResponse.get("NetworkSettings")).isEqualTo(originalNetworkSettings);
266+
assertThat(originalNetworkSettings).containsKey("HairpinMode");
267+
assertThat(originalNetworkSettings).containsKey("Bridge");
268+
269+
// Verify adapted response has deprecated fields removed
270+
@SuppressWarnings("unchecked")
271+
Map<String, Object> adaptedNetworkSettings =
272+
(Map<String, Object>) adapted.get("NetworkSettings");
273+
assertThat(adaptedNetworkSettings).doesNotContainKey("HairpinMode");
274+
assertThat(adaptedNetworkSettings).doesNotContainKey("Bridge");
275+
assertThat(adaptedNetworkSettings).containsKey("IPAddress");
276+
}
245277
}

0 commit comments

Comments
 (0)