Repository navigation
feat: config validation hardening — enrichment variants, max_open_files, path checks, log_level - #903
Conversation
334ec1a to
6003dd9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds support for multiple enrichment types and validates/enforces related config rules. New enrichment config structs and tagged Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd-config/src/lib.rs`:
- Around line 662-699: The validation currently performed in Config::validate
checks filesystem existence for enrichment paths (enrichment variants
GeoDatabase, Csv, Jsonl) using Path::new(...).exists() before base_path is known
(Config::load_str), which rejects relative paths that are later resolved in
Pipeline::from_config; remove or skip the existence checks from Config::validate
and instead perform them in Pipeline::from_config after resolving paths against
the provided base_path (or alternatively only validate existence in
Config::validate if the path is absolute). Update the checks around
EnrichmentConfig::GeoDatabase, EnrichmentConfig::Csv, and
EnrichmentConfig::Jsonl accordingly so resolution+existence happens in
Pipeline::from_config while Config::validate only enforces syntactic rules (or
absolute-path existence).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5780abcb-72d0-4266-9367-5438af65838d
📒 Files selected for processing (2)
crates/logfwd-config/src/lib.rscrates/logfwd/src/pipeline.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd-config/src/lib.rs`:
- Around line 478-480: The deserializer currently allows RawConfig.enrichment
alongside RawConfig.pipelines but from_raw() only applies raw.enrichment in the
simple-form branch; to fix, add a validation in from_raw() (or the RawConfig ->
Config conversion) that checks if raw.pipelines.is_some() and raw.enrichment is
not empty and return an error (reject) in that case; reference
RawConfig.enrichment, RawConfig.pipelines, and the from_raw() function to locate
where to perform this check so advanced-form configs cannot silently ignore
top-level enrichment.
- Around line 716-757: In Config::validate(), while iterating pipe.enrichment
(the loop shown) add the same table-name validation that Pipeline::from_config()
enforces: when encountering the table-backed enrichment variant (the
EnrichmentConfig variant that contains table_name) check that table_name is not
a reserved name (e.g., "logs" or any other reserved identifiers used by
Pipeline::from_config()) and that it is not a duplicate within the same pipeline
(track seen names in a HashSet per pipeline); on violation return
ConfigError::Validation with a message referencing pipeline '{name}' and
enrichment #{j} so validation fails early rather than letting
Pipeline::from_config() reject it later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: dabbbb1b-d58d-43e4-87c4-cc12ed88d7d9
📒 Files selected for processing (2)
crates/logfwd-config/src/lib.rscrates/logfwd/src/pipeline.rs
| /// Enrichment sources for the simple-form default pipeline. | ||
| #[serde(default)] | ||
| enrichment: Vec<EnrichmentConfig>, |
There was a problem hiding this comment.
Reject top-level enrichment when pipelines is present.
Adding RawConfig.enrichment means pipelines: + top-level enrichment: now deserializes cleanly, but from_raw() only forwards raw.enrichment in the simple-form branch. In the advanced-form branch those entries are silently dropped, which reintroduces exactly the “accepted but ignored” config behavior this PR is trying to remove.
Suggested fix
fn from_raw(raw: RawConfig) -> Result<Self, ConfigError> {
+ if raw.pipelines.is_some() && !raw.enrichment.is_empty() {
+ return Err(ConfigError::Validation(
+ "cannot mix top-level enrichment with pipelines".into(),
+ ));
+ }
+
let pipelines = match (raw.pipelines, raw.input, raw.output) {
(Some(p), None, None) => p,
(None, Some(input), Some(output)) => {
let pipeline = PipelineConfig {
inputs: vec![input],
transform: raw.transform,
outputs: vec![output],
enrichment: raw.enrichment,Also applies to: 526-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-config/src/lib.rs` around lines 478 - 480, The deserializer
currently allows RawConfig.enrichment alongside RawConfig.pipelines but
from_raw() only applies raw.enrichment in the simple-form branch; to fix, add a
validation in from_raw() (or the RawConfig -> Config conversion) that checks if
raw.pipelines.is_some() and raw.enrichment is not empty and return an error
(reject) in that case; reference RawConfig.enrichment, RawConfig.pipelines, and
the from_raw() function to locate where to perform this check so advanced-form
configs cannot silently ignore top-level enrichment.
| // Validate enrichment entries (#550). | ||
| for (j, enrichment) in pipe.enrichment.iter().enumerate() { | ||
| match enrichment { | ||
| EnrichmentConfig::GeoDatabase(geo_cfg) => { | ||
| // Only check existence for absolute paths; relative paths | ||
| // are resolved against base_path in Pipeline::from_config. | ||
| let p = Path::new(&geo_cfg.path); | ||
| if p.is_absolute() && !p.exists() { | ||
| return Err(ConfigError::Validation(format!( | ||
| "pipeline '{name}' enrichment #{j}: geo database file not found: {}", | ||
| geo_cfg.path, | ||
| ))); | ||
| } | ||
| } | ||
| EnrichmentConfig::Static(cfg) => { | ||
| if cfg.labels.is_empty() { | ||
| return Err(ConfigError::Validation(format!( | ||
| "pipeline '{name}' enrichment #{j}: static enrichment requires at least one label" | ||
| ))); | ||
| } | ||
| } | ||
| EnrichmentConfig::Csv(cfg) => { | ||
| let p = Path::new(&cfg.path); | ||
| if p.is_absolute() && !p.exists() { | ||
| return Err(ConfigError::Validation(format!( | ||
| "pipeline '{name}' enrichment #{j}: csv file not found: {}", | ||
| cfg.path, | ||
| ))); | ||
| } | ||
| } | ||
| EnrichmentConfig::Jsonl(cfg) => { | ||
| let p = Path::new(&cfg.path); | ||
| if p.is_absolute() && !p.exists() { | ||
| return Err(ConfigError::Validation(format!( | ||
| "pipeline '{name}' enrichment #{j}: jsonl file not found: {}", | ||
| cfg.path, | ||
| ))); | ||
| } | ||
| } | ||
| EnrichmentConfig::HostInfo(_) | EnrichmentConfig::K8sPath(_) => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
Mirror enrichment table-name checks in Config::validate().
The new table-backed enrichments still pass Config::load_str() with names that downstream registration will reject later, e.g. table_name: logs or two k8s_path entries that both default to k8s_pods. That means --validate still misses configs that are guaranteed to fail during Pipeline::from_config(). Please reject reserved and duplicate table names here while iterating pipe.enrichment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-config/src/lib.rs` around lines 716 - 757, In
Config::validate(), while iterating pipe.enrichment (the loop shown) add the
same table-name validation that Pipeline::from_config() enforces: when
encountering the table-backed enrichment variant (the EnrichmentConfig variant
that contains table_name) check that table_name is not a reserved name (e.g.,
"logs" or any other reserved identifiers used by Pipeline::from_config()) and
that it is not a duplicate within the same pipeline (track seen names in a
HashSet per pipeline); on violation return ConfigError::Validation with a
message referencing pipeline '{name}' and enrichment #{j} so validation fails
early rather than letting Pipeline::from_config() reject it later.
strawgate
left a comment
strawgate
left a comment
There was a problem hiding this comment.
Review — PR #903 (config validation hardening)
Verdict: Needs fix for one usability bug.
Validations are correct and well-tested (16 new tests). Enrichment config variants, path checks, max_open_files=0 rejection, log_level validation all look good.
Blocking issue: Top-level enrichment alongside pipelines is silently dropped.
If a user writes:
pipelines:
app: ...
enrichment:
- type: host_infoThe top-level enrichment is silently ignored — only used in simple-form configs. This was flagged by CodeRabbit and appears unaddressed. Either reject with an error ("enrichment must be inside pipeline when using pipelines: form") or wire it into all pipelines.
Also: no table name collision validation (if two enrichments use same name, error surfaces late in Pipeline::from_config). Nice-to-have, not blocking.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR adds 5 new enrichment types (Static, HostInfo, K8sPath, Csv, Jsonl) with full runtime implementation, not just config validation. New feature capabilities that affect log processing pipelines warrant human review, and an open review comment notes that table-name validation is incomplete. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
224f4e0 to
a398a4d
Compare
…es, log_level, path checks - #540: Add enrichment config variants (static, host_info, k8s_path, csv, jsonl) and wire simple-form enrichment instead of dropping it - #550: Validate enrichment file paths at config load time - #696: Reject max_open_files: 0 with clear error message - #481: Validate server.log_level values at config load time - Wire new enrichment types through pipeline.rs Agent-Logs-Url: https://github.com/strawgate/memagent/sessions/79125bcc-e668-4c2d-9d03-f7a6347877ca Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
Relative enrichment paths are resolved against base_path in Pipeline::from_config, so Config::validate() must not reject them with a "not found" error. Guard .exists() checks with .is_absolute() for GeoDatabase, Csv, and Jsonl enrichment types. Adds a test confirming relative paths pass validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When `pipelines:` is present, top-level `enrichment:` was silently ignored. Now returns a validation error directing users to move enrichment configuration inside each pipeline, matching the existing pattern for top-level input/output rejection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Same silent-drop pattern as the enrichment fix — top-level resource_attrs was silently ignored in the pipelines: form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a398a4d to
91cbfbe
Compare
Closes five config-load-time validation gaps where invalid config was silently accepted instead of producing an error at startup.
Enrichment config variants (#540)
EnrichmentConfigonly hadGeoDatabasedespite implementations existing for five other types. Added variants and config structs forstatic,host_info,k8s_path,csv,jsonl. Wired them throughpipeline.rsviaadd_enrichment_table().Simple-form configs silently dropped enrichment (
enrichment: Vec::new()). Addedenrichmentfield toRawConfigand pass it through infrom_raw().Enrichment path validation (#550)
Config::validate()now checks thatgeo_database,csv, andjsonlenrichment file paths exist at load time. Static enrichment rejects empty labels.max_open_files: 0 (#696)
max_open_files: 0caused the tailer to evict every file on every poll, silently producing zero data. Now rejected at config load:max_open_files must be at least 1.log_level validation (#481)
server.log_levelaccepted any string. Now validates against recognized levels (trace,debug,info,warn,error, case-insensitive).Scope
crates/logfwd-config/src/lib.rs— config structs, validation, 16 new tests (34→50)crates/logfwd/src/pipeline.rs— wire new enrichment typesNote
Closes five config-load-time validation gaps where invalid config was silently accepted instead of producing an error at startup.
Enrichment config variants (#540)
EnrichmentConfigonly hadGeoDatabasedespite implementations existing for five other types. Added variants and config structs forstatic,host_info,k8s_path,csv,jsonl. Wired them throughpipeline.rsviaadd_enrichment_table().Simple-form configs silently dropped enrichment (
enrichment: Vec::new()). Addedenrichmentfield toRawConfigand pass it through infrom_raw().Enrichment path validation (#550)
Config::validate()now checks thatgeo_database,csv, andjsonlenrichment file paths exist at load time. Static enrichment rejects empty labels.max_open_files: 0 (#696)
max_open_files: 0caused the tailer to evict every file on every poll, silently producing zero data. Now rejected at config load:max_open_files must be at least 1.log_level validation (#481)
server.log_levelaccepted any string. Now validates against recognized levels (trace,debug,info,warn,error, case-insensitive).Scope
crates/logfwd-config/src/lib.rs— config structs, validation, 16 new tests (34→50)crates/logfwd/src/pipeline.rs— wire new enrichment typesChanges since #903 opened
Config::from_rawmethod to reject non-empty top-levelresource_attrswhen using pipelines-only configuration [224f4e0]