BUG/MEDIUM: sink: do not create one implicit ring per logger copy

A "log tcp@..." directive gets its implicit ring created from
sink_resolve_logger_buffer(), which calls sink_new_from_logger() as soon
as the target has an address (to differentiate it from log ring@...).

However, one case was overlooked: when using "log global", the same log
target is inherited everywhere, and the current code doesn't check for
this and systematically tries to recreates the sink. This seems to be
the case since 2.9 with commit a9b185f34e ("MEDIUM: log: introduce log
target"), which added dup_log_target().

sink_new_buf() returns the pre-existing sink when a buffer of that name
already exists, and all the copies carry the same generated "ring#N"
name, so the second and later copies did not create a new sink: they
added one more server to the very same one, then sink_finalize()
registered it and sink_init_forward() replaced sink->forward_task,
(leaking the previous task along with the freshly allocated forward
proxy BTW but that's secondary).

The result for a "log tcp@" in the global section with a single frontend
was 4 forwarding servers on one 16 kB ring (due to ocsp and http-client
also referencing the global logger), hence 4 connection attempts to the
log server, each receiving a full copy of every message. Worse, when more
connections than acceptable by the server would be reached, extraneous
connections fail and are repeatedly attempted, as can be seen with tcpdump
showing lots of SYN/RST pairs, and the extraneous servers that cannot be
served remain attached to the ring, preventing it from being recycled.
Because of this, running tests with a single-connection server like netcat
would only show 16 kB received (a single buffer) before stalling for
roughly as many times as there are references to the global logger, then
would not receive anything anymore until haproxy is reloaded. This would
also likely cause log duplication on the servers sometimes (e.g. after
reconnecting, where another server would start from its recovery point).

Interestingly, placing the same directive in a defaults section did not
show the bug, because only the frontend's copy is resolved there, and
the internal proxies only duplicate the loggers of the global section.

Let's lookup the sink by its ring name first and only create it when it
does not exist yet, just like the explicit ring case does a few lines
below. Ring names are generated once per parsed directive, and dup_logger()
copies the name rather than regenerating it, so distinct directives keep
one forwarding server each even when they point to the same address with
different parameters, while pure copies now correctly share the same
ring.

This must be backported to 3.0.

diff --git a/src/sink.c b/src/sink.c
index 2f98424f4cc31113aa15ca1b6d3398b48d6607a4..8d660ce71fddbf66db358b4420a8e23e0fe9326c 100644 (file)
--- a/src/sink.c
+++ b/src/sink.c
@@ -1401,7 +1401,14 @@ int sink_resolve_logger_buffer(struct logger *logger, char **msg)
 
        BUG_ON(target->type != LOG_TARGET_BUFFER || (target->flags & LOG_TARGET_FL_RESOLVED));
        if (target->addr) {
-               sink = sink_new_from_logger(logger);
+               /* The same "log" directive may exist as multiple logger copies:
+                * dup_logger() is used for "log global", either directly or via
+                * a defaults section, as well as for internal proxies. Let's be
+                * careful not to re-create a new ring for each reference!
+                */
+               sink = sink_find(target->ring_name);
+               if (!sink)
+                       sink = sink_new_from_logger(logger);
                if (!sink) {
                        memprintf(msg, "cannot be initialized (failed to create implicit ring)");
                        err_code |= ERR_ALERT | ERR_FATAL;