BUG/MEDIUM: counters: flush content counters after each request

One year ago, commit 5d5b5d8 ("MEDIUM: proto_tcp: add support for tracking
L7 information") brought support for tracking L7 information in tcp-request
content rules. Two years earlier, commit 0a4838c ("[MEDIUM] session-counters:
correctly unbind the counters tracked by the backend") used to flush the
backend counters after processing a request.

While that earliest patch was correct at the time, it became wrong after
the second patch was merged. The code does what it says, but the concept
is flawed. "TCP request content" rules are evaluated for each HTTP request
over a single connection. So if such a rule in the frontend decides to
track any L7 information or to track L4 information when an L7 condition
matches, then it is applied to all requests over the same connection even
if they don't match. This means that a rule such as :

     tcp-request content track-sc0 src if { path /index.html }

will count one request for index.html, and another one for each of the
objects present on this page that are fetched over the same connection
which sent the initial matching request.

Worse, it is possible to make the code do stupid things by using multiple
counters:

     tcp-request content track-sc0 src if { path /foo }
     tcp-request content track-sc1 src if { path /bar }

Just sending two requests first, one with /foo, one with /bar, shows
twice the number of requests for all subsequent requests. Just because
both of them persist after the end of the request.

So the decision to flush backend-tracked counters was not the correct
one. In practice, what is important is to flush countent-based rules
since they are the ones evaluated for each request.

Doing so requires new flags in the session however, to keep track of
which stick-counter was tracked by what ruleset. A later change might
make this easier to maintain over time.

This bug is 1.5-specific, no backport to stable is needed.

doc/configuration.txt

@@ -6918,11 +6918,14 @@ tcp-request content <action> [{if | unless} <condition>]
   is that "tcp-request content" rules can make use of contents to take a
   decision. Most often, these decisions will consider a protocol recognition or
   validity. The second difference is that content-based rules can be used in
-  both frontends and backends. In frontends, they will be evaluated upon new
-  connections. In backends, they will be evaluated once a session is assigned
-  a backend. This means that a single frontend connection may be evaluated
-  several times by one or multiple backends when a session gets reassigned
-  (for instance after a client-side HTTP keep-alive request).
+  both frontends and backends. In case of HTTP keep-alive with the client, all
+  tcp-request content rules are evaluated again, so haproxy keeps a record of
+  what sticky counters were assigned by a "tcp-request connection" versus a
+  "tcp-request content" rule, and flushes all the content-related ones after
+  processing an HTTP request, so that they may be evaluated again by the rules
+  being evaluated again for the next request. This is of particular importance
+  when the rule tracks some L7 information or when it is conditionned by an
+  L7-based ACL, since tracking may change between requests.
 
   Content-based rules are evaluated in their exact declaration order. If no
   rule matches or if there is no rule, the default action is to accept the
@@ -6937,16 +6940,12 @@ tcp-request content <action> [{if | unless} <condition>]
   They have the same meaning as their counter-parts in "tcp-request connection"
   so please refer to that section for a complete description.
 
-  Also, it is worth noting that if sticky counters are tracked from a rule
-  defined in a backend, this tracking will automatically end when the session
-  releases the backend. That allows per-backend counter tracking even in case
-  of HTTP keep-alive requests when the backend changes. This makes a subtle
-  difference because tracking rules in "frontend" and "listen" section last for
-  all the session, as opposed to the backend rules. The difference appears when
-  some layer 7 information is tracked. While there is nothing mandatory about
-  it, it is recommended to use the track-sc0 pointer to track per-frontend
-  counters and track-sc1 to track per-backend counters, but this is just a
-  guideline and all counters may be used everywhere.
+  While there is nothing mandatory about it, it is recommended to use the
+  track-sc0 in "tcp-request connection" rules, track-sc1 for "tcp-request
+  content" rules in the frontend, and track-sc2 for "tcp-request content"
+  rules in the backend, because that makes the configuration more readable
+  and easier to troubleshoot, but this is just a guideline and all counters
+  may be used everywhere.
 
   Note that the "if/unless" condition is optional. If no condition is set on
   the action, it is simply performed unconditionally. That can be useful for
@@ -6957,7 +6956,9 @@ tcp-request content <action> [{if | unless} <condition>]
   contents of a buffer before extracting the required data. If the buffered
   contents do not parse as a valid HTTP message, then the ACL does not match.
   The parser which is involved there is exactly the same as for all other HTTP
-  processing, so there is no risk of parsing something differently.
+  processing, so there is no risk of parsing something differently. In an HTTP
+  backend connected to from an HTTP frontend, it is guaranteed that HTTP
+  contents will always be immediately present when the rule is evaluated first.
 
   Tracking layer7 information is also possible provided that the information
   are present when the rule is processed. The current solution for making the

include/common/defaults.h

@@ -75,8 +75,8 @@
 #endif
 
 // max # of stick counters per session (at least 3 for sc0..sc2)
-// Some changes are needed in TCP_ACT_TRK_SC* and SN_BE_TRACK_SC* if more
-// values are required.
+// Some changes are needed in TCP_ACT_TRK_SC*, SN_BE_TRACK_SC*, and
+// SN_CT_TRACK_SC* if more values are required.
 #ifndef MAX_SESS_STKCTR
 #define MAX_SESS_STKCTR 3
 #endif

include/proto/session.h

@@ -77,23 +77,23 @@ static inline void session_store_counters(struct session *s)
 	}
 }
 
-/* Remove the refcount from the session counters tracked only by the backend if
+/* Remove the refcount from the session counters tracked at the content level if
  * any, and clear the pointer to ensure this is only performed once. The caller
  * is responsible for ensuring that the pointer is valid first.
  */
-static inline void session_stop_backend_counters(struct session *s)
+static inline void session_stop_content_counters(struct session *s)
 {
 	void *ptr;
 	int i;
 
-	if (likely(!(s->flags & SN_BE_TRACK_ANY)))
+	if (likely(!(s->flags & SN_CT_TRACK_ANY)))
 		return;
 
 	for (i = 0; i < MAX_SESS_STKCTR; i++) {
 		if (!s->stkctr[i].entry)
 			continue;
 
-		if (!(s->flags & (SN_BE_TRACK_SC0 << i)))
+		if (!(s->flags & (SN_CT_TRACK_SC0 << i)))
 			continue;
 
 		ptr = stktable_data_ptr(s->stkctr[i].table, s->stkctr[i].entry, STKTABLE_DT_CONN_CUR);
@@ -103,7 +103,7 @@ static inline void session_stop_backend_counters(struct session *s)
 		stksess_kill_if_expired(s->stkctr[i].table, s->stkctr[i].entry);
 		s->stkctr[i].entry = NULL;
 	}
-	s->flags &= ~SN_BE_TRACK_ANY;
+	s->flags &= ~SN_CT_TRACK_ANY;
 }
 
 /* Increase total and concurrent connection count for stick entry <ts> of table

include/types/session.h

@@ -98,6 +98,11 @@
 #define SN_BE_TRACK_SC2 0x00800000	/* backend tracks stick-counter 2 */
 #define SN_BE_TRACK_ANY 0x00E00000      /* union of all SN_BE_TRACK_* above */
 
+#define SN_CT_TRACK_SC0 0x01000000      /* stick-counter 0 tracked at the content level */
+#define SN_CT_TRACK_SC1 0x02000000      /* stick-counter 1 tracked at the content level */
+#define SN_CT_TRACK_SC2 0x04000000      /* stick-counter 2 tracked at the content level */
+#define SN_CT_TRACK_ANY 0x07000000      /* union of all SN_CT_TRACK_* above */
+
 
 /* WARNING: if new fields are added, they must be initialized in session_accept()
  * and freed in session_free() !

src/proto_http.c

@@ -4312,7 +4312,7 @@ void http_end_txn_clean_session(struct session *s)
 
 	s->logs.t_close = tv_ms_elapsed(&s->logs.tv_accept, &now);
 	session_process_counters(s);
-	session_stop_backend_counters(s);
+	session_stop_content_counters(s);
 
 	if (s->txn.status) {
 		int n;

src/proto_tcp.c

@@ -1029,6 +1029,7 @@ int tcp_inspect_request(struct session *s, struct channel *req, int an_bit)
 
 				if (key && (ts = stktable_get_entry(t, key))) {
 					session_track_stkctr(&s->stkctr[tcp_trk_idx(rule->action)], t, ts);
+					s->flags |= SN_CT_TRACK_SC0 << tcp_trk_idx(rule->action);
 					if (s->fe != s->be)
 						s->flags |= SN_BE_TRACK_SC0 << tcp_trk_idx(rule->action);
 				}