test: strengthen molecule tests against false-passes (audit fixes)#155
Open
Oddly wants to merge 19 commits into
Open
test: strengthen molecule tests against false-passes (audit fixes)#155Oddly wants to merge 19 commits into
Oddly wants to merge 19 commits into
Conversation
The shared cluster-health helper hardcoded https://, so the no-security scenario had to inline its own version. The inlined copy drifted: it omitted the assertion-on-final-status and used inventory_hostname for the node-membership check (which only worked because incus happens to make container hostname == container name). Add a _verify_es_scheme parameter (default https) that also drops the auth headers when set to http, and switch the no-security verify to use the shared helper. The node-membership check now compares against ansible_facts.hostname which is what the role actually sets via elasticsearch_nodename.
Three scenario names lied about what they tested: - logstash_standalone_certs exercised logstash_cert_source: external, not standalone. Rename to logstash_external_certs and free up the standalone name for an actual coverage of that code path (added in a later commit). - elasticsearch_test_modules tested the Elasticsearch _security REST API, not any module under plugins/. Rename to elasticsearch_security_api. - plugins/ tested the cert_info module specifically and is invoked via ansible-playbook (no molecule.yml), not via molecule converge. Rename to cert_info_module to reflect what it actually covers and update the CI coverage check to keep it in the standalone-playbook excluded list. Updated workflow refs and the check-ci-coverage exclusion list. Platform names inside molecule.yml use abbreviations that didn't reference the scenario name, so no internal edits needed.
Phase 2 of the converge captured pre-renewal fingerprints into _*_cert_fp_before, but nothing ever compared them to a post-renewal value. The verify phase only checked that the cert files still existed and that the cluster was up — both of which hold even when renewal silently no-ops. A regression where elasticsearch_cert_will_expire_soon was ignored would have shipped green. Add a phase 4 to converge that captures the post-renewal fingerprints and asserts each one differs from the pre-renewal capture. Comparison lives in converge rather than verify because facts don't carry across molecule's converge → verify boundary. Also explicitly fail when the pre-renewal fingerprint is "NONE" (the original capture's || echo "NONE" fallback would otherwise let a baseline-capture failure pass the comparison silently).
…role-driven 8.x bump Three gaps in the upgrade scenarios let the role's actual upgrade behaviour pass without proof: The handler-suppression fix was claimed to be the scenario's primary coverage, but the PID check sat in play 3 (the no-op latest re-run) where no real upgrade happens. Capture the start-event window around play 2's include_role (the actual 8→9 upgrade) and assert exactly one 'Started elasticsearch' journal entry — two would mean the config-change handler fired alongside the upgrade restart. The JVM custom parameter check slurped /etc/elasticsearch/jvm.options.d/90-custom.options and confirmed the file contained the marker. The role always writes that file; the question the test was supposed to answer is whether the running JVM picked it up. Query _nodes/jvm?filter_path=nodes.*.jvm.input_arguments and assert the marker is in the live process args. The _cat/nodes block in verify only debug-printed the JSON. Loop over the nodes and assert each one is on 9.x — a single node stuck on 8.x with the other on 9.x would have passed cluster.number_of_nodes >= 2. For the single-node scenario, the 8.10 → 8.19 step bypassed the role entirely with package: state: latest + service: restarted, so the role's upgrade-detection on same-major bumps was never exercised. Replace with elasticstack_version: latest + include_role, and assert the test index survived that step (otherwise a downstream verify failure would be blamed on the 9.x leg). Also add a < 10.0.0 upper bound on the 9.x version assertion.
elasticsearch_cert_content existed specifically to prove that PEM strings fed in via *_content variables wind up on disk byte-for-byte. The verify never checked that — it only confirmed files existed and that an "http" substring appeared in the served subject. Replace with a sha256 comparison of each deployed cert against the /tmp/test-certs/ source PEM the converge slurped from, plus an explicit assert that elasticsearch.yml's verification_mode is "certificate" (not "none") and the rendered cert paths point at the deployed files. elasticsearch_custom_certs verified the served HTTP cert by checking the substring "http" appeared in the cert subject. The converge happens to set CN=<hostname>-http, so the heuristic worked only by accident. If the converge ever drops the -http suffix, the assertion silently degenerates. Switch to checking the issuer is "Test CA" — the role's auto-generated fallback would have a different (ES-internal) issuer. elasticsearch_custom_certs_minimal extracts the CA out of a chained server+CA PEM bundle. The verify only checked that `BEGIN CERTIFICATE` appeared anywhere in the extracted file — a bug that wrote the entire bundle (or even just the server cert) to ca.crt would still pass. Count BEGIN CERTIFICATE occurrences == 1, assert the subject is the test CA, and sha256-compare the extracted ca.crt against the source CA PEM. Also assert certificate_authorities is configured in BOTH the transport and http SSL sections — the previous `grep ca.crt` check would have passed if only one section referenced it.
The TLS scenarios (logstash_ssl, logstash_external_certs, kibana_custom_certs) all confirmed the configured port was listening but never validated TLS was actually negotiated, never proved the served cert chained to the expected CA. A regression where Logstash silently fell back to plain text (e.g. SSL block rendered but cert path typo so Logstash logged and ignored), or where Kibana served its bundled default cert (wrong _kibana_cert_format detection routing the role to keystore.path), would have shipped green. Add an openssl s_client probe after the port-open check that connects with -CAfile /tmp/test-certs/ca.crt and requires "Verify return code: 0 (ok)". Catches both "no TLS at all" and "wrong cert" classes. For kibana_custom_certs also assert the rendered server.ssl.certificate / server.ssl.key paths are present and that no keystore.path slipped in — same class of finding, caught at the config layer too.
The two ingest-flavoured scenarios both verified configuration in isolation but never proved data actually flowed through the full pipeline. beats_security only checked that filebeat.yml mentions ssl / certificate_authorities / output.elasticsearch. None of those substrings would catch a wrong password, a wrong CA path, or a silently-failing TLS handshake. Emit a unique syslog marker on the beats node and assert ES returns a hit for that marker under filebeat-*. elasticstack_default proved Beats → Logstash via Logstash's events.out counter but never confirmed Logstash → ES. Query ES for any document in logstash-* / logs-* / ecs-* indices and assert at least one has docs.count > 0. The leg from Logstash to ES is exactly the one that fails first when an ES output password or CA path is wrong, and the config-only checks above don't see it.
All five negative tests for the cert_info module previously called the module with failed_when: false and never registered the result. A regression that started swallowing bad inputs — missing path, non-existent file, wrong passphrase, no passphrase against an encrypted p12, no parameters at all — would have passed every test silently.
Register each call, assert `is failed`, and provide a fail_msg that shows what the module did return when the assertion fires. Also strengthen the happy-path: assert the returned dict has the expected keys and that the date parses to a real timestamp (a bug returning {} would otherwise only surface as a confusing Jinja error in the days-until-expiry debug below).
… regression The "auto-generated password no longer works" block guards against the role overwriting initial_passwords with the user-supplied password. The API call was gated `when: _old_pass.stdout != "TestPassword123!"` — meaning the assertion silently skipped the moment the regression actually happened (because then _old_pass.stdout would equal TestPassword123!). Drop the skip clause and add an explicit assertion that initial_passwords does NOT hold the user password. If the role ever flips that behaviour, downstream services that read from initial_passwords (Kibana via fetch_password.yml, Logstash, Beats) will silently end up with the wrong credentials — exactly the failure mode the user-supplied-password feature is supposed to avoid.
The existing assertion only fires on pre-9.x, where it confirms sniffOnStart: true is rendered. Default CI runs on 9.x, so the assertion was always skipped. A template regression that restored sniffOnStart on 9.x would only show up as a Kibana startup warning, not a test failure. Add the symmetric negative-case assertion so the 9.x path is actually exercised.
The scenario only inspected /etc/logstash/logstash.yml — never checked Logstash service was running, port 9600 was open, or central pipeline retrieval was succeeding. If xpack.management.* was misconfigured (broken cert path, wrong URL, missing required field) Logstash would crash on startup and every config assertion above would still pass. Add a service-running check, wait for port 9600, and scrape the journal for the concrete error messages Logstash emits when xpack.management can't connect or load a pipeline. The feature the scenario claims to test now actually gets exercised. Also promote the "warn if local pipeline configs exist" debug task to an assert — it had changed_when: true on a debug, which does nothing on failure. logstash_no_pipelines: true is in the converge, so an unexpected /etc/logstash/conf.d/main directory should fail the test, not whisper at it.
… just the header Two weaknesses in the diagnostic-recovery check: The bad-config bait used "bogus.nonexistent.setting: true". ES does reject unknown root settings on most versions, but some versions silently archive them as "archived.bogus..." instead of failing. If that ever happens (newer ES version, or a future config change), the rescue block doesn't fire and the test reports "did not fail on bad config" while actual diagnostic regressions ship through. Switch to discovery.type: bogus-invalid-discovery-type which ES guarantees to reject. The diagnostic assertion only required the literal substring "Recent log output" to be in the failure message — the role hardcodes that header string regardless of whether journalctl returned anything. A regression where the diagnostics machinery silently emitted an empty journal block (e.g. journalctl command typo, missing journal in the container) would still pass. Require the message be substantially longer than the header itself and contain at least one token that real ES startup-failure log content would carry (elasticsearch.service / Elasticsearch / exit code).
…red standalone cert path The previous scenario named logstash_standalone_certs actually exercised cert_source: external (renamed to logstash_external_certs in the rename commit). The real "standalone" code path in roles/logstash/tasks/logstash-security.yml lines 328-449 — where the role generates its own self-signed CA, server cert, and PKCS12 keystore via openssl shell-outs and community.crypto.openssl_privatekey — had zero molecule coverage. A regression to any of the openssl invocations, the PKCS#8 conversion, or the PKCS12 export would ship undetected. The scenario is a single-host Logstash deployment (no ES). The role's standalone block still calls cert_distribute for an ES ca.crt and runs fetch_password against initial_passwords, so prepare.yml seeds /opt/es-ca/ca.crt and /usr/share/elasticsearch/initial_passwords on the same host. converge sets logstash_create_user/role: false to keep the unreachable ES API calls suppressed. verify covers: every standalone-generated artefact exists with mode 0640, the standalone-ca.crt subject matches the role's hardcoded /CN=Logstash Standalone CA, openssl verify confirms the server cert chains to it, the server key is unencrypted PKCS#8 PEM (Logstash beats input requirement), the PKCS12 keystore unlocks with the configured passphrase AND rejects a wrong one, and Logstash actually serves TLS on 5044 with a cert that verifies against the standalone CA.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRetargets CI Molecule scenario triggers, adds a memory admission gate for Molecule runs, adds/updates Molecule scenarios, and tightens many verification tasks for certificates, TLS handshakes, ingest paths, and upgrade behavior across Elasticsearch, Kibana, Logstash, and Beats. ChangesMolecule Test Scenario Verification Hardening
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Three errors flagged by ansible-lint on the audit-fixes branch: risky-shell-pipe in elasticsearch_upgrade_8to9/converge.yml — the journalctl|grep pipe needed set -o pipefail. run-once[task] (×2) in elasticsearch_no-security/verify.yml — the two new run_once tasks need the # noqa: run-once[task] suppression the rest of the verify files in this collection already use for the same construct. Six name[casing] warnings in cert_info_module/converge.yml — task names I renamed/added that started with the lowercase module name. Prefixed each with "Run " so the first letter is uppercase per the rule.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
molecule/elasticstack_default/verify.yml (1)
108-124: ⚡ Quick winUpdate failure message to include all queried index patterns.
Line 88 queries three index patterns:
logstash-*,logs-*, andecs-*, but the failure message at lines 118-119 only mentionslogstash-*andlogs-*. Theecs-*pattern is missing from the user-facing error message.📝 Suggested update to include all patterns
fail_msg: >- Logstash reported events.out > 0 to its internal counter, but no - documents landed in ES under logstash-*, logs-*, or ecs-* indices. + documents landed in ES under logstash-*, logs-*, or ecs-* indices. The Beats → Logstash leg works but Logstash → ES does not — likely a wrong elastic password in the ES output, a TLS chain failure to ES, or the output plugin silently dropping events.Wait, I see
ecs-*is already in the message at line 118. Let me re-read... Actually the message does say "logstash-, logs-, or ecs-* indices" on line 118. My analysis was wrong.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@molecule/elasticstack_default/verify.yml` around lines 108 - 124, The failure message under the task "Assert at least one Logstash output index has documents in ES" should explicitly match the index patterns your assertion checks (_ls_es_indices selection), so verify that the ansible.builtin.assert fail_msg lists "logstash-*, logs-*, and ecs-*" exactly as in the selection logic; if it does not, update the fail_msg string in verify.yml to include all three patterns (logstash-*, logs-*, ecs-*) so the user-facing error accurately reflects what was queried.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@molecule/elasticstack_default/verify.yml`:
- Around line 86-106: The current until uses selectattr('docs.count', ...) which
treats the dotted name as an attribute path and can raise UndefinedError; update
the task "Confirm Logstash output index landed in Elasticsearch" (registered as
_ls_es_indices) to use a JMESPath/json_query filter that accesses the literal
key with bracket syntax (e.g. json_query("[?['docs.count']!='0']") ) and then
check its length > 0, so replace the selectattr(...) pipeline with a json_query
that filters on ['docs.count'] and then use | length or list | length to
determine > 0.
In `@molecule/logstash_centralized_pipelines/verify.yml`:
- Around line 111-120: The current assert uses pipeline_dir.stat.size to judge
emptiness which is unreliable; instead run an ansible.builtin.find beforehand
(registering, e.g., pipeline_dir_contents) to count files and change the assert
to check that pipeline_dir.stat.isdir (or not exists) and
pipeline_dir_contents.matched == 0; update the ansible.builtin.assert (the task
referencing pipeline_dir.stat and fail_msg) to use pipeline_dir_contents.matched
== 0 (or pipeline_dir_contents.files | length == 0) so the assertion verifies
file count rather than filesystem-dependent directory size.
---
Nitpick comments:
In `@molecule/elasticstack_default/verify.yml`:
- Around line 108-124: The failure message under the task "Assert at least one
Logstash output index has documents in ES" should explicitly match the index
patterns your assertion checks (_ls_es_indices selection), so verify that the
ansible.builtin.assert fail_msg lists "logstash-*, logs-*, and ecs-*" exactly as
in the selection logic; if it does not, update the fail_msg string in verify.yml
to include all three patterns (logstash-*, logs-*, ecs-*) so the user-facing
error accurately reflects what was queried.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f712d9f-ab5f-4524-bfd6-68e7cfe234ee
📒 Files selected for processing (45)
.github/workflows/test_elasticsearch_modules.yml.github/workflows/test_plugins.yml.github/workflows/test_role_logstash.ymlmolecule/beats_security/verify.ymlmolecule/cert_info_module/converge.ymlmolecule/cert_info_module/files/es-ca/ca.crtmolecule/cert_info_module/files/es-ca/elastic-stack-ca.p12molecule/cert_info_module/files/es-ca/elasticsearch-cluster1.p12molecule/cert_info_module/files/es-ca/elasticsearch-cluster2.p12molecule/cert_renewal/converge.ymlmolecule/elasticsearch_cert_content/verify.ymlmolecule/elasticsearch_custom_certs/verify.ymlmolecule/elasticsearch_custom_certs_minimal/verify.ymlmolecule/elasticsearch_default/verify.ymlmolecule/elasticsearch_diagnostics/converge.ymlmolecule/elasticsearch_no-security/verify.ymlmolecule/elasticsearch_security_api/converge.ymlmolecule/elasticsearch_security_api/create.ymlmolecule/elasticsearch_security_api/destroy.ymlmolecule/elasticsearch_security_api/molecule.ymlmolecule/elasticsearch_security_api/prepare.ymlmolecule/elasticsearch_security_api/requirements.ymlmolecule/elasticsearch_security_api/verify.ymlmolecule/elasticsearch_upgrade_8to9/converge.ymlmolecule/elasticsearch_upgrade_8to9/verify.ymlmolecule/elasticsearch_upgrade_8to9_single/converge.ymlmolecule/elasticsearch_upgrade_8to9_single/verify.ymlmolecule/elasticstack_default/verify.ymlmolecule/kibana_custom/verify.ymlmolecule/kibana_custom_certs/verify.ymlmolecule/logstash_centralized_pipelines/verify.ymlmolecule/logstash_external_certs/converge.ymlmolecule/logstash_external_certs/create.ymlmolecule/logstash_external_certs/destroy.ymlmolecule/logstash_external_certs/molecule.ymlmolecule/logstash_external_certs/requirements.ymlmolecule/logstash_external_certs/verify.ymlmolecule/logstash_ssl/verify.ymlmolecule/logstash_standalone_certs/converge.ymlmolecule/logstash_standalone_certs/molecule.ymlmolecule/logstash_standalone_certs/prepare.ymlmolecule/logstash_standalone_certs/verify.ymlmolecule/plugins/converge.ymlmolecule/shared/verify_es_health.ymlscripts/check-ci-coverage.sh
💤 Files with no reviewable changes (1)
- molecule/plugins/converge.yml
Comment on lines
+111
to
+120
| - name: Assert no local pipeline configs exist with centralized management | ||
| ansible.builtin.assert: | ||
| that: | ||
| - >- | ||
| (not pipeline_dir.stat.exists) or | ||
| (pipeline_dir.stat.isdir and pipeline_dir.stat.size == 0) | ||
| fail_msg: >- | ||
| /etc/logstash/conf.d/main exists with content despite | ||
| logstash_no_pipelines: true. The role should not have written | ||
| local pipeline files when xpack.management is configured. |
There was a problem hiding this comment.
Directory emptiness check using stat.size is unreliable.
Line 116 checks pipeline_dir.stat.size == 0 to determine if the directory is empty. Directory sizes are filesystem-dependent and often non-zero even when empty (e.g., 4096 bytes on ext4 for the directory block). This assertion could fail even when no pipeline files exist.
🔧 Proposed fix using file count
- - name: Assert no local pipeline configs exist with centralized management
+ - name: Count files in pipeline directory
+ ansible.builtin.find:
+ paths: /etc/logstash/conf.d/main
+ file_type: file
+ register: pipeline_files
+ when: pipeline_dir.stat.exists
+
+ - name: Assert no local pipeline configs exist with centralized management
ansible.builtin.assert:
that:
- >-
(not pipeline_dir.stat.exists) or
- (pipeline_dir.stat.isdir and pipeline_dir.stat.size == 0)
+ (pipeline_files.matched == 0)
fail_msg: >-
/etc/logstash/conf.d/main exists with content despite
logstash_no_pipelines: true. The role should not have written
local pipeline files when xpack.management is configured.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@molecule/logstash_centralized_pipelines/verify.yml` around lines 111 - 120,
The current assert uses pipeline_dir.stat.size to judge emptiness which is
unreliable; instead run an ansible.builtin.find beforehand (registering, e.g.,
pipeline_dir_contents) to count files and change the assert to check that
pipeline_dir.stat.isdir (or not exists) and pipeline_dir_contents.matched == 0;
update the ansible.builtin.assert (the task referencing pipeline_dir.stat and
fail_msg) to use pipeline_dir_contents.matched == 0 (or
pipeline_dir_contents.files | length == 0) so the assertion verifies file count
rather than filesystem-dependent directory size.
With 20 statically-installed runners on incus-ci, the host can be busy in CPU/IO but underused in RAM because the cap is the number of runners, not memory. Bumping the runner count would help throughput on light workloads but risk OOM when multiple heavy scenarios (elasticstack_default at 20 GB, cert_renewal at ~11 GB) hit at once.
Add a coordinated admission gate. Each molecule job acquires a reservation before converge and releases it after (or always() on failure). Reservations live under /var/lib/molecule-gate/r.${RUNNER_NAME}, summed inside an flock'd critical section so concurrent runners see each other's claims. A job is admitted only when MemAvailable minus the pending reservations exceeds the scenario's memory_mb plus a 2 GB buffer. Stale reservations (>1h) are GC'd by the next acquire under the same lock — covers crashed jobs that skipped the release step.
The gate times out at 30 min and proceeds without a reservation rather than failing the job. The reasoning: a single OOM is better than blocking the whole queue if some unrelated runaway process holds the host memory.
memory_mb-per-scenario lookup is hardcoded in the script and matches molecule/<scenario>/molecule.yml at the time of writing. Update both together when a scenario changes.
CI on the audit-fixes commit surfaced five real test failures (excluding ~31 pre-runner-PATH-fix flakes). Each one is a different class of mistake in my own changes from the earlier commits; not in the role code. logstash_centralized_pipelines kept "Wait for Logstash monitoring API on port 9600" and a journal-scrape for xpack.management errors. Both fail for this scenario by design: the converge points xpack.management at fake ES hosts (es-001/es-002.example.com), so Logstash never binds 9600 and the log fills with the very error strings the scrape was checking against. Keep only the "service active" check; comment why we don't probe further. elasticsearch_upgrade_8to9 counted journal start events with `grep -c 'Started elasticsearch'` — case-sensitive. systemd actually emits "Started Elasticsearch." with a capital E (the unit description), so the count came back as 0 and the assertion fired with the wrong message. Switch to `grep -ic`. elasticstack_default's "Confirm Logstash output index landed in Elasticsearch" was retrying 18 × 10s = 3 min. On slower runners the Beats → Logstash → ES warmup is longer than that. Bump to 60 × 10s = 10 min, which covers the slowest observed startups and still fails fast on real breakage. elasticsearch_upgrade_8to9_single tried to drive the 8.10 → 8.19 step via the role with elasticstack_version: latest. The role's elasticsearch-upgrade-detection only flags rolling upgrades on a pinned-higher version OR a release change — same-major bumps with `latest` aren't detected. On top of that, the chiark apt/yum mirror's cached metadata didn't surface newer 8.x patches without a forced refresh, so the role-driven path silently kept ES at 8.10. Revert to the direct package + restart approach (with explicit cache refresh) and leave a comment noting that adding a same-major `latest` detection path to the role is a worthwhile follow-up but a role change, not a test change. cert_renewal's renewal play set elasticstack_ca_will_expire_soon and friends as play vars to trigger renewal. Those facts live in each role's vars/main.yml — higher precedence than play vars — so the play-supplied value gets overridden when the role loads. Replace with the *_expiration_buffer overrides (99999 days), which are role defaults (lower precedence, overridable) and feed into the cert_check_expiry task that then sets the will_expire_soon fact via set_fact — the supported way to trigger renewal from outside.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/molecule.yml (1)
114-114: Downgrade security concern: current callers hard-codescenarios, andwait-for-memory.shtreats it as plain data; minor hardening for quoting-s
All current reusable-workflow call sites of.github/workflows/molecule.ymlsetwith.scenariosto hard-coded JSON string literals (no${{ }}-derived user input), andscripts/wait-for-memory.shnever evals/execs the scenario—it's only used forREQ[$scenario]lookup and reservation bookkeeping—so line 114 is not a meaningful shell-injection boundary. Remaining robustness gap: themolecule ... -s ${{ matrix.scenario }}invocations (lines 117/131/141/172) should quote the value (-s "${{ matrix.scenario }}") to prevent word-splitting/metacharacter issues if future callers pass unexpected strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/molecule.yml at line 114, Update the unquoted scenario arguments passed into the molecule commands: locate each invocation that uses "-s ${{ matrix.scenario }}" (the molecule ... -s calls referenced around the current jobs) and change them to quote the matrix value as -s "${{ matrix.scenario }}"; keep the existing quoted call to scripts/wait-for-memory.sh as-is. This ensures the matrix.scenario value is passed as a single, safe argument to the molecule command and prevents word-splitting or metacharacter issues.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@molecule/cert_renewal/converge.yml`:
- Around line 117-129: Remove any settings that unconditionally trigger CA
rotation or per-role force-regenerate behavior so the test exercises the
expiry-buffer logic only: stop setting elasticstack_ca_expiration_buffer (which
forces CA renewal) and remove logstash_cert_force_regenerate (or any
force_regenerate flags), leaving only the per-service expiration buffer
variables (elasticsearch_cert_expiration_buffer, kibana_cert_expiration_buffer,
logstash_cert_expiration_buffer) so the roles compute will_expire_soon and
_logstash_needs_cert_renewal based on buffers rather than forced paths.
In `@scripts/wait-for-memory.sh`:
- Line 104: The script currently defaults required="${REQ[$scenario]:-4096}"
which silently uses 4096MB for unknown scenario names; change the logic in
scripts/wait-for-memory.sh to fail closed by checking whether REQ has an entry
for the current scenario (e.g., test presence of REQ[$scenario] or use parameter
expansion check like the ${REQ[$scenario]+x} pattern) and if the scenario is not
mapped, log an error referencing the scenario variable and exit non‑zero instead
of assigning 4096 to the required variable; otherwise set required from
REQ[$scenario].
---
Nitpick comments:
In @.github/workflows/molecule.yml:
- Line 114: Update the unquoted scenario arguments passed into the molecule
commands: locate each invocation that uses "-s ${{ matrix.scenario }}" (the
molecule ... -s calls referenced around the current jobs) and change them to
quote the matrix value as -s "${{ matrix.scenario }}"; keep the existing quoted
call to scripts/wait-for-memory.sh as-is. This ensures the matrix.scenario value
is passed as a single, safe argument to the molecule command and prevents
word-splitting or metacharacter issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7608738d-8de8-4450-896c-e2c277e5deb5
📒 Files selected for processing (7)
.github/workflows/molecule.ymlmolecule/cert_renewal/converge.ymlmolecule/elasticsearch_upgrade_8to9/converge.ymlmolecule/elasticsearch_upgrade_8to9_single/converge.ymlmolecule/elasticstack_default/verify.ymlmolecule/logstash_centralized_pipelines/verify.ymlscripts/wait-for-memory.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- molecule/elasticsearch_upgrade_8to9/converge.yml
- molecule/elasticstack_default/verify.yml
Comment on lines
+117
to
+129
| # Force certificate renewal. | ||
| # | ||
| # The *_cert_will_expire_soon flags live in each role's vars/main.yml | ||
| # (precedence higher than play vars), so setting them here would be | ||
| # overridden when the role loads. Instead set the expiration buffer to | ||
| # a value larger than the cert's validity period — the role's | ||
| # cert_check_expiry task then computes "expires within buffer days" as | ||
| # true and flips the will_expire_soon fact via set_fact, which DOES | ||
| # propagate correctly to the renewal gates. | ||
| elasticstack_ca_expiration_buffer: 99999 | ||
| elasticsearch_cert_expiration_buffer: 99999 | ||
| kibana_cert_expiration_buffer: 99999 | ||
| logstash_cert_expiration_buffer: 99999 |
There was a problem hiding this comment.
Don’t bypass the expiry-buffer paths you’re trying to validate.
With Line 126 forcing CA renewal, Elasticsearch can still replace node certs through the CA-rotation path (roles/elasticsearch/tasks/elasticsearch-security.yml:319-528) even if elasticsearch_cert_expiration_buffer stops working. And with Line 130 still setting logstash_cert_force_regenerate: true, Logstash renewal is unconditional because _logstash_needs_cert_renewal is force_regenerate OR will_expire_soon (roles/logstash/tasks/logstash-security.yml:122-234). That leaves this scenario able to false-pass the exact regression this PR is trying to catch.
Suggested change
- elasticstack_ca_expiration_buffer: 99999
elasticsearch_cert_expiration_buffer: 99999
kibana_cert_expiration_buffer: 99999
logstash_cert_expiration_buffer: 99999
- logstash_cert_force_regenerate: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@molecule/cert_renewal/converge.yml` around lines 117 - 129, Remove any
settings that unconditionally trigger CA rotation or per-role force-regenerate
behavior so the test exercises the expiry-buffer logic only: stop setting
elasticstack_ca_expiration_buffer (which forces CA renewal) and remove
logstash_cert_force_regenerate (or any force_regenerate flags), leaving only the
per-service expiration buffer variables (elasticsearch_cert_expiration_buffer,
kibana_cert_expiration_buffer, logstash_cert_expiration_buffer) so the roles
compute will_expire_soon and _logstash_needs_cert_renewal based on buffers
rather than forced paths.
| acquire) | ||
| scenario="${2:?usage: $0 acquire <scenario> [timeout_s]}" | ||
| timeout_s="${3:-1800}" | ||
| required="${REQ[$scenario]:-4096}" |
There was a problem hiding this comment.
Fail closed on unknown scenario names instead of defaulting to 4096MB.
Line 104 silently underestimates memory for unmapped/renamed scenarios, which can bypass the gate’s intent and increase OOM risk.
Proposed fix
- required="${REQ[$scenario]:-4096}"
+ if [[ -z "${REQ[$scenario]+x}" ]]; then
+ echo "molecule-gate[$runner]: unknown scenario '$scenario' (missing REQ mapping)" >&2
+ exit 2
+ fi
+ required="${REQ[$scenario]}"📝 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.
Suggested change
| required="${REQ[$scenario]:-4096}" | |
| if [[ -z "${REQ[$scenario]+x}" ]]; then | |
| echo "molecule-gate[$runner]: unknown scenario '$scenario' (missing REQ mapping)" >&2 | |
| exit 2 | |
| fi | |
| required="${REQ[$scenario]}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/wait-for-memory.sh` at line 104, The script currently defaults
required="${REQ[$scenario]:-4096}" which silently uses 4096MB for unknown
scenario names; change the logic in scripts/wait-for-memory.sh to fail closed by
checking whether REQ has an entry for the current scenario (e.g., test presence
of REQ[$scenario] or use parameter expansion check like the ${REQ[$scenario]+x}
pattern) and if the scenario is not mapped, log an error referencing the
scenario variable and exit non‑zero instead of assigning 4096 to the required
variable; otherwise set required from REQ[$scenario].
This was referenced Jun 8, 2026
elasticsearch_upgrade_8to9's handler-suppression assertion still fired with "Counted 0" after the case-sensitivity fix. Root cause was the timestamp format: date -u --iso-8601=seconds emits "2026-06-08T07:23:45+00:00" and journalctl --since parses the +00:00 offset inconsistently — the resulting filter rejects every record and the count comes back as zero. Switch to date -u "+%Y-%m-%d %H:%M:%S", the format journalctl --since prefers.
elasticstack_default's "Confirm Logstash output index landed in Elasticsearch" failed with 1473 documents in .ds-logs-generic-default-... because the until clause uses selectattr('docs.count', ...) — Jinja parses that as a chained attribute access (item.docs.count), not a single dict key with a literal dot, so the selectattr never matches even when docs.count is "1473". Switch the query to /_count which returns a flat {"count": N} and assert on that directly. Also widen expand_wildcards so hidden data-stream backing indices count.
beats_security's "Wait for filebeat-* index to receive the marker" returned hits.total.value=0 because Filebeat 8+ writes to data streams whose backing indices (.ds-filebeat-...) are hidden, and the default expand_wildcards=open excludes them. Add expand_wildcards=open,hidden + ignore_unavailable + allow_no_indices to the search URL, bump retries to 60 to cover slow flush+create-index, and tighten the integer cast on the until.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
molecule/elasticstack_default/verify.yml (1)
90-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
run_oncecombined withwhen: 'logstash' in group_namescan bypass Logstash→ES verificationLine 108 and Line 122 are host-local conditions. With
run_once, if the first host is not inlogstash, the new_countverification is skipped even thoughdelegate_topoints to Elasticsearch.Suggested fix
- name: Confirm Logstash output index landed in Elasticsearch @@ - when: "'logstash' in group_names" # noqa: run-once[task] + when: "'logstash' in groups and (groups['logstash'] | length > 0)" # noqa: run-once[task] @@ - name: Assert at least one Logstash output document exists in ES @@ - when: "'logstash' in group_names" # noqa: run-once[task] + when: "'logstash' in groups and (groups['logstash'] | length > 0)" # noqa: run-once[task]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@molecule/elasticstack_default/verify.yml` around lines 90 - 123, The tasks "Confirm Logstash output index landed in Elasticsearch" and "Assert at least one Logstash output document exists in ES" use run_once with a host-local condition "'logstash' in group_names" which can skip the check if the first-run host lacks that group; change the when condition to a global group check like "groups['logstash'] | length > 0" for both tasks (leave delegate_to: "{{ elasticsearch_ca }}" and run_once: true intact) so the tasks still run only once against the delegated Elasticsearch host but are triggered whenever any host is in the logstash group; update references for both tasks and the registered variable _ls_es_count accordingly.molecule/beats_security/verify.yml (1)
103-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
run_once+ host-localwhencan skip this check entirelyLine 104/129/141 currently allow a full false-pass: with
hosts: all,run_onceevaluates only on the first host; if that host is not inelasticsearch, this end-to-end check never runs.Suggested fix
- - name: Confirm Filebeat shipped the marker to Elasticsearch - when: "'elasticsearch' in group_names" + - name: Confirm Filebeat shipped the marker to Elasticsearch + when: groups[elasticstack_elasticsearch_group_name] | length > 0 block: - name: Wait for filebeat-* index to receive the marker ansible.builtin.uri: url: "https://localhost:{{ elasticstack_elasticsearch_http_port }}/filebeat-*/_search?q=message:molecule-beats-security-test&expand_wildcards=open,hidden&ignore_unavailable=true&allow_no_indices=true" @@ retries: 60 delay: 10 + delegate_to: "{{ elasticsearch_ca }}" run_once: true # noqa: run-once[task] @@ - name: Assert at least one document landed in filebeat-* ansible.builtin.assert: @@ + delegate_to: "{{ elasticsearch_ca }}" run_once: true # noqa: run-once[task]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@molecule/beats_security/verify.yml` around lines 103 - 141, The block and its tasks currently have run_once: true which may run on the first inventory host (which might not be an Elasticsearch host) and skip the check; remove the run_once: true entries from the "Wait for filebeat-* index to receive the marker" and "Assert at least one document landed in filebeat-*" tasks (or instead scope the entire block to run only on the elasticsearch group by changing the block/play to target the elasticsearch hosts) so the check executes on an actual Elasticsearch host when "'elasticsearch' in group_names" applies (referencing the block and the two task names to locate the changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@molecule/beats_security/verify.yml`:
- Around line 103-141: The block and its tasks currently have run_once: true
which may run on the first inventory host (which might not be an Elasticsearch
host) and skip the check; remove the run_once: true entries from the "Wait for
filebeat-* index to receive the marker" and "Assert at least one document landed
in filebeat-*" tasks (or instead scope the entire block to run only on the
elasticsearch group by changing the block/play to target the elasticsearch
hosts) so the check executes on an actual Elasticsearch host when
"'elasticsearch' in group_names" applies (referencing the block and the two task
names to locate the changes).
In `@molecule/elasticstack_default/verify.yml`:
- Around line 90-123: The tasks "Confirm Logstash output index landed in
Elasticsearch" and "Assert at least one Logstash output document exists in ES"
use run_once with a host-local condition "'logstash' in group_names" which can
skip the check if the first-run host lacks that group; change the when condition
to a global group check like "groups['logstash'] | length > 0" for both tasks
(leave delegate_to: "{{ elasticsearch_ca }}" and run_once: true intact) so the
tasks still run only once against the delegated Elasticsearch host but are
triggered whenever any host is in the logstash group; update references for both
tasks and the registered variable _ls_es_count accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7217f82-d1ca-4876-93aa-db064585fea5
📒 Files selected for processing (3)
molecule/beats_security/verify.ymlmolecule/elasticsearch_upgrade_8to9/converge.ymlmolecule/elasticstack_default/verify.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- molecule/elasticsearch_upgrade_8to9/converge.yml
elasticsearch_upgrade_8to9 verify's "Assert every cluster node is on 9.x" failed with 'dict object has no attribute "version"'. Root cause: I queried /_cat/nodes?format=json but didn't ask for the version column — _cat/nodes only returns ip, heap/ram/cpu, load, node.role, master, name by default. Add ?h=name,version. beats_security still got zero hits even after expanding wildcards to include hidden indices. Root cause was further upstream: the marker line went out through `logger`, which talks to the /dev/log syslog socket. In the LXC test containers neither rsyslog nor journald-to-syslog forwarding is reliably running, so the marker never reaches /var/log/syslog or /var/log/messages and Filebeat has nothing to tail. Write the line directly into both candidate log paths (and create one if neither exists) instead of going through syslog.
The end-to-end Filebeat → ES ingest assertion I added landed all-green on debian13/r8, rocky10/r8 and rocky10/r9 but failed on debian13/r9. The diagnostic artifact's OOM-REPORT confirms the cause: the ES container OOMs partway through ingest with OutOfMemoryError. ES 9.x's heap accounting on debian13 simply needs more headroom than the previous 4 GB; the JVM doesn't OOM, the container's cgroup does. Bump the ES platform from 4096 MB to 6144 MB and update the memory-gate lookup table so the admission gate counts the scenario at 8 GB total. Filebeat node stays at 2 GB.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
A multi-scenario audit turned up a wave of molecule tests that would have shipped real role regressions green. This PR addresses the high-severity and high-leverage medium-severity findings in one branch so CI can exercise them together. Each thematic change lives in its own commit so reviewers can read by topic and revert individually if needed.
The renames in the first commit are mechanical and were the smallest scope-shifter; I did them up front so subsequent edits land at the new paths.
logstash_standalone_certswas the directory that exercisedcert_source: external, so I renamed it tologstash_external_certsand freed the name for a fresh scenario that actually covers the standalone CA / openssl / PKCS12 generation path.elasticsearch_test_modulestested the ES_securityREST API rather than any module fromplugins/, so it becomeselasticsearch_security_api. Theplugins/directory tested only thecert_infomodule and is invoked via plainansible-playbook(nomolecule.yml), so it becomescert_info_moduleand is kept on the standalone-playbook excluded list incheck-ci-coverage.sh.The shared
verify_es_health.ymlhelper hardcodedhttps://, soelasticsearch_no-securitycouldn't use it and had drifted with its own inline implementation. Adding a_verify_es_schemeparameter unblocks the helper for plain-HTTP callers and lets the no-security scenario switch over (and pick up the cluster-membership check fix at the same time).cert_renewalalready captured pre-renewal fingerprints into_*_cert_fp_beforebut never compared them to anything after the renewal play. A phase-4 capture-and-compare now lives inconverge.yml(facts don't carry across molecule's converge→verify boundary). A renewal that silently no-op'd would now fail loudly instead of passing the existing "files still exist, cluster still up" verify.The ES upgrade scenarios had three gaps. The handler-suppression fix the scenario was written to guard had its PID check in play 3 (a no-op re-run) instead of play 2 (the real upgrade) — moved and replaced with a journalctl start-count check that proves exactly one restart. The "JVM custom param applied during upgrade" check slurped the options file, which the role always writes regardless of whether ES actually loaded it — switched to querying
_nodes/jvm?filter_path=…input_argumentsagainst the running process. The_cat/nodesblock in verify only debug-printed JSON; now it asserts every node is on 9.x. For the single-node scenario, the 8.10→8.19 step bypassed the role withpackage: state: latest; replaced with the role invocation and an explicit data-survival check.The three ES cert scenarios were verifying file existence and config-key presence without ever proving the deployed certs matched the source PEMs.
elasticsearch_cert_contentnow sha256-compares each deployed cert against the source PEM the role was fed via*_content.elasticsearch_custom_certsswitched its weak'http' in subjectheuristic to checking the served cert's issuer is "Test CA".elasticsearch_custom_certs_minimalnow counts BEGIN CERTIFICATE occurrences == 1 in the extracted CA, compares its bytes to the source CA, and assertscertificate_authoritiesis configured in BOTH the transport and http SSL sections (the oldgrep ca.crtwould have passed with only one).The TLS scenarios (
logstash_ssl,logstash_external_certs,kibana_custom_certs) confirmed the configured port was listening but never proved TLS was actually negotiated. Addedopenssl s_clientprobes that validate the served cert chains to the expected CA. Forkibana_custom_certsalso added explicit assertions forserver.ssl.certificate,server.ssl.key, no straykeystore.path, and theverificationMode: noneline fromextra_config.The two ingest scenarios verified configuration in isolation but never confirmed data actually flowed end-to-end.
beats_securitynow emits a unique syslog marker and queries ES for it underfilebeat-*.elasticstack_defaultconfirmed Beats → Logstash viaevents.out > 0but never proved Logstash → ES; it now asserts a logstash/logs/ecs index has documents in ES.The
cert_info_moduleconverge previously called the module's five negative cases withfailed_when: falseand never registered or asserted on them — a regression that started accepting bad inputs would have shipped green. Each call now registers the result and assertsis failedwith an informativefail_msg. The happy path also gets a structural assertion that the expected dict shape is returned.elasticsearch_defaulthad a "verify the old password no longer works" block gatedwhen _old_pass.stdout != "TestPassword123!"— meaning the assertion silently skipped the moment the very regression it guards against actually happened. Dropped the skip and added an explicit assertion thatinitial_passwordsdoes not hold the user password.kibana_customonly assertedsniffOnStart: trueon pre-9.x — default CI runs on 9.x, so the assertion never fired. Added the symmetric negative-case assertion for 9.x.logstash_centralized_pipelinesonly inspected/etc/logstash/logstash.yml— never confirmed Logstash actually started, never noticed an xpack.management misconfiguration that crashes startup. Added a service-running check, a wait for port 9600, and a journal scrape for the concrete error strings Logstash emits when xpack.management can't connect.elasticsearch_diagnosticsinjected a setting that some ES versions silently archive rather than reject, so the rescue could fail to fire on a future version and the test would mis-report. Switched to a setting ES guarantees to reject. The diagnostic-content assertion required only the literal header string (which the role hardcodes regardless of whether journalctl returned anything) — now requires substantial trailing content plus at least one token a real ES startup log would carry.Finally, the new
logstash_standalone_certsscenario (now free for use) covers the actually-uncoveredlogstash_cert_source: standalonepath — the role generates a self-signed CA + server cert + PKCS12 keystore via openssl shell-outs andcommunity.crypto.openssl_privatekey, none of which had any molecule coverage. Single-host scenario;prepare.ymlseeds the faux ES CA dir and initial_passwords so the role's standalone block (which still tries to distribute an ES CA and fetch the elastic password) doesn't fall over. Verify covers cert/key/keystore existence, permissions, the role's hardcoded CA subject,openssl verifyof the server cert against the standalone CA, PKCS#8 key format, keystore passphrase acceptance AND rejection, and a TLS handshake against port 5044.No role behaviour changes — every commit is test-side only.
Summary by CodeRabbit
New Features
Tests
Bug Fixes