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

K8s: Node Relay to extend autoscaling Grid with test cloud resources #2703

Merged
merged 5 commits into from
Mar 18, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Mar 11, 2025

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

To extend your Grid, you can use Relay Node (which allows you to route Grid tests to another Grid, another network, or a cloud vendor).
Besides on-prem browser Nodes with Linux-based, you also can serve test requests with other platforms, browsers or even mobile devices which provided by cloud vendors.
Your teams will not worry about the underlying infrastructure, they just request to the single Grid endpoint hosted in your organization.
Check out values file [multiple-nodes-platform-relay.yaml] in chart for more details.

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


Description

  • Introduced Relay Node for hybrid autoscaling with cloud providers.

    • Added support for test cloud credentials and relay configurations.
    • Updated Helm charts to include relay-specific configurations.
  • Enhanced Kubernetes test strategies with relay node testing.

  • Updated environment variables and configurations for relay node support.

  • Adjusted platform and browser configurations for compatibility.


Changes walkthrough 📝

Relevant files
Tests
1 files
chart_test.sh
Added relay node testing and secret creation.                       
+18/-2   
Enhancement
5 files
__init__.py
Added relay-specific environment variables and configurations.
+9/-1     
Dockerfile
Introduced `SE_NODE_RELAY_ONLY` environment variable.       
+1/-0     
generate_config
Adjusted stereotype generation for relay nodes.                   
+5/-2     
generate_relay_config
Enhanced relay configuration with dynamic URL and stereotype merging.
+13/-3   
generate_config
Updated stereotype generation for standalone relay nodes.
+4/-1     
Configuration changes
10 files
_helpers.tpl
Added relay node browser name configuration.                         
+4/-0     
helm-chart-test.yml
Added relay node test strategy in CI workflow.                     
+9/-0     
Makefile
Added relay node autoscaling job target.                                 
+9/-1     
multiple-nodes-platform-relay.yaml
Added relay node configurations for hybrid autoscaling.   
+82/-0   
multiple-nodes-platform.yaml
Updated platform names for compatibility.                               
+3/-3     
relay-node-scaledjobs.yaml
Adjusted relay node autoscaling template.                               
+1/-1     
values.yaml
Updated relay node default values.                                             
+2/-2     
DeploymentAutoscaling-values.yaml
Added relay node environment variable configuration.         
+6/-0     
JobAutoscaling-values.yaml
Added relay node environment variable configuration.         
+5/-0     
base-resources-values.yaml
Added resource limits for relay nodes.                                     
+9/-0     
Documentation
1 files
CONFIGURATION.md
Updated relay node configuration documentation.                   
+2/-2     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The PR adds code that creates a Kubernetes secret containing cloud provider credentials (SAUCE_USERNAME, SAUCE_ACCESS_KEY) in the chart_test.sh file. While these are stored as Kubernetes secrets, the values are passed as environment variables and could potentially be exposed in logs or error messages. Additionally, the SE_NODE_RELAY_URL contains these credentials directly in the URL, which is a security risk if the URL is logged or exposed.

    ⚡ Recommended focus areas for review

    Logic Error

    The conditional logic for setting SE_NODE_STEREOTYPE has been changed, but the 'else' statement is now misplaced. This could cause unexpected behavior where SE_NODE_STEREOTYPE is not properly set in some scenarios.

    if [[ -z "${SE_NODE_STEREOTYPE}" ]] && [[ -n "${SE_NODE_BROWSER_NAME}" ]] && ([[ -z "${SE_NODE_RELAY_URL}" ]] || [[ "${SE_NODE_RELAY_ONLY}" = "false" ]]) ; then
    	SE_NODE_STEREOTYPE="{\"browserName\": \"${SE_NODE_BROWSER_NAME}\", \"browserVersion\": \"${SE_NODE_BROWSER_VERSION}\", \"platformName\": \"${SE_NODE_PLATFORM_NAME}\", \"se:containerName\": \"${SE_NODE_CONTAINER_NAME}\", \"container:hostname\": \"$(hostname)\"}"
    	if [[ -n "${SE_BROWSER_BINARY_LOCATION}" ]]; then
    	    SE_NODE_STEREOTYPE="$(python3 /opt/bin/json_merge.py "${SE_NODE_STEREOTYPE}" "${SE_BROWSER_BINARY_LOCATION}")"
    	fi
    else
    	SE_NODE_STEREOTYPE="${SE_NODE_STEREOTYPE}"
    fi
    Variable Reference

    The uploader variable reference was changed from '.Values.videoRecorder.uploader.name' to '$podScope.recorder.uploader.name', which might cause issues if the recorder structure doesn't contain the expected uploader name property.

    {{- $_ =  set $podScope "uploader" (get $.Values.videoRecorder ($podScope.recorder.uploader.name | toString)) -}}
    Environment Variable Expansion

    The implementation of environment variable substitution in the relay URL might not work as expected if the environment variables contain special characters or if the envsubst command is not available in the container.

    echo "url = \"$(envsubst < <(echo ${SE_NODE_RELAY_URL}))\"" >>"$FILENAME"

    Copy link

    qodo-merge-pro bot commented Mar 11, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix process substitution syntax

    The command using envsubst with process substitution has a syntax error. The
    current implementation pipes the output of echo into envsubst using process
    substitution, but this is not the correct way to use envsubst. This could cause
    the relay URL to be incorrectly processed.

    NodeBase/generate_relay_config [11]

     if [[ -n "${SE_NODE_RELAY_URL}" ]]; then
     	echo "[relay]" >>"$FILENAME"
    -	echo "url = \"$(envsubst < <(echo ${SE_NODE_RELAY_URL}))\"" >>"$FILENAME"
    +	echo "url = \"$(echo ${SE_NODE_RELAY_URL} | envsubst)\"" >>"$FILENAME"

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The current process substitution syntax is incorrect and could lead to the relay URL being improperly processed. The suggested fix uses a proper pipe to envsubst, which is the standard way to handle variable substitution in shell scripts.

    Medium
    • Update

    Copy link

    qodo-merge-pro bot commented Mar 11, 2025

    CI Feedback 🧐

    (Feedback updated until commit ab6cee1)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub token provided in GH_CLI_TOKEN_PR is missing the required scope
    'read:org'. When attempting to authenticate with GitHub CLI using this token, the authentication
    failed with the error "missing required scope 'read:org'".

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    28:  SecurityEvents: write
    29:  Statuses: write
    30:  ##[endgroup]
    31:  Secret source: Actions
    32:  Prepare workflow directory
    33:  Prepare all required actions
    34:  Getting action download info
    35:  Download action repository 'actions/checkout@main' (SHA:85e6279cec87321a52edac9c87bce653a07cf6c2)
    36:  Complete job name: Rerun workflow when failure
    ...
    
    48:  show-progress: true
    49:  lfs: false
    50:  submodules: false
    51:  set-safe-directory: true
    52:  env:
    53:  GH_CLI_TOKEN: ***
    54:  GH_CLI_TOKEN_PR: ***
    55:  RUN_ID: 13892337741
    56:  RERUN_FAILED_ONLY: true
    ...
    
    119:  ##[group]Run sudo apt update
    120:  �[36;1msudo apt update�[0m
    121:  �[36;1msudo apt install gh�[0m
    122:  shell: /usr/bin/bash -e {0}
    123:  env:
    124:  GH_CLI_TOKEN: ***
    125:  GH_CLI_TOKEN_PR: ***
    126:  RUN_ID: 13892337741
    127:  RERUN_FAILED_ONLY: true
    ...
    
    181:  0 upgraded, 0 newly installed, 0 to remove and 50 not upgraded.
    182:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    183:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    184:  shell: /usr/bin/bash -e {0}
    185:  env:
    186:  GH_CLI_TOKEN: ***
    187:  GH_CLI_TOKEN_PR: ***
    188:  RUN_ID: 13892337741
    189:  RERUN_FAILED_ONLY: true
    190:  RUN_ATTEMPT: 1
    191:  ##[endgroup]
    192:  error validating token: missing required scope 'read:org'
    193:  ##[error]Process completed with exit code 1.
    

    @VietND96 VietND96 force-pushed the node-relay-cloud branch 9 times, most recently from d1f8ff9 to 4b3ac15 Compare March 16, 2025 20:42
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 force-pushed the node-relay-cloud branch 3 times, most recently from cc3e866 to ed24971 Compare March 17, 2025 21:06
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 merged commit d574906 into trunk Mar 18, 2025
    25 of 28 checks passed
    @VietND96 VietND96 deleted the node-relay-cloud branch March 18, 2025 08:20
    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