diff mbox series

[conntrack-tools,v2,2/2] conntrackd: external_inject: report inject issues as warning

Message ID 160707895903.12188.7283813676910230247.stgit@endurance
State Accepted
Delegated to: Pablo Neira
Headers show
Series [conntrack-tools,v2,1/2] .gitignore: add nano swap file | expand

Commit Message

Arturo Borrero Gonzalez Dec. 4, 2020, 10:50 a.m. UTC 
In busy firewalls that run conntrackd in NOTRACK with both internal and external caches disabled,
external_inject can get lots of traffic. In case of issues injecting or updating conntrack entries
a log entry will be generated, the infamous inject-addX, inject-updX messages.

But there is nothing end users can do about this error message, which is purely internal. This
patch is basically cosmetic, relaxing the message from ERROR to WARNING. The information reported
is also extended a bit. The idea is to leave ERROR messages to issues that would *stop* or
*prevent* conntrackd from working at all.

Another nice thing to do in the future is to rate-limit this message, which is generated in the
data path and can easily fill log files. But ideally, the actual root cause would be fixed, and
there would be no WARNING message reported at all, meaning that all conntrack entries are smoothly
synced between the firewalls in the cluster. We can work on that later.

Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
---
v2: fix commit message. Add the same pattern for expectation functions. Add more informative error
messages in general.

 src/external_inject.c |   28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

Comments

Pablo Neira Ayuso Dec. 8, 2020, 11:50 a.m. UTC | #1 
On Fri, Dec 04, 2020 at 11:50:25AM +0100, Arturo Borrero Gonzalez wrote:
> In busy firewalls that run conntrackd in NOTRACK with both internal and external caches disabled,
> external_inject can get lots of traffic. In case of issues injecting or updating conntrack entries
> a log entry will be generated, the infamous inject-addX, inject-updX messages.
> 
> But there is nothing end users can do about this error message, which is purely internal. This
> patch is basically cosmetic, relaxing the message from ERROR to WARNING. The information reported
> is also extended a bit. The idea is to leave ERROR messages to issues that would *stop* or
> *prevent* conntrackd from working at all.
> 
> Another nice thing to do in the future is to rate-limit this message, which is generated in the
> data path and can easily fill log files. But ideally, the actual root cause would be fixed, and
> there would be no WARNING message reported at all, meaning that all conntrack entries are smoothly
> synced between the firewalls in the cluster. We can work on that later.

Also applied.
diff mbox series

Patch

diff --git a/src/external_inject.c b/src/external_inject.c
index 0ad3478..920d7c4 100644
--- a/src/external_inject.c
+++ b/src/external_inject.c
@@ -76,12 +76,14 @@  retry:
 				}
 			}
 			external_inject_stat.add_fail++;
-			dlog(LOG_ERR, "inject-add1: %s", strerror(errno));
+			dlog(LOG_WARNING,
+			     "could not add new ct entry, even when deleting it first: %s",
+			     strerror(errno));
 			dlog_ct(STATE(log), ct, NFCT_O_PLAIN);
 			return;
 		}
 		external_inject_stat.add_fail++;
-		dlog(LOG_ERR, "inject-add2: %s", strerror(errno));
+		dlog(LOG_WARNING, "could not add new ct entry: %s", strerror(errno));
 		dlog_ct(STATE(log), ct, NFCT_O_PLAIN);
 	} else {
 		external_inject_stat.add_ok++;
@@ -102,7 +104,9 @@  static void external_inject_ct_upd(struct nf_conntrack *ct)
 	if (errno == ENOENT) {
 		if (nl_create_conntrack(inject, ct, 0) == -1) {
 			external_inject_stat.upd_fail++;
-			dlog(LOG_ERR, "inject-upd1: %s", strerror(errno));
+			dlog(LOG_WARNING,
+			     "could not update ct entry, even if creating it instead: %s",
+			     strerror(errno));
 			dlog_ct(STATE(log), ct, NFCT_O_PLAIN);
 		} else {
 			external_inject_stat.upd_ok++;
@@ -117,7 +121,9 @@  static void external_inject_ct_upd(struct nf_conntrack *ct)
 	if (ret == 0 || (ret == -1 && errno == ENOENT)) {
 		if (nl_create_conntrack(inject, ct, 0) == -1) {
 			external_inject_stat.upd_fail++;
-			dlog(LOG_ERR, "inject-upd2: %s", strerror(errno));
+			dlog(LOG_WARNING,
+			     "could not update ct entry, even when deleting it first: %s",
+			     strerror(errno));
 			dlog_ct(STATE(log), ct, NFCT_O_PLAIN);
 		} else {
 			external_inject_stat.upd_ok++;
@@ -125,7 +131,7 @@  static void external_inject_ct_upd(struct nf_conntrack *ct)
 		return;
 	}
 	external_inject_stat.upd_fail++;
-	dlog(LOG_ERR, "inject-upd3: %s", strerror(errno));
+	dlog(LOG_WARNING, "could not update ct entry: %s", strerror(errno));
 	dlog_ct(STATE(log), ct, NFCT_O_PLAIN);
 }
 
@@ -134,7 +140,7 @@  static void external_inject_ct_del(struct nf_conntrack *ct)
 	if (nl_destroy_conntrack(inject, ct) == -1) {
 		if (errno != ENOENT) {
 			external_inject_stat.del_fail++;
-			dlog(LOG_ERR, "inject-del: %s", strerror(errno));
+			dlog(LOG_WARNING, "could not destroy ct entry: %s", strerror(errno));
 			dlog_ct(STATE(log), ct, NFCT_O_PLAIN);
 		}
 	} else {
@@ -200,12 +206,15 @@  retry:
 				}
 			}
 			exp_external_inject_stat.add_fail++;
-			dlog(LOG_ERR, "inject-add1: %s", strerror(errno));
+			dlog(LOG_WARNING,
+			     "could not add new exp entry, even when deleting it first: %s",
+			     strerror(errno));
 			dlog_exp(STATE(log), exp, NFCT_O_PLAIN);
 			return;
 		}
 		exp_external_inject_stat.add_fail++;
-		dlog(LOG_ERR, "inject-add2: %s", strerror(errno));
+		dlog(LOG_WARNING,
+		     "could not add new exp entry: %s", strerror(errno));
 		dlog_exp(STATE(log), exp, NFCT_O_PLAIN);
 	} else {
 		exp_external_inject_stat.add_ok++;
@@ -217,7 +226,8 @@  static void external_inject_exp_del(struct nf_expect *exp)
 	if (nl_destroy_expect(inject, exp) == -1) {
 		if (errno != ENOENT) {
 			exp_external_inject_stat.del_fail++;
-			dlog(LOG_ERR, "inject-del: %s", strerror(errno));
+			dlog(LOG_WARNING,
+                             "could not delete exp entry: %s", strerror(errno));
 			dlog_exp(STATE(log), exp, NFCT_O_PLAIN);
 		}
 	} else {