[Cisco ise] Tag device-sensor fragments instead of erroring per line - #20528
[Cisco ise] Tag device-sensor fragments instead of erroring per line#20528robester0403 wants to merge 4 commits into
Conversation
robester0403
added
Integration:cisco_ise
|
Pinging @elastic/integration-experience (Team:Integration-Experience) |
Elastic Docs Style Checker (Vale)Summary: 1 suggestion found 💡 Suggestions (1): Optional style improvements. Apply when helpful.
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. |
![vera-review-bot[bot]](https://avatars.githubusercontent.com/in/3297022?s=60&v=4){kind=small}
| CISE_Profiler 0000034521 2 1 cisco-av-pair=cdp-tlv=cdpCacheAddress=192.0.2.10, cisco-av-pair=lldp-tlv=lldpSystemDescription=Cisco IOS Software C3700 Software, cisco-av-pair=cdp-tlv=cdpCacheDeviceId=SAMPLE-AP-0001, | ||
| Technical Support: http://www.cisco.com/techsupport\ | ||
| Copyright (c) 1986-2024 by Cisco Systems, Inc.\ | ||
| Compiled Tue 16-Jul-24 14:24 by mcpre, cisco-av-pair=cdp-tlv=cdpCacheDeviceId=SAMPLE-AP-0001.8, cisco-av-pair=audit-session-id=C000020A00001111AAAA2222, cisco-av-pair=method=mab, AcsSessionID=SAMPLE-ISE-01/000000000/1111111, Step=11004, NetworkDeviceGroups=Location#All Locations#SAMPLE, CPMSessionID=C000020A00001111AAAA2222, StepLatency=1=0;2=0;3=0;4=1;5=0, TotalAuthenLatency=2, Operation=Operation#Operation#CLOSED, |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/cisco_ise/data_stream/log/_dev/test/pipeline/test-pipeline-device-sensor-fragments.log:4
The new guard suppresses error.message for every grok header failure it classifies as a fragment, but no pipeline test exercises the negative branch. Add a non-fragment malformed line to this fixture so the error path stays covered.
Details
append_error_message_62c1fc01 is now conditional on ctx._tmp?.device_sensor_fragment == null, so this change decides whether a header parse failure is reported or silently tagged. The tag append_error_message_62c1fc01 appears in no expected-events file in the package, so no existing test asserts that a genuinely unparseable ISE line still yields error.message. A later tweak to the fragment condition could widen the suppression to all header failures and every pipeline test would still pass.
Recommendation:
Append a malformed line that is not a device-sensor continuation to the fixture and assert the error is still reported in the expected output:
Completely malformed ISE line with no header
{
"error": {
"message": [
"grok_time_details: Provided Grok expressions do not match field value: [Completely malformed ISE line with no header]"
]
},
"event": {
"original": "Completely malformed ISE line with no header"
},
"tags": [
"preserve_original_event"
]
}🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| value: preserve_original_event | ||
| allow_duplicates: false | ||
| if: ctx.error?.message != null | ||
| - append: |
There was a problem hiding this comment.
let's revert this, because we will still need @timestamp to be set.
| - '^%{TIMEONLYSTAMP_ISO8601:_tmp.timestamp}%{ISO8601_TIMEZONE:_tmp.timezone} %{DATA:host.hostname} %{DATA:cisco_ise.log.category.name} %{GREEDYDATA:_tmp.message}$' | ||
| - '^%{DATA:cisco_ise.log.category.name} %{DATA:cisco_ise.log.message.id} %{NONNEGINT:cisco_ise.log.segment.total:long} %{NONNEGINT:cisco_ise.log.segment.number:long} %{GREEDYDATA:_tmp.message}$' | ||
| - '^Compiled %{DATA} by %{DATA}, %{GREEDYDATA:_tmp.message}$' | ||
| - '^%{GREEDYDATA:_tmp.message}$' |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: medium path: packages/cisco_ise/data_stream/log/elasticsearch/ingest_pipeline/default.yml:23
The unconditional ^%{GREEDYDATA:_tmp.message}$ catch-all makes grok_time_details succeed for every input, so genuinely malformed ISE logs no longer produce an error.message and its on_failure block is now dead code; add an explicit tag for header-less lines that are not device-sensor fragments so malformed logs stay visible.
Details
Before this change, a line that matched none of the four header patterns failed grok_time_details, which appended {{{_ingest.on_failure_processor_tag}}}: ... to error.message and consequently added the preserve_original_event tag (via append_preserve_original_event_on_error). The new final pattern ^%{GREEDYDATA:_tmp.message}$ matches any single-line input including the empty string, so the processor can never fail. That is the intended outcome for device-sensor continuation lines, but the pattern is unconditional, so it also silences every other malformed or truncated ISE record: those documents now index with message set to the raw line, no error.message, and no tag distinguishing them from a fully parsed event. The cisco_ise.timestamp_defaulted_to_ingest_time tag does not fill the gap because it also fires for correctly parsed records that use the header pattern without a timestamp (see the first document in test-pipeline-device-sensor-fragments.log-expected.json, which is a well-formed CISE_Profiler record and still carries that tag). The on_failure handler on grok_time_details (lines 26-30) is now unreachable.
Recommendation:
Keep the catch-all, but mark the lines it absorbs that are not device-sensor fragments so malformed logs remain findable. Add this after the append_tag_device_sensor_fragment processor:
- append:
tag: append_tag_unparsed_header
description: >-
The trailing catch-all grok pattern absorbs any line without an ISE
header, so flag the ones that are not device-sensor continuation lines
to keep genuinely malformed records visible.
field: tags
value: cisco_ise.unparsed_header
allow_duplicates: false
if: ctx.cisco_ise?.log?.category?.name == null && ctx._tmp?.device_sensor_fragment != true🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| # newer versions go on top | ||
| - version: "1.32.11" | ||
| changes: | ||
| - description: Tag CDP/LLDP device-sensor continuation lines as `cisco_ise.device_sensor_fragment` and keep their text in `message` instead of reporting a parse error for each line. Remove the unreachable `@timestamp` fallback added in 1.32.9. |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/cisco_ise/changelog.yml:4
The 1.32.11 changelog says it removes "the unreachable @timestamp fallback added in 1.32.9", but that fallback is still in default.yml and is demonstrably reachable; drop that sentence from the entry.
Details
The set_fallback_timestamp_to_ingest_time processor added in 1.32.9 is still present at data_stream/log/elasticsearch/ingest_pipeline/default.yml lines 323-333, and this PR removes nothing from that file (the only pipeline changes are two added grok patterns and two added processors). It is also not unreachable: every document in test-pipeline-device-sensor-fragments.log-expected.json gets its @timestamp from it (the nanosecond-precision _ingest.timestamp values) together with the cisco_ise.timestamp_defaulted_to_ingest_time tag. The changelog is user-facing release notes, so it currently tells operators a behaviour was removed when it was not.
Recommendation:
Describe only what this PR actually changes:
- version: "1.32.11"
changes:
- description: Tag CDP/LLDP device-sensor continuation lines as `cisco_ise.device_sensor_fragment` and keep their text in `message` instead of reporting a parse error for each line.
type: bugfix
link: https://github.com/elastic/integrations/pull/20528🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| banner (lldpSystemDescription) arrives with its newlines intact, each | ||
| escaped as a trailing backslash. The inputs split on them, leaving | ||
| continuation lines that carry no ISE header, so no category name is | ||
| parsed. Flag them so the attributes they carry can be processed as the |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: high path: packages/cisco_ise/data_stream/log/elasticsearch/ingest_pipeline/default.yml:38
The set_device_sensor_fragment_flag description claims the flag exists "so the attributes they carry can be processed as the tail of the preceding record", but nothing joins fragments to the preceding record; reword it to describe what the flag actually does.
Details
_tmp.device_sensor_fragment is consumed only by append_tag_device_sensor_fragment (line 47) and is then dropped with the rest of _tmp by remove_25df5c8b. No processor correlates a fragment with the record that preceded it. The README added in this same PR states the opposite of the processor description: "any attributes that followed the embedded newline (such as AcsSessionID or NetworkDeviceGroups) aren't parsed into fields". A future maintainer reading the processor will look for reassembly logic that does not exist.
Recommendation:
Make the description match the behaviour:
- set:
tag: set_device_sensor_fragment_flag
description: >-
ISE forwards device-sensor values verbatim, so a neighbor's multi-line
banner (lldpSystemDescription) arrives with its newlines intact, each
escaped as a trailing backslash. The inputs split on them, leaving
continuation lines that carry no ISE header, so no category name is
parsed. Flag them so they can be tagged and filtered out rather than
reported as a parse error; their attributes are not reassembled into
the preceding record.
field: _tmp.device_sensor_fragment
value: true🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
|
✅ All changelog entries have the correct PR link. |
| # newer versions go on top | ||
| - version: "1.32.11" | ||
| changes: | ||
| - description: Parse CDP/LLDP device-sensor continuation lines instead of reporting a parse error for each one. Tag them as `cisco_ise.device_sensor_fragment`, keep their text in `message`, and recover the attributes that follow an embedded newline. Remove the unreachable `@timestamp` fallback added in 1.32.9. |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/cisco_ise/changelog.yml:4
The reworded 1.32.11 entry now claims it will "recover the attributes that follow an embedded newline", but fragments never reach a category pipeline so no attributes are parsed; drop that clause so the changelog matches the README and the test expectations.
Details
The new wording promises attribute recovery, but the pipeline does not do that. Fragment lines leave grok_time_details with cisco_ise.log.category.name unset (patterns at default.yml:22-26 capture only _tmp.message, never a category), and every category sub-pipeline at default.yml:156-227 is gated on ctx.cisco_ise?.log?.category?.name == '<CISE_...>'. With no category, no sub-pipeline runs and the key-value parsing that would produce AcsSessionID, NetworkDeviceGroups, CPMSessionID, etc. never executes.
The PR's own artifacts confirm this. Every fragment document in test-pipeline-device-sensor-fragments.log-expected.json contains only @timestamp, ecs.version, event.original, message and tags - no parsed attributes, even for the Compiled ... line whose message is the full AcsSessionID=.../NetworkDeviceGroups=... attribute list. The README bullet added in this same PR states the opposite of the changelog: "any attributes that followed the embedded newline (such as AcsSessionID or NetworkDeviceGroups) aren't parsed into fields." A user reading the changelog will expect fields that never appear.
Recommendation:
Describe only what the change does - tag the fragments and keep their text searchable:
- version: "1.32.11"
changes:
- description: Tag CDP/LLDP device-sensor continuation lines as `cisco_ise.device_sensor_fragment` and keep their text in `message` instead of reporting a parse error for each line.
type: bugfix
link: https://github.com/elastic/integrations/pull/20528🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
Review summaryIssues found across the latest commits f9bb5d1 — 1 medium
Issues found across earlier commits 306138c — 2 medium, 2 low
Issues found across earlier commits f00d64c — 1 medium, 2 low
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
🚀 Benchmarks reportTo see the full report comment with |
💚 Build Succeeded
History
|
| - '^Technical Support: %{GREEDYDATA}$' | ||
| - '^Copyright %{GREEDYDATA}$' | ||
| - '^Compiled %{DATA} by %{DATA}, %{GREEDYDATA:_tmp.message}$' | ||
| - '^#%{NONNEGINT} %{GREEDYDATA}$' |
There was a problem hiding this comment.
Hey @taylor-swanson - Do you remember how we've these cases where events are truncated?
There was a problem hiding this comment.
If messages are truncated, that's usually a sign of a problem upstream (namely, TCP framing issues).
In this case, I guess the message contains newlines and it's getting split by the tcp input?
Ugh. The proper way to solve this is to have the Cisco device use octet framing (which completely bypasses the need for a character-based delimiter). This feels like the wrong solution.
taylor-swanson
left a comment
taylor-swanson
left a comment
There was a problem hiding this comment.
I don't think this is the correct solution to the problem. The fix is to have octet framing enabled on the sending device. Unfortunately, this integration doesn't expose TCP options, so we'd have to add that, but that is a trivial thing to add (it's done on plenty of other integrations).
Other option is to use UDP, since that is one message per datagram.
3 participants
Cisco ISE embeds a neighbor's multi-line banner in device-sensor attributes, so the inputs split one record into continuation lines that can't be parsed, producing an error document per line.
Those lines are now tagged
cisco_ise.device_sensor_fragmentwith their text kept inmessage, and an unreachable@timestampfallback is removed.Proposed commit message
Tag device-sensor fragments instead of erroring per line
Checklist
- [ ] I have verified that all data streams collect metrics or logs.changelog.ymlfile.- [ ] I have verified that Kibana version constraints are current according to guidelines.- [ ] I have verified that any added dashboard complies with Kibana's Dashboard good practicesAuthor's Checklist
How to test this PR locally