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(update): update env vars and health check scripts #2334

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jul 30, 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

Motivation and Context

Update in bash scripts is used for probe checks in K8s deployment. Separate reusable script.
Pre-steps to add graceful shutdown for Node container when deploying by docker, docker-compose

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, Configuration changes


Description

  • Updated environment variable names for router username and password across multiple scripts.
  • Refactored distributor, node, and router probe scripts to use external scripts for constructing URLs.
  • Added new scripts for constructing and validating Grid and GraphQL URLs.
  • Added SE_SERVER_PROTOCOL environment variable with default value "http" in Dockerfiles.
  • Updated default screen resolution environment variables and documentation.
  • Added router configuration and script mounts to distributor and hub deployments.
  • Added deployment and service configurations for session queue.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
start-selenium-grid-hub.sh
Update environment variable names for router credentials 

Hub/start-selenium-grid-hub.sh

  • Updated environment variable names for router username and password.
  • +6/-6     
    start-selenium-grid-router.sh
    Update environment variable names for router credentials 

    Router/start-selenium-grid-router.sh

  • Updated environment variable names for router username and password.
  • +6/-6     
    start-selenium-standalone.sh
    Update environment variable names for router credentials 

    Standalone/start-selenium-standalone.sh

  • Updated environment variable names for router username and password.
  • +6/-6     
    distributorProbe.sh
    Refactor distributor probe script to use external GraphQL URL script

    charts/selenium-grid/configs/distributor/distributorProbe.sh

  • Removed inline construction of GraphQL URL.
  • Added usage of external script to get GraphQL URL.
  • +9/-24   
    nodeGridUrl.sh
    Add script for constructing and validating Grid URL           

    charts/selenium-grid/configs/node/nodeGridUrl.sh

    • Added new script to construct and validate Grid URL.
    +33/-0   
    nodePreStop.sh
    Refactor node preStop script to use external Grid URL script

    charts/selenium-grid/configs/node/nodePreStop.sh

  • Refactored to use external script for Grid URL.
  • Removed functions for distributed mode and grid URL construction.
  • +7/-47   
    nodeProbe.sh
    Refactor node probe script to use external Grid URL script

    charts/selenium-grid/configs/node/nodeProbe.sh

  • Refactored to use external script for Grid URL.
  • Removed inline construction of Grid URL.
  • +2/-26   
    routerGraphQLUrl.sh
    Add script for constructing and validating GraphQL URL     

    charts/selenium-grid/configs/router/routerGraphQLUrl.sh

    • Added new script to construct and validate GraphQL URL.
    +25/-0   
    routerProbe.sh
    Refactor router probe script to use external GraphQL URL script

    charts/selenium-grid/configs/router/routerProbe.sh

  • Removed inline construction of GraphQL URL.
  • Added usage of external script to get GraphQL URL.
  • +6/-21   
    Configuration changes
    12 files
    Dockerfile
    Add SE_SERVER_PROTOCOL environment variable                           

    Base/Dockerfile

  • Added SE_SERVER_PROTOCOL environment variable with default value
    "http".
  • +1/-0     
    Dockerfile
    Update default screen resolution environment variables     

    NodeBase/Dockerfile

    • Updated default screen resolution environment variables.
    +2/-2     
    Dockerfile
    Add SE_SERVER_PROTOCOL and update screen resolution           

    Video/Dockerfile

  • Added SE_SERVER_PROTOCOL environment variable with default value
    "http".
  • Updated default screen resolution environment variables.
  • +3/-2     
    distributor-configmap.yaml
    Remove SE_GRID_GRAPHQL_URL from distributor ConfigMap       

    charts/selenium-grid/templates/distributor-configmap.yaml

    • Removed SE_GRID_GRAPHQL_URL from ConfigMap.
    +0/-1     
    distributor-deployment.yaml
    Add router configuration and scripts to distributor deployment

    charts/selenium-grid/templates/distributor-deployment.yaml

  • Added router configuration to distributor deployment.
  • Mounted router config scripts.
  • +16/-0   
    hub-deployment.yaml
    Add router configuration and scripts to hub deployment     

    charts/selenium-grid/templates/hub-deployment.yaml

  • Added router configuration to hub deployment.
  • Mounted router config scripts.
  • +12/-0   
    node-configmap.yaml
    Add NODE_CONFIG_DIRECTORY to node ConfigMap                           

    charts/selenium-grid/templates/node-configmap.yaml

    • Added NODE_CONFIG_DIRECTORY environment variable.
    +1/-1     
    router-configmap.yaml
    Add ROUTER_CONFIG_DIRECTORY to router ConfigMap                   

    charts/selenium-grid/templates/router-configmap.yaml

    • Added ROUTER_CONFIG_DIRECTORY environment variable.
    +1/-0     
    secrets.yaml
    Update environment variable names for router credentials in secrets

    charts/selenium-grid/templates/secrets.yaml

    • Updated environment variable names for router credentials.
    +2/-2     
    session-queue-deployment.yaml
    Add deployment configuration for session queue                     

    charts/selenium-grid/templates/session-queue-deployment.yaml

    • Added deployment configuration for session queue.
    +98/-1   
    session-queue-service.yaml
    Add service configuration for session queue                           

    charts/selenium-grid/templates/session-queue-service.yaml

    • Added service configuration for session queue.
    +30/-1   
    values.yaml
    Add new script entries for router and node configurations

    charts/selenium-grid/values.yaml

    • Added new script entries for router and node configurations.
    +2/-0     
    Documentation
    1 files
    README.md
    Update documentation for default screen resolution             

    README.md

    • Updated documentation for default screen resolution.
    +1/-1     

    💡 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
    🔒 Security concerns

    Sensitive Information Exposure:
    The PR introduces environment variables for router username and password, which are sensitive. Ensure these credentials are handled securely throughout the system, especially in logs and during error handling.

    ⚡ Key issues to review

    Sensitive Information Exposure
    The PR introduces environment variables SE_ROUTER_USERNAME and SE_ROUTER_PASSWORD which are base64 encoded. Ensure these are securely handled and not logged or exposed in any debug outputs or logs.

    Hardcoded Path
    The script uses a hardcoded path /opt/selenium for ROUTER_CONFIG_DIRECTORY. Consider making this configurable or clearly document its necessity and usage.

    Error Handling
    The script exits or returns status codes like 401 or 404 without any error message or logging. This might make debugging issues in production environments difficult.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Ensure new script files in config maps are secured

    To avoid potential security risks, ensure that the new script files added to the
    routerConfigMap are properly secured and only contain necessary executable
    permissions.

    charts/selenium-grid/templates/distributor-deployment.yaml [94-96]

     - name: {{ tpl (default (include "seleniumGrid.router.configmap.fullname" $) $.Values.routerConfigMap.scriptVolumeMountName) $ | quote }}
       mountPath: {{ $.Values.routerConfigMap.extraScriptsDirectory }}/{{ $fileName }}
       subPath: {{ $fileName }}
    +  readOnly: true
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Ensuring that script files are read-only is a critical security measure to prevent unauthorized modifications, which could lead to security vulnerabilities.

    10
    Validate and secure the default mode of new config maps

    Validate the new routerConfigMap default mode settings to ensure they are consistent
    with security best practices, especially if they contain sensitive data or scripts.

    charts/selenium-grid/templates/distributor-deployment.yaml [187-188]

     - name: {{ tpl (default (include "seleniumGrid.router.configmap.fullname" $) $.Values.routerConfigMap.scriptVolumeMountName) $ | quote }}
       configMap:
         name: {{ template "seleniumGrid.router.configmap.fullname" $ }}
    -    defaultMode: {{ $.Values.routerConfigMap.defaultMode }}
    +    defaultMode: {{ .Values.security.configMapDefaultMode | default 0444 }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Validating and securing the default mode of config maps is essential to ensure that sensitive data or scripts are not exposed or misconfigured, aligning with security best practices.

    10
    Possible bug
    Improve the check for a non-empty grid_url to prevent potential errors

    Use a more robust method for checking if grid_url is non-empty to prevent executing
    curl on an empty URL.

    charts/selenium-grid/configs/node/nodePreStop.sh [32]

    -if [ -n "${grid_url}" ]; then
    +if [[ -n "${grid_url}" && "${grid_url}" != "http://" && "${grid_url}" != "https://" ]]; then
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug by ensuring that the grid_url is not only non-empty but also valid, preventing erroneous curl executions. This significantly improves the script's reliability.

    9
    Possible issue
    Add error handling for router config map references

    Consider adding error handling or validation for the new routerConfigMap entries to
    ensure that the configuration does not break deployments if the values are not set
    properly.

    charts/selenium-grid/templates/distributor-deployment.yaml [75]

     - configMapRef:
    -    name: {{ template "seleniumGrid.router.configmap.fullname" . }}
    +    name: {{ required "router.configmap.fullname is required" (template "seleniumGrid.router.configmap.fullname" .) }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the router config map references ensures that deployments do not fail silently due to misconfigurations, which is important for robustness and reliability.

    9
    Maintainability
    Add documentation for new environment variables in the Helm chart

    Ensure that the new environment variable SE_ROUTER_HOST and SE_ROUTER_PORT are
    properly documented in the Helm chart's values file and README to avoid confusion
    for new users or when updating the chart.

    charts/selenium-grid/templates/distributor-deployment.yaml [49-52]

    +# Please add documentation in the values.yaml and README.md
     - name: SE_ROUTER_HOST
       value: '{{ template "seleniumGrid.router.fullname" . }}.{{ .Release.Namespace }}'
     - name: SE_ROUTER_PORT
       value: {{ .Values.components.router.port | quote }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding documentation for new environment variables is crucial for maintainability and helps users understand and correctly use the new settings. This suggestion addresses a significant aspect of usability.

    8
    Replace direct exit commands with a logging function to improve debuggability

    Replace the direct exit command with a function that logs the exit reason and then
    exits. This will improve debugging and maintainability.

    charts/selenium-grid/configs/distributor/distributorProbe.sh [13]

    -exit 0
    +log_and_exit "Exiting due to missing GraphQL URL" 0
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion improves maintainability by providing a clear log message before exiting, which aids in debugging. However, it is not crucial for the functionality of the script.

    7
    Use a variable for repeated date commands to improve code clarity

    Consolidate the repeated use of date ${ts_format} into a variable to reduce
    redundancy and improve code clarity.

    charts/selenium-grid/configs/router/routerProbe.sh [12]

    -echo "$(date ${ts_format}) DEBUG [${probe_name}] - Could not construct GraphQL endpoint, please provide SE_HUB_HOST (or SE_ROUTER_HOST) and SE_HUB_PORT (or SE_ROUTER_PORT). Bypass the probe checks for now."
    +current_time=$(date ${ts_format})
    +echo "${current_time} DEBUG [${probe_name}] - Could not construct GraphQL endpoint, please provide SE_HUB_HOST (or SE_ROUTER_HOST) and SE_HUB_PORT (or SE_ROUTER_PORT). Bypass the probe checks for now."
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Consolidating repeated date commands into a variable reduces redundancy and improves code clarity. However, this is a minor improvement and does not affect the script's core functionality.

    6
    Error handling
    Add error handling for curl commands to handle network errors

    Add error handling for the curl command to ensure that the script can handle network
    errors gracefully.

    charts/selenium-grid/configs/distributor/distributorProbe.sh [16]

    -GRAPHQL_PRE_CHECK=$(curl --noproxy "*" -m ${max_time} -k -X POST -H "Content-Type: application/json" --data '{"query":"{ grid { sessionCount } }"}' -s -o /dev/null -w "%{http_code}" ${GRID_GRAPHQL_URL})
    +GRAPHQL_PRE_CHECK=$(curl --noproxy "*" -m ${max_time} -k -X POST -H "Content-Type: application/json" --data '{"query":"{ grid { sessionCount } }"}' -s -o /dev/null -w "%{http_code}" ${GRID_GRAPHQL_URL} || echo "curl_error")
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the curl command is important for robustness, ensuring the script can handle network errors gracefully. This enhances the reliability of the script.

    8

    @VietND96 VietND96 merged commit 1f519c6 into SeleniumHQ:trunk Jul 30, 2024
    16 checks passed
    @VietND96 VietND96 added this to the 4.23.1 milestone Aug 9, 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