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

Implement rfc6587 auto detection #154

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

bazsi
Copy link
Member

@bazsi bazsi commented Jun 9, 2024

This is a pretty common misconfiguration that should be handled automatically.

Open questions were resolved this way:

  • syslog(transport(tcp/tls)) will default to enable framing detection
  • if you want to require framing transport(force-framed) will do that, but I doubt anyone needs that, so let's not document it.
  • auto is only the name of the class, it's not user visible anymore.
  • testcases are added

Comment on lines +115 to +109
self->proto_impl = _construct_detected_proto(self, detect_buffer, rc);
if (self->proto_impl)
{
/* transport is handed over to the new proto */
self->super.transport = NULL;
return LPS_SUCCESS;
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we do a handshake after we autodetected the proto?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we are doing just that. This part is actually the handshake for LogProtoAutoServer and once we finish our own handshake we delegate that logic back to the proto implementation.

+  /* allow the impl to do its handshake */
+  if (self->proto_impl)
+    return log_proto_server_handshake(self->proto_impl, handshake_finished);

octet-count based framing to avoid confusion that stems from the sender
using a different protocol to the server. This is automatically enabled for
both the `transport(tcp)` and the `transport(tls)` settings. You can disable
auto-detection by using `transport(text)` or `transport(framed)`.
Copy link
Member

Choose a reason for hiding this comment

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

We disable auto detection with transport(force-framed), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. syslog() style sources default to "framed" that enables auto detection
  2. network() style sources default to "text" which does no auto detection and assumes there's no framing
  3. syslog(transport(text)) disables auto detection and disables framing
  4. syslog(transport(force-framed)) disables auto detection and requires framing

So the defaults are sane, network/tcp/udp would remain as is. syslog() would accept both with or without framing, thereby allowing the use of the syslog source driver for all cases.

Instead of having to call an extra virtual method for each I/O event,
bundle this information into the handshake call itself and track whether
handshake is in progress in the caller.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
…e is 0

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
…lected

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi bazsi force-pushed the implement-rfc6587-auto-detection branch from 902b563 to 6c992ba Compare August 10, 2024 15:26
@MrAnno MrAnno self-requested a review August 10, 2024 19:28
@bazsi
Copy link
Member Author

bazsi commented Sep 18, 2024

This should wait until #156 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants