Skip to content

[jaeger] Re-add anonymous Elasticsearch access support #743

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.

Sign up for GitHub

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

Open
domolitom wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[jaeger] Re-add anonymous Elasticsearch access support #743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5bb751
[jaeger] Re-add anonymous Elasticsearch access support
domolitom Feb 16, 2026
e9f4f39
[jaeger] Simplify anonymous ES access by removing defaults from helper
domolitom Mar 11, 2026
4ac06de
[jaeger] Fix whitespace trimming in elasticsearch.env helper
domolitom Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/jaeger/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ appVersion: 2.15.1
description: A Jaeger Helm chart for Kubernetes
name: jaeger
type: application
version: 4.5.0
version: 4.5.1
Comment thread
domolitom marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@Stevenpc3 Stevenpc3 Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set a user value and password value in the values.yaml to eliminate this concern. Simply uncomment the values here for user and password.

elasticsearch:
    tls:
      enabled: false
      secretName: ""
      mountPath: ""
      subPath: ""
    url: http://elasticsearch-master:9200
    user: elastic                                         <----------------------  update here
    password: changeme                          <----------------------  update here

This will ensure the same default values. If anyone wants anonymous they can comment them again or set them to "" in an override

Copy link
Copy Markdown
Contributor

@Stevenpc3 Stevenpc3 Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domolitom can you please add this to ensure the backwards compatibility is fine

# Artifact Hub annotations
# The jaeger image is whitelisted from security scanning because the reported
# CVEs are in the upstream Alpine base image (OpenSSL libcrypto3/libssl3) and
Expand Down
16 changes: 9 additions & 7 deletions charts/jaeger/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,17 +182,19 @@ Elasticsearch related environment variables
{{- define "elasticsearch.env" -}}
{{- if eq .Values.storage.type "elasticsearch" -}}
{{- $es := .Values.storage.elasticsearch | default dict -}}
{{- $user := $es.user | default "elastic" -}}
{{- $password := $es.password | default "changeme" -}}
{{- $url := $es.url | default "http://elasticsearch-master:9200" -}}
{{- if $es.user }}
- name: ES_USERNAME
value: {{ $es.user | quote }}
{{- end }}
{{- if $es.password }}
- name: ES_PASSWORD
value: {{ $es.password | quote }}
Comment on lines +185 to +191
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES_USERNAME and ES_PASSWORD are rendered independently. If a user sets only one of storage.elasticsearch.user or storage.elasticsearch.password, the job will get a partial basic-auth configuration and likely fail. It’s safer to render both together (or neither), and/or fail template rendering with a clear message when only one is set.

Suggested change
{{- if $es.user }}
- name: ES_USERNAME
value: {{ $es.user | quote }}
{{- end }}
{{- if $es.password }}
- name: ES_PASSWORD
value: {{ $es.password | quote }}
{{- if and $es.user $es.password }}
- name: ES_USERNAME
value: {{ $es.user | quote }}
- name: ES_PASSWORD
value: {{ $es.password | quote }}
{{- else if or $es.user $es.password }}
{{- fail "both storage.elasticsearch.user and storage.elasticsearch.password must be set or both omitted" }}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@Stevenpc3 Stevenpc3 Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, this might be true only because of how Jaeger creates basic auth, but I think it's beneficial to keep separate as you can create an anonymous user in elastcsearch that takes no password, but does require a user name

Copy link
Copy Markdown
Author

@domolitom domolitom Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — keeping them independent makes sense since ES supports users without passwords. No changes needed here.

{{- end }}
{{- $url := $es.url | default "http://elasticsearch-master:9200" }}
- name: ES_SERVER_URLS
value: {{ $url | quote }}
- name: ES_NODES
value: {{ $url | quote }}
- name: ES_USERNAME
value: {{ $user | quote }}
- name: ES_PASSWORD
value: {{ $password | quote }}
{{- /* Handle TLS insecurity */ -}}
{{- if and (($es).tls).enabled (($es).tls).insecure }}
- name: ES_TLS_SKIP_HOST_VERIFY
Expand Down
Loading