You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
Below is a summary of compliance checks for this PR:
Security Compliance
⚪
TLS verification disabled
Description:ingress.traefik.serversTransport.insecureSkipVerify defaults to true, which can disable backend TLS certificate verification and enables realistic MITM risk between Traefik and the Grid backend unless users explicitly set it to false. values.yaml [232-247]
Referred Code
serversTransport:
# -- Enable creating a Traefik ServersTransport resource and auto-link it to ingress annotation `traefik.ingress.kubernetes.io/service.serverstransport`enabled: true# -- Override ServersTransport resource name. Defaults to `<ingress-fullname>-serverstransport`nameOverride: ""# -- Use an existing ServersTransport reference `<namespace>-<name>@kubernetescrd` when `enabled` is falsereference: ""# -- For backend HTTPS with self-signed certsinsecureSkipVerify: trueforwardingTimeouts:
# -- Maximum duration Traefik waits when establishing a connection to backend serversdialTimeout: "120s"# -- Maximum duration Traefik waits for backend response headersresponseHeaderTimeout: "3600s"# -- Maximum duration an idle keep-alive backend connection remains openidleConnTimeout: "3600s"
Replace the deprecated ingress-nginx Helm chart dependency with an alternative ingress controller in the Selenium Grid Helm chart.
Update Helm chart configuration/values and documentation accordingly so consumers can use the new default ingress controller setup.
⚪
Verify in a real Kubernetes cluster that Traefik installs and functions correctly as a drop-in default (ingress routing, TLS behavior, timeouts) across the documented scenarios.
Codebase Duplication Compliance
⚪
Codebase context is not defined
Follow the guide to enable codebase context checks.
Custom Compliance
🟢
Generic: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance.
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities
Status: Insecure TLS default: The new default ingress.traefik.serversTransport.insecureSkipVerify is set to true, which weakens TLS verification for backend connections by default.
Referred Code
serversTransport:
# -- Enable creating a Traefik ServersTransport resource and auto-link it to ingress annotation `traefik.ingress.kubernetes.io/service.serverstransport`enabled: true# -- Override ServersTransport resource name. Defaults to `<ingress-fullname>-serverstransport`nameOverride: ""# -- Use an existing ServersTransport reference `<namespace>-<name>@kubernetescrd` when `enabled` is falsereference: ""# -- For backend HTTPS with self-signed certsinsecureSkipVerify: trueforwardingTimeouts:
# -- Maximum duration Traefik waits when establishing a connection to backend serversdialTimeout: "120s"# -- Maximum duration Traefik waits for backend response headersresponseHeaderTimeout: "3600s"# -- Maximum duration an idle keep-alive backend connection remains openidleConnTimeout: "3600s"
Consider the impact of sub-path handling simplification
The PR simplifies sub-path handling by replacing NGINX's regex-based path rewriting with Traefik's PathPrefix matcher. This is a behavioral change that should be documented as it may affect users with advanced path configurations.
ingress:
enabled: trueannotations:
traefik.ingress.kubernetes.io/router.pathmatcher: "PathPrefix"className: traefikhostname: "aws.ndviet.org"# Replace with your hostnamepaths:
- path: /seleniumpathType: Prefixbackend:
... (clipped 4 lines)
Solution Walkthrough:
Before:
# Ingress configuration with NGINXingress:
annotations:
nginx.ingress.kubernetes.io/use-regex: "true"nginx.ingress.kubernetes.io/rewrite-target: /$2paths:
- path: /selenium(/|$)(.*)pathType: ImplementationSpecific...# Backend (hub/router) configurationhub:
subPath: "/selenium"# Used for URL generation, not for listening path
After:
# Ingress configuration with Traefikingress:
traefik:
pathMatcher: "PathPrefix"paths:
- path: /seleniumpathType: Prefix...# Backend (hub/router) configurationhub:
subPath: "/selenium"# Backend now listens on this sub-path
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a significant behavioral change in ingress path handling, which could impact users with custom configurations, and rightly advises documenting it.
Medium
Possible issue
Avoid hardcoding namespace in test
In the test test_ingress_traefik_servers_transport, avoid hardcoding the namespace as "default" by dynamically extracting it from the rendered ServersTransport resource to make the test more robust.
def test_ingress_traefik_servers_transport(self):
ingress_name = f'{RELEASE_NAME}selenium-ingress'
servers_transport_name = f'{ingress_name}-serverstransport'
- namespace = "default"+ namespace = None
ingress_found = False
servers_transport_found = False
++ # First, find the ServersTransport and get its namespace+ for doc in LIST_OF_DOCUMENTS:+ if doc['kind'] == 'ServersTransport' and doc['metadata']['name'] == servers_transport_name:+ namespace = doc['metadata']['namespace']+ break++ self.assertIsNotNone(namespace, "Could not determine namespace from ServersTransport resource")+
for doc in LIST_OF_DOCUMENTS:
if doc['kind'] == 'Ingress' and doc['metadata']['name'] == ingress_name:
logger.info(f"Assert ingress has Traefik ServersTransport reference annotation")
expected_ref = f'{namespace}-{servers_transport_name}@kubernetescrd'
self.assertEqual(
doc['metadata']['annotations']['traefik.ingress.kubernetes.io/service.serverstransport'],
expected_ref,
)
ingress_found = True
if doc['kind'] == 'ServersTransport' and doc['metadata']['name'] == servers_transport_name:
logger.info(f"Assert Traefik ServersTransport forwarding timeouts are set")
self.assertFalse(doc['spec']['insecureSkipVerify'])
forwarding_timeouts = doc['spec']['forwardingTimeouts']
self.assertEqual(forwarding_timeouts['dialTimeout'], '30s')
self.assertEqual(forwarding_timeouts['responseHeaderTimeout'], '360s')
self.assertEqual(forwarding_timeouts['idleConnTimeout'], '360s')
servers_transport_found = True
self.assertTrue(ingress_found, "No ingress resource found with ServersTransport annotation")
self.assertTrue(servers_transport_found, "No Traefik ServersTransport resource found")
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a hardcoded namespace in a test, which makes the test brittle. The proposed fix to dynamically determine the namespace makes the test more robust and reliable.
Medium
Apply extra settings for ServersTransport
In traefik-servers-transport.yaml, merge the extraSettings value into the $spec dictionary to allow users to configure additional ServersTransport options.
{{- $spec := dict -}}
{{- $timeouts := dict -}}
{{- with .Values.ingress.traefik.serversTransport }}
{{- if kindIs "bool" .insecureSkipVerify }}
{{- $_ := set $spec "insecureSkipVerify" .insecureSkipVerify -}}
{{- end }}
{{- end }}
{{- with .Values.ingress.traefik.serversTransport.forwardingTimeouts }}
{{- with .dialTimeout }}
{{- $_ := set $timeouts "dialTimeout" (tpl (. | toString) $) -}}
{{- end }}
{{- with .responseHeaderTimeout }}
{{- $_ := set $timeouts "responseHeaderTimeout" (tpl (. | toString) $) -}}
{{- end }}
{{- with .idleConnTimeout }}
{{- $_ := set $timeouts "idleConnTimeout" (tpl (. | toString) $) -}}
{{- end }}
{{- end }}
{{- if not (empty $timeouts) }}
{{- $_ := set $spec "forwardingTimeouts" $timeouts -}}
{{- end }}
+{{- $spec = mergeOverwrite .Values.ingress.traefik.serversTransport.extraSettings $spec -}}
{{- if empty $spec }}
spec: {}
{{- else }}
spec:
{{- toYaml $spec | nindent 2 }}
{{- end }}
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that the extraSettings value is not used in the template, and the proposed fix enables greater customization of the ServersTransport resource, improving the chart's flexibility.
Low
Use boolean ingress.enabled check
In traefik-servers-transport.yaml, modify the conditional to check the boolean .Values.ingress.enabled directly instead of using (eq (include "seleniumGrid.ingress.enabled" $) "true").
-{{- if and (eq (include "seleniumGrid.ingress.enabled" $) "true") .Values.ingress.traefik.enabled .Values.ingress.traefik.serversTransport.enabled }}+{{- if and .Values.ingress.enabled .Values.ingress.traefik.enabled .Values.ingress.traefik.serversTransport.enabled }}
Apply / Chat
Suggestion importance[1-10]: 5
__
Why: The suggestion correctly points out that checking a boolean value directly is better practice than comparing the string output of a helper template, improving code clarity and robustness.
Low
Learned best practice
Guard null nested config lookups
Users can set ingress.traefik: null (as documented), so guard access to .Values.ingress.traefik.* using a with block (or similar) to prevent template rendering errors.
-{{- if and (eq (include "seleniumGrid.ingress.enabled" $) "true") .Values.ingress.traefik.enabled .Values.ingress.traefik.serversTransport.enabled }}+{{- if eq (include "seleniumGrid.ingress.enabled" $) "true" }}+{{- with .Values.ingress.traefik }}+{{- if and .enabled .serversTransport.enabled }}
apiVersion: traefik.io/v1alpha1
kind: ServersTransport
metadata:
- name: {{ include "seleniumGrid.ingress.traefik.serversTransport.name" . }}- namespace: {{ .Release.Namespace }}+ name: {{ include "seleniumGrid.ingress.traefik.serversTransport.name" $ }}+ namespace: {{ $.Release.Namespace }}
labels:
{{- include "seleniumGrid.commonLabels" $ | nindent 4 }}
{{- $spec := dict -}}
{{- $timeouts := dict -}}
-{{- with .Values.ingress.traefik.serversTransport }}+{{- with .serversTransport }}
{{- if kindIs "bool" .insecureSkipVerify }}
{{- $_ := set $spec "insecureSkipVerify" .insecureSkipVerify -}}
{{- end }}
{{- end }}
-{{- with .Values.ingress.traefik.serversTransport.forwardingTimeouts }}+{{- with .serversTransport.forwardingTimeouts }}
{{- with .dialTimeout }}
{{- $_ := set $timeouts "dialTimeout" (tpl (. | toString) $) -}}
{{- end }}
{{- with .responseHeaderTimeout }}
{{- $_ := set $timeouts "responseHeaderTimeout" (tpl (. | toString) $) -}}
{{- end }}
{{- with .idleConnTimeout }}
{{- $_ := set $timeouts "idleConnTimeout" (tpl (. | toString) $) -}}
{{- end }}
{{- end }}
{{- if not (empty $timeouts) }}
{{- $_ := set $spec "forwardingTimeouts" $timeouts -}}
{{- end }}
{{- if empty $spec }}
spec: {}
{{- else }}
spec:
{{- toYaml $spec | nindent 2 }}
{{- end }}
{{- end }}
+{{- end }}+{{- end }}
Apply / Chat
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Make Helm templates resilient to optional/null values by guarding nested lookups (avoid dereferencing fields on a possibly null map).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #3076
Motivation and Context
Types of changes
Checklist
PR Type
Enhancement
Description
Replace Ingress NGINX with Traefik as default ingress controller
Update Helm chart dependencies and configuration values
Refactor ingress annotations from NGINX to Traefik format
Create Traefik ServersTransport resource for backend connectivity
Update test cases and documentation for Traefik integration
Diagram Walkthrough
File Walkthrough
11 files
Update ingress namespace from nginx to traefikUpdate ingress namespace and helm parameters for traefikReplace nginx config with traefik config in valuesUpdate ingress class and traefik configurationSimplify subpath configuration for traefikUpdate chart repository to traefik helm chartsMigrate aws sample values to traefik configurationMigrate docker desktop sample to traefik setupMigrate minikube sample values to traefikUpdate dummy test values for traefik configurationUpdate solution template values for traefik1 files
Refactor ingress annotation tests for traefik3 files
Update documentation for traefik ingress configurationUpdate ingress configuration guide for traefikUpdate test documentation for traefik annotations1 files
Replace ingress-nginx dependency with traefik4 files
Replace nginx annotation helpers with traefik helpersAdd traefik serverstransport naming helpersUpdate ingress template for traefik annotationsCreate new traefik serverstransport resource template