Improve syslog parser/processor error handling #31305

taylor-swanson · 2022-04-14T14:31:23Z

What does this PR do?

Use multierr to catch multiple errors during processing.
Priority field for RFC 3164 messages are now optional, as some
syslog providers will omit the priority part of the header.
If an error occurs, any fields that were already parsed will
be returned, alongside error.message.
Added flag for timestamp set, which prevents setting the event
timestamp to the zero time in case it was omitted or not parsed.

Why is it important?

We should be giving back as much data to the user as possible, even if an error occurs. This will also lay the foundation for further improvements to the parser itself. Currently, when a parsing error is encountered, the parser will stop processing and exit early. While this is fine for early EOFs, it can be problematic if we hit a more minor problem, such as a slight deviation from RFC. I attempted to change the parser to do this, but it proved to be too time consuming and complicated, at least for right now.

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

go test github.com/elastic/beats/v7/libbeat/reader/syslog
go test github.com/elastic/beats/v7/libbeat/processors/syslog

Related issues

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

- Use multierr to catch multiple errors during processing. - Priority field for RFC 3164 messages are now optional, as some syslog providers will omit the priority part of the header. - If an error occurs, any fields that were already parsed will be returned, alongside error.message. - Added flag for timestamp set, which prevents setting the event timestamp to the zero time in case it was omitted or not parsed.

…r layout

elasticmachine · 2022-04-14T15:09:49Z

💚 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-04-26T13:50:44.667+0000
Duration: 31 min 40 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3978
Skipped	915
Total	4893

💚 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!)

efd6

Initial comments.

libbeat/reader/syslog/message.go

efd6 · 2022-04-14T22:45:21Z

libbeat/reader/syslog/message.go

-	}
-	if v, ok := mapIndexToString(m.facility, facilityLabels); ok {
-		_, _ = f.Put("log.syslog.facility.name", v)
+	if m.prioritySet {


I'd replace all these , with a single nolint on the if statements

If you're referring to removing the explicit discards on the return values, I'd rather keep them in place. While it is more verbose, it makes it more clear to the reader that return values are being ignored here.

libbeat/reader/syslog/syslog.go

mergify · 2022-04-15T08:48:13Z

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-error-handling upstream/syslog-error-handling
git merge upstream/main
git push upstream syslog-error-handling

libbeat/processors/syslog/syslog_test.go

libbeat/reader/syslog/parser/parser_rfc5424.rl

elasticmachine · 2022-04-19T15:24:40Z

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

efd6 · 2022-04-19T21:30:41Z

libbeat/processors/syslog/syslog_test.go

-	if layout == time.Stamp {
-		t = t.AddDate(time.Now().In(loc).Year(), 0, 0)
-	}
+	t = t.AddDate(time.Now().In(loc).Year(), 0, 0)


t = t.AddDate(time.Now().In(loc).Year()-t.Year()+1, 0, 0) or add a comment about why this is being done (the parameter provided context, but now that is lost).

Alternatively, the parameter could come back and the two functions be merged into func mustParseTimeLoc(layout string, value string, loc *time.Location) time.Time where the loc is not used for formats that provide it (e.g. called like mustParseTime(time.RFC3339Nano, "2003-10-11T22:14:15.003Z", nil) and mustParseTime(time.Stamp, "Oct 11 22:14:15", cfgtype.MustNewTimezone("America/Chicago").Location())).

This is one of the reasons why I didn't want to remove the parameter. Context is being lost on why this is being done. What I had before wasn't perfect and only added the year when time.Stamp was used. Alternatively, I can look at the time's year and if it's zero I'll enrich it with the current year. I'd rather do that than blindly apply this to all timestamps that are seen (no need to do it if the time already has a valid year).

I like what you're suggesting in the second paragraph. I never liked getting rid of the layout parameter since meaning was lost on the caller side. I'll combine the two functions into one and update the calls, and add some doc strings as well.

This is the unfortunate consequence of including style linting in the CI failure path; it is entirely possible for a linter to push towards worse code.

efd6 · 2022-04-19T21:46:53Z

libbeat/reader/syslog/syslog.go


-	return m.fields(), m.timestamp, nil
+	return m.fields(), m.timestamp, err


Can we differentiate error states that are recoverable from error states that give us no usable information. Also add a not on the godoc to the effect that the error doesn't necessarily preclude use of the fields.

At the moment, I don't think so. The parser isn't set up to recover from an error state yet. It can either try again for the exact same result, or it tumbles out with a bunch of errors and partially filled fields, all depends on where and how it fails. That's something I plan to address in the next round of changes.

libbeat/processors/syslog/syslog_test.go

kvch · 2022-04-21T15:44:40Z

libbeat/reader/syslog/rfc3164_test.go

@@ -99,10 +110,6 @@ var parseRFC3164Cases = map[string]struct {
 		In:      "<-1>Oct 11 22:14:15 test-host this is the message",
 		WantErr: ErrPriority,
 	},
-	"err-pri-missing-brackets": {


Why did you remove this test case? It is true that priority might be missing, but what if the priority is supposed to be there but formatted incorrectly? I think this test case is still valid.

It isn't, at least in terms of checking if the priority is correct or not. The parser looks for a starting <, if it's not present, then it goes to the next state, which is the timestamp. It can do this now since priority values are now optional. Since we start with a number here, it will try to parse it as an RFC3339 timestamp, which will ultimately fail.

I'll restore the test, but it will have to switch to checking for an ErrTimestamp instead of ErrPriority for the reason stated above.

taylor-swanson · 2022-04-21T19:25:53Z

/test

taylor-swanson · 2022-04-26T13:50:10Z

/test

taylor-swanson · 2022-04-27T16:50:30Z

I'm going to abandon this PR in favor of my next round of changes that I'm working on, which are nearly done. I'll apply the discussions we've had here towards the new code as well (it was building on everything we talked about here).

taylor-swanson added enhancement :Processors Team:Security-External Integrations backport-v8.2.0 Automated backport with mergify labels Apr 14, 2022

taylor-swanson requested a review from a team April 14, 2022 14:31

taylor-swanson self-assigned this Apr 14, 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 Apr 14, 2022

taylor-swanson added 3 commits April 14, 2022 09:37

Update changelog

ea480b0

Exclude unparam lint, trips due to always receiving the same value fo…

7df28d8

…r layout

Add explanation to nolint

59fc9f5

taylor-swanson force-pushed the syslog-error-handling branch from faae2d5 to b6ebd19 Compare April 14, 2022 15:25

Remove linting directives

9f88332

taylor-swanson force-pushed the syslog-error-handling branch from b6ebd19 to 9f88332 Compare April 14, 2022 15:33

efd6 reviewed Apr 14, 2022

View reviewed changes

taylor-swanson added 3 commits April 15, 2022 09:05

review comments

5cac78a

Merge branch 'main' into syslog-error-handling

75995e5

Reformat generated files after merge

bb983f3

kvch reviewed Apr 19, 2022

View reviewed changes

libbeat/processors/syslog/syslog_test.go Outdated Show resolved Hide resolved

efd6 reviewed Apr 19, 2022

View reviewed changes

libbeat/reader/syslog/parser/parser_rfc5424.rl Outdated Show resolved Hide resolved

taylor-swanson added 2 commits April 19, 2022 08:24

Remove layout parameter from mustParseTime* functions

2ab6aa9

import format

fa50a41

taylor-swanson marked this pull request as ready for review April 19, 2022 15:24

taylor-swanson requested a review from a team as a code owner April 19, 2022 15:24

taylor-swanson requested review from cmacknz and kvch and removed request for a team April 19, 2022 15:24

taylor-swanson requested a review from a team April 19, 2022 15:35

efd6 reviewed Apr 19, 2022

View reviewed changes

taylor-swanson added 2 commits April 20, 2022 09:32

Merge mustParseTime* functions and restore layout parameter

d0d4e8a

Update docstring

90cf14a

efd6 reviewed Apr 20, 2022

View reviewed changes

libbeat/processors/syslog/syslog_test.go Show resolved Hide resolved

kvch reviewed Apr 21, 2022

View reviewed changes

Restore test and updated expected result

0207819

taylor-swanson closed this Apr 27, 2022

taylor-swanson deleted the syslog-error-handling branch May 3, 2022 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve syslog parser/processor error handling #31305

Improve syslog parser/processor error handling #31305

taylor-swanson commented Apr 14, 2022 •

edited

elasticmachine commented Apr 14, 2022 •

edited by jenkins-beats-ci bot

Build stats

Test stats 🧪

efd6 left a comment

efd6 Apr 14, 2022

taylor-swanson Apr 15, 2022

mergify bot commented Apr 15, 2022

elasticmachine commented Apr 19, 2022

efd6 Apr 19, 2022

taylor-swanson Apr 20, 2022

efd6 Apr 20, 2022

efd6 Apr 19, 2022

taylor-swanson Apr 20, 2022

efd6 Apr 20, 2022

kvch Apr 21, 2022

taylor-swanson Apr 21, 2022

taylor-swanson commented Apr 21, 2022

taylor-swanson commented Apr 26, 2022

taylor-swanson commented Apr 27, 2022


		return m.fields(), m.timestamp, nil
		return m.fields(), m.timestamp, err

Improve syslog parser/processor error handling #31305

Improve syslog parser/processor error handling #31305

Conversation

taylor-swanson commented Apr 14, 2022 • edited

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

elasticmachine commented Apr 14, 2022 • edited by jenkins-beats-ci bot

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

efd6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Apr 15, 2022

elasticmachine commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylor-swanson commented Apr 21, 2022

taylor-swanson commented Apr 26, 2022

taylor-swanson commented Apr 27, 2022

taylor-swanson commented Apr 14, 2022 •

edited

elasticmachine commented Apr 14, 2022 •

edited by jenkins-beats-ci bot