Skip to content

K8s: Disable annotation when ingress.tls[0].secretName is null #2724

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
Mar 21, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Mar 21, 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

Fixes #2720

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


Description

  • Added conditional logic to disable proxy-ssl-secret annotation when ingress.tls[0].secretName is null.

  • Updated values.yaml to set useHttp2 default value to false.

  • Modified test configurations to include a secretName for TLS in the dummy solution.


Changes walkthrough 📝

Relevant files
Enhancement
_helpers.tpl
Refined logic for `proxy-ssl-secret` annotation inclusion

charts/selenium-grid/templates/_helpers.tpl

  • Adjusted conditional logic for proxy-ssl-secret annotation.
  • Ensured annotation is only added if ingress.tls[0].secretName is
    defined.
  • +2/-2     
    Configuration changes
    values.yaml
    Updated default settings for `useHttp2` and comments         

    charts/selenium-grid/values.yaml

  • Changed default value of useHttp2 to false.
  • Updated comments for better clarity on sslSecret usage.
  • +1/-1     
    Tests
    dummy_solution.yaml
    Updated test configurations for TLS `secretName`                 

    tests/charts/templates/render/dummy_solution.yaml

  • Added secretName to TLS configuration for testing.
  • Ensured test cases reflect new annotation logic.
  • +1/-0     

    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:

    🎫 Ticket compliance analysis ✅

    2720 - PR Code Verified

    Compliant requirements:

    • Add option to disable the nginx.ingress.kubernetes.io/proxy-ssl-secret annotation in selenium chart

    Requires further human verification:

    • Verify that the implementation works correctly when the TLS secret is created by cert manager

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

    String Formatting Error

    The printf statement is missing the %s/%s format which could lead to incorrect path generation for the secret. The namespace and secret name need to be properly concatenated.

    nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}
    Logic Change

    The PR changes the behavior from always adding the annotation to conditionally adding it. This might affect existing deployments that rely on the annotation being present.

        {{- else if (index $.Values.ingress.tls 0).secretName }}
    nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}

    Copy link

    qodo-merge-pro bot commented Mar 21, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix format string parameters

    The format string in the printf statement is missing format specifiers for the
    namespace and secretName values. This will result in only the namespace being
    included in the output, without the secretName.

    charts/selenium-grid/templates/_helpers.tpl [138-139]

     {{- else if (index $.Values.ingress.tls 0).secretName }}
    -nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}
    +nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s/%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    __

    Why: This suggestion fixes a critical bug in the format string that would cause the secretName to be omitted from the proxy-ssl-secret annotation. The original code only has one "%s" placeholder despite passing two values (namespace and secretName), which would result in incorrect configuration and potential security issues.

    High
    • Update

    Copy link

    qodo-merge-pro bot commented Mar 21, 2025

    CI Feedback 🧐

    (Feedback updated until commit 7ed1d75)

    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 to the workflow lacks the required read:org
    permission scope. The error occurred during the authentication step when trying to log in with the
    GitHub CLI using the token stored in the GH_CLI_TOKEN_PR environment variable. The specific error
    message was: "error validating token: missing required scope 'read:org'".

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    22:  Issues: write
    23:  Metadata: read
    24:  Models: read
    25:  Packages: write
    26:  Pages: write
    27:  PullRequests: write
    28:  RepositoryProjects: write
    29:  SecurityEvents: write
    30:  Statuses: write
    31:  ##[endgroup]
    32:  Secret source: Actions
    33:  Prepare workflow directory
    34:  Prepare all required actions
    35:  Getting action download info
    36:  Download action repository 'actions/checkout@main' (SHA:85e6279cec87321a52edac9c87bce653a07cf6c2)
    37:  Complete job name: Rerun workflow when failure
    38:  ##[group]Run actions/checkout@main
    ...
    
    42:  ssh-strict: true
    43:  ssh-user: git
    44:  persist-credentials: true
    45:  clean: true
    46:  sparse-checkout-cone-mode: true
    47:  fetch-depth: 1
    48:  fetch-tags: false
    49:  show-progress: true
    50:  lfs: false
    51:  submodules: false
    52:  set-safe-directory: true
    53:  env:
    54:  GH_CLI_TOKEN: ***
    55:  GH_CLI_TOKEN_PR: ***
    56:  RUN_ID: 13986152384
    57:  RERUN_FAILED_ONLY: true
    58:  RUN_ATTEMPT: 1
    ...
    
    113:  Or undo this operation with:
    114:  git switch -
    115:  Turn off this advice by setting config variable advice.detachedHead to false
    116:  HEAD is now at 7c25440 Merge 7ed1d75447dcc347fec0b9c8cd34e17ae62cf266 into 3a61a7c5ae5f1bb0d9d4373ab5bdd841567186cc
    117:  ##[endgroup]
    118:  [command]/usr/bin/git log -1 --format=%H
    119:  7c25440159dbd3bab65942f863249c8cbc4f7be7
    120:  ##[group]Run sudo apt update
    121:  �[36;1msudo apt update�[0m
    122:  �[36;1msudo apt install gh�[0m
    123:  shell: /usr/bin/bash -e {0}
    124:  env:
    125:  GH_CLI_TOKEN: ***
    126:  GH_CLI_TOKEN_PR: ***
    127:  RUN_ID: 13986152384
    128:  RERUN_FAILED_ONLY: true
    129:  RUN_ATTEMPT: 1
    ...
    
    170:  Reading state information...
    171:  54 packages can be upgraded. Run 'apt list --upgradable' to see them.
    172:  WARNING: apt does not have a stable CLI interface. Use with caution in scripts.
    173:  Reading package lists...
    174:  Building dependency tree...
    175:  Reading state information...
    176:  gh is already the newest version (2.68.1).
    177:  0 upgraded, 0 newly installed, 0 to remove and 54 not upgraded.
    178:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    179:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    180:  shell: /usr/bin/bash -e {0}
    181:  env:
    182:  GH_CLI_TOKEN: ***
    183:  GH_CLI_TOKEN_PR: ***
    184:  RUN_ID: 13986152384
    185:  RERUN_FAILED_ONLY: true
    186:  RUN_ATTEMPT: 1
    187:  ##[endgroup]
    188:  error validating token: missing required scope 'read:org'
    189:  ##[error]Process completed with exit code 1.
    190:  Post job cleanup.
    

    @VietND96 VietND96 force-pushed the chart-tls-annotation branch from f615929 to 7ed1d75 Compare March 21, 2025 06:42
    @VietND96 VietND96 merged commit da16f65 into trunk Mar 21, 2025
    26 of 28 checks passed
    @VietND96 VietND96 deleted the chart-tls-annotation branch March 21, 2025 09:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    1 participant