Skip to content

fix(machine-a-tron): validate ipmi_sim before startup #3753

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

Merged
poroh merged 1 commit into from
Jul 21, 2026
+73 −2
Merged

fix(machine-a-tron): validate ipmi_sim before startup #3753

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
71 changes: 69 additions & 2 deletions crates/bmc-mock/src/ipmi_sim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

use std::ffi::OsStr;
use std::io::ErrorKind;
use std::net::{IpAddr, SocketAddr, TcpListener, UdpSocket};
use std::os::unix::fs::PermissionsExt;
Expand All @@ -37,6 +38,7 @@ const READY_TIMEOUT: Duration = Duration::from_secs(5);
const READY_POLL_INTERVAL: Duration = Duration::from_millis(50);
const PASSWORD_UPDATE_TIMEOUT: Duration = Duration::from_secs(10);
pub const STANDARD_IPMI_PORT: u16 = 623;
const IPMI_SIM_EXECUTABLE: &str = "ipmi_sim";

#[derive(Debug, Clone)]
pub struct IpmiSimConfig {
Expand Down Expand Up @@ -90,6 +92,10 @@ impl Drop for IpmiSimHandle {

#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error(
"IPMI simulation requires {executable}, but it was not found or is not executable in PATH"
)]
ExecutableUnavailable { executable: &'static str },
Comment on lines +95 to +98

@coderabbitai coderabbitai Bot Jul 21, 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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Lowercase the new error message.

lint-error-messages requires normal error wording to start lowercase.

-        "IPMI simulation requires {executable}, but it was not found or is not executable in PATH"
+        "ipmi simulation requires {executable}, but it was not found or is not executable in PATH"

Based on learnings, normal Rust #[error(...)] wording must be lowercased.

📝 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
#[error(
"IPMI simulation requires {executable}, but it was not found or is not executable in PATH"
)]
ExecutableUnavailable { executable: &'static str },
#[error(
"ipmi simulation requires {executable}, but it was not found or is not executable in PATH"
)]
ExecutableUnavailable { executable: &'static str },
🤖 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/bmc-mock/src/ipmi_sim.rs` around lines 76 - 79, Update the #[error]
message for ExecutableUnavailable to begin with lowercase wording, changing the
initial “IPMI” capitalization while preserving the existing message content and
formatting.

Source: Learnings

#[error("the BMC mock has no administrative account")]
MissingAdministrativeAccount,
#[error("the IPMI simulator {0} contains unsupported characters")]
Expand All @@ -104,6 +110,30 @@ pub enum Error {
AttemptsExhausted(Box<Error>),
}

pub fn validate_executable() -> Result<(), Error> {
validate_executable_in_path(std::env::var_os("PATH").as_deref())
}

fn validate_executable_in_path(path: Option<&OsStr>) -> Result<(), Error> {
let executable_available = path
.into_iter()
.flat_map(std::env::split_paths)
.map(|directory| directory.join(IPMI_SIM_EXECUTABLE))
.any(|candidate| {
candidate.metadata().is_ok_and(|metadata| {
metadata.is_file() && metadata.permissions().mode() & 0o111 != 0
})
});

if executable_available {
Ok(())
} else {
Err(Error::ExecutableUnavailable {
executable: IPMI_SIM_EXECUTABLE,
})
}
}
Comment on lines +117 to +135

@coderabbitai coderabbitai Bot Jul 21, 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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline crates/bmc-mock/src/ipmi_sim.rs --items all
sed -n '94,116p;176,176p;479,508p' crates/bmc-mock/src/ipmi_sim.rs

workdir="$(mktemp -d)"
trap 'rm -rf "$workdir"' EXIT
mkdir -p "$workdir/bin" "$workdir/child"
printf '#!/bin/sh\nexit 0\n' > "$workdir/bin/ipmi_sim"
chmod 700 "$workdir/bin/ipmi_sim"

(
  cd "$workdir"
  PATH='bin' command -v ipmi_sim >/dev/null
  cd child
  ! PATH='bin' command -v ipmi_sim >/dev/null
)

Repository: NVIDIA/infra-controller

Length of output: 5120

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '143,240p' crates/bmc-mock/src/ipmi_sim.rs

python3 - <<'PY'
import os, tempfile, subprocess, textwrap, shutil, stat

workdir = tempfile.mkdtemp()
try:
    os.makedirs(os.path.join(workdir, "bin"))
    os.makedirs(os.path.join(workdir, "child"))
    exe = os.path.join(workdir, "bin", "ipmi_sim")
    with open(exe, "w") as f:
        f.write("#!/bin/sh\nexit 0\n")
    os.chmod(exe, 0o700)

    for cwd in [workdir, os.path.join(workdir, "child")]:
        p = subprocess.run(
            ["sh", "-c", "PATH='bin' command -v ipmi_sim"],
            cwd=cwd,
            stdout=subprocess.PIPE,
            stderr=subprocess.PIPE,
            text=True,
        )
        print("cwd=", cwd)
        print("rc=", p.returncode)
        print("stdout=", p.stdout.strip())
        print("stderr=", p.stderr.strip())
finally:
    shutil.rmtree(workdir)
PY

Repository: NVIDIA/infra-controller

Length of output: 3784

Resolve the executable before changing the child CWD. A relative PATH entry can pass the lookup here, but current_dir(temp_dir.path()) makes Command::new(IPMI_SIM_EXECUTABLE) search from a different directory and fail to launch. Return the resolved absolute PathBuf from the helper, pass it to Command::new, and add a regression test for a relative PATH entry.

🤖 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/bmc-mock/src/ipmi_sim.rs` around lines 98 - 116, Update
validate_executable_in_path to resolve and return the executable’s absolute
PathBuf rather than only validating availability; preserve the existing
executable-file and permission checks, and return the unavailable error when no
candidate matches. Update the command-launching flow to pass that resolved path
to Command::new before applying the child current_dir, and add a regression test
covering a relative PATH entry.

Source: Path instructions


struct Reservations {
ipmi_sim_lan_socket: UdpSocket,
ipmi_sim_serial_listener: TcpListener,
Expand Down Expand Up @@ -162,7 +192,7 @@ pub async fn start(state: &BmcState, config: IpmiSimConfig) -> Result<IpmiSimHan
)?;

drop(reservations);
let mut child = tokio::process::Command::new("ipmi_sim")
let mut child = tokio::process::Command::new(IPMI_SIM_EXECUTABLE)
.current_dir(temp_dir.path())
.arg("-c")
.arg(temp_dir.path().join("lan.conf"))
Expand Down Expand Up @@ -460,7 +490,13 @@ async fn serve_console(mut stream: TcpStream, prompt: &str) -> Result<(), std::i

#[cfg(test)]
mod tests {
use super::{Error, IpmiEndpoint, MockConsole, stable_guid, validate_credential};
use std::fs;
use std::os::unix::fs::PermissionsExt;

use super::{
Error, IPMI_SIM_EXECUTABLE, IpmiEndpoint, MockConsole, stable_guid, validate_credential,
validate_executable_in_path,
};

#[test]
fn endpoint_uses_configured_reachable_port_or_listen_port() {
Expand Down Expand Up @@ -492,6 +528,37 @@ mod tests {
}
}

#[test]
fn ipmi_sim_executable_is_required() {
let temp_dir = tempfile::tempdir().unwrap();
let missing_dir = temp_dir.path().join("missing");
let non_executable_dir = temp_dir.path().join("non-executable");
let executable_dir = temp_dir.path().join("executable");
fs::create_dir_all(&missing_dir).unwrap();
fs::create_dir_all(&non_executable_dir).unwrap();
fs::create_dir_all(&executable_dir).unwrap();

fs::write(non_executable_dir.join(IPMI_SIM_EXECUTABLE), []).unwrap();

let executable = executable_dir.join(IPMI_SIM_EXECUTABLE);
fs::write(&executable, []).unwrap();
let mut permissions = executable.metadata().unwrap().permissions();
permissions.set_mode(0o700);
fs::set_permissions(&executable, permissions).unwrap();

for (scenario, path, expected) in [
("missing executable", missing_dir, false),
("non-executable file", non_executable_dir, false),
("executable file", executable_dir, true),
] {
assert_eq!(
validate_executable_in_path(Some(path.as_os_str())).is_ok(),
expected,
"{scenario}"
);
}
}

#[test]
fn stable_guid_is_stable_and_has_ipmi_length() {
assert_eq!(stable_guid("machine-1"), stable_guid("machine-1"));
Expand Down
4 changes: 4 additions & 0 deletions crates/machine-a-tron/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,10 @@ impl MachineATronConfig {
);
}

if self.enable_ipmi_simulation {
bmc_mock::ipmi_sim::validate_executable()?;
}

for (rack_id, rack) in &self.racks {
eyre::ensure!(!rack_id.as_str().is_empty(), "rack ID cannot be empty");
eyre::ensure!(
Expand Down
Loading