Add log rotation functionality as alternative to log truncation by jnovy · Pull Request #596 · containers/conmon

jnovy · 2025-09-11T12:53:09Z

This commit implements comprehensive log rotation functionality for conmon:

Add --log-rotate and --log-max-files CLI options
Implement rotate_k8s_file() and shift_backup_files() functions
Add comprehensive BATS test suite covering rotation and truncation scenarios
Maintain backward compatibility with existing truncation behavior
Update existing tests to work with new functionality

The log rotation creates backup files (.1, .2, etc.) instead of truncating when --log-rotate is enabled, providing better log preservation.

Fixes: #211

packit-as-a-service · 2025-09-11T13:03:12Z

Ephemeral COPR build failed. @containers/packit-build please check.

jankaluza · 2025-09-11T17:52:02Z

I'll review that one tomorrow morning, FYI.

jankaluza

I will give it one more go later, but so far I've finished.

jankaluza · 2025-09-12T04:19:00Z

src/cli.c

+	{"log-rotate", 0, 0, G_OPTION_ARG_NONE, &opt_log_rotate, "Enable log rotation instead of truncation when log-size-max is reached",
+	 NULL},
+	{"log-max-files", 0, 0, G_OPTION_ARG_INT, &opt_log_max_files, "Number of backup log files to keep (default: 1)", NULL},
+	{"log-allowlist-dirs", 0, 0, G_OPTION_ARG_STRING, &opt_log_allowlist_dirs, "Colon-separated list of allowed log directories", NULL},


I think this one needs a documentation in conmon.8.md.

jankaluza · 2025-09-12T04:27:44Z

src/cli.c

+		fprintf(stderr, "conmon: log-max-files must be non-negative, got %d\n", opt_log_max_files);
+		exit(EXIT_FAILURE);
+	}
+	if (opt_log_max_files > 1000) {


Strictly speaking, the limit here is INT_MAX. I understand that 1000 should be enough for everyone, but personally I do not like the artificial limits like that. If someone wants to have 10000 files, why not?

This is definitely not a blocker and my subjective opinion. Just something to consider.

jankaluza · 2025-09-12T04:31:20Z

src/cli.c

+	{"log-rotate", 0, 0, G_OPTION_ARG_NONE, &opt_log_rotate, "Enable log rotation instead of truncation when log-size-max is reached",
+	 NULL},
+	{"log-max-files", 0, 0, G_OPTION_ARG_INT, &opt_log_max_files, "Number of backup log files to keep (default: 1)", NULL},
+	{"log-allowlist-dirs", 0, 0, G_OPTION_ARG_STRING, &opt_log_allowlist_dirs, "Colon-separated list of allowed log directories", NULL},


I'm also thinking if we should use G_OPTION_ARG_STRING_ARRAY here to stay consistent with the rest of the options. The log-allowlist-dirs is the first option where we use colon-separated values while other options allowing multiple values use G_OPTION_ARG_STRING_ARRAY. What do you think?

jankaluza · 2025-09-12T05:02:45Z

src/ctr_logging.c

+		return -1;
+
+	/* Check for embedded null bytes */
+	if (memchr(path, '\0', strlen(path)))


I think this is redundant. The strlen(path) returns length up to first 0, so memchr will never find anything.

jankaluza · 2025-09-12T05:13:54Z

src/ctr_logging.c

+		}
+
+		/* Validate canonical path doesn't escape filesystem boundaries */
+		if (strstr(canonical_path, "/../") || g_str_has_suffix(canonical_path, "/..") || strcmp(canonical_path, "..") == 0


I understand the extra caution here, but after the realpath, the canonical_path will not have any directory traversals. That should be the point of that function. I'm not against keeping it here, but it does not give you any extra safety I think.

jankaluza · 2025-09-12T05:15:49Z

src/ctr_logging.c

+	gboolean had_errors = FALSE;
+
+	/* Enhanced bounds checking to prevent integer overflow and underflow */
+	if (opt_log_max_files <= 0 || opt_log_max_files > 1000) {


Use the INT_MAX here as well or replace 1000 with definition so it can be later changed in single place if needed.

jnovy · 2025-09-15T13:58:31Z

Updated, PTAL @jankaluza

jankaluza · 2025-09-18T12:05:24Z

test/06-log-management.bats

+
+    run_conmon --cid "$CTR_ID" --cuuid "$CTR_ID" --runtime "$VALID_PATH" \
+        --log-path "k8s-file:$allowed_log" \
+        --log-allowlist-dirs "$allowed_dir" \


Can you also test second --log-allowlist-dirs?

--log-allowlist-dirs "$allowed_dir" \ --log-allowlist-dirs /foo

So we know that multiple options are enabled and /foo does not override the first one.

jankaluza · 2025-09-18T12:05:55Z

src/cli.c

+	{"log-rotate", 0, 0, G_OPTION_ARG_NONE, &opt_log_rotate, "Enable log rotation instead of truncation when log-size-max is reached",
+	 NULL},
+	{"log-max-files", 0, 0, G_OPTION_ARG_INT, &opt_log_max_files, "Number of backup log files to keep (default: 1)", NULL},
+	{"log-allowlist-dirs", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_log_allowlist_dirs, "Allowed log directories", NULL},


Reading it now, maybe make it singular to be consistent with other options?

log-allowlist-dir

jankaluza · 2025-09-18T12:06:20Z

docs/conmon.8.md

+Enable log rotation instead of log truncation. When enabled, log files are rotated
+with numbered suffixes (.1, .2, etc.) instead of being truncated when they reach
+the maximum size.
+


I think the log-allowlist-dir should still be documented here.

This commit implements comprehensive log rotation functionality for conmon: - Add --log-rotate option to enable rotation instead of truncation - Add --log-max-files option to specify number of backup files (default: 1) - Add --log-allowlist-dir option to specify allowed log directories - Implement rotate_k8s_file() and shift_backup_files() functions - Add comprehensive BATS test suite covering rotation and truncation scenarios - Maintain backward compatibility with existing truncation behavior - Update existing tests to work with new functionality The log rotation creates backup files (.1, .2, etc.) instead of truncating when --log-rotate is enabled, providing better log preservation. Fixes: containers#211 Signed-off-by: Jindrich Novy <jnovy@redhat.com>

jnovy · 2025-09-18T13:45:17Z

Updated the option name, commit description and man page, @jankaluza PTAL

jankaluza · 2025-09-22T07:35:47Z

LGTM.

I think this is OK from the code point of view and the test coverage is also solid.

jnovy requested a review from rhatdan September 11, 2025 12:53

jnovy force-pushed the 211 branch from c945daa to 16cdf47 Compare September 11, 2025 13:01

jnovy force-pushed the 211 branch from 16cdf47 to 3fd7075 Compare September 11, 2025 13:46

jnovy requested a review from haircommander September 11, 2025 15:59

jankaluza reviewed Sep 12, 2025

View reviewed changes

jnovy force-pushed the 211 branch from 3fd7075 to de44c12 Compare September 15, 2025 13:58

jnovy force-pushed the 211 branch from de44c12 to 0ad2a97 Compare September 18, 2025 12:00

jankaluza reviewed Sep 18, 2025

View reviewed changes

jnovy force-pushed the 211 branch from 0ad2a97 to 61c8da9 Compare September 18, 2025 13:44

jnovy merged commit d3d5d68 into containers:main Sep 22, 2025
34 checks passed

Conversation

jnovy commented Sep 11, 2025

Uh oh!

packit-as-a-service bot commented Sep 11, 2025

Uh oh!

jankaluza commented Sep 11, 2025

Uh oh!

jankaluza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnovy commented Sep 15, 2025

Uh oh!

jankaluza Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnovy commented Sep 18, 2025

Uh oh!

jankaluza commented Sep 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jankaluza Sep 18, 2025 •

edited

Loading