[RORDEV-1069] reproducer#94
Conversation
This comment was marked as spam.
This comment was marked as spam.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ror-demo-cluster/conf/kbn/enterprise-ror-newplatform-kibana.yml`:
- Around line 15-16: The hardcoded secret in
xpack.encryptedSavedObjects.encryptionKey must be replaced with a non-committed
secret reference: change the static value to read from an environment variable
(e.g. KIBANA_ENCRYPTION_KEY) or a secrets store, update any deployment/start
scripts to set that env var, and remove the literal key from the file and repo
history if possible; ensure the config now references the env var and add
instructions or a .env/secrets sample (excluded from VCS) so developers can
provide their own keys securely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b26c772-918c-4897-b70e-3dbbade09e8c
⛔ Files ignored due to path filters (1)
ror-demo-cluster/readonlyrest-1.69.0-pre7_es8.19.0.zipis excluded by!**/*.zip
📒 Files selected for processing (1)
ror-demo-cluster/conf/kbn/enterprise-ror-newplatform-kibana.yml
| xpack.encryptedSavedObjects: | ||
| encryptionKey: "min-32-byte-long-strong-encryption-key" |
There was a problem hiding this comment.
Hardcoded encryption key detected.
The static analysis tool flagged this as a generic API key. Even for a demo/reproducer environment, committing secrets to version control is risky—this value could inadvertently be reused or copied into production configurations.
Consider using an environment variable or a secrets file that's excluded from version control:
xpack.encryptedSavedObjects:
encryptionKey: ${KIBANA_ENCRYPTION_KEY}🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ror-demo-cluster/conf/kbn/enterprise-ror-newplatform-kibana.yml` around lines
15 - 16, The hardcoded secret in xpack.encryptedSavedObjects.encryptionKey must
be replaced with a non-committed secret reference: change the static value to
read from an environment variable (e.g. KIBANA_ENCRYPTION_KEY) or a secrets
store, update any deployment/start scripts to set that env var, and remove the
literal key from the file and repo history if possible; ensure the config now
references the env var and add instructions or a .env/secrets sample (excluded
from VCS) so developers can provide their own keys securely.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
shared/init-scripts/utils/lib.sh (1)
16-29: Usereturnhere instead ofexit.
shared/init-scripts/init.shsources this file on Line 5, soexit 2/3/4aborts the parent setup script instead of letting callers handle the failure. Returning the status keeps these helpers composable and consistent with the other validation paths.Suggested change
if ! [ -v ELASTICSEARCH_ADDRESS ] || [ -z "$ELASTICSEARCH_ADDRESS" ]; then echo "ERROR: required variable ELASTICSEARCH_ADDRESS not set or empty" - exit 2 + return 2 fiApply the same change to the user/password checks here and in
createDataStream.Also applies to: 55-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/init-scripts/utils/lib.sh` around lines 16 - 29, The validation helpers currently call exit (e.g., checks for ELASTICSEARCH_ADDRESS, ELASTICSEARCH_USER, ELASTICSEARCH_PASSWORD) which will terminate the entire sourcing script; change those exit 2/3/4 calls to return with the same status so callers can handle failures; update the same pattern for the other validation block (lines around the second set of user/password checks) and inside the createDataStream function so all helper checks return instead of exiting.shared/init-scripts/init.sh (1)
7-13: Prefer a flag over checking the seed commands in as comments.Lines 7-13 leave this script as a no-op after
source utils/lib.shon Line 5, so fresh runs no longer seed the sample indices/data streams and the new helper paths are never exercised. If the reproducer needs seeding disabled, gate it behind an env switch instead of permanent comments.Suggested guard
+if [ "${SEED_SAMPLE_DATA:-1}" = "1" ]; then + createDataStream "frontend_logs_ds" && generate_log_documents 100 | putDocument "frontend_logs_ds" + createDataStream "business_logs_ds" && generate_log_documents 50 | putDocument "business_logs_ds" + createDataStream "system_logs_ds" && generate_log_documents 60 | putDocument "system_logs_ds" + + createIndex "frontend_logs_index" && generate_log_documents 100 | putDocument "frontend_logs_index" + createIndex "business_logs_index" && generate_log_documents 50 | putDocument "business_logs_index" + createIndex "system_logs_index" && generate_log_documents 60 | putDocument "system_logs_index" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/init-scripts/init.sh` around lines 7 - 13, Restore the commented seeding commands (createDataStream, createIndex, generate_log_documents, putDocument) and wrap them behind an environment flag check (e.g., SEED_SAMPLE_DATA or ENABLE_SEEDING) so seeding runs by default but can be disabled via env; place the guard right after sourcing utils/lib.sh and conditionally execute the createDataStream/createIndex and associated generate_log_documents | putDocument sequences only when the flag is truthy, leaving the commands commented out only when the flag explicitly disables seeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ror-demo-cluster/conf/es/readonlyrest.yml`:
- Around line 33-41: The read-only tenant config ("Test users" with groups
"TestUsers" and kibana access "ro_strict") currently only hides "Security" and
"Observability" which leaves "Management" exposed; update the kibana hide_apps
for read-only profiles (e.g., the "Test users" block and any other ro/ro_strict
entries) to include "Management" so read-only users cannot access
Index/Management pages until those pages are supported by the permission/index
scope.
- Around line 21-25: The role's indices list currently allows only
"frontend_logs*" and "kibana_sample_data_*", which causes queries against the
Data Streams pattern "heartbeat-*" to hit IDX_NOT_FOUND and crash Kibana; update
the indices entry in the readonlyrest.yml role to include "heartbeat-*" (or
alternatively disable/hide the Index Management/Data Streams feature for this
role) so EndUsers can access Data Streams; locate the indices:
["frontend_logs*", "kibana_sample_data_*"] line and add "heartbeat-*" (or
implement feature-hiding for the kibana: index .kibana_end_@{user} role)
accordingly.
In `@ror-demo-cluster/docker-compose.yml`:
- Around line 35-43: kbn-ror currently only waits on es-ror, but because
ELASTICSEARCH_HOSTS now points at es-ror-proxy you must gate kbn-ror on proxy
readiness; add a healthcheck to es-ror-proxy (or reuse an existing one) that
verifies the proxy is forwarding (e.g., a simple HTTP request to the reverse
target) and update kbn-ror's startup dependency to depend_on: es-ror-proxy with
condition: service_healthy (or implement an explicit wait-for startup step in
kbn-ror that polls es-ror-proxy); reference the es-ror-proxy service, the
kbn-ror service and ELASTICSEARCH_HOSTS when making the change.
In `@shared/init-scripts/utils/lib.sh`:
- Around line 33-35: The curl invocations are expanding ELASTICSEARCH_USER and
ELASTICSEARCH_PASSWORD unquoted (seen in the PUT call and similar calls in
createDataStream and putSingleDocument), which breaks when passwords contain
spaces or glob chars; update the -u argument to quote the credentials as a
single shell word (e.g., wrap the combined user:password value in double quotes)
for every curl call that uses -u $ELASTICSEARCH_USER:$ELASTICSEARCH_PASSWORD so
the shell does not split or glob the values.
- Around line 116-122: The stdin ingestion loop and the single-document branch
do not check the exit status of putSingleDocument, allowing later successes to
mask earlier failures; update both the direct call (when "$#" -eq 2) and the
while-read loop to capture the exit code from putSingleDocument (called with
"$INDEX_NAME" and the document content) and if it is non-zero immediately
break/return/exit with that code so the function returns a failure on the first
write error instead of continuing to consume input.
---
Nitpick comments:
In `@shared/init-scripts/init.sh`:
- Around line 7-13: Restore the commented seeding commands (createDataStream,
createIndex, generate_log_documents, putDocument) and wrap them behind an
environment flag check (e.g., SEED_SAMPLE_DATA or ENABLE_SEEDING) so seeding
runs by default but can be disabled via env; place the guard right after
sourcing utils/lib.sh and conditionally execute the createDataStream/createIndex
and associated generate_log_documents | putDocument sequences only when the flag
is truthy, leaving the commands commented out only when the flag explicitly
disables seeding.
In `@shared/init-scripts/utils/lib.sh`:
- Around line 16-29: The validation helpers currently call exit (e.g., checks
for ELASTICSEARCH_ADDRESS, ELASTICSEARCH_USER, ELASTICSEARCH_PASSWORD) which
will terminate the entire sourcing script; change those exit 2/3/4 calls to
return with the same status so callers can handle failures; update the same
pattern for the other validation block (lines around the second set of
user/password checks) and inside the createDataStream function so all helper
checks return instead of exiting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1e1cc3a-7cdd-4a7e-a045-6cb49d3d990e
⛔ Files ignored due to path filters (2)
ror-demo-cluster/readonlyrest-1.69.0-pre7_es8.19.0.zipis excluded by!**/*.zipror-demo-cluster/readonlyrest_kbn_universal-1.69.0-pre6_es8.19.0.zipis excluded by!**/*.zip
📒 Files selected for processing (4)
ror-demo-cluster/conf/es/readonlyrest.ymlror-demo-cluster/docker-compose.ymlshared/init-scripts/init.shshared/init-scripts/utils/lib.sh
| indices: ["frontend_logs*", "kibana_sample_data_*"] | ||
| kibana: | ||
| index: .kibana_end_@{user} | ||
| access: rw | ||
| hide_apps: ["Security", "Observability"] |
There was a problem hiding this comment.
End users still miss the heartbeat-* index pattern used by Data Streams requests.
Line 21 excludes heartbeat-*, but your repro shows EndUsers querying that pattern and hitting IDX_NOT_FOUND, which then surfaces as Kibana 500 in Index Management. If End users should use Data Streams, this needs to be allowed (or the feature hidden for this role).
Suggested config adjustment
- indices: ["frontend_logs*", "kibana_sample_data_*"]
+ indices: ["frontend_logs*", "heartbeat-*", "kibana_sample_data_*"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| indices: ["frontend_logs*", "kibana_sample_data_*"] | |
| kibana: | |
| index: .kibana_end_@{user} | |
| access: rw | |
| hide_apps: ["Security", "Observability"] | |
| indices: ["frontend_logs*", "heartbeat-*", "kibana_sample_data_*"] | |
| kibana: | |
| index: .kibana_end_@{user} | |
| access: rw | |
| hide_apps: ["Security", "Observability"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ror-demo-cluster/conf/es/readonlyrest.yml` around lines 21 - 25, The role's
indices list currently allows only "frontend_logs*" and "kibana_sample_data_*",
which causes queries against the Data Streams pattern "heartbeat-*" to hit
IDX_NOT_FOUND and crash Kibana; update the indices entry in the readonlyrest.yml
role to include "heartbeat-*" (or alternatively disable/hide the Index
Management/Data Streams feature for this role) so EndUsers can access Data
Streams; locate the indices: ["frontend_logs*", "kibana_sample_data_*"] line and
add "heartbeat-*" (or implement feature-hiding for the kibana: index
.kibana_end_@{user} role) accordingly.
| hide_apps: ["Security", "Observability"] | ||
|
|
||
| - name: "Test users" | ||
| groups: ["TestUsers"] | ||
| indices: ["business_logs*", "kibana_sample_data_*"] | ||
| kibana: | ||
| index: .kibana_test_@{user} | ||
| access: ro_strict | ||
| hide_apps: ["Security", "Observability"] |
There was a problem hiding this comment.
Management is still exposed for read-only tenants, which can lead to broken flows.
Line 33 and Line 41 hide Security/Observability only. Read-only users can still navigate to Index Management and hit failing endpoints. Consider hiding Management for ro/ro_strict profiles until permissions/index scope fully support those pages.
Suggested config adjustment
- hide_apps: ["Security", "Observability"]
+ hide_apps: ["Security", "Observability", "Management"]
...
- hide_apps: ["Security", "Observability"]
+ hide_apps: ["Security", "Observability", "Management"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hide_apps: ["Security", "Observability"] | |
| - name: "Test users" | |
| groups: ["TestUsers"] | |
| indices: ["business_logs*", "kibana_sample_data_*"] | |
| kibana: | |
| index: .kibana_test_@{user} | |
| access: ro_strict | |
| hide_apps: ["Security", "Observability"] | |
| hide_apps: ["Security", "Observability", "Management"] | |
| - name: "Test users" | |
| groups: ["TestUsers"] | |
| indices: ["business_logs*", "kibana_sample_data_*"] | |
| kibana: | |
| index: .kibana_test_@{user} | |
| access: ro_strict | |
| hide_apps: ["Security", "Observability", "Management"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ror-demo-cluster/conf/es/readonlyrest.yml` around lines 33 - 41, The
read-only tenant config ("Test users" with groups "TestUsers" and kibana access
"ro_strict") currently only hides "Security" and "Observability" which leaves
"Management" exposed; update the kibana hide_apps for read-only profiles (e.g.,
the "Test users" block and any other ro/ro_strict entries) to include
"Management" so read-only users cannot access Index/Management pages until those
pages are supported by the permission/index scope.
| es-ror-proxy: | ||
| image: mitmproxy/mitmproxy | ||
| command: | ||
| mitmdump --set validate_inbound_headers=false --mode reverse:https://es-ror:9200 --ssl-insecure --verbose | ||
| depends_on: | ||
| - es-ror | ||
| networks: | ||
| - es-ror-network | ||
|
|
There was a problem hiding this comment.
Add readiness gating for es-ror-proxy now that Kibana depends on it.
After Line 59 switched ELASTICSEARCH_HOSTS to es-ror-proxy, kbn-ror should also wait for proxy readiness. Right now it only waits for es-ror, which can produce startup/transient 5xx when proxy isn’t ready yet.
Suggested compose patch
es-ror-proxy:
image: mitmproxy/mitmproxy
command:
mitmdump --set validate_inbound_headers=false --mode reverse:https://es-ror:9200 --ssl-insecure --verbose
depends_on:
- es-ror
+ healthcheck:
+ test: ["CMD-SHELL", "curl -fksS --connect-timeout 3 --max-time 5 --retry 2 --retry-connrefused -u admin:admin https://127.0.0.1:8080/_cluster/health >/dev/null || exit 1"]
+ interval: 10s
+ timeout: 10s
+ retries: 30
+ start_period: 20s
networks:
- es-ror-network
kbn-ror:
@@
depends_on:
es-ror:
condition: service_healthy
+ es-ror-proxy:
+ condition: service_healthyAlso applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ror-demo-cluster/docker-compose.yml` around lines 35 - 43, kbn-ror currently
only waits on es-ror, but because ELASTICSEARCH_HOSTS now points at es-ror-proxy
you must gate kbn-ror on proxy readiness; add a healthcheck to es-ror-proxy (or
reuse an existing one) that verifies the proxy is forwarding (e.g., a simple
HTTP request to the reverse target) and update kbn-ror's startup dependency to
depend_on: es-ror-proxy with condition: service_healthy (or implement an
explicit wait-for startup step in kbn-ror that polls es-ror-proxy); reference
the es-ror-proxy service, the kbn-ror service and ELASTICSEARCH_HOSTS when
making the change.
| response=$(curl -k -s -L -w "\n%{http_code}" -u $ELASTICSEARCH_USER:$ELASTICSEARCH_PASSWORD \ | ||
| -X PUT "$ELASTICSEARCH_ADDRESS/$INDEX_NAME" \ | ||
| -H "Content-Type: application/json" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "lib.sh" shared/Repository: beshu-tech/ror-sandbox
Length of output: 98
🏁 Script executed:
cat -n shared/init-scripts/utils/lib.sh | head -150Repository: beshu-tech/ror-sandbox
Length of output: 5356
Quote the curl credentials.
Lines 33, 73, and 129 expand ELASTICSEARCH_USER/ELASTICSEARCH_PASSWORD unquoted in the curl -u flag. A password with whitespace or glob characters will be split or expanded by the shell, breaking authentication.
Suggested change
-response=$(curl -k -s -L -w "\n%{http_code}" -u $ELASTICSEARCH_USER:$ELASTICSEARCH_PASSWORD \
+response=$(curl -k -s -L -w "\n%{http_code}" -u "${ELASTICSEARCH_USER}:${ELASTICSEARCH_PASSWORD}" \Apply the same quoting in createDataStream (line 73) and putSingleDocument (line 129).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response=$(curl -k -s -L -w "\n%{http_code}" -u $ELASTICSEARCH_USER:$ELASTICSEARCH_PASSWORD \ | |
| -X PUT "$ELASTICSEARCH_ADDRESS/$INDEX_NAME" \ | |
| -H "Content-Type: application/json" | |
| response=$(curl -k -s -L -w "\n%{http_code}" -u "${ELASTICSEARCH_USER}:${ELASTICSEARCH_PASSWORD}" \ | |
| -X PUT "$ELASTICSEARCH_ADDRESS/$INDEX_NAME" \ | |
| -H "Content-Type: application/json" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared/init-scripts/utils/lib.sh` around lines 33 - 35, The curl invocations
are expanding ELASTICSEARCH_USER and ELASTICSEARCH_PASSWORD unquoted (seen in
the PUT call and similar calls in createDataStream and putSingleDocument), which
breaks when passwords contain spaces or glob chars; update the -u argument to
quote the credentials as a single shell word (e.g., wrap the combined
user:password value in double quotes) for every curl call that uses -u
$ELASTICSEARCH_USER:$ELASTICSEARCH_PASSWORD so the shell does not split or glob
the values.
| if [ "$#" -eq 2 ]; then | ||
| putSingleDocument "$INDEX_NAME" "$2" | ||
| else | ||
| while IFS= read -r DOCUMENT_CONTENT; do | ||
| putSingleDocument "$INDEX_NAME" "$DOCUMENT_CONTENT" | ||
| done | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n shared/init-scripts/utils/lib.sh | sed -n '110,130p'Repository: beshu-tech/ror-sandbox
Length of output: 738
🏁 Script executed:
ls -la shared/init-scripts/utils/lib.sh
wc -l shared/init-scripts/utils/lib.shRepository: beshu-tech/ror-sandbox
Length of output: 184
🏁 Script executed:
rg -n "putSingleDocument" shared/init-scripts/utils/lib.sh -B 2 -A 5Repository: beshu-tech/ror-sandbox
Length of output: 546
🏁 Script executed:
cat -n shared/init-scripts/utils/lib.sh | sed -n '100,160p'Repository: beshu-tech/ror-sandbox
Length of output: 2237
🏁 Script executed:
cat -n shared/init-scripts/utils/lib.sh | sed -n '90,125p'Repository: beshu-tech/ror-sandbox
Length of output: 1254
🏁 Script executed:
rg -n "putDocument" shared/init-scripts/ -B 2 -A 2Repository: beshu-tech/ror-sandbox
Length of output: 1300
🏁 Script executed:
rg -n "set -e" shared/init-scripts/Repository: beshu-tech/ror-sandbox
Length of output: 48
Stop the stdin ingestion loop on the first failed write.
The while loop at lines 119-121 continues consuming input after a failed putSingleDocument call. Since putSingleDocument returns 5 on HTTP errors (line 139), but the loop doesn't check the return status, a partial ingestion failure will be silently hidden. If a later document succeeds, the function returns 0, reporting false success to the caller. Additionally, line 117's direct call also lacks error checking for the non-stdin case.
else
while IFS= read -r DOCUMENT_CONTENT; do
- putSingleDocument "$INDEX_NAME" "$DOCUMENT_CONTENT"
+ putSingleDocument "$INDEX_NAME" "$DOCUMENT_CONTENT" || return $?
done
fiAlso apply error checking to line 117:
if [ "$#" -eq 2 ]; then
- putSingleDocument "$INDEX_NAME" "$2"
+ putSingleDocument "$INDEX_NAME" "$2" || return $?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$#" -eq 2 ]; then | |
| putSingleDocument "$INDEX_NAME" "$2" | |
| else | |
| while IFS= read -r DOCUMENT_CONTENT; do | |
| putSingleDocument "$INDEX_NAME" "$DOCUMENT_CONTENT" | |
| done | |
| fi | |
| if [ "$#" -eq 2 ]; then | |
| putSingleDocument "$INDEX_NAME" "$2" || return $? | |
| else | |
| while IFS= read -r DOCUMENT_CONTENT; do | |
| putSingleDocument "$INDEX_NAME" "$DOCUMENT_CONTENT" || return $? | |
| done | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared/init-scripts/utils/lib.sh` around lines 116 - 122, The stdin ingestion
loop and the single-document branch do not check the exit status of
putSingleDocument, allowing later successes to mask earlier failures; update
both the direct call (when "$#" -eq 2) and the while-read loop to capture the
exit code from putSingleDocument (called with "$INDEX_NAME" and the document
content) and if it is non-zero immediately break/return/exit with that code so
the function returns a failure on the first write error instead of continuing to
consume input.
2 participants
ror-demo-cluster./run.sh:admin:admina) switch tenant to
End users- in logs you should see:b) as
End userstenant go tohttps://localhost:15601/s/default/app/management/data/index_management/data_streamsYou will see "Error loading data streams" error. In KBN logs I see:
Summary by CodeRabbit
New Features
Configuration Updates
Infrastructure
Chores
Removed