Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Filebeat] add RFC6587 framing support #23724

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

leehinman
Copy link
Contributor

@leehinman leehinman commented Jan 27, 2021

What does this PR do?

Adds support for RFC6587 framing.

  • Adds new config option "framing" for tcp & unix inputs
  • supported options are "delimiter" & rfc6587
  • delimiter is current option of newline or custom character(s)
    delimiter
  • rfc6587 adds support for octet counting and non-transparent framing
    as described in RFC6587
  • rfc6587 supports changing of framing on a frame by frame basis
  • Default is "delimiter"

Why is it important?

  • Required for Syslog inside TLS
  • Required for some RFC6587 compliant Syslog clients

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

mage test

Related issues

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 27, 2021
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 27, 2021
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The automatic detection should make this easy to use. I like the approach.

func SplitFunc(lineDelimiter []byte) bufio.SplitFunc {
// SplitFunc allows to create a `bufio.SplitFunc` based on a framing &
// delimiter provided.
func SplitFunc(framing string, lineDelimiter []byte) bufio.SplitFunc {
if len(lineDelimiter) == 0 {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this return an error for this case. Then the message can be constructed in this one place rather than in each input.

if len(lineDelimiter) == 0 {
return nil
}

ld := []byte(lineDelimiter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason not to directly use lineDelimiter?

@@ -48,6 +48,7 @@ var defaultConfig = config{
type syslogTCP struct {
tcp.Config `config:",inline"`
LineDelimiter string `config:"line_delimiter" validate:"nonzero"`
Framing string `config:"framing"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be make into a type FramingType that implements Validate() error such that config unpacking will automatically validate the values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried that using type FramingType string and when beats would start I'd get the following panic:

instance/beat.go:167    Failed due to panic.    {"panic": "reflect.nameFrom: name too long: *************************************************************************
....

Copy link
Member

@andrewkroh andrewkroh Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to implement https://pkg.go.dev/github.com/elastic/go-ucfg#StringUnpacker. Another option is to make it a type FramingType uint8 and also implement Unpack (that's probably more common in the codebase than having the type be a string).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help. Is there any benefit to doing Validate() if Unpack() does validation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. If the type itself has Unpack then no need to add another Validate step with similar logic.

- Adds new config option "framing"
- supported options are "delimiter" & rfc6587
- delimiter is current option of newline or custom character(s)
  delimiter
- rfc6587 adds support for octet counting and non-transparent framing
  as described in RFC6587
- rfc6587 supports changing of framing on a frame by frame basis
- Default is "delimiter"

Closes elastic#23663
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #23724 updated

    • Start Time: 2021-01-29T19:11:53.743+0000
  • Duration: 43 min 18 sec

  • Commit: 1f52de4

Test stats 🧪

Test Results
Failed 0
Passed 12908
Skipped 2033
Total 14941

Steps errors 1

Expand to view the steps failures

x-pack/filebeat-Lint - Install Go/Mage/Python/Docker/Terraform 1.15.7
  • Took 0 min 1 sec . View more details on here
  • Description: .ci/scripts/install-tools.sh

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 12908
Skipped 2033
Total 14941

@leehinman leehinman marked this pull request as ready for review January 29, 2021 19:57
@elasticmachine
Copy link
Collaborator

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

@leehinman leehinman added the needs_backport PR is waiting to be backported to other branches. label Jan 29, 2021
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@leehinman leehinman merged commit 5cb370e into elastic:master Feb 1, 2021
@leehinman leehinman deleted the 23663_octet_counting branch February 1, 2021 15:08
@leehinman leehinman added v7.12.0 and removed needs_backport PR is waiting to be backported to other branches. labels Feb 1, 2021
leehinman added a commit to leehinman/beats that referenced this pull request Feb 1, 2021
- Adds new config option "framing"
- supported options are "delimiter" & rfc6587
- delimiter is current option of newline or custom character(s)
  delimiter
- rfc6587 adds support for octet counting and non-transparent framing
  as described in RFC6587
- rfc6587 supports changing of framing on a frame by frame basis
- Default is "delimiter"

Closes elastic#23663

(cherry picked from commit 5cb370e)
leehinman added a commit that referenced this pull request Feb 1, 2021
- Adds new config option "framing"
- supported options are "delimiter" & rfc6587
- delimiter is current option of newline or custom character(s)
  delimiter
- rfc6587 adds support for octet counting and non-transparent framing
  as described in RFC6587
- rfc6587 supports changing of framing on a frame by frame basis
- Default is "delimiter"

Closes #23663

(cherry picked from commit 5cb370e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding support for Octet counting in TCP input
3 participants