[grid][java] browser containers provisioned in dynamic grid can get hostconfig from node-docker

[VietND96]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Fix: SeleniumHQ/docker-selenium#2143
Fix: SeleniumHQ/docker-selenium#2098
Fix: SeleniumHQ/docker-selenium#1812
Fix: SeleniumHQ/docker-selenium#1642

Docker compose file to deploy node-docker looks like

services:
  node-docker:
    image: ${NAMESPACE}/node-docker:${TAG}
    user: root
    volumes:
      - ./videos:/opt/selenium/assets
      - ./videos/config.toml:/opt/bin/config.toml
      - /var/run/docker.sock:/var/run/docker.sock
      - ./videos/Downloads:/home/seluser/Downloads
    dns:
      - 8.8.8.8
      - 8.8.4.4
    dns_search: selenium-grid.local
    extra_hosts:
      - "prod.domain.com:${HOST_IP}"
    depends_on:
      - selenium-hub
    environment:
      - SE_EVENT_BUS_HOST=selenium-hub
      - SE_EVENT_BUS_PUBLISH_PORT=4442
      - SE_EVENT_BUS_SUBSCRIBE_PORT=4443
      - SE_NODE_ENABLE_MANAGED_DOWNLOADS=true
      - SE_LOG_LEVEL=${LOG_LEVEL}

config.toml looks like

[docker]
configs = [
    "${NAMESPACE}/standalone-firefox:${TAG}", '{"browserName": "firefox", "platformName": "linux"}',
    "${NAMESPACE}/standalone-chrome:${TAG}", '{"browserName": "chrome", "platformName": "linux"}',
    "${NAMESPACE}/standalone-edge:${TAG}", '{"browserName": "MicrosoftEdge", "platformName": "linux"}'
    ]

host-config-keys = ["Dns", "DnsOptions", "DnsSearch", "ExtraHosts", "Binds"]

url = "http://127.0.0.1:2375"
video-image = "${NAMESPACE}/video:${VIDEO_TAG}"

[node]
enable-managed-downloads = true
override-max-sessions = true
max-sessions = 4

With the config key host-config-keys under section [docker] in a config.toml file (or CLI option --docker-host-config-keys). Users can specify a list of docker host configuration keys that should be passed to browser containers.

Valid key names for Docker host config can be found in the Docker API documentation or via the command docker inspect the node-docker container.

Steps to debug

Set the log-level to FINE, and you are able to see the Container config looks like

10:15:38.988 DEBUG [DockerSessionFactory.createBrowserContainer] - Container config: ContainerConfig{image=Image{summary=org.openqa.selenium.docker.internal.ImageSummary@3bcbb589}, portBindings={}, envVars={SE_EVENT_BUS_PUBLISH_PORT=4442, SE_EVENT_BUS_HOST=selenium-hub, SE_REJECT_UNSUPPORTED_CAPS=false, SE_EVENT_BUS_SUBSCRIBE_PORT=4443, TZ=UTC, SE_NODE_ENABLE_MANAGED_DOWNLOADS=false, SE_OTEL_TRACES_EXPORTER=otlp, SE_BIND_HOST=false, SE_OTEL_JAVA_GLOBAL_AUTOCONFIGURE_ENABLED=true, SE_OTEL_SERVICE_NAME=selenium-node-docker, SE_LOG_LEVEL=FINE}, volumeBinds={}, networkName=tests_default, devices=[], autoRemove=true, shmSize=2147483648, hostConfig={ExtraHosts=[prod.domain.com:10.1.0.178], Binds=[/home/runner/work/docker-selenium/docker-selenium/tests/videos:/opt/selenium/assets:rw, /home/runner/work/docker-selenium/docker-selenium/tests/videos/config.toml:/opt/bin/config.toml:rw, /var/run/docker.sock:/var/run/docker.sock:rw, /home/runner/work/docker-selenium/docker-selenium/tests/videos/Downloads:/home/seluser/Downloads:rw], Dns=[8.8.8.8, 8.8.4.4], DnsSearch=[selenium-grid.local]}}

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

Type

enhancement

---

Description

• Added support for Docker host configurations in Selenium Grid's Docker integration, allowing for more granular control over browser containers.
• Introduced hostConfig in ContainerConfig and ContainerInfo to manage and apply Docker container configurations.
• Added a new flag --docker-host-config-keys to specify which host configuration keys should be passed to browser containers.
• Enhanced DockerSessionFactory to apply specified host configurations to browser containers, improving flexibility in container management.

---

Changes walkthrough

Relevant files

Enhancement

ContainerConfig.java Enhance ContainerConfig with Host Configuration Support    java/src/org/openqa/selenium/docker/ContainerConfig.java Added support for hostConfig in ContainerConfig to manage Docker container configurations. Introduced a new constructor and a method getHostConfig to filter and apply host configurations based on provided keys. +34/-0    ContainerInfo.java Include Host Config in ContainerInfo                                          java/src/org/openqa/selenium/docker/ContainerInfo.java Added hostConfig map to store Docker host configurations. Updated constructor to include hostConfig and added a getter method for it. +11/-1    InspectContainer.java Update InspectContainer to Fetch HostConfig                            java/src/org/openqa/selenium/docker/v1_41/InspectContainer.java Modified apply method to include hostConfig in ContainerInfo instantiation. +2/-1      DockerFlags.java Add Docker Host Config Keys Flag                                                  java/src/org/openqa/selenium/grid/node/docker/DockerFlags.java Introduced --docker-host-config-keys flag to specify Docker container host configuration keys. +12/-0    DockerOptions.java Implement Host Config Keys Handling in DockerOptions          java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java Added handling for hostConfigKeys to filter and apply specific Docker host configurations. +12/-1    DockerSessionFactory.java Enhance DockerSessionFactory with Host Config Management  java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java Added hostConfig and hostConfigKeys to manage Docker host configurations for browser containers. Updated createBrowserContainer method to apply host configurations based on the provided keys. +14/-4

---

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR Description updated to latest commit (a73ba2f)

• Copy walkthrough table to "Files Changed" Tab

[qodo-merge-pro]

PR Review

⏱️ Estimated effort to review [1-5]	4, because the PR involves multiple changes across various files that integrate new functionality for handling host configurations in Docker containers. The changes are spread across the creation, configuration, and inspection of containers, which requires a thorough understanding of the existing Docker integration within the Selenium Grid. Additionally, the introduction of new methods and modifications to existing ones necessitate a detailed review to ensure compatibility and adherence to project standards.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: The method getHostConfig in DockerSessionFactory.java non-null asserts hostConfig and hostConfigKeys, but getDockerHostConfig in DockerOptions.java can potentially return null, which could lead to a NullPointerException.
	Inconsistency: The handling of hostConfig in ContainerConfig.java merges the provided hostConfig map with itself, which seems redundant and could be a mistake. This occurs in the toJson method where copyMap.putAll(this.hostConfig); is called.
	Performance Concern: The use of streams and filters in ContainerConfig.java for getHostConfig method might introduce performance overhead, especially for large configurations. It's worth considering optimizing this part of the code.
🔒 Security concerns	No

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[qodo-merge-pro]

PR Code Suggestions

Category	Suggestions
Best practice	Use ImmutableMap for initializing hostConfig in the constructor. Consider using ImmutableMap instead of HashMap for initializing hostConfig in the constructor to ensure immutability of the configuration maps. This change enhances thread safety and predictability of the ContainerConfig instances. java/src/org/openqa/selenium/docker/ContainerConfig.java [56] -this(image, portBindings, envVars, volumeBinds, devices, networkName, shmSize, new HashMap<>()); +this(image, portBindings, envVars, volumeBinds, devices, networkName, shmSize, ImmutableMap.of());
Possible issue	Add null check for hostConfig in getHostConfig method. To avoid potential NullPointerException, add a null check for hostConfig parameter in the getHostConfig method before streaming and collecting keys. java/src/org/openqa/selenium/docker/ContainerConfig.java [142-146] -configKeys.stream() - .filter(hostConfig::containsKey) - .filter(key -> hostConfig.get(key) != null) - .collect(Collectors.toMap(key -> key, hostConfig::get)); +if (hostConfig != null) { + return configKeys.stream() + .filter(hostConfig::containsKey) + .filter(key -> hostConfig.get(key) != null) + .collect(Collectors.toMap(key -> key, hostConfig::get)); +} +return Collections.emptyMap();
	Handle potential null hostConfig in ContainerInfo constructor. Ensure hostConfig is safely handled when it's null. The constructor currently requires it to be non-null, but there might be cases where hostConfig is not provided. Consider allowing hostConfig to be nullable or provide a default empty map. java/src/org/openqa/selenium/docker/ContainerInfo.java [44] -this.hostConfig = Require.nonNull("Host config", hostConfig); +this.hostConfig = hostConfig != null ? hostConfig : Collections.emptyMap();
Enhancement	Simplify merging logic in toJson method for hostConfig. The logic to merge hostConfig maps in toJson method seems redundant and potentially incorrect as it copies the map onto itself. Consider simplifying this by directly using ImmutableMap.copyOf(hostConfig) if not empty, or just skip adding to the final map if hostConfig is empty. java/src/org/openqa/selenium/docker/ContainerConfig.java [206-209] if (this.hostConfig != null && !this.hostConfig.isEmpty()) { - Map<String, Object> copyMap = new HashMap<>(hostConfig); - copyMap.putAll(this.hostConfig); - hostConfig = ImmutableMap.copyOf(copyMap); + hostConfig = ImmutableMap.copyOf(this.hostConfig); }
	Ensure hostConfigKeys are utilized in createBrowserContainer. When creating a new ContainerConfig instance in createBrowserContainer, it's important to ensure that hostConfig and hostConfigKeys are properly utilized. Currently, the method signature was changed to include hostConfig, but the implementation does not show how hostConfigKeys is used. Ensure that hostConfigKeys are utilized to filter or modify the hostConfig before passing it to ContainerConfig. java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [297] -.getHostConfig(hostConfig, hostConfigKeys); +.getHostConfig(filterHostConfig(hostConfig, hostConfigKeys), hostConfigKeys);

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[VietND96]

@diemol, could you please review this?

[diemol]

Thank you, @VietND96!

Just a comment in one of the changes.

[diemol]

Thank you, @VietND96!