Skip to content

fix(cli): unify PF diagnostics without sudo #318

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
munezaclovis wants to merge 3 commits into
base: agent/fix-pf-gateway-readiness
Choose a base branch
from
Draft

fix(cli): unify PF diagnostics without sudo #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aba571c
fix(daemon): bind Gateway health identity to ports
munezaclovis Aug 4, 2026
d797507
feat(cli): unify PF routing diagnostics
munezaclovis Aug 4, 2026
898e44c
test(cli): cover unprivileged PF diagnostics
munezaclovis Aug 4, 2026
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/cli/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ sha2 = { workspace = true }
self-update = { path = "../self-update" }
state = { path = "../state" }
thiserror = { workspace = true }
time = { workspace = true }
tokio = { workspace = true }
yaml_serde = { workspace = true }

Expand Down
13 changes: 11 additions & 2 deletions crates/cli/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub(crate) enum Command {
DnsUninstall,

#[command(name = "ports:status", about = "Show PV pf redirect status")]
PortsStatus,
PortsStatus(PortsStatusArgs),

#[command(name = "ports:install", about = "Install or repair PV pf redirects")]
PortsInstall,
Expand Down Expand Up @@ -509,6 +509,12 @@ pub(crate) struct StatusArgs {
pub(crate) json: bool,
}

#[derive(Debug, clap::Args)]
pub(crate) struct PortsStatusArgs {
#[arg(long, help = "Print pf redirect status as JSON")]
pub(crate) json: bool,
}

#[derive(Debug, clap::Args)]
pub(crate) struct LogsArgs {
#[arg(
Expand Down Expand Up @@ -552,7 +558,10 @@ pub(crate) struct LogsArgs {
}

#[derive(Debug, clap::Args)]
pub(crate) struct DoctorArgs {}
pub(crate) struct DoctorArgs {
#[arg(long, help = "Print diagnostics as JSON")]
pub(crate) json: bool,
}

#[derive(Debug, clap::Args)]
pub(crate) struct JobsArgs {
Expand Down
170 changes: 51 additions & 119 deletions crates/cli/src/commands/doctor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ use std::process::ExitCode;

use camino::Utf8PathBuf;
use platform::{
CaFileState, LaunchAgentFileState, LocalCaMetadata, PfConfReference, PfFileState,
PfRedirectConfig, ResolverConfig, ResolverFileState, TrustDomainState,
CaFileState, LaunchAgentFileState, LocalCaMetadata, ResolverConfig, ResolverFileState,
TrustDomainState,
};
use serde::Serialize;
use state::{Database, JobStatus, PvPaths, RuntimeObservedStatus, StateError};

use crate::args::DoctorArgs;
Expand All @@ -14,8 +15,10 @@ use crate::error::CliError;
use crate::error::ExecuteError;
use crate::output::{Output, OutputMode};

use super::pf_diagnostics::{PfRoutingDiagnostic, PfRoutingState};

pub(crate) fn run(
_args: DoctorArgs,
args: DoctorArgs,
environment: &impl Environment,
stdout: &mut impl Write,
) -> Result<ExitCode, ExecuteError> {
Expand All @@ -25,12 +28,19 @@ pub(crate) fn run(
} else {
ExitCode::SUCCESS
};
if args.json {
serde_json::to_writer(&mut *stdout, &report)?;
writeln!(stdout)?;

return Ok(exit_code);
}
let mut output = Output::new(stdout, OutputMode::plain());
report.write_plain(&mut output)?;

Ok(exit_code)
}

#[derive(Serialize)]
struct DoctorReport {
checks: Vec<DoctorCheck>,
}
Expand All @@ -47,7 +57,7 @@ impl DoctorReport {
launch_agent_check(&launch_agent),
daemon_socket_check(&paths, &launch_agent),
dns_check(environment, &paths)?,
ports_check(environment, &paths)?,
ports_check(environment, &paths, database.as_ref())?,
ca_check(environment, &paths),
recent_jobs_check(database.as_ref())?,
runtime_states_check(database.as_ref())?,
Expand Down Expand Up @@ -104,7 +114,8 @@ impl DoctorReport {
}
}

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize)]
#[serde(rename_all = "lowercase")]
enum CheckStatus {
Pass,
Warn,
Expand All @@ -121,12 +132,15 @@ impl CheckStatus {
}
}

#[derive(Serialize)]
struct DoctorCheck {
status: CheckStatus,
name: &'static str,
message: String,
detail: Option<String>,
repair: Option<&'static str>,
#[serde(skip_serializing_if = "Option::is_none")]
routing: Option<PfRoutingDiagnostic>,
}

impl DoctorCheck {
Expand All @@ -137,6 +151,7 @@ impl DoctorCheck {
message: message.into(),
detail: None,
repair: None,
routing: None,
}
}

Expand All @@ -147,6 +162,7 @@ impl DoctorCheck {
message: message.into(),
detail: None,
repair,
routing: None,
}
}

Expand All @@ -157,13 +173,19 @@ impl DoctorCheck {
message: message.into(),
detail: None,
repair: Some(repair),
routing: None,
}
}

fn with_detail(mut self, detail: impl Into<String>) -> Self {
self.detail = Some(detail.into());
self
}

fn with_routing(mut self, routing: PfRoutingDiagnostic) -> Self {
self.routing = Some(routing);
self
}
}

fn layout_check(paths: &PvPaths) -> DoctorCheck {
Expand Down Expand Up @@ -338,95 +360,35 @@ fn dns_check(environment: &impl Environment, paths: &PvPaths) -> Result<DoctorCh
fn ports_check(
environment: &impl Environment,
paths: &PvPaths,
database: Option<&Database>,
) -> Result<DoctorCheck, ExecuteError> {
let prepared_anchor = platform::inspect_pf_anchor_file(&paths.pf_anchor_config(), None);
let prepared_reference =
platform::inspect_pf_conf_reference(&paths.pf_conf_reference_config(), None);
if let Some(check) = pf_file_failure(
"Port redirect config",
"prepared pf config",
&prepared_anchor,
&prepared_reference,
) {
return Ok(check);
}

let expected_anchor = pf_config_from_anchor_state(&prepared_anchor);
let expected_reference = pf_reference_from_state(&prepared_reference);
let system_anchor_path = pf_anchor_path(environment)?;
let system_pf_conf_path = pf_conf_path(environment)?;
let system_anchor =
platform::inspect_pf_anchor_file(&system_anchor_path, expected_anchor.as_ref());
let system_reference =
platform::inspect_pf_conf_reference(&system_pf_conf_path, expected_reference.as_ref());
if let Some(check) = pf_file_failure(
"Port redirect config",
"system pf config",
&system_anchor,
&system_reference,
) {
return Ok(check);
}

let active = match environment.active_pf_redirect_config() {
Ok(active) => active,
Err(error) => {
return Ok(DoctorCheck::fail(
"Port redirect config",
"active pf redirects could not be inspected",
"pv ports:install",
)
.with_detail(error.to_string()));
}
let routing = PfRoutingDiagnostic::read(environment, paths, database)?;
let message = match routing.state {
PfRoutingState::Active => "low-port routing is active",
PfRoutingState::Inactive => "low-port redirects are inactive",
PfRoutingState::Drifted => "low-port routing has drifted",
PfRoutingState::Unknown => "low-port routing could not be verified",
};
let detail = format!(
"evidence: {}; expected: HTTP {}, HTTPS {}; active: HTTP {}, HTTPS {}; observed: {}",
routing.evidence.as_str(),
display_port(routing.expected_http_port),
display_port(routing.expected_https_port),
display_port(routing.active_http_port),
display_port(routing.active_https_port),
routing.observed_at,
);
let check = if routing.is_active() {
DoctorCheck::pass("Port redirect config", message)
} else {
DoctorCheck::fail("Port redirect config", message, "pv ports:install")
};
if active.as_ref() == expected_anchor.as_ref() {
return Ok(DoctorCheck::pass(
"Port redirect config",
"system pf config and active redirects are current",
));
}

Ok(DoctorCheck::fail(
"Port redirect config",
"active pf redirects are not loaded",
"pv ports:install",
))
Ok(check.with_detail(detail).with_routing(routing))
}

fn pf_file_failure(
name: &'static str,
label: &'static str,
anchor: &PfFileState<PfRedirectConfig>,
reference: &PfFileState<PfConfReference>,
) -> Option<DoctorCheck> {
match (anchor, reference) {
(PfFileState::Current { .. }, PfFileState::Current { .. }) => None,
(PfFileState::Missing { path }, _) | (_, PfFileState::Missing { path }) => Some(
DoctorCheck::fail(name, format!("{label} is missing"), "pv ports:install")
.with_detail(format!("path: {path}")),
),
(PfFileState::Conflict { path }, _) | (_, PfFileState::Conflict { path }) => Some(
DoctorCheck::fail(name, format!("{label} is not PV-owned"), "pv ports:install")
.with_detail(format!("path: {path}")),
),
(PfFileState::Unreadable { path, message }, _)
| (_, PfFileState::Unreadable { path, message }) => Some(
DoctorCheck::fail(
name,
format!("{label} could not be inspected"),
"pv ports:install",
)
.with_detail(format!("{path}: {message}")),
),
(PfFileState::Stale { path, .. }, _) | (_, PfFileState::Stale { path, .. }) => Some(
DoctorCheck::fail(
name,
format!("{label} is PV-owned but stale"),
"pv ports:install",
)
.with_detail(format!("path: {path}")),
),
}
fn display_port(port: Option<u16>) -> String {
port.map_or_else(|| "-".to_owned(), |port| port.to_string())
}

fn ca_check(environment: &impl Environment, paths: &PvPaths) -> DoctorCheck {
Expand Down Expand Up @@ -649,26 +611,6 @@ fn resolver_config_from_state(state: &ResolverFileState) -> Option<ResolverConfi
}
}

fn pf_config_from_anchor_state(state: &PfFileState<PfRedirectConfig>) -> Option<PfRedirectConfig> {
match state {
PfFileState::Current { value, .. } => Some(value.clone()),
PfFileState::Missing { .. }
| PfFileState::Stale { .. }
| PfFileState::Conflict { .. }
| PfFileState::Unreadable { .. } => None,
}
}

fn pf_reference_from_state(state: &PfFileState<PfConfReference>) -> Option<PfConfReference> {
match state {
PfFileState::Current { value, .. } => Some(*value),
PfFileState::Missing { .. }
| PfFileState::Stale { .. }
| PfFileState::Conflict { .. }
| PfFileState::Unreadable { .. } => None,
}
}

fn metadata_from_local_ca(state: &CaFileState) -> Option<LocalCaMetadata> {
match state {
CaFileState::Current { metadata, .. } => Some(metadata.clone()),
Expand Down Expand Up @@ -714,13 +656,3 @@ fn resolver_test_path(environment: &impl Environment) -> Result<Utf8PathBuf, Exe
Utf8PathBuf::from_path_buf(environment.resolver_test_path())
.map_err(|path| CliError::NonUtf8Path { path }.into())
}

fn pf_anchor_path(environment: &impl Environment) -> Result<Utf8PathBuf, ExecuteError> {
Utf8PathBuf::from_path_buf(environment.pf_anchor_path())
.map_err(|path| CliError::NonUtf8Path { path }.into())
}

fn pf_conf_path(environment: &impl Environment) -> Result<Utf8PathBuf, ExecuteError> {
Utf8PathBuf::from_path_buf(environment.pf_conf_path())
.map_err(|path| CliError::NonUtf8Path { path }.into())
}
9 changes: 5 additions & 4 deletions crates/cli/src/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ mod jobs;
mod logs;
mod mailpit;
mod mysql;
mod pf_diagnostics;
mod php;
mod ports;
mod postgres;
Expand Down Expand Up @@ -69,7 +70,7 @@ where
Command::DnsStatus => dns::status(environment, stdout),
Command::DnsInstall => dns::install(environment, stdout),
Command::DnsUninstall => dns::uninstall(environment, stdout),
Command::PortsStatus => ports::status(environment, stdout),
Command::PortsStatus(args) => ports::status(args, environment, stdout),
Command::PortsInstall => ports::install(environment, stdout),
Command::PortsUninstall => ports::uninstall(environment, stdout),
Command::CaStatus => ca::status(environment, stdout),
Expand Down Expand Up @@ -165,7 +166,7 @@ fn required_capability(command: &Command) -> Option<PlatformCapability> {
Command::DaemonRun | Command::Link(_) | Command::Unlink(_) => {
Some(PlatformCapability::DaemonIpc)
}
Command::PortsStatus | Command::PortsInstall | Command::PortsUninstall => {
Command::PortsStatus(_) | Command::PortsInstall | Command::PortsUninstall => {
Some(PlatformCapability::LowPortFrontend)
}
Command::CaStatus | Command::CaTrust | Command::CaUntrust => {
Expand Down Expand Up @@ -395,7 +396,7 @@ mod tests {
Command::DaemonDisable,
Command::DaemonRestart,
Command::Status(StatusArgs { json: false }),
Command::Doctor(DoctorArgs {}),
Command::Doctor(DoctorArgs { json: false }),
Command::Update(UpdateArgs {
check: false,
json: false,
Expand Down Expand Up @@ -432,7 +433,7 @@ mod tests {
fn required_capability_maps_low_port_frontend_commands() {
assert_required_capability(
&[
Command::PortsStatus,
Command::PortsStatus(crate::args::PortsStatusArgs { json: false }),
Command::PortsInstall,
Command::PortsUninstall,
],
Expand Down
Loading
Loading