[jaeger] Re-add anonymous Elasticsearch access support#743
[jaeger] Re-add anonymous Elasticsearch access support#743domolitom wants to merge 3 commits intojaegertracing:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Re-introduces support for anonymous (no basic auth) access when running Elasticsearch-related jobs in the Jaeger v2 Helm chart, addressing a regression that broke OpenSearch/AWS deployments that rely on network-level access instead of credentials.
Changes:
- Adds a new
storage.elasticsearch.anonymousvalue (defaultfalse) to control whether ES credentials env vars are rendered. - Updates the
elasticsearch.envhelper to skipES_USERNAME/ES_PASSWORDwhenanonymousis enabled. - Bumps the chart version from
4.5.0to4.5.1.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
charts/jaeger/values.yaml |
Introduces the new storage.elasticsearch.anonymous configuration value. |
charts/jaeger/templates/_helpers.tpl |
Implements the conditional rendering of ES credential env vars based on anonymous. |
charts/jaeger/Chart.yaml |
Bumps chart version to ship the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9ff9110 to
3b216b0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The `storage.elasticsearch.anonymous` option was added in PR jaegertracing#442 (chart v0.67.1) but was lost when the chart was rewritten for Jaeger v2. The elasticsearch.env helper unconditionally injects ES_USERNAME and ES_PASSWORD with defaults (elastic/changeme), which breaks AWS OpenSearch deployments that use VPC-level access without basic auth. AWS rejects the basic auth Authorization header with a 403 because it expects SigV4 format. Setting `storage.elasticsearch.anonymous: true` now skips injecting ES_USERNAME and ES_PASSWORD, restoring the behavior from jaegertracing#442. Signed-off-by: Oliver Domokos <domolitom@gmail.com>
3b216b0 to
f5bb751
Compare
|
I do not think this is the correct solution. I also ran into this issue when I had to force elasticsearch to be anonymous and I was going to fix it. I think a better solution is to remove the defaults from the helper. Then set the defaults as examples in the values.yaml and then you can override them to be "" as expected. The helper would just check if userName then set and separately if password then set. There is no need to introduce a third variable. There is already examples in the values.yaml here https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/values.yaml#L271 Something like This was untested, but it should work |
Remove the `anonymous` flag and hardcoded default credentials from the elasticsearch.env helper. ES_USERNAME and ES_PASSWORD are now only injected when `storage.elasticsearch.user` and `storage.elasticsearch.password` are explicitly set. Omitting them gives anonymous access. Signed-off-by: Oliver Domokos <domolitom@gmail.com>
|
Thanks for the suggestion! Implemented:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- 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 }} | ||
| {{- end }} | ||
| {{ $url := $es.url | default "http://elasticsearch-master:9200" -}} |
There was a problem hiding this comment.
The PR description says it re-adds storage.elasticsearch.anonymous so credential env vars are omitted only when that flag is true, but the template no longer references an anonymous value at all and instead changes behavior to omit credentials whenever user/password are unset. This doesn’t implement the documented option and can be a breaking change for users who relied on the previous default credential injection. Consider reintroducing an explicit $es.anonymous guard (and keeping the prior defaults when not anonymous), or update the PR scope/docs to reflect the new behavior.
There was a problem hiding this comment.
To solve this you can set the credentials to the default in the values.yaml instead. This will prevent and backwards compat issues... Though I highly doubt anyone uses the current default lol
| {{- if $es.user }} | ||
| - name: ES_USERNAME | ||
| value: {{ $es.user | quote }} | ||
| {{- end }} | ||
| {{- if $es.password }} | ||
| - name: ES_PASSWORD | ||
| value: {{ $es.password | quote }} |
There was a problem hiding this comment.
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.
| {{- 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" }} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Agreed — keeping them independent makes sense since ES supports users without passwords. No changes needed here.
| - name: ES_PASSWORD | ||
| value: {{ $es.password | quote }} | ||
| {{- end }} | ||
| {{ $url := $es.url | default "http://elasticsearch-master:9200" -}} |
There was a problem hiding this comment.
Whitespace trimming is inconsistent for the $url assignment ({{ $url := ... -}}) compared to the surrounding helper which consistently uses {{- ... -}}. Aligning this to {{- $url := ... -}} avoids emitting stray whitespace lines in the rendered YAML and matches the established style in this file.
| {{ $url := $es.url | default "http://elasticsearch-master:9200" -}} | |
| {{- $url := $es.url | default "http://elasticsearch-master:9200" -}} |
There was a problem hiding this comment.
@domolitom can you run a few tests that outputs the yaml with combinations of certain values set and not set in this section to ensure the resulting yaml whitespace is correct?
There was a problem hiding this comment.
Thanks for catching the whitespace issue! I ran helm template with 7 combinations of storage.elasticsearch.* values against the es-index-cleaner-cronjob.yaml template to verify correctness.
I also fixed the whitespace trimming on the $url assignment line — the original {{ $url := ... -}} (no left trim) produced a stray blank line, but adding both trims {{- ... -}} collapsed lines when credentials were present. The fix uses {{- ... }} (left trim only, keep right newline). See commit 4ac06de.
Test results (env: section from rendered es-index-cleaner-cronjob.yaml):
Test 1: Both user + password
env:
- name: ES_USERNAME
value: "elastic"
- name: ES_PASSWORD
value: "changeme"
- name: ES_SERVER_URLS
value: "http://elasticsearch-master:9200"
- name: ES_NODES
value: "http://elasticsearch-master:9200"Test 2: Only user (no password)
env:
- name: ES_USERNAME
value: "elastic"
- name: ES_SERVER_URLS
value: "http://elasticsearch-master:9200"
- name: ES_NODES
value: "http://elasticsearch-master:9200"Test 3: Only password (no user)
env:
- name: ES_PASSWORD
value: "changeme"
- name: ES_SERVER_URLS
value: "http://elasticsearch-master:9200"
- name: ES_NODES
value: "http://elasticsearch-master:9200"Test 4: Neither set (anonymous access)
env:
- name: ES_SERVER_URLS
value: "http://elasticsearch-master:9200"
- name: ES_NODES
value: "http://elasticsearch-master:9200"Test 5: Custom URL + credentials
env:
- name: ES_USERNAME
value: "elastic"
- name: ES_PASSWORD
value: "changeme"
- name: ES_SERVER_URLS
value: "https://my-es:9200"
- name: ES_NODES
value: "https://my-es:9200"Test 6: TLS insecure + credentials
env:
- name: ES_USERNAME
value: "elastic"
- name: ES_PASSWORD
value: "changeme"
- name: ES_SERVER_URLS
value: "http://elasticsearch-master:9200"
- name: ES_NODES
value: "http://elasticsearch-master:9200"
- name: ES_TLS_SKIP_HOST_VERIFY
value: "true"Test 7: Anonymous + TLS insecure
env:
- name: ES_SERVER_URLS
value: "http://elasticsearch-master:9200"
- name: ES_NODES
value: "http://elasticsearch-master:9200"
- name: ES_TLS_SKIP_HOST_VERIFY
value: "true"All 7 pass helm lint ✅
Align whitespace trimming on the $url variable assignment to prevent stray blank lines when credentials are omitted and avoid collapsing lines when credentials are present. Signed-off-by: Oliver Domokos <domolitom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: jaeger | ||
| type: application | ||
| version: 4.5.0 | ||
| version: 4.5.1 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@domolitom can you please add this to ensure the backwards compatibility is fine
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: jaeger | ||
| type: application | ||
| version: 4.5.0 | ||
| version: 4.5.1 |
What this PR does
Re-adds the
storage.elasticsearch.anonymousoption that was introduced in PR #442 (chart v0.67.1) but lost during the Jaeger v2 chart rewrite.When
storage.elasticsearch.anonymous: true, theelasticsearch.envhelper skips injectingES_USERNAMEandES_PASSWORDenv vars into ES maintenance jobs (esIndexCleaner, esRollover, esLookback).Which issue this PR fixes
Checklist
[jaeger])