Fix thread usage issue for Palo Alto Panorama Syslog payload - #26960
Fix thread usage issue for Palo Alto Panorama Syslog payload#26960Nithin-Kasam wants to merge 3 commits into
Conversation
danotorrey
left a comment
danotorrey
left a comment
There was a problem hiding this comment.
Looking good and tested successfully on my end. One additional case that needs covering (noted inline below), plus a few smaller test improvements and nits.
| private static final Pattern PANORAMA_SYSLOG_PARSER = Pattern.compile("<\\d+>[0-9] (.+?) (.+?)\\s[-]\\s[-]\\s[-]\\s[-]\\s(\\d,.*)"); | ||
| // NOTE: (\S+) is used for the timestamp and hostname groups — both tokens are | ||
| // guaranteed to be whitespace-free, so \S+ is fully equivalent to the original (.+?). | ||
| private static final Pattern PANORAMA_SYSLOG_PARSER = Pattern.compile("<\\d+>[0-9] (\\S+) (\\S+)\\s[-]\\s[-]\\s[-]\\s[-]\\s(\\d,.*)"); |
There was a problem hiding this comment.
From testing it looks like the standard-syslog pattern(STANDARD_SYSLOG_PARSER, line 57) just below this one needs the same fix as the others. The same issue still happened for that pattern. Could we apply the same fix there and add test coverage for it?
| // NOTE: (\S+) is used for the timestamp and hostname groups — both tokens are | ||
| // guaranteed to be whitespace-free, so \S+ is fully equivalent to the original (.+?). |
There was a problem hiding this comment.
The comment isn't quite right: \S+ isn't equivalent to (.+?) (the old groups could span spaces, so whitespace in a token or extra fields before the delimiter matched before and won't now). Fix is fine thought. Maybe tighten up the comment to the following would be best:
| // NOTE: (\S+) is used for the timestamp and hostname groups — both tokens are | |
| // guaranteed to be whitespace-free, so \S+ is fully equivalent to the original (.+?). | |
| // Timestamp and hostname are single whitespace-free tokens, so \S+ matches | |
| // every conformant Panorama header. |
|
|
||
| @Test | ||
| public void parse_craftedPanoramaPrefix_missingDelimiter_completesQuickly_small() { | ||
| // 200 space-separated tokens — enough to be slow with old O(n²) regex |
There was a problem hiding this comment.
These timing tests finish well under the 2s budget either way (I tried the previous pattern and they still pass), so they don't really lock in the change. Might be worth asserting the parse behavior directly, or sizing the payload so it would actually fail on the old pattern, so this stays a real regression guard.
| } | ||
|
|
||
| @Test | ||
| public void parse_panoramaMessageWithNoTimezoneOffset_parsesWithProvidedZone() { |
There was a problem hiding this comment.
Nit: since this is the only test hitting the no-offset path with a non-UTC zone, it'd be good to also assert the resulting timestamp (right now it only checks source), otherwise the timezone handling here isn't really covered.
| // 200 space-separated tokens — enough to be slow with old O(n²) regex | ||
| final String payload = buildCraftedPayload(200); | ||
|
|
||
| assertTimeout(Duration.ofSeconds(2), () -> |
There was a problem hiding this comment.
Nit: I think we should swap assertTimeout for assertTimeoutPreemptively here (all three spots). assertTimeout doesn't interrupt the block, it lets it run to completion and only then checks whether it took too long, so if a parse ever actually hangs the test hangs with it instead of failing at the 2s mark. assertTimeoutPreemptively runs the block on a separate thread and aborts it when the timeout hits, which is what you want here. Let me know if you see any issue with this or if it doesn't sound correct.
2 participants
Description
Changed the PANORAMA_SYSLOG_PARSER regex in PaloAltoParser from adjacent unbounded lazy groups (.+?) (.+?) to non-whitespace token groups (\S+) (\S+) for the timestamp and hostname captures.
Motivation and Context
closes https://github.com/Graylog2/graylog-plugin-enterprise/issues/14949
How Has This Been Tested?
-> Added unit test cases which parses both old and new pattern payloads.
-> Manually set up palo Alto VM and created Palo Alto Networks TCP (PAN-OS v11+) input and verified the logs.
Screenshots (if appropriate):
Types of changes
Checklist: