Sanity/elasticsearch-TLS: add TLS parameter phases for PQC support#68
Open
prikryla wants to merge 6 commits into
Open
Sanity/elasticsearch-TLS: add TLS parameter phases for PQC support#68prikryla wants to merge 6 commits into
prikryla wants to merge 6 commits into
Conversation
Add test phases for the new omelasticsearch TLS parameters introduced in upstream PR #7278: tls.tlsversion, tls.ciphersuites, and tls.keyexchangegroups. Restructure the test to use the section-replace pattern for multi-phase reconfiguration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewer's GuideAdds multi-phase TLS test coverage for omelasticsearch by restructuring rsyslog configuration to use a replaceable section and introducing helper functions to drive phased testing of new TLS parameters (tls.tlsversion, tls.ciphersuites, tls.keyexchangegroups) and PQC-related key exchange groups. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
send_and_verifyhelper uses a fixedsleep 15, which may be either excessive or flaky depending on environment; consider a loop with shorter sleeps and a bounded retry window to make the check more robust and faster when ES responds quickly. - In the PQC phase, the
openssl list -kem-algorithms | grep X25519MLKEM768check will treat any failure of theopenssl listsubcommand the same as “algorithm not found”; consider explicitly guarding for missing/unsupportedlist -kem-algorithms(e.g., checking OpenSSL version or return code) before deciding to skip or run the test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `send_and_verify` helper uses a fixed `sleep 15`, which may be either excessive or flaky depending on environment; consider a loop with shorter sleeps and a bounded retry window to make the check more robust and faster when ES responds quickly.
- In the PQC phase, the `openssl list -kem-algorithms | grep X25519MLKEM768` check will treat any failure of the `openssl list` subcommand the same as “algorithm not found”; consider explicitly guarding for missing/unsupported `list -kem-algorithms` (e.g., checking OpenSSL version or return code) before deciding to skip or run the test.
## Individual Comments
### Comment 1
<location path="Sanity/elasticsearch-TLS/runtest.sh" line_range="62-63" />
<code_context>
+ local msg="$1"
+ rlRun "logger '${msg}'"
+ rlRun "sleep 15"
+ rlRun -s "curl $CURL_TLS_OPTS -u elastic:$ELASTIC_PASSWORD -XGET 'https://127.0.0.1:9200/_all/_search?q=${msg}&pretty'"
+ rlAssertGrep "\"message\"" "$rlRun_LOG"
+ rlAssertGrep "${msg}" "$rlRun_LOG"
+ rm -f "$rlRun_LOG"
</code_context>
<issue_to_address>
**issue (bug_risk):** The `${msg}` placeholder in the curl URL is never expanded due to single quotes around the URL.
In `send_and_verify`, the URL is single-quoted (`'https://127.0.0.1:9200/_all/_search?q=${msg}&pretty'`), so `${msg}` is not expanded and every search uses the literal `${msg}` query. This makes the test assertions unreliable. Please allow variable expansion by dropping the single quotes or switching to double quotes, e.g.:
```sh
rlRun -s "curl $CURL_TLS_OPTS -u elastic:$ELASTIC_PASSWORD -XGET https://127.0.0.1:9200/_all/_search?q=${msg}&pretty"
# or
rlRun -s "curl $CURL_TLS_OPTS -u elastic:$ELASTIC_PASSWORD -XGET \"https://127.0.0.1:9200/_all/_search?q=${msg}&pretty\""
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Replace fixed sleep 15 in send_and_verify with a polling loop
(2s intervals, 15 attempts) for faster and more robust ES indexing checks
- Fix URL quoting: use escaped double quotes so ${msg} expansion is
unambiguous across all shells
- Guard PQC phase with explicit OpenSSL version check (>= 3.5) before
attempting 'openssl list -kem-algorithms', so older OpenSSL versions
that lack this subcommand are handled cleanly
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflict in Sanity/elasticsearch-TLS/main.fmf — keep our updated summary and description covering the new TLS parameter phases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The rsyslog Library/basic init callback requires the patch tool, which is not installed by default on minimal RHEL 10 images. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
libcurl validates TLS version at connection time, not at config parse, so rsyslog starts successfully with an invalid value. Change the phase to verify that message delivery fails instead of expecting startup failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Local execution passed on both rhel-9 and rhel-10. |
Bash rejects 'local' in compound command blocks (&&{ ... }),
causing the variable to stay empty and the negative test to
match everything.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
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.
Add test phases for the new omelasticsearch TLS parameters introduced in upstream PR #7278: tls.tlsversion, tls.ciphersuites, and tls.keyexchangegroups. Restructure the test to use the section-replace pattern for multi-phase reconfiguration.
Summary by Sourcery
Add phased TLS integration tests for omelasticsearch with new TLS configuration parameters and reusable helpers.
New Features:
Tests: