Skip to content
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

chart: proactive to set browser args via container env var #2308

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jul 17, 2024

User description

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

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

Description

Change in Docker image

SE_BROWSER_ARGS_* Add arguments for launching browser

Instead of adding arguments via the browser options from language bindings, for example:

options = ChromeOptions()
options.add_argument('--incognito')
options.add_argument('--disable-dev-shm-usage')
driver = webdriver.Remote(options=options, command_executor="http://localhost:4444/wd/hub")

You also can proactive to force applying arguments directly from (node, standalone or node-docker) container environment variables. Define the environment variable with name starts with SE_BROWSER_ARGS_ and following by config key is up to you (ensure those are unique when you define multiple arguments). For example:

docker run -d -p 4444:4444 \
  -e SE_BROWSER_ARGS_INCOGNITO=--incognito \
  -e SE_BROWSER_ARGS_DISABLE_DSHM=--disable-dev-shm-usage \
  selenium/standalone-chrome:latest

List chromium command-line arguments for your reference.

Note: Currently, this is applicable for node browsers Chrome/Chromium, Edge.

Change in Helm chart

Configuration of shm size limit for browser nodes

By default, node browsers (Chrome/Chromium, Edge) leave the config key dshmVolumeSizeLimit as empty. It means the /dev/shm volume mount is disabled, and argument --disable-dev-shm-usage is passed to the browser via container environment variable (get motivation from this post). You can set another valid value to enable it back. For example:

chromeNode:
    dshmVolumeSizeLimit: "2Gi"
edgeNode:
    dshmVolumeSizeLimit: "2Gi"

For Firefox node, the default value is kept as 2Gi. You can override it via firefoxNode.dshmVolumeSizeLimit.

Motivation and Context

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.

PR Type

Enhancement, Documentation


Description

  • Added support for setting browser arguments via environment variables prefixed with SE_BROWSER_ARGS_ for Chrome, Chromium, and Edge.
  • Updated Helm chart templates to conditionally configure dshm volume mounts and set SE_BROWSER_ARGS_DISABLE_DSHM if dshmVolumeSizeLimit is empty.
  • Enhanced documentation to include instructions for using SE_BROWSER_ARGS_* environment variables and configuring dshmVolumeSizeLimit.
  • Updated default memory limits for browser nodes in values.yaml.

Changes walkthrough 📝

Relevant files
Enhancement
_helpers.tpl
Conditional Configuration for Browser Arguments and Volume Mounts

charts/selenium-grid/templates/_helpers.tpl

  • Added conditional logic to set SE_BROWSER_ARGS_DISABLE_DSHM
    environment variable.
  • Modified volume mounts to conditionally include /dev/shm.
  • Updated size limit configuration for dshm volume.
  • +11/-1   
    wrap_chrome_binary
    Capture and Apply Browser Arguments from Environment Variables

    NodeChrome/wrap_chrome_binary

  • Added logic to capture and apply environment variables starting with
    SE_BROWSER_ARGS_.
  • Appended captured arguments to the Chrome binary execution command.
  • +10/-1   
    wrap_chromium_binary
    Capture and Apply Browser Arguments from Environment Variables

    NodeChromium/wrap_chromium_binary

  • Added logic to capture and apply environment variables starting with
    SE_BROWSER_ARGS_.
  • Appended captured arguments to the Chromium binary execution command.
  • +25/-1   
    wrap_edge_binary
    Capture and Apply Browser Arguments from Environment Variables

    NodeEdge/wrap_edge_binary

  • Added logic to capture and apply environment variables starting with
    SE_BROWSER_ARGS_.
  • Appended captured arguments to the Edge binary execution command.
  • +10/-1   
    values.yaml
    Update Default Memory Limits and shm Size Configuration   

    charts/selenium-grid/values.yaml

  • Updated default memory limits for Chrome, Firefox, and Edge nodes.
  • Modified dshmVolumeSizeLimit to be empty by default for Chrome and
    Edge nodes.
  • +9/-9     
    Documentation
    README.md
    Document Browser Argument Environment Variables                   

    README.md

  • Added documentation for SE_BROWSER_ARGS_* environment variables.
  • Provided examples and references for browser arguments.
  • +25/-0   
    README.md
    Document Configuration of shm Size Limit for Browser Nodes

    charts/selenium-grid/README.md

  • Added documentation for configuring dshmVolumeSizeLimit for browser
    nodes.
  • Provided examples for setting dshmVolumeSizeLimit.
  • +16/-3   

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Conditional Logic
    The conditional logic for setting the SE_BROWSER_ARGS_DISABLE_DSHM environment variable based on .node.dshmVolumeSizeLimit being empty might not be intuitive. Consider adding comments or documentation in the Helm chart to explain why this condition leads to the disabling of shared memory usage, which is a critical aspect for browser stability in container environments.

    Environment Variable Parsing
    The script captures and parses environment variables prefixed with SE_BROWSER_ARGS_ to construct browser arguments. Ensure that this approach is secure and robust against potential issues such as environment variable injection or improper handling of special characters in arguments.

    Script Robustness
    Similar to the Chrome wrapper, this script also processes SE_BROWSER_ARGS_ prefixed environment variables. It's crucial to validate the robustness and security of this parsing logic, especially in a production environment where incorrect handling can lead to command execution vulnerabilities.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure correct handling of environment variables containing spaces

    To avoid potential issues with spaces in the values of environment variables, wrap
    the value variable in quotes when appending it to SE_BROWSER_ARGS. This ensures that
    all arguments are correctly passed to the executable even if they contain spaces.

    NodeChrome/wrap_chrome_binary [30]

    -SE_BROWSER_ARGS="$SE_BROWSER_ARGS $value"
    +SE_BROWSER_ARGS="$SE_BROWSER_ARGS '$value'"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Wrapping the value variable in quotes ensures that environment variables containing spaces are correctly handled, preventing potential issues when passing arguments to the executable.

    9
    Enhancement
    Improve the condition check for setting the SE_BROWSER_ARGS_DISABLE_DSHM environment variable

    To ensure that the environment variable SE_BROWSER_ARGS_DISABLE_DSHM is only set
    when dshmVolumeSizeLimit is explicitly empty, consider using a more explicit check
    for an empty string rather than using the empty function. This avoids potential
    issues where dshmVolumeSizeLimit might be set to a value that is considered empty by
    Helm but is not actually an empty string.

    charts/selenium-grid/templates/_helpers.tpl [253-255]

    -{{- if empty .node.dshmVolumeSizeLimit }}
    +{{- if eq .node.dshmVolumeSizeLimit "" }}
       - name: SE_BROWSER_ARGS_DISABLE_DSHM
         value: "--disable-dev-shm-usage"
     {{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves the robustness of the condition check by explicitly checking for an empty string, which avoids potential issues where dshmVolumeSizeLimit might be set to a value considered empty by Helm but is not actually an empty string.

    8
    Maintainability
    Add error handling to the mapfile command to enhance script robustness

    To improve the robustness of the script, add error handling for the mapfile command
    to ensure that the script continues to function correctly even if the grep command
    fails to find any matches.

    NodeChromium/wrap_chromium_binary [25]

    -mapfile -t BROWSER_ARGS_ARRAY < <(printenv | grep ^SE_BROWSER_ARGS_)
    +mapfile -t BROWSER_ARGS_ARRAY < <(printenv | grep ^SE_BROWSER_ARGS_ || echo "")
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding error handling for the mapfile command ensures that the script continues to function correctly even if the grep command fails to find any matches, improving the script's robustness.

    7

    @VietND96 VietND96 merged commit 9bd30b0 into SeleniumHQ:trunk Jul 17, 2024
    28 checks passed
    @VietND96 VietND96 added this to the 4.23 milestone Jul 17, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant