SyslogAppender - let max message size be configurable

[vinhn]

No description provided.

[kiwigaffa]

The get/setMaxMessageBytes() methods seem potentially confusing to me because they do silent magic on the value given with a magic number (5). The RFCs (3164, 5424/5425/5426) are at least consistent in that when the use the term "Message Length" they mean just the syslog message, and NOT the UDP or TCP header. So the new variable (maxMessageBytes) is correct, but the getter/setter is confusing.

Put it this way: if I was to look at this code 6 months from now, would I wonder why we don't just use the getter on line 291? i.e. instead of

  if (byteCount <= maxMessageBytes) {

use

  if (byteCount <= getMaxMessageBytes()) {

If I made that change, this would break pretty catastrophically. If, OTOH, we didn't do the magic addition/subtraction, then the getter/setter would be fine. Alternatively, if we want to do the magic, we should use different names that won't be confusing. Either get/setMaxPacketBytes() or get/setMaxMessagePlusHeaderBytes()

We should also get rid of the magic number (5) and use a static class var (HEADER_BYTES)

We should enforce a max packet size of 64k. If I call setMaxPacketSize(128*1024), we should at least log an error and then force use of 64k as the actual limit. We could throw an exception rather than logging, or even exit. Given how important it is to get this value right, I'd be ok with going medieval on the process if the config is wrong.

Finally, we should think about enforcing a minimum maximum size, as defined in RFC 5424. Here is what it says:

    Each transport mapping defines the minimum maximum required message length 
    support, and the minimum maximum MUST be at least 480 octets in length.

Sorry to be so anal here, but I think its important we get this right, because its something a lot of our customers also face and if we're going to propose the same change for them, we need to make sure its all crystal clear and fully RFC compliant. And the changes aren't huge, just very very persnickety.

[hooverjbeaver]

By the way, I'm also OK with just hard coding it to 65530 (65535 - 5)
instead of 1019 on line 291 and not having the getter/setter or
maxMessageBytes var. Thats an even simpler change that is compliant, and
can be explained with a single comment. It does lose the flexibility of the
getter/setter, but since this value is unlikely to ever be changed, I don't
think its that big a deal.

On Wed, Apr 22, 2015 at 9:51 AM, kiwigaffa notifications@github.com wrote:

The get/setMaxMessageBytes() methods seem potentially confusing to me
because they do silent magic on the value given with a magic number (5).
The RFCs (3164, 5424/5425/5426) are at least consistent in that when the
use the term "Message Length" they mean just the syslog message, and NOT
the UDP or TCP header. So the new variable (maxMessageBytes) is correct,
but the getter/setter is confusing.

Put it this way: if I was to look at this code 6 months from now, would I
wonder why we don't just use the getter on line 291? i.e. instead of

if (byteCount <= maxMessageBytes) {

use

if (byteCount <= getMaxMessageBytes()) {

If I made that change, this would break pretty catastrophically. If, OTOH,
we didn't do the magic addition/subtraction, then the getter/setter would
be fine. Alternatively, if we want to do the magic, we should use different
names that won't be confusing. Either get/setMaxPacketBytes() or
get/setMaxMessagePlusHeaderBytes()

We should also get rid of the magic number (5) and use a static class var
(HEADER_BYTES)

We should enforce a max packet size of 64k. If I call
setMaxPacketSize(128*1024), we should at least log an error and then force
use of 64k as the actual limit. We could throw an exception rather than
logging, or even exit. Given how important it is to get this value right,
I'd be ok with going medieval on the process if the config is wrong.

Finally, we should think about enforcing a minimum maximum size, as
defined in RFC 5424. Here is what it says:

Each transport mapping defines the minimum maximum required message length
support, and the minimum maximum MUST be at least 480 octets in length.

Sorry to be so anal here, but I think its important we get this right,
because its something a lot of our customers also face and if we're going
to propose the same change for them, we need to make sure its all crystal
clear and fully RFC compliant. And the changes aren't huge, just very very
persnickety.

—
Reply to this email directly or view it on GitHub
#1 (comment).

[vinhn]

Thanks Jon. I have the same concern, but I was hoping to just make minimal changes so that I don't hack up too much of the original code and cause even more confusion. The original code itself is a bit tacky, especially with these magic numbers that are enforced in other parts of the library.

I'm not sure about enforcing a maximum. I thought the spec was 1K, and we ourselves are breaking that by going higher? Also, in reality, we know that different syslog implementations are going beyond the spec to support configurations of even larger sizes. I'm also not certain if 65K is the absolute max size.

http://en.wikipedia.org/wiki/User_Datagram_Protocol

"The practical limit for the data length which is imposed by the underlying IPv4 protocol is 65,507 bytes (65,535 − 8 byte UDP header − 20 byte IP header).[2] In IPv6 Jumbograms it is possible to have UDP packets of size greater than 65,535 bytes.[5]"

Overall, I strongly believe that as a pass-thru tool to various syslog implementations, log4j should be flexible rather strict.

Perhaps we need to take this project private first so that we can quickly make it work for us internally, and then later decide how to pretty up this thing so that external users can use it?

[kiwigaffa]

Absolutely agree that its private first, but I guess I was thinking that
its a small enough change that "prettying it up" could be done in the first
pass.

The 1k limit is from RFC 3164, which is obsoleted by 5424. I think its ok
for us to just say "we're basing this on 5424, and recognize that it does
not conform to 3164". 5424 has been around since 2009, and all modern
syslogs support it.

I get what you say about log4j being flexible rather than strict, but given
that SyslogAppender only supports IPv4 UDP (unless I'm missing something),
there is actually a hard maximum size, and some kind of safe-guard makes
sense to me.

jon

On Wed, Apr 22, 2015 at 10:25 AM, vinhn notifications@github.com wrote:

Thanks Jon. I have the same concern, but I was hoping to just make minimal
changes so that I don't hack up too much of the original code and cause
even more confusion. The original code itself is a bit tacky, especially
with these magic numbers that are enforced in other parts to the library.

I'm not sure about enforcing a maximum. I thought the spec was 1K, and we
ourselves are breaking that by going higher? Also, in reality, we know that
different syslog implementations are going beyond the spec to support
configurations of even larger sizes. I'm also not certain if 65K is the
absolute max size.

http://en.wikipedia.org/wiki/User_Datagram_Protocol

"The practical limit for the data length which is imposed by the
underlying IPv4 protocol is 65,507 bytes (65,535 − 8 byte UDP header − 20
byte IP header).[2] In IPv6 Jumbograms it is possible to have UDP packets
of size greater than 65,535 bytes.[5]"

Overall, I strongly believe that as a pass-thru tool to various syslog
implementations, log4j should be flexible rather strict.

Perhaps we need to take this project private first so that we can quickly
make it work for us internally, and then later decide how to pretty up this
thing so that external users can use it?

—
Reply to this email directly or view it on GitHub
#1 (comment).

[vinhn]

Sounds good. I'll make the change and post another request soon.