fix(collector): only declare hostPath volumes when needed (LOG-9713)

[vparfonov]

Description

/cc @Clee2691
/assign @jcantrill

Links

• Depending on PR(s):
• GitHub issue:
• Port-forward: fix(collector): only declare hostPath volumes when needed (LOG-9713) #3385
• JIRA:
• Enhancement proposal:

Summary by CodeRabbit

• Bug Fixes
  • DaemonSet hostPath volumes are now included only for enabled log collection sources.
  • Prevents unnecessary volume mounts when container, journal, audit, Kubernetes, OpenShift, OAuth, or OVN inputs are not configured.

[coderabbitai]

📝 Walkthrough

Walkthrough

The collector now adds DaemonSet hostPath volumes only for enabled application, infrastructure, audit, Kubernetes, OpenShift, OAuth, and OVN sources. The pod-spec test provides the corresponding input specifications.

Changes

Conditional volume wiring

Layer / File(s)	Summary
Input-driven volume selection internal/collector/collector.go, internal/collector/collector_test.go	NewPodSpec conditionally adds hostPath volume groups based on forwarder inputs and audit sources. The test supplies application, infrastructure, and audit InputSpec entries.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description omits the mandatory summary of the issue, rationale, and implementation, although reviewer and approver assignments are present.	Add a concise description that explains the issue, rationale, and implementation, and retain the required reviewer, approver, and related-link sections.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely states that collector hostPath volumes are declared only when needed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-for-rh-openshift]

PR Summary by Qodo

Collector: gate hostPath volume declarations on enabled input sources

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Declare hostPath volumes only for the input sources actually enabled in the Forwarder spec.
• Align PodSpec hostPath volumes with existing conditional volumeMount logic.
• Update daemonset unit test setup to include inputs so hostPath volume assertions stay valid.

Diagram

graph TD
  A["ClusterLogForwarderSpec.inputs"] --> B["internalobs.Inputs"] --> C["Factory.NewPodSpec (DaemonSet)"] --> D[("PodSpec hostPath volumes")]
  B --> E["Factory.NewCollectorContainer"] --> F["Collector volumeMounts"]
  C --> G["DaemonSet manifest"]
  C --> H["Deployment manifest"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize volume + mount selection in one helper

• ➕ Eliminates duplication between NewPodSpec (Volumes) and NewCollectorContainer (VolumeMounts)
• ➕ Reduces risk of future drift where mounts and volumes diverge again
• ➖ More refactor churn for a security/behavior bug fix
• ➖ Would touch more code paths and may require broader test updates

2. Keep declaring all hostPath volumes but rely on conditional mounts

• ➕ Very small code change; fewer branching paths in PodSpec generation
• ➖ Still requests unnecessary hostPath access on the pod spec (the core issue)
• ➖ Increases security/PSA friction even when sources are unused

Recommendation: The PR’s approach is the right minimal fix: gate hostPath volume declarations on the same input conditions already used for volumeMounts, reducing unnecessary host filesystem exposure. Consider a follow-up refactor to compute a single "required sources"/"required volumes" set used by both PodSpec and container construction to prevent future drift.

Files changed (2) +38 / -10

Bug fix (1) +33 / -10

collector.goConditionally add hostPath volumes based on enabled inputs +33/-10

Conditionally add hostPath volumes based on enabled inputs

• Replaces unconditional hostPath volume declarations for container, journal, and multiple audit sources with per-source conditional appends driven by internalobs.Inputs. This ensures collectors don’t request hostPath access unless the corresponding input source is enabled.

internal/collector/collector.go

Tests (1) +5 / -0

collector_test.goProvide input specs in daemonset test fixture +5/-0

Provide input specs in daemonset test fixture

• Updates the daemonset test setup to include application, infrastructure, and audit inputs so hostPath volume expectations remain consistent with the new conditional volume logic.

internal/collector/collector_test.go

[coderabbitai]

🧹 Nitpick comments (1)

internal/collector/collector_test.go (1)

80-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a negative volume-selection test.

This fixture enables every hostPath group. Empty Infrastructure.Sources and Audit.Sources mean all sources. The existing test therefore checks only the positive case. A regression that restores unconditional hostPath volumes could still pass.

Add cases with no inputs and with selected source lists. Assert that both podSpec.Volumes and collector.VolumeMounts omit disabled source names.

🤖 Prompt for AI Agents

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

In `@internal/collector/collector_test.go` around lines 80 - 84, Extend the
collector volume-selection tests around the existing Inputs fixture with cases
for no inputs and explicitly selected infrastructure/audit sources. Assert that
disabled source names are absent from both podSpec.Volumes and
collector.VolumeMounts, while preserving coverage for enabled sources.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@internal/collector/collector_test.go`:
- Around line 80-84: Extend the collector volume-selection tests around the
existing Inputs fixture with cases for no inputs and explicitly selected
infrastructure/audit sources. Assert that disabled source names are absent from
both podSpec.Volumes and collector.VolumeMounts, while preserving coverage for
enabled sources.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f453f819-2aae-4ff4-927d-5571b52a6cd7

📥 Commits

Reviewing files that changed from the base of the PR and between 16b2d68 and a0e539c.

📒 Files selected for processing (2)

• internal/collector/collector.go
• internal/collector/collector_test.go

[qodo-for-rh-openshift]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[openshift-ci]

@vparfonov: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-using-bundle	a0e539c	link	false	/test e2e-using-bundle

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[jcantrill]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, vparfonov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment