fix(validation): allow empty sources for audit and infrastructure inputs#3323
fix(validation): allow empty sources for audit and infrastructure inputs#3323Clee2691 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR changes how missing or empty audit/infrastructure input sources are handled: ChangesReserved source defaults and validation relaxation
Estimated code review effort: 2 (Simple) | ~12 minutes Related PRs: None identified. Suggested labels: validation, observability, lokistack Suggested reviewers: None identified. 🐰 A hop, a skip, an empty list— 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoFix validation to allow empty sources for audit/infrastructure inputs
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Code Review by Qodo
Context used✅ Compliance rules (platform):
9 rules 1. OTLP panic with empty sources
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
| return []metav1.Condition{ | ||
| internalobs.NewConditionFromPrefix(obs.ConditionTypeValidInputPrefix, spec.Name, true, obs.ReasonValidationSuccess, fmt.Sprintf("input %q is valid", spec.Name)), | ||
| } |
There was a problem hiding this comment.
1. Otlp panic with empty sources 🐞 Bug ☼ Reliability
ValidateAudit now accepts empty/nil Sources, but Inputs.InputSources returns an empty list for audit when Sources is omitted/empty, and the LokiStack OTEL migration passes that list to otlp.New. otlp.New panics when the provided otlpLogSourcesOption is empty, crashing config generation for audit inputs relying on the new "all sources" default.
Agent Prompt
### Issue description
Allowing empty/nil audit `sources` (meaning "all sources") now permits configurations where `observability.Inputs(...).InputSources(obs.InputTypeAudit)` returns an empty slice, which is then passed into the LokiStack OpenTelemetry migration path as `otlpLogSourcesOption`. The OTLP generator panics when that option is empty.
### Issue Context
- `ValidateAudit` no longer rejects empty sources, so empty/nil becomes a supported state.
- `Inputs.InputSources()` currently just stringifies `i.Audit.Sources` and does not default empty -> all.
- LokiStack OTEL migration sets `op[otlp.OtlpLogSourcesOption]` to that value.
- `otlp.New()` panics if `otlpLogSourcesOption` is an empty list.
### Fix
Make the “empty/nil sources means all sources” semantic true in the code that computes sources for OTLP migration. Two acceptable approaches:
1) **Preferred (central fix):** Update `internal/api/observability/Inputs.InputSources()` so that:
- for `InputTypeAudit`: if `i.Audit == nil` or `len(i.Audit.Sources)==0`, insert **all** audit sources (`obs.AuditSources`) instead of inserting none.
- (optionally) similarly default infra empty -> all infra sources for consistency.
2) **Minimal fix:** Update `getInputSources()` in the LokiStack OTEL migration to add an audit fallback (mirroring the infra fallback) when `len(inputSources)==0 && inputType==obs.InputTypeAudit`.
Also add/adjust a unit test covering LokiStack OTEL datamodel + audit input with `sources: []` (or omitted) to prevent regression.
### Fix Focus Areas
- internal/api/observability/input_types.go[84-100]
- internal/generator/vector/output/lokistack/lokistack.go[117-127]
- internal/generator/vector/output/otlp/otlp.go[55-60]
- internal/generator/vector/output/lokistack/lokistack_test.go[81-100]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Clee2691 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/generator/vector/output/lokistack/lokistack_test.go (1)
156-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating the new test entry against
initOptions().This entry duplicates nearly all of
initOptions()(Lines 82-101) solely to change the audit input'sSourcesto empty. Extracting a shared builder (e.g., a parameterizedinitOptions(auditSources []obs.AuditSource)or a small helper reused by both) would reduce duplication and ease future maintenance as more edge-case entries are added.♻️ Example refactor sketch
- initOptions = func() utils.Options { + buildOptions = func(auditSources []obs.AuditSource) utils.Options { output := initOutput() return utils.Options{ framework.OptionServiceAccountTokenSecretName: saTokenSecretName, helpers.CLFSpec: observability.ClusterLogForwarderSpec(obs.ClusterLogForwarderSpec{ Outputs: []obs.OutputSpec{output}, Pipelines: []obs.PipelineSpec{ { Name: "lokistack", InputRefs: []string{obs.InputTypeApplication.String(), obs.InputTypeInfrastructure.String(), obs.InputTypeAudit.String()}, OutputRefs: []string{output.Name}, }, }, Inputs: []obs.InputSpec{ {Name: obs.InputTypeApplication.String(), Type: obs.InputTypeApplication}, {Name: obs.InputTypeInfrastructure.String(), Type: obs.InputTypeInfrastructure, Infrastructure: &obs.Infrastructure{Sources: obs.InfrastructureSources}}, - {Name: obs.InputTypeAudit.String(), Type: obs.InputTypeAudit, Audit: &obs.Audit{Sources: obs.AuditSources}}, + {Name: obs.InputTypeAudit.String(), Type: obs.InputTypeAudit, Audit: &obs.Audit{Sources: auditSources}}, }, }), } } + initOptions = func() utils.Options { return buildOptions(obs.AuditSources) }Then the new entry becomes:
buildOptions(nil).🤖 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/generator/vector/output/lokistack/lokistack_test.go` around lines 156 - 178, The new lokistack test case duplicates almost all of initOptions() and only differs by using an empty Audit Sources value. Refactor the option construction in lokistack_test.go by extracting a shared builder, such as a parameterized initOptions(auditSources []obs.AuditSource) or similar helper, and reuse it for both the existing initOptions() path and the new "with Otel datamodel and empty audit sources" entry.internal/api/observability/input_types.go (1)
86-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReserved-default fallback logic looks correct.
The conditional insertion mirrors the pre-existing
ReservedApplicationSourcespattern and correctly falls back to reserved sets only when the section isnilorSourcesis empty. Confirmed consistent with theInfrastructure/AuditCRD doc comments stating omission means "collect all sources."Minor optional note: the
InfrastructureandAuditbranches (Lines 93-104) are structurally identical (nil/empty check → reserved fallback vs. derived sources). Could be extracted into a small generic helper, but the duplication is minimal (2 branches) and doesn't currently hurt readability.🤖 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/api/observability/input_types.go` around lines 86 - 109, No functional bug needs fixing in InputSources; the reserved-default fallback behavior for obs.InputTypeInfrastructure and obs.InputTypeAudit is already correct. If you want to address the review’s optional cleanup, factor the repeated nil/empty Sources fallback logic in Inputs.InputSources into a small helper used by the Infrastructure and Audit branches, while preserving the current reserved-set behavior and the existing Application path.
🤖 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/api/observability/input_types.go`:
- Around line 86-109: No functional bug needs fixing in InputSources; the
reserved-default fallback behavior for obs.InputTypeInfrastructure and
obs.InputTypeAudit is already correct. If you want to address the review’s
optional cleanup, factor the repeated nil/empty Sources fallback logic in
Inputs.InputSources into a small helper used by the Infrastructure and Audit
branches, while preserving the current reserved-set behavior and the existing
Application path.
In `@internal/generator/vector/output/lokistack/lokistack_test.go`:
- Around line 156-178: The new lokistack test case duplicates almost all of
initOptions() and only differs by using an empty Audit Sources value. Refactor
the option construction in lokistack_test.go by extracting a shared builder,
such as a parameterized initOptions(auditSources []obs.AuditSource) or similar
helper, and reuse it for both the existing initOptions() path and the new "with
Otel datamodel and empty audit sources" entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ea27578d-1453-4904-b0be-321508013d95
📒 Files selected for processing (8)
internal/api/observability/input_types.gointernal/api/observability/input_types_test.gointernal/generator/vector/output/lokistack/lokistack.gointernal/generator/vector/output/lokistack/lokistack_test.gointernal/validations/observability/inputs/audit.gointernal/validations/observability/inputs/audit_test.gointernal/validations/observability/inputs/infrastructure.gointernal/validations/observability/inputs/infrastructure_test.go
💤 Files with no reviewable changes (2)
- internal/generator/vector/output/lokistack/lokistack.go
- internal/validations/observability/inputs/audit.go
|
/retest |
|
@Clee2691: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
Description
/cc @vparfonov
/assign @jcantrill
Links
Summary by CodeRabbit