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

Enforce HTTP/1.1 for internal component JdkHttpClient #2521

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Dec 19, 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

When Grid deployed behind ingress with SSL termination, where HTTP/2 involve, in debug logs can see something like

08:18:35.315 DEBUG [HttpTracing.inject] - Injecting (POST) /se/grid/newsessionqueue/session/next into OpenTelemetryContext{tracer=io.opentelemetry.sdk.trace.SdkTracer@2accdbb5, context={}, span id=0000000000000000, trace id=00000000000000000000000000000000} at org.openqa.selenium.grid.sessionqueue.remote.RemoteNewSessionQueue:156
08:18:35.315 DEBUG [JdkHttpClient.execute0] - Executing request: (POST) /se/grid/newsessionqueue/session/next
08:18:35.315 DEBUG [HttpClientImpl.sendAsync] - [JdkHttpClient-1-0] [461s 188ms] HttpClientImpl(2) ClientImpl (async) send http://selenium-session-queue.selenium:5559/se/grid/newsessionqueue/session/next POST
08:18:35.315 DEBUG [Exchange.establishExchange] - [JdkHttpClient-1-0] [461s 188ms] Exchange establishing exchange for http://selenium-session-queue.selenium:5559/se/grid/newsessionqueue/session/next POST,
proxy=null
08:18:35.315 DEBUG [Http2ClientImpl.getConnectionFor] - [JdkHttpClient-1-0] [461s 188ms] Http2ClientImpl not found in connection pool

With the support of SeleniumHQ/selenium#14306 - system prop webdriver.httpclient.version can use to set HTTP_1_1 while components establish connection internally using ClientConfig.

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


Description

  • Added support for enforcing HTTP/1.1 for internal component JdkHttpClient through new environment variable SE_JAVA_HTTPCLIENT_VERSION
  • Implemented HTTP client version configuration across all Selenium Grid components
  • Enhanced security by:
    • Adding specific version of lettuce-core to address CVEs
    • Adding extra security updates for Firefox dependencies
  • Added pull request trigger for FFmpeg Dockerfile changes in CI workflow

Changes walkthrough 📝

Relevant files
Configuration changes
Dockerfile
Add HTTP client version config and update dependencies     

Base/Dockerfile

  • Added SE_JAVA_HTTPCLIENT_VERSION environment variable set to
    "HTTP_1_1"
  • Added lettuce-core dependency with specific version for CVE fixes
  • +3/-0     
    build-ffmpeg.yml
    Add PR trigger for FFmpeg workflow                                             

    .github/workflows/build-ffmpeg.yml

    • Added pull request trigger for .ffmpeg/Dockerfile changes
    +3/-0     
    Enhancement
    Dockerfile
    Add security updates for Firefox dependencies                       

    NodeFirefox/Dockerfile

  • Added additional apt-get upgrade step to fix potential CVEs from
    Firefox dependencies
  • +3/-0     
    start-selenium-grid-*.sh
    Add HTTP client version configuration to grid components 

    /start-selenium-grid-.sh

  • Added support for SE_JAVA_HTTPCLIENT_VERSION environment variable in
    all grid component scripts
  • Added Java system property setting for HTTP client version

  • 💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @VietND96 VietND96 merged commit 8e8f074 into trunk Dec 19, 2024
    47 of 52 checks passed
    @VietND96 VietND96 deleted the enforce-http_1_1 branch December 19, 2024 06:33
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Instability
    The additional apt-get upgrade step after Firefox installation could potentially introduce incompatibilities with Firefox dependencies. Consider pinning specific package versions instead.

    Version Constraint
    The hardcoded lettuce-core version (6.5.1.RELEASE) should be defined as a variable to make future updates easier and consistent across the codebase.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Optimize package management commands to ensure system stability and reduce image size

    The apt-get upgrade command should be combined with the previous apt-get update to
    avoid potential package inconsistencies and reduce layer size.

    NodeFirefox/Dockerfile [60-63]

     && /opt/bin/get_lang_package.sh \
    -# Do one more upgrade to fix possible CVEs from Firefox dependencies
    +# Upgrade packages to fix possible CVEs from Firefox dependencies
     && apt-get update -qqy \
    -&& apt-get upgrade -yq \
    +&& apt-get upgrade -yq --no-install-recommends \
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to combine apt-get commands and add --no-install-recommends flag is a good practice that would reduce the Docker image size and ensure package consistency. This is a meaningful optimization for container security and efficiency.

    7

    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