Skip to content

fix(logcollection): stop expanding explicitly empty CommonLogs lists to defaults - #332

Merged
wendyz-am merged 3 commits into
aws:mainfrom
matcris81:fix-commonlogs-default-expansion
Oct 27, 2026
Merged

wendyz-am merged 3 commits into
aws:mainfrom
matcris81:fix-commonlogs-default-expansion

Conversation

@matcris81

@matcris81 matcris81 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

An explicitly empty list ([]string{}) in the category map means 'collect none of this kind' but was expanded to the full defaults, so category-scoped captures over-collected (e.g. Networking captured all of /var/log/messages) and /var/log files were copied by 4 instances under All. Unset (nil) now means defaults, empty means none. user-data.log is added to Base's list, since it was previously only collected via the expansion. A contract test pins what each category's CommonLogs collectors capture.

Issue #, if available:
None — behaviour discussed with maintainers (raised with @mselim00 / Prasad: whether category-scoped captures expanding empty lists to defaults was intended)

Description of changes:
CommonLogs.Collect treated a non-nil empty list ([]string{}) as "use the defaults", but the category map in pkg/logcollection/collectors.go passes []string{} to mean "collect none of this kind". As a result, category-scoped captures over-collected: a categories: [Networking] NodeDiagnostic also collected every default /var/log file (full messages, syslog, cloud-init logs), Base collected all default pod-log patterns, and under categories: [All] the same /var/log files were read and copied by 4 separate CommonLogs instances (redundant I/O, and a log-rotation race deciding which snapshot survives).

This change inverts the sentinel to the idiomatic form: unset (nil) means defaults, explicitly empty means none. The category map literals now mean what they say, and a zero-value &CommonLogs{} collects the defaults instead of silently collecting nothing. user-data.log is added to Base's explicit list, since it was previously only collected via the accidental expansion and would otherwise drop out of every bundle.

Testing Done:
New pkg/logcollection/category_contract_test.go pins the contract per category against a fake host root: Base collects /var/log files (including user-data.log) and no pod logs; Device/Networking/System collect only their pod allowlists; Runtime collects neither; exactly one CommonLogs instance under All collects the /var/log files. Full go test ./pkg/logcollection/ ./pkg/log_collector/... passes. Before the fix, the same harness confirmed the defect empirically (Networking captured messages; 4 instances copied it under All).

What customers will see:
[Networking], [Device] or [System] on their own no longer include the default /var/log files (messages, syslog, cloud-init logs and so on). Those come from [Base].
[Base] no longer includes pod logs. Pod logs come from the category that owns each component.
[All] (the default) collects the same content as before, and the /var/log files are read once instead of by up to four collectors.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

An explicitly empty list ([]string{}) in the category map means 'collect
none of this kind' but was expanded to the full defaults, so category-scoped
captures over-collected (e.g. Networking captured all of /var/log/messages)
and /var/log files were copied by 4 instances under All. Unset (nil) now
means defaults, empty means none. user-data.log is added to Base's list,
since it was previously only collected via the expansion. A contract test
pins what each category's CommonLogs collectors capture.
@matcris81
matcris81 requested a review from a team as a code owner October 26, 2026 00:26
@wendyz-am

wendyz-am commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Thank you for this PR. Two notes though:

  1. TestLogCollectorAccessor in accessor_test.go (not in this diff) runs &collect.CommonLogs{} with Root: "/". That used to collect nothing. With this change it copies the real host's default /var/log files and matching pod logs. Could we drop it from that list or use a temp root?
  2. Please add a short note to the description about what changes the customers will see:[Networking], [Device] and [System] alone no longer include the default /var/log files, and [Base] no longer includes pod logs.

With unset lists now meaning the defaults, &collect.CommonLogs{} copies
the default /var/log files and pod logs. Run it against a temp root so
the test doesn't read the build host's logs.
@matcris81

Copy link
Copy Markdown
Contributor Author

Thanks! Both done:
TestLogCollectorAccessor now runs CommonLogs against a temp root instead of /, so it no longer reads the build host's logs (3e012f1).
Added a "What customers will see" section to the description.

@wendyz-am wendyz-am changed the title Fix CommonLogs default expansion for explicitly empty lists fix(logcollection): stop expanding explicitly empty CommonLogs lists to defaults Oct 27, 2026
@wendyz-am
wendyz-am merged commit e06a253 into aws:main Oct 27, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants