Conversation
|
Pinging @elastic/integration-experience (Team:Integration-Experience) |
✅ Elastic Docs Style Checker (Vale)No issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
…-insensitively, mapping the MSG segment to the resource or message and the client MAC to ECS source.mac.
|
✅ All changelog entries have the correct PR link. |
Review summaryIssues found across the latest commits fba1ef6 — 3 medium
Package-level:
Issues found across earlier commits efd7d9f — 1 high, 4 medium, 1 low
Package-level:
🤖 AI-Generated Review | Vera Review Bot - v0.2.6 | 📚 Knowledge base: integration-skills
|
🚀 Benchmarks reportTo see the full report comment with |
💚 Build Succeeded
History
|
|
@robester0403 are you going to address/resolve the review bot issues? |
ilyannn
left a comment
There was a problem hiding this comment.
Thanks for the RFC 5424 work — the structured-data path, MAC normalisation, and extra fixtures look solid, and CI is green.
Two things I would like fixed before merge:
- Docs still say RFC-3164 only.
packages/qnap_nas/_dev/build/docs/README.md(the source for the generated README) still tells users the integration is only compatible with RFC-3164 and to set QuLog Center to that format. This PR is specifically adding RFC 5424, so that guidance is now wrong and will send operators the wrong way. - Missing
action_resulton action 512 is treated as a successful login. See the inline comment.
Nit on the Access MSG grok is inline as well (bare paths land in qnap.nas.application instead of qnap.nas.file.path, which the RFC-3164 branch already handles).
This review was written with 🤖 Cursor/Grok 4.6 under my supervision.
| String action = ctx._tmp.sd.action; | ||
| String result = ctx._tmp.sd.action_result; | ||
| if (action == '512') { | ||
| ctx.event.action = (result == null || result == '0') ? 'login-success' : 'login-fail'; |
There was a problem hiding this comment.
A QuLog@Access event with action=512 and no action_result is classified as login-success because of result == null || result == '0'.
Missing result is not evidence of success. Prefer mapping success only when the device actually said so:
That also covers empty string, which the normaliser already drops so it arrives here as null.
There was a problem hiding this comment.
Clarification: a missing action_result should not be inferred as failure either. Map 0 to login-success and an explicit non-zero result to login-fail; when the result is missing or empty, preserve the raw qnap.nas.action: "512" (or otherwise leave the outcome unknown) without setting either login result.
This clarification has been drafted with 🤖 Cursor/ChatGPT-5.6 Sol under my supervision.
| patterns: | ||
| - '^\[%{DATA:_tmp.msg_application}\] ?%{FILE_PATH:_tmp.msg_path}$' | ||
| - '^\[%{DATA:_tmp.msg_application}\]$' | ||
| - '^%{GREEDYDATA:_tmp.msg_application}$' |
There was a problem hiding this comment.
Nit / parity with the RFC-3164 RESOURCE pattern: any Access MSG that is a bare file path (no [App] prefix) is swallowed by this catch-all and stored in qnap.nas.application instead of qnap.nas.file.path.
Adding ^%{FILE_PATH:_tmp.msg_path}$ above this line would match what the existing RFC-3164 branch already does.
|
Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as |
Executive summary
This fix adds support for RFC 5424 syslog format emitted by QNAP QTS 5.x devices that send structured-data events using the QuLog@Access SD-ID. The ingest pipeline gains a new grok pattern to match the RFC 5424 header (version 1, ISO8601 timestamp, structured data block), a kv processor to parse the key-value pairs within the structured data, and a Painless script to map numeric action codes (e.g. 512) and action_result values to ECS event.action strings (login-success, login-fail). New fields are declared in fields.yml and ecs.yml, the README is updated, and three representative pipeline test fixtures are added.
Proposed commit message
Root cause
The first grok processor (
grok_event_original_cad2ef7a) uses only a%{SYSLOGTIMESTAMP}(RFC 3164 BSD timestamp) pattern; QTS 5.x devices emit RFC 5424 syslog with a version byte1and an ISO 8601 timestamp before the hostname, causing the pattern to fail immediately after the optional PRI field. No alternative pattern exists to match the RFC 5424 envelope or parse the[QuLog@Access key="val" ...]structured-data block.Approach
Add a second grok pattern to
grok_event_original_cad2ef7athat matches RFC 5424 format (<PRI>1 ISO8601-TIMESTAMP HOSTNAME APPNAME: PROCID - [QuLog@Access KV-PAIRS] MSG) alongside the existing RFC 3164 pattern. Extract structured-data key-value pairs via akvprocessor into_tmp.sd.*, then rename them to ECS/custom fields. Map numeric action codes (512 → login-success/login-fail based on action_result=0) toevent.actionvia a Painless script so the existing ECS categorization script sets correctevent.outcome,event.category, andevent.type. Guard RFC-3164-only processors (grok__tmp_message_c75b80dc, both date processors) withctx._tmp?.sd_params == nullorctx._tmp?.timestamp != nullconditions to prevent failure on RFC 5424 events.Implementation
packages/qnap_nas/data_stream/log/elasticsearch/ingest_pipeline/default.yml, add a second pattern togrok_event_original_cad2ef7a:'^(%{ECS_SYSLOG_PRI})?1 %{TIMESTAMP_ISO8601:_tmp.timestamp_iso8601} %{NAS} %{PROG:process.name}: %{POSINT:process.pid:int} - \[%{SD_ID} %{DATA:_tmp.sd_params}\] %{GREEDYDATA:_tmp.message}'. Add pattern definitionsTIMESTAMP_ISO8601: '%{YEAR}-%{MONTHNUM}-%{MONTHDAY}T%{HOUR}:%{MINUTE}:%{SECOND}(?:[.,]\d+)?(?:%{ISO8601_TIMEZONE:event.timezone})?',ISO8601_TIMEZONE: '(?:Z|[+-]%{HOUR}:?%{MINUTE})', andSD_ID: '%{WORD}@%{WORD}'.date__tmp_timestamp_to_@timestamp_e440143cifcondition fromctx.event?.timezone != nulltoctx.event?.timezone != null && ctx._tmp?.timestamp != null. Updatedate__tmp_timestamp_to_@timestamp_baf3310eiffromctx.event?.timezone == nulltoctx.event?.timezone == null && ctx._tmp?.timestamp != null. Insert a new date processor (tagdate_rfc5424_timestamp_to_@timestamp) withfield: _tmp.timestamp_iso8601,target_field: '@timestamp',formats: [ISO8601],if: ctx._tmp?.timestamp_iso8601 != nullimmediately after the two existing date processors and beforeset_event_created_e3f09e3b.kvprocessor block (tagkv_tmp_sd_params_to_tmp_sd) withfield: _tmp.sd_params,field_split: '" ',value_split: '="',trim_value: '"',target_field: _tmp.sd,ignore_missing: true,if: ctx._tmp?.sd_params != nullimmediately after theset_event_created_e3f09e3bprocessor.renameprocessors (each withignore_missing: true, guarded byctx._tmp?.sd_params != null) to map:_tmp.sd.ip→source.address;_tmp.sd.user→user.name;_tmp.sd.computer→source.domain;_tmp.sd.application→qnap.nas.application;_tmp.sd.client_agent→user_agent.original;_tmp.sd.client_app→qnap.nas.client_app;_tmp.sd.client_id→qnap.nas.client_id;_tmp.sd.mac→qnap.nas.mac;_tmp.sd.service→qnap.nas.service.script_rfc5424_action_mapping,if: ctx._tmp?.sd?.action != null) that maps action code'512'toevent.action = 'login-success'when_tmp.sd.action_result == '0'and to'login-fail'otherwise. For any unmapped action code, write the raw numeric value toqnap.nas.actionand leaveevent.actionunset so the existing ECS categorization script returns without overwriting. Explicitly handles fallback: all unrecognised codes produce noevent.action(the ECS script'sparams.get(ctx.event.action) == nullguard then returns early).if: ctx._tmp?.sd_params == nullto thegrok__tmp_message_c75b80dcprocessor so it is skipped for RFC 5424 events whose_tmp.messageis just the MSG word (e.g.Administration) not aUsers: …formatted string.packages/qnap_nas/data_stream/log/fields/fields.yml, add five new fields underqnap.nas:client_app(keyword),client_id(keyword),mac(keyword),action(keyword, for raw numeric code),service(keyword, for raw numeric service code), andsource(keyword, for the structured-datasourceattribute).packages/qnap_nas/data_stream/log/fields/ecs.yml, add- external: ecs\n name: user_agent.originalto expose the mapped client agent field.packages/qnap_nas/data_stream/log/_dev/test/pipeline/test-access.log, append the sanitized RFC 5424 event:<30>1 2026-06-28T19:39:06.466+01:00 host-example qulogd: 19683 - [QuLog@Access mac="00-00-5E-00-53-23" ip="192.0.2.10" user="alice.johnson" source="example-source" computer="---" application="---" action="512" action_result="0" service="1024" extra_data="" client_id="89a1d5c1-2b3e-4f67-8a9b-0c1d2e3f4a5b" client_app="Web Desktop" client_agent="Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/149.0.0.0 Safari/537.36 Edg/149.0.0.0"] Administration. Add corresponding expected-output object totest-access.log-expected.jsonverifying@timestamp,user.name,source.ip,event.action=login-success,event.outcome=success,user_agent.original,qnap.nas.client_app,related.ip,related.user, and absence of---values.packages/qnap_nas/manifest.yml, bump version from1.25.3to1.26.0. Inpackages/qnap_nas/changelog.yml, prepend a new entry:version: '1.26.0',description: Add support for RFC 5424 syslog format emitted by QTS 5.x devices (QuLog@Access structured-data events),type: enhancement.Pipeline changes
<PRI>1 ISO8601 HOSTNAME PROG: PID - [QuLog@Access SD-PARAMS] MSG; add pattern definitions TIMESTAMP_ISO8601, ISO8601_TIMEZONE (capturing into event.timezone), SD_ID&& ctx._tmp?.timestamp != nullguard to prevent failure when RFC 5424 path leaves _tmp.timestamp unsetif: ctx._tmp?.sd_params == nullso RFC 5424 events skip the Users:/Connection-type parsing branchField / mapping changes
Sanitized error message
Processor 'grok' with tag 'grok_event_original_cad2ef7a' in pipeline 'logs-qnap_nas.log-default' failed with message '[on_failure_message]'Sanitized log (
event_sanitizedexcerpt)<30>1 2026-06-28T19:39:06.466+01:00 host-example qulogd: 19683 - [QuLog@Access mac="00-00-5E-00-53-23" ip="192.0.2.10" user="alice.johnson" source="example-source" computer="---" application="---" action="512" action_result="0" service="1024" extra_data="" client_id="89a1d5c1-2b3e-4f67-8a9b-0c1d2e3f4a5b" client_app="Web Desktop" client_agent="Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/149.0.0.0 Safari/537.36 Edg/149.0.0.0"] AdministrationReviewer concerns
source.domainrename from_tmp.sd.computerfires only whencomputer != '---', but the condition checksctx._tmp?.sd?.computer != '---'before the kv processor has necessarily confirmed the field is non-null; this is safe becauseignore_missing: trueis set, but reviewers should confirm kv always populates_tmp.sd.computerbefore the rename runs.512; all other numeric action codes fall through toqnap.nas.actionas a raw string with no ECS mapping, which may surprise users who expect richer categorisation.event.typeis set to["start"]foraction_result=0(success) and["info"]for failure — usinginfofor an authentication failure is unusual; ECS recommends["start"]for successful auth and["end"]or["info"]for failures, so["info"]is defensible but worth a reviewer double-check.event.categoryandevent.kindfields are present in the expected output but the pipeline diff does not show explicitsetprocessors for them for the RFC 5424 path; reviewers should verify these are populated by a downstream processor already present in the pipeline (e.g. the existing event categorisation section not shown in the diff).changelog.ymlsetslink: https://github.com/elastic/integrations/pull/1which is a placeholder and must be updated to the real PR URL before merge.Self-review findings
Self-review invoked: no (1 cycle)
Final validation passed: yes
Risk and classification
Links
b57b0e02f567c44d