[libbeat] Improve syslog parser/processor error handling #31798

taylor-swanson · 2022-06-01T14:21:01Z

What does this PR do?

Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields.
Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message.
Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files).
Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space.
Structured data now uses map[string]interface{}
Update testify to v1.7.1 dependency for assert.ErrorContains support

Why is it important?

These changes make the parser/processor more tolerant of non-RFC compliant syslog messages. Many products/services will introduce changes that slightly deviate from RFC.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
~~[ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Run unit tests in these packages:

libbeat/processors/syslog
libbeat/reader/syslog

Related issues

Closes Make Syslog parser/processor handle errors more gracefully #31246

elasticmachine · 2022-06-01T14:28:25Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-06-27T13:21:02.925+0000
Duration: 111 min 53 sec

Test stats 🧪

Test	Results
Failed	0
Passed	22380
Skipped	1937
Total	24317

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

taylor-swanson · 2022-06-01T18:51:37Z

/test

taylor-swanson · 2022-06-02T12:59:19Z

/test

- Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support

mergify · 2022-06-06T18:34:26Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b syslog-parsing upstream/syslog-parsing
git merge upstream/main
git push upstream syslog-parsing

taylor-swanson · 2022-06-07T16:44:41Z

/test

elasticmachine · 2022-06-07T16:48:30Z

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

taylor-swanson · 2022-06-14T19:35:53Z

/package

rdner

All changes make sense to me.

libbeat/reader/syslog/util_test.go

libbeat/reader/syslog/syslog.go

libbeat/reader/syslog/util_test.go

libbeat/reader/syslog/message.go

libbeat/reader/syslog/message_test.go

libbeat/reader/syslog/rfc3164_test.go

efd6 · 2022-06-15T12:02:38Z

libbeat/reader/syslog/syslog.go

+	// ErrPriority indicates a priority value is outside the acceptable range.
+	ErrPriority = errors.New("priority value out of range (expected 0..191)")
+	// ErrEOF indicates the message is truncated and cannot be parsed.
+	ErrEOF = errors.New("message is truncated (unexpected EOF)")


Is there a reason to not use io.ErrUnexpectedEOF? It's commonly used for thing like this.

Maybe EOF is the wrong terminology here. I don't want to confuse an improperly formatted/shaped message with not receiving enough data, but then again, how do you accurately tell the difference? I'd be okay with using the one from the standard library as long as it doesn't lose its meaning here (does it really matter saying "message is truncated"?).

This doesn't make a lot of sense either (error that is):

input: <13> test-host this is the message error: "parsing error at position 5: unexpected EOF"

The actual error here is that the input doesn't match the expected shape. There should be a timestamp directly after the <13>, but for this test, it's missing. The parser is supposed to pull out the individual fields (timestamp, hostname, etc) and validation on those values happens in the host language, after the parser has run. If, however, the parser can't pull out all the fields, it fails completely. Aside from error-ing out, the only other thing I can think of is that whatever can't be parsed out will be put into the message field and we move on. This already happens, but I believe the original, whole message gets used. It seems a bit pointless to do that, though, and I would argue using something like grok instead.

Yeah. Depending on how much you want to go into it, a way to do this might be to have (parse error([new error type]missing field(unexpected EOF))), which would end up looking like "parsing error at position 5: missing timestamp: unexpected EOF" when rendered. The logic would be to look for the first missing field when there is an io.ErrUnexpectedEOF added by the parser and interpose the missing field error, or similar.

In terms of use of the io.ErrUnexpectedEOF sentinel, its used generally in decoders for this kind of thing. It's best to put yourself in the mindset of a plan9 developer where everything is a file to see why.

- Add explicit wants to test cases - Un-export test input fields - Add checks for errors in timestamp tests

taylor-swanson · 2022-06-22T18:04:28Z

I'm going to let this sit for a good week or two. Beats CI is just too unstable right now to get a clean build out of it. As far as I can tell, all of the failures recently have been completely unrelated to any changes introduced in this PR.

- Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support (cherry picked from commit cabc8ba) # Conflicts: # libbeat/processors/syslog/syslog_test.go # libbeat/reader/syslog/message.go # libbeat/reader/syslog/message_test.go # libbeat/reader/syslog/syslog.go # libbeat/reader/syslog/syslog_test.go # libbeat/reader/syslog/util_test.go

- Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support (cherry picked from commit cabc8ba)

…2118) - Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support (cherry picked from commit cabc8ba) Co-authored-by: Taylor Swanson <90622908+taylor-swanson@users.noreply.github.com>

- Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support (cherry picked from commit cabc8ba)

…2117) - Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support (cherry picked from commit cabc8ba) Co-authored-by: Taylor Swanson <90622908+taylor-swanson@users.noreply.github.com>

…) (elastic#32117) - Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support (cherry picked from commit 7ba9a7b) Co-authored-by: Taylor Swanson <90622908+taylor-swanson@users.noreply.github.com>

- Shifted most of the validation logic to the host language instead of the Ragel parser. This allows for the message to be fully parsed even if there are validation errors with one or more fields. - Errors are accumulated as they occur, any fields that have already been parsed and validated will be returned, alongside error.message. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit this part of the header (especially when writing to files). - Structured data fields for RFC 5424 messages are processed in a second pass. If the structured data format doesn't fit the RFC, it will be prepended to the message, separated by a space. - Structured data now uses map[string]interface{} - Update testify to v1.7.1 dependency for assert.ErrorContains support

taylor-swanson added enhancement libbeat :Processors Team:Security-External Integrations backport-v8.3.0 Automated backport with mergify backport-v8.2.0 Automated backport with mergify 8.4-candidate labels Jun 1, 2022

taylor-swanson requested a review from a team June 1, 2022 14:21

taylor-swanson self-assigned this Jun 1, 2022

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 1, 2022

taylor-swanson added 4 commits June 2, 2022 08:07

Update changelog

2c7af1a

Update notice

d2b5bf5

Fix tests

dfbb75c

taylor-swanson force-pushed the syslog-parsing branch from 6854691 to dfbb75c Compare June 2, 2022 13:07

taylor-swanson added 2 commits June 3, 2022 08:00

Merge branch 'main' into syslog-parsing

37d3174

Merge branch 'main' into syslog-parsing

8f6a50d

Merge branch 'main' into syslog-parsing

4f00b69

taylor-swanson marked this pull request as ready for review June 7, 2022 16:48

taylor-swanson requested a review from a team as a code owner June 7, 2022 16:48

taylor-swanson requested review from rdner and cmacknz and removed request for a team June 7, 2022 16:48

taylor-swanson added 4 commits June 13, 2022 09:17

Add docs about structured data

d883099

Merge branch 'main' into syslog-parsing

4823329

Remove line directives from ragel generated files

b6397c0

formatting

25119b9

rdner approved these changes Jun 15, 2022

View reviewed changes

efd6 reviewed Jun 15, 2022

View reviewed changes

taylor-swanson added 3 commits June 15, 2022 08:06

Merge branch 'main' into syslog-parsing

2b6376c

Improve tests

3f435d6

- Add explicit wants to test cases - Un-export test input fields - Add checks for errors in timestamp tests

Rename errors, improve test checks, readability improvements

e63dfb8

efd6 approved these changes Jun 15, 2022

View reviewed changes

taylor-swanson added 3 commits June 16, 2022 10:54

Merge branch 'main' into syslog-parsing

94cf155

Merge branch 'main' into syslog-parsing

19bd255

Merge branch 'main' into syslog-parsing

15c2a91

leehinman mentioned this pull request Jun 24, 2022

[pfsense] switch from grok to syslog processor elastic/integrations#3586

Closed

Merge branch 'main' into syslog-parsing

1577e9b

taylor-swanson merged commit cabc8ba into elastic:main Jun 27, 2022

taylor-swanson deleted the syslog-parsing branch June 27, 2022 16:27

mergify bot mentioned this pull request Jun 27, 2022

[8.2](backport #31798) [libbeat] Improve syslog parser/processor error handling #32117

Merged

mergify bot mentioned this pull request Jun 27, 2022

[8.3](backport #31798) [libbeat] Improve syslog parser/processor error handling #32118

Merged

kcreddy mentioned this pull request Jun 28, 2022

Fix pfsense grok to support new opensense log format elastic/integrations#3612

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libbeat] Improve syslog parser/processor error handling #31798

[libbeat] Improve syslog parser/processor error handling #31798

taylor-swanson commented Jun 1, 2022 •

edited

Loading

elasticmachine commented Jun 1, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

taylor-swanson commented Jun 1, 2022

taylor-swanson commented Jun 2, 2022

mergify bot commented Jun 6, 2022

taylor-swanson commented Jun 7, 2022

elasticmachine commented Jun 7, 2022

taylor-swanson commented Jun 14, 2022

rdner left a comment

efd6 Jun 15, 2022

taylor-swanson Jun 15, 2022 •

edited

Loading

taylor-swanson Jun 15, 2022

efd6 Jun 15, 2022

taylor-swanson commented Jun 22, 2022

[libbeat] Improve syslog parser/processor error handling #31798

[libbeat] Improve syslog parser/processor error handling #31798

Conversation

taylor-swanson commented Jun 1, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

elasticmachine commented Jun 1, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

taylor-swanson commented Jun 1, 2022

taylor-swanson commented Jun 2, 2022

mergify bot commented Jun 6, 2022

taylor-swanson commented Jun 7, 2022

elasticmachine commented Jun 7, 2022

taylor-swanson commented Jun 14, 2022

rdner left a comment

Choose a reason for hiding this comment

efd6 Jun 15, 2022

Choose a reason for hiding this comment

taylor-swanson Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

taylor-swanson Jun 15, 2022

Choose a reason for hiding this comment

efd6 Jun 15, 2022

Choose a reason for hiding this comment

taylor-swanson commented Jun 22, 2022

taylor-swanson commented Jun 1, 2022 •

edited

Loading

elasticmachine commented Jun 1, 2022 •

edited by jenkins-beats-ci bot

Loading

taylor-swanson Jun 15, 2022 •

edited

Loading