Skip to content

feat(firmware): allow custom version detection regexes #3420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
rahmonov wants to merge 1 commit into
base: main
Choose a base branch
from
+448 −21
Draft

feat(firmware): allow custom version detection regexes #3420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
257 changes: 244 additions & 13 deletions crates/api-core/src/handlers/firmware.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ use url::Url;
use crate::CarbideError;
use crate::api::{Api, log_request_data, log_request_data_redacted};

const MAX_CURRENT_VERSION_REPORTED_AS_LENGTH: usize = 1024;

pub(crate) async fn set_firmware_update_time_window(
api: &Api,
request: Request<rpc::SetFirmwareUpdateTimeWindowRequest>,
Expand Down Expand Up @@ -236,7 +238,7 @@ struct HostFirmwareConfigPatch {
}

struct FirmwareComponentPatch {
current_version_reported_as: Regex,
current_version_reported_as: Option<Regex>,
preingest_upgrade_when_below: Option<String>,
known_firmware: Vec<FirmwareEntryPatch>,
}
Expand All @@ -247,16 +249,26 @@ struct FirmwareEntryPatch {
}

impl FirmwareComponentPatch {
fn into_component(self) -> FirmwareComponent {
FirmwareComponent {
current_version_reported_as: Some(self.current_version_reported_as),
fn into_component(
self,
vendor: bmc_vendor::BMCVendor,
model: &str,
component_type: FirmwareComponentType,
) -> Result<FirmwareComponent, CarbideError> {
let current_version_reported_as = match self.current_version_reported_as {
Some(regex) => regex,
None => component_regex(vendor, model, component_type)?,
};

Ok(FirmwareComponent {
current_version_reported_as: Some(current_version_reported_as),
preingest_upgrade_when_below: self.preingest_upgrade_when_below,
known_firmware: self
.known_firmware
.into_iter()
.map(FirmwareEntryPatch::into_entry)
.collect(),
}
})
}
}

Expand Down Expand Up @@ -292,7 +304,12 @@ impl HostFirmwareConfigPatch {
)));
}

let current_version_reported_as = component_regex(vendor, &model, component_type)?;
let current_version_reported_as = parse_component_regex(
vendor,
&model,
component_type,
component.current_version_reported_as.as_deref(),
)?;
let preingest_upgrade_when_below = parse_preingest_upgrade_when_below(
component_type,
component.preingest_upgrade_when_below.as_deref(),
Expand Down Expand Up @@ -345,9 +362,12 @@ fn merge_host_firmware_config_patch(
if let Some(existing_component) = runtime_config.components.get_mut(&component_type) {
merge_host_firmware_component(existing_component, incoming_component);
} else {
runtime_config
.components
.insert(component_type, incoming_component.into_component());
let component = incoming_component.into_component(
runtime_config.vendor,
&runtime_config.model,
component_type,
)?;
runtime_config.components.insert(component_type, component);
}
}

Expand All @@ -364,8 +384,9 @@ fn merge_host_firmware_component(
existing_component: &mut FirmwareComponent,
incoming_component: FirmwareComponentPatch,
) {
existing_component.current_version_reported_as =
Some(incoming_component.current_version_reported_as);
if let Some(regex) = incoming_component.current_version_reported_as {
existing_component.current_version_reported_as = Some(regex);
}
if incoming_component.preingest_upgrade_when_below.is_some() {
existing_component.preingest_upgrade_when_below =
incoming_component.preingest_upgrade_when_below;
Expand Down Expand Up @@ -505,6 +526,34 @@ fn parse_vendor(vendor: &str) -> Result<bmc_vendor::BMCVendor, CarbideError> {
Ok(parsed)
}

fn parse_component_regex(
vendor: bmc_vendor::BMCVendor,
model: &str,
component_type: FirmwareComponentType,
value: Option<&str>,
) -> Result<Option<Regex>, CarbideError> {
value
.map(|value| {
if value.trim().is_empty() {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as is empty for {vendor} {model} {component_type}"
)));
}
if value.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}"
)));
}

Regex::new(value).map_err(|error| {
CarbideError::InvalidArgument(format!(
"invalid current_version_reported_as for {vendor} {model} {component_type}: {error}"
))
})
})
.transpose()
}

Comment on lines +529 to +556

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Trim the regex value before validating/compiling it.

parse_component_regex checks emptiness via value.trim() but compiles and length-checks the untrimmed value. A value with stray leading/trailing whitespace (e.g. " ^BMC-Firmware$") passes the emptiness check yet compiles into a Regex that can never match anything, since the literal space forces a position that contradicts the ^ anchor — silently breaking version detection for that component with no error surfaced. The sibling parse_preingest_upgrade_when_below already trims consistently; this function should do the same.

🐛 Proposed fix: trim before validating/compiling 
 fn parse_component_regex(
     vendor: bmc_vendor::BMCVendor,
     model: &str,
     component_type: FirmwareComponentType,
     value: Option<&str>,
 ) -> Result<Option<Regex>, CarbideError> {
     value
         .map(|value| {
-            if value.trim().is_empty() {
+            let trimmed = value.trim();
+            if trimmed.is_empty() {
                 return Err(CarbideError::InvalidArgument(format!(
                     "current_version_reported_as is empty for {vendor} {model} {component_type}"
                 )));
             }
-            if value.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH {
+            if trimmed.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH {
                 return Err(CarbideError::InvalidArgument(format!(
                     "current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}"
                 )));
             }

-            Regex::new(value).map_err(|error| {
+            Regex::new(trimmed).map_err(|error| {
                 CarbideError::InvalidArgument(format!(
                     "invalid current_version_reported_as for {vendor} {model} {component_type}: {error}"
                 ))
             })
         })
         .transpose()
 }

Consider also adding a case to parse_component_regex_validates_supplied_values for a value like " ^BMC-Firmware$ " to lock in the trimmed-compile behavior.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn parse_component_regex(
vendor: bmc_vendor::BMCVendor,
model: &str,
component_type: FirmwareComponentType,
value: Option<&str>,
) -> Result<Option<Regex>, CarbideError> {
value
.map(|value| {
if value.trim().is_empty() {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as is empty for {vendor} {model} {component_type}"
)));
}
if value.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}"
)));
}
Regex::new(value).map_err(|error| {
CarbideError::InvalidArgument(format!(
"invalid current_version_reported_as for {vendor} {model} {component_type}: {error}"
))
})
})
.transpose()
}
fn parse_component_regex(
vendor: bmc_vendor::BMCVendor,
model: &str,
component_type: FirmwareComponentType,
value: Option<&str>,
) -> Result<Option<Regex>, CarbideError> {
value
.map(|value| {
let trimmed = value.trim();
if trimmed.is_empty() {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as is empty for {vendor} {model} {component_type}"
)));
}
if trimmed.chars().count() > MAX_CURRENT_VERSION_REPORTED_AS_LENGTH {
return Err(CarbideError::InvalidArgument(format!(
"current_version_reported_as exceeds {MAX_CURRENT_VERSION_REPORTED_AS_LENGTH} characters for {vendor} {model} {component_type}"
)));
}
Regex::new(trimmed).map_err(|error| {
CarbideError::InvalidArgument(format!(
"invalid current_version_reported_as for {vendor} {model} {component_type}: {error}"
))
})
})
.transpose()
}
🤖 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 `@crates/api-core/src/handlers/firmware.rs` around lines 529 - 556, Update
parse_component_regex to trim the supplied value before emptiness validation,
length validation, and Regex compilation, so leading or trailing whitespace does
not become part of the pattern. Preserve the existing error handling and add a
regression case to parse_component_regex_validates_supplied_values for a
whitespace-padded anchored regex.

fn component_regex(
vendor: bmc_vendor::BMCVendor,
model: &str,
Expand Down Expand Up @@ -837,6 +886,59 @@ mod tests {
);
}

#[test]
fn parse_component_regex_validates_supplied_values() {
let regex = parse_component_regex(
bmc_vendor::BMCVendor::Supermicro,
"SYS-121H-TNR",
FirmwareComponentType::Bmc,
Some("^BMC-Firmware$"),
)
.unwrap()
.expect("supplied regex");
assert_eq!(regex.as_str(), "^BMC-Firmware$");

assert!(
parse_component_regex(
bmc_vendor::BMCVendor::Supermicro,
"SYS-121H-TNR",
FirmwareComponentType::Bmc,
None,
)
.unwrap()
.is_none()
);
assert!(
parse_component_regex(
bmc_vendor::BMCVendor::Supermicro,
"SYS-121H-TNR",
FirmwareComponentType::Bmc,
Some(" "),
)
.is_err()
);
assert!(
parse_component_regex(
bmc_vendor::BMCVendor::Supermicro,
"SYS-121H-TNR",
FirmwareComponentType::Bmc,
Some("("),
)
.is_err()
);

let oversized = "a".repeat(MAX_CURRENT_VERSION_REPORTED_AS_LENGTH + 1);
assert!(
parse_component_regex(
bmc_vendor::BMCVendor::Supermicro,
"SYS-121H-TNR",
FirmwareComponentType::Bmc,
Some(&oversized),
)
.is_err()
);
}

#[test]
fn parse_host_firmware_config_model_trims_and_rejects_empty_values() {
scenarios!(run = |input| parse_host_firmware_config_model(input).map_err(drop);
Expand Down Expand Up @@ -909,6 +1011,7 @@ mod tests {
preingestion_exclusive_config: None,
}],
preingest_upgrade_when_below: Some("1.0.0".to_string()),
current_version_reported_as: None,
}],
explicit_start_needed: Some(true),
ordering: vec![rpc::HostFirmwareComponentType::Bmc as i32],
Expand Down Expand Up @@ -1045,6 +1148,134 @@ mod tests {
);
}

#[test]
fn merge_host_firmware_config_preserves_omitted_component_regex() {
let existing = test_firmware(HashMap::from([(
FirmwareComponentType::Bmc,
test_component(
"^existing-bmc$",
vec![test_entry(
"7.20",
true,
"https://firmware.example.invalid/bmc-7.20.bin",
)],
),
)]));
let mut patch = test_patch(HashMap::from([(
FirmwareComponentType::Bmc,
test_component(
"^ignored$",
vec![test_entry(
"7.30",
true,
"https://firmware.example.invalid/bmc-7.30.bin",
)],
),
)]));
patch
.components
.get_mut(&FirmwareComponentType::Bmc)
.unwrap()
.current_version_reported_as = None;

let merged = merge_host_firmware_config_patch(Some(existing), patch).unwrap();

assert_eq!(
merged.components[&FirmwareComponentType::Bmc]
.current_version_reported_as
.as_ref()
.unwrap()
.as_str(),
"^existing-bmc$"
);
}

#[test]
fn merge_host_firmware_config_uses_catalog_regex_for_new_component() {
let mut patch = test_patch(HashMap::from([(
FirmwareComponentType::Cx7,
test_component(
"^ignored$",
vec![test_entry(
"28.47.2682",
true,
"https://firmware.example.invalid/cx7.bin",
)],
),
)]));
patch.ordering = vec![FirmwareComponentType::Cx7];
patch
.components
.get_mut(&FirmwareComponentType::Cx7)
.unwrap()
.current_version_reported_as = None;

let merged = merge_host_firmware_config_patch(None, patch).unwrap();

assert_eq!(
merged.components[&FirmwareComponentType::Cx7]
.current_version_reported_as
.as_ref()
.unwrap()
.as_str(),
"^CX7_[0-9]+$"
);
}

#[test]
fn merge_host_firmware_config_accepts_custom_regex_without_catalog_mapping() {
let mut patch = test_patch(HashMap::from([(
FirmwareComponentType::Bmc,
test_component(
"^BMC-Firmware$",
vec![test_entry(
"1.0.0",
true,
"https://firmware.example.invalid/bmc.bin",
)],
),
)]));
patch.vendor = bmc_vendor::BMCVendor::Supermicro;
patch.model = "SYS-121H-TNR".to_string();
patch.ordering = vec![FirmwareComponentType::Bmc];

let merged = merge_host_firmware_config_patch(None, patch).unwrap();

assert_eq!(
merged.components[&FirmwareComponentType::Bmc]
.current_version_reported_as
.as_ref()
.unwrap()
.as_str(),
"^BMC-Firmware$"
);
}

#[test]
fn merge_host_firmware_config_rejects_new_component_without_regex_mapping() {
let mut patch = test_patch(HashMap::from([(
FirmwareComponentType::Bmc,
test_component(
"^ignored$",
vec![test_entry(
"1.0.0",
true,
"https://firmware.example.invalid/bmc.bin",
)],
),
)]));
patch.vendor = bmc_vendor::BMCVendor::Supermicro;
patch.model = "SYS-121H-TNR".to_string();
patch.ordering = vec![FirmwareComponentType::Bmc];
patch
.components
.get_mut(&FirmwareComponentType::Bmc)
.unwrap()
.current_version_reported_as = None;

assert!(merge_host_firmware_config_patch(None, patch).is_err());
}

#[test]
fn merge_host_firmware_config_replaces_existing_version_and_preserves_default() {
let existing = test_firmware(HashMap::from([(
Expand Down Expand Up @@ -1480,7 +1711,7 @@ mod tests {
.collect();

FirmwareComponentPatch {
current_version_reported_as: component.current_version_reported_as.unwrap(),
current_version_reported_as: component.current_version_reported_as,
preingest_upgrade_when_below: component.preingest_upgrade_when_below,
known_firmware: firmware,
}
Expand All @@ -1491,7 +1722,7 @@ mod tests {
entries: Vec<(FirmwareEntry, Option<bool>)>,
) -> FirmwareComponentPatch {
FirmwareComponentPatch {
current_version_reported_as: Regex::new(regex).unwrap(),
current_version_reported_as: Some(Regex::new(regex).unwrap()),
preingest_upgrade_when_below: None,
known_firmware: entries
.into_iter()
Expand Down
Loading