Skip to content

Enable passing securityContext to initContainers in the Helm chart #2607

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

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

calendir
Copy link
Contributor

@calendir calendir commented Jan 21, 2025

User description

Description

A simple change adding the missing securityContext fields to the initContainers so that they can be set for those along with the regular containers.

Motivation and Context

When security policies are enforced in a cluster that the default configuration does not comply with the pre-puller-* pods will not be allowed to start, even though securityContext is set.

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

Bug fix, Enhancement


Description

  • Added securityContext support for initContainers in Helm chart.

  • Ensured compatibility with enforced cluster security policies.

  • Updated pre-puller-* pods to include securityContext.


Changes walkthrough 📝

Relevant files
Enhancement
_helpers.tpl
Add `securityContext` to `initContainers` and containers 

charts/selenium-grid/templates/_helpers.tpl

  • Added securityContext field for node containers.
  • Added securityContext field for recorder containers.
  • Ensured securityContext is configurable for initContainers.
  • +6/-0     

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

    CLAassistant commented Jan 21, 2025

    CLA assistant check
    All committers have signed the CLA.

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Command Validation

    The command ["bash", "-c", "'true'"] has unnecessary single quotes around 'true' which may cause the command to fail. Should be ["bash", "-c", "true"] instead.

    command: ["bash", "-c", "'true'"]

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix command string literal format

    Remove the single quotes around 'true' in the command field. In Kubernetes/Helm, the
    command should be a plain string without quotes, as the single quotes will be
    interpreted literally.

    charts/selenium-grid/templates/_helpers.tpl [318]

    -command: ["bash", "-c", "'true'"]
    +command: ["bash", "-c", "true"]
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The single quotes around 'true' in the command field would be interpreted literally by Kubernetes, potentially causing command execution issues. This is a significant fix that affects container initialization functionality.

    8

    @VietND96 VietND96 merged commit dd1731d into SeleniumHQ:trunk Jan 21, 2025
    25 of 26 checks passed
    @VietND96
    Copy link
    Member

    Thank you, @calendir. I merged to include it in next chart deployment.

    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.

    3 participants