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

ensure syslog parser gets an EOF-terminated reader on udp receive #5297

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

joshuapare
Copy link
Contributor

PR Description

Simple fix to convert the incoming datagram into a bytes reader that the ParseStream can handle. currently it freezes up because the datagrams are sent as is without separation so the reader routine blocks until an interrupt.

Which issue(s) this PR fixes

Fixes #5197

PR Checklist

  • CHANGELOG.md updated
  • Tests updated

@joshuapare joshuapare requested a review from a team as a code owner September 26, 2023 01:27
@tpaschalis
Copy link
Member

cc @erikbaranowski do I correctly remember you'd investigated this issue in the past? Could you take a look here?

@erikbaranowski
Copy link
Contributor

This looks good. @joshuapare can we add a test to syslogtarget_test.go to demonstrate the scenario that is now working with this fix?

@rfratto
Copy link
Member

rfratto commented Nov 1, 2023

@joshuapare Do you still have time to work on this? It looks like all we need is a test for the fix, and then this is good to be merged :)

This looks good. @joshuapare can we add a test to syslogtarget_test.go to demonstrate the scenario that is now working with this fix?

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

I looked through the existing code, and I think there's enough tests to prove that this doesn't break anything, even if we don't have a test for testing this specific case, I think we can just merge this for now. If it breaks again we'll want a regression test for sure.

@rfratto rfratto enabled auto-merge (squash) November 2, 2023 15:44
Copy link
Contributor

@erikbaranowski erikbaranowski left a comment

Choose a reason for hiding this comment

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

seconded @rfratto. let's get this fix out and give it a try.

@rfratto rfratto merged commit 0a23b6d into grafana:main Nov 2, 2023
10 checks passed
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loki.source.syslog udp protocol not working
4 participants