feat(talos-log-collector): add talos-log-collector package - #3260

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/system/talos-log-collector/README.md`:
- Around line 65-66: Update the resources table in README so the `resources.cpu`
entry matches `daemonset.yaml`: it is request-only, not request-and-limit. Keep
the `resources.memory` description unchanged, and make sure the wording for
`resources.cpu` reflects the actual behavior of the `resources` settings in
`packages/system/talos-log-collector/templates/daemonset.yaml`.

---

Nitpick comments:
In `@packages/system/talos-log-collector/tests/talos-log-collector_test.yaml`:
- Around line 88-102: Add test coverage for the security-relevant templates that
are currently untested: the ServiceAccount should assert token automount is
disabled, and the CiliumNetworkPolicy should assert ingress is restricted to the
host entity. Update the talos-log-collector test suite in
talos-log-collector_test.yaml by either expanding the suite-level templates list
or adding a separate suite that targets the ServiceAccount and
CiliumNetworkPolicy templates, using their template names to keep the checks
stable against refactors.

Code Review

This pull request introduces the talos-log-collector package, which deploys a node-local Vector DaemonSet to collect Talos machine and kernel logs and forward them to VictoriaLogs. The feedback suggests using the dig function in the ConfigMap template to safely access .Values.global.target and avoid nil pointer errors, updating the PR scope to comply with the repository's conventional commit guidelines, and adding the CriticalAddonsOnly toleration to the DaemonSet to ensure proper scheduling on control-plane nodes.

NOT LGTM — the documented Talos config does not deliver kernel logs (the package's headline promise), and the CiliumNetworkPolicy that the comment calls optional is in fact the only thing preventing log injection from any pod in the cluster.

Business context: Talos keeps machine and kernel logs inside the OS, where the existing file-tail collector cannot reach them, so this package adds a node-local receiver that Talos pushes into and that forwards to the platform's VictoriaLogs.

Blockers

B1: The required Talos config delivers service logs only — kernel/kmsg logs never arrive

File: packages/system/talos-log-collector/README.md:20-22 (and the config block at :29-35)

Issue: The README states that machine.logging "already forwards the runtime kernel (kmsg) stream as the kernel service ... so no separate KmsgLogConfig is needed". Talos does not work that way.

Evidence: Checked against Talos v1.12.0 and v1.13.0 source (v1.13.6 is what this platform ships, packages/apps/kubernetes/values.yaml:291). KmsgLogConfigController builds its destination list from exactly two sources — the talos.logging.kernel kernel command-line parameter, and cfg.Config().Runtime().KmsgLogURLs() (internal/app/machined/pkg/controllers/runtime/kmsg_log_config.go). KmsgLogURLs() is aggregated across config documents, and the only document returning a non-empty list is KmsgLogV1Alpha1, i.e. the KmsgLogConfig document (pkg/machinery/config/types/runtime/kmsg_log.go); EventSinkV1Alpha1 and WatchdogTimerV1Alpha1 both return nil. The v1alpha1 config type that owns machine.logging.destinations does not implement KmsgLogURLs at all — grepping it across pkg/machinery/config/types/v1alpha1/ returns nothing on both tags. machine.logging.destinations feeds only updateLoggingConfig → the JSON-lines service-log senders (internal/app/machined/pkg/runtime/v1alpha2/v1alpha2_controller.go:586), and the string kernel does not appear anywhere under internal/app/machined/pkg/runtime/logging/. The chart itself assumes both streams share the socket: facility is a kmsg-only field, and it is listed in _stream_fields (templates/configmap.yaml:29).

Impact: An operator applying exactly the "Talos configuration (required)" block gets service logs and nothing else. Kernel logs — including the DRBD kmsg case named in the PR description, which is the operationally interesting one here — silently never arrive, while Chart.yaml:3, the README opening, the package name and the PR title all promise them. If kernel logs did show up on the dev cluster, that node almost certainly also carried talos.logging.kernel= in its kernel args, or a KmsgLogConfig document from earlier setup; the README snippet alone cannot produce them.

Fix: Add the kmsg destination to the required-config section — a KmsgLogConfig document, or talos.logging.kernel=tcp://127.0.0.1:5170/ in the kernel args, either of which can point at the same port — or drop the kernel-log claim from the README, Chart.yaml and _stream_fields.

B2: The comment calls the network policy optional; it is the only thing preventing any pod from injecting logs

File: packages/system/talos-log-collector/templates/networkpolicy.yaml:5-8

Issue: "even without this policy the loopback hostPort is not reachable from the pod network, so its absence is safe" is false, and it contradicts the sentence three lines above it.

Evidence: Vector binds every address inside the pod network namespace — address: 0.0.0.0:{{ .Values.listenPort }} (templates/configmap.yaml:15), exposed as containerPort (templates/daemonset.yaml:49). The pod's own IP is routable from every pod in the cluster; the hostPort is irrelevant to that path. Any workload can open a TCP connection to <podIP>:5170 and push JSON lines, which the tag transform stamps with .log_source = "talos_system" and the real .node name (templates/configmap.yaml:22-24) before inserting them into the platform's VictoriaLogs. The policy is load-bearing, not defence-in-depth. It is also conditional — {{- if .Capabilities.APIVersions.Has "cilium.io/v2/CiliumNetworkPolicy" }} (networkpolicy.yaml:1); helm template with default capabilities renders only ServiceAccount, ConfigMap and DaemonSet. The isp-hosted system-bundle variant installs the noop networking variant (packages/core/platform/templates/bundles/system.yaml:32) — no Cilium, hence no cilium.io/v2 CRD — and the optional-package line this PR adds (system.yaml:229) sits in the variant-agnostic tail of that file, so the package is offerable there with no ingress protection whatsoever.

Impact: Forged log records attributed to a trusted source label and a real node name in the platform's log store — an audit and forensic integrity problem, reachable from any tenant workload wherever the policy does not render.

Fix: Correct the comment, and ship a CNI-agnostic networking.k8s.io/v1 NetworkPolicy as a fallback so the socket is never unprotected — or make Cilium a hard requirement of the package and fail rendering without it.

B3: resources.cpu is documented as a request and a limit; only a request is set

File: packages/system/talos-log-collector/values.yaml:23

Issue: ## @field {quantity} [cpu] - CPU request and limit. — but the DaemonSet sets requests.cpu with no CPU limit (templates/daemonset.yaml:53-58); only memory is request-and-limit. The wrong description propagates into values.schema.json:43 and README.md:65.

Evidence: templates/daemonset.yaml:53-58 — the limits: block contains memory only.

Impact: The generated user-facing docs and the values schema describe behaviour the chart does not implement.

Fix: Reword the annotation to "CPU request." and regenerate (make generate in the package). Omitting the CPU limit is the right call; only the description is wrong.

The package ships a helm-unittest suite, so each of the three needs a case that fails without the fix — a resources assertion for B3, and for B2 an assertion pinning what renders when the Cilium CRD is absent.

Non-blocking follow-ups

1. The loopback hostIP works only because the platform chains the portmap CNI plugin ahead of Cilium (packages/system/cilium/templates/cni.yaml: "type": "portmap", "snat": true, pulled in via values-kubeovn.yaml on the kubeovn-cilium* variants). Cilium's own eBPF hostPort implementation does not support binding a hostPort to the loopback address. The cilium, cilium-generic and cilium-kilo variants in packages/core/platform/sources/networking.yaml are not selected by any bundle today, but if one ever is, this receiver silently stops receiving. Worth stating in the DaemonSet comment.

2. Every record from the existing node agent carries tenant and cluster (packages/system/monitoring-agents/values.yaml:440-444); records from this pipeline carry neither. Same log store, two schemas — operators filtering on those fields will not see Talos logs.

3. No liveness probe on a system-node-critical DaemonSet with a hard 128Mi limit. A wedged or OOM-looping Vector is invisible and never restarted. Enabling Vector's health endpoint and probing it would let the DaemonSet self-heal.

4. values.schema.json carries no constraints: logLevel lists its valid values in prose but has no enum, and listenPort has no minimum/maximum, so 0 or 70000 validate and then render an invalid DaemonSet.

5. make update (Makefile:10-12) rewrites the Dockerfile's ARG VERSION but leaves Chart.yaml's appVersion: "0.56.0" untouched, so the two drift on the next bump.

6. The README never says why the existing node-level agent was not extended instead. The answer looks legitimate — the vendored fluent-bit chart renders only name/containerPort/protocol for extraPorts (charts/fluent-bit/templates/_pod.tpl:72-78), so a loopback receiver there would need a chart patch — but every reader will ask, and one sentence in the README would preempt it.

For what it is worth, the parts that are right are right: registration matches the hetzner-robotlb precedent exactly, install.privileged: true is genuinely required (PSA baseline forbids a non-zero hostPort), the generated schema and README are free of codegen drift, and the chart lints and its unit tests pass.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/system/talos-log-collector/README.md`:
- Around line 55-57: Remove the blank line within the blockquote warning in the
KmsgLogConfig documentation so the service-log and kernel-log statements remain
one continuous blockquote and satisfy markdownlint MD028.

In `@packages/system/talos-log-collector/templates/networkpolicy.yaml`:
- Around line 11-13: Replace the .Capabilities.APIVersions.Has gate in the
network policy template with an explicit prerequisite that verifies Cilium
policy enforcement is active, not merely that its CRD is installed. Ensure
unsupported, NOOP, or BYO networking configurations do not render an unprotected
listener; fail closed or select only a known-enforcing network path before
allowing the Cilium-specific policy.

---

Nitpick comments:
In `@packages/system/talos-log-collector/README.md`:
- Around line 8-14: Reflow the ordinary Markdown prose paragraphs in the README,
including the paragraphs at the referenced sections, so each paragraph occupies
one physical source line. Preserve meaningful line breaks in lists, blockquotes,
tables, and fenced code blocks, and do not alter the prose content.

In `@packages/system/talos-log-collector/tests/talos-log-collector_test.yaml`:
- Around line 78-86: Add a second assertion to the “destination tenant is
overridable via global.target” test that verifies Vector’s generated
configuration sets the tenant label to “tenant-ktj” alongside the existing
vlinsert hostname assertion. Keep the current destination override assertion
unchanged.

NOT LGTM — every blocker from the previous review is resolved, but the generated values.schema.json now carries a first-party image reference that the image-ref enumeration guard rejects, so the required "Unit & controller tests" check is red and the branch cannot merge as-is.

Disposition of the previous review's blockers (all resolved on f96d0ec):

• B1 (kernel/kmsg logs never arrive): resolved. The README now documents that machine.logging.destinations carries only service logs and that a separate KmsgLogConfig document is required for the kmsg stream, and the required-config block ships both (README.md:24-56). This matches how Talos builds its kmsg destination list.
• B2 (network policy wrongly called optional): resolved. The comment is corrected to state the policy is load-bearing (networkpolicy.yaml:1-10), and a CNI-agnostic default-deny networking.k8s.io/v1 fallback now renders whenever the Cilium CRD is absent — helm template with default capabilities emits it. See non-blocking note 1 for the residual noop case.
• B3 (resources.cpu documented as request-and-limit): resolved. The annotation, schema and README now read "CPU request." and a test pins that only memory carries a limit (values.yaml:32, values.schema.json:52, README.md:90, test cpu is a request only).

All six non-blocking follow-ups from the previous review are also addressed: portmap dependency documented (daemonset.yaml:28-32), tenant/cluster labels stamped with the same convention as the existing node agent (configmap.yaml:25-26; tenant=global.target, cluster=root-cluster, matching monitoring-agents/values.yaml:440,444), liveness probe added (daemonset.yaml:45-49), schema constraints added (logLevel enum, listenPort min/max), Chart.yaml appVersion synced on make update (Makefile:13), and the README explains why the existing node agent was not extended.

Blockers

B1: values.schema.json carries a first-party image ref that the enumeration guard rejects

File: packages/system/talos-log-collector/values.schema.json:34-43 (root cause in values.yaml:20-29)

Issue: The required "Unit & controller tests" check is failing. The image-ref enumeration guard greps packages/** for ghcr.io/cozystack/cozystack/ and requires every match to be either enumerated by the promote-retag rewriter or allowlisted with a reason; values.schema.json is neither, so the check exits non-zero.

Evidence: The failing job ends with files carry a first-party image ref but are neither enumerated nor allowlisted: packages/system/talos-log-collector/values.schema.json then make: *** [Makefile:167: bats-unit-tests] Error 1. This schema is the only system-package values.schema.json in the tree containing ghcr.io/cozystack/cozystack/. It lands there because values.yaml annotates the image struct (@typedef Image + @field repository/tag), so the schema generator bakes the ref into the generated default; metallb and every other package leave the image block un-annotated, so their schema never carries the ref. The guard was added to main after this branch's merge-base, which is why a local helm unittest passes while the merge check fails.

Impact: A required check is red; the branch is not mergeable until it goes green.

Fix: Drop the @typedef Image / @field annotations from the image block in values.yaml and regenerate, so the schema no longer carries the ref — this matches every other package and needs no change on main. Alternatively, add packages/system/talos-log-collector/values.schema.json to the guard's allowlist with a reason (the schema default is not a runtime ref; the runtime ref lives in values.yaml, which the rewriter already handles). Rebasing onto current main first surfaces the failure locally.

Non-blocking follow-ups

1. On the noop networking variant — the only non-Cilium variant shipped, and the one the template comment itself names — the fallback NetworkPolicy is inert, because noop enforces no NetworkPolicy at all. An operator who opts the package in there gets the log-injection socket with no protection. The template documents this honestly (networkpolicy.yaml:9-10,32-41). If protecting that path matters, make a policy-enforcing CNI a hard requirement of the package or don't offer it on noop; otherwise the current documented posture is defensible, since noop is a deliberately policy-free environment where nothing is netpol-protected.
2. The new README prose is hard-wrapped (README.md:3-14, 24-34, and the other prose paragraphs); the repository convention is one continuous line per prose paragraph. Not CI-enforced, but worth reflowing while the file is new. There is also an MD028 blank-line-in-blockquote run at README.md:55-63.
3. The tcpSocket liveness probe detects only a dead listener, not a Vector process that is alive but has stopped forwarding (for example a wedged HTTP sink). Vector's health endpoint over an HTTP probe would catch more; the current probe is an acceptable minimum.

The rest of the package is sound: registration mirrors the hetzner-robotlb opt-in precedent, install.privileged: true is genuinely required for the loopback hostPort under PSA baseline, the helm-unittest suite is substantive (14 cases pinning both NetworkPolicy branches, the resources split, the liveness probe and the tenant override), and the generated schema and README are free of codegen drift.