Skip to content

Commit b01dd62

Browse files
authored
fix(server): warn on dangling provider refs at startup (#4661)
* fix(server): warn on dangling provider refs at startup Signed-off-by: Eric Curtin <eric.curtin@docker.com> * fix(server): load providers concurrently at startup Signed-off-by: Eric Curtin <eric.curtin@docker.com> --------- Signed-off-by: Eric Curtin <eric.curtin@docker.com>
1 parent 50b7fcd commit b01dd62

2 files changed

Lines changed: 178 additions & 23 deletions

File tree

‎crates/openshell-server/src/grpc/policy.rs‎

Lines changed: 105 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2541,14 +2541,23 @@ async fn validate_provider_composition_for_existing_sandboxes(
25412541
.expect("catalog was inserted for sandbox workspace");
25422542
let base_policy =
25432543
current_base_policy_for_sandbox(state.store.as_ref(), &sandbox).await?;
2544-
let provider_layers = provider_policy_context_with_catalog(
2544+
// A provider deleted under a sandbox leaves a dangling reference. The
2545+
// sandbox already fails closed, so warn and validate the rest.
2546+
let (records, missing) = super::provider::load_existing_provider_environment_records(
25452547
state.store.as_ref(),
2546-
catalog,
25472548
&workspace,
25482549
provider_names,
25492550
)
2550-
.await?
2551-
.layers;
2551+
.await?;
2552+
if !missing.is_empty() {
2553+
warn!(
2554+
workspace = %workspace,
2555+
sandbox = %sandbox.object_name(),
2556+
providers = %missing.join(","),
2557+
"sandbox references providers that no longer exist"
2558+
);
2559+
}
2560+
let provider_layers = provider_policy_context_from_records(catalog, &records).layers;
25522561
validate_candidate_effective_policy(&base_policy, &provider_layers).map_err(|error| {
25532562
Status::failed_precondition(format!(
25542563
"cannot activate provider policy composition: sandbox '{}/{}' has an invalid effective policy: {}",
@@ -22582,6 +22591,98 @@ mod tests {
2258222591
assert!(error.message().contains("invalid effective policy"));
2258322592
}
2258422593

22594+
async fn install_dangling_provider_sandbox(state: &Arc<ServerState>) {
22595+
state
22596+
.store
22597+
.put_message(&test_sandbox(
22598+
"sandbox-dangling",
22599+
"sandbox-dangling",
22600+
test_policy_with_rule("base", "api.example.com"),
22601+
vec!["deleted-provider".to_string()],
22602+
))
22603+
.await
22604+
.unwrap();
22605+
}
22606+
22607+
#[tokio::test]
22608+
async fn startup_preflight_tolerates_dangling_provider_reference() {
22609+
let state = test_server_state().await;
22610+
install_dangling_provider_sandbox(&state).await;
22611+
22612+
validate_provider_composition_startup_preflight(&state)
22613+
.await
22614+
.expect("a dangling provider reference must not block startup");
22615+
}
22616+
22617+
#[tokio::test]
22618+
async fn startup_preflight_still_rejects_invalid_policy_beside_dangling_reference() {
22619+
let state = test_server_state().await;
22620+
install_dangling_provider_sandbox(&state).await;
22621+
// Sorts after `sandbox-dangling`, so the dangling sandbox is checked first.
22622+
install_ambiguous_provider_binding(&state, "z-beside-dangling").await;
22623+
22624+
let error = validate_provider_composition_startup_preflight(&state)
22625+
.await
22626+
.expect_err("an invalid composed policy must stay fatal");
22627+
22628+
assert_eq!(error.code(), Code::FailedPrecondition);
22629+
assert!(error.message().contains("sandbox-z-beside-dangling"));
22630+
}
22631+
22632+
#[tokio::test]
22633+
async fn startup_preflight_validates_providers_beside_dangling_reference_on_one_sandbox() {
22634+
let state = test_server_state().await;
22635+
install_ambiguous_provider_binding(&state, "mixed").await;
22636+
state
22637+
.store
22638+
.put_message(&test_sandbox(
22639+
"sandbox-mixed",
22640+
"sandbox-mixed",
22641+
test_policy_with_rule("base", "api.example.com"),
22642+
vec!["deleted-provider".to_string(), "provider-mixed".to_string()],
22643+
))
22644+
.await
22645+
.unwrap();
22646+
22647+
let error = validate_provider_composition_startup_preflight(&state)
22648+
.await
22649+
.expect_err("a dangling reference must not skip the sandbox's other providers");
22650+
22651+
assert_eq!(error.code(), Code::FailedPrecondition);
22652+
assert!(error.message().contains("sandbox-mixed"));
22653+
}
22654+
22655+
#[tokio::test]
22656+
async fn deleting_global_policy_tolerates_dangling_provider_reference() {
22657+
let state = test_server_state().await;
22658+
install_dangling_provider_sandbox(&state).await;
22659+
22660+
handle_update_config(
22661+
&state,
22662+
with_user(Request::new(UpdateConfigRequest {
22663+
global: true,
22664+
policy: Some(test_policy_with_rule("global", "global.example.com")),
22665+
..Default::default()
22666+
})),
22667+
)
22668+
.await
22669+
.expect("global policy should suppress provider composition");
22670+
handle_update_config(
22671+
&state,
22672+
with_user(Request::new(UpdateConfigRequest {
22673+
global: true,
22674+
setting_key: POLICY_SETTING_KEY.to_string(),
22675+
delete_setting: true,
22676+
..Default::default()
22677+
})),
22678+
)
22679+
.await
22680+
.expect("a dangling provider reference must not block global policy deletion");
22681+
22682+
let settings = load_global_settings(state.store.as_ref()).await.unwrap();
22683+
assert!(!settings.settings.contains_key(POLICY_SETTING_KEY));
22684+
}
22685+
2258522686
#[test]
2258622687
fn merge_effective_settings_global_overrides_sandbox_key() {
2258722688
let global = StoredSettings {

‎crates/openshell-server/src/grpc/provider.rs‎

Lines changed: 73 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,31 +1027,85 @@ pub(super) async fn resolve_provider_environment_with_credentials(
10271027
resolve_provider_environment_from_records_with_credentials(catalog, &records, credentials).await
10281028
}
10291029

1030+
/// Loads one provider; `Ok(None)` means it does not exist.
1031+
async fn load_provider_environment_record(
1032+
store: &Store,
1033+
workspace: &str,
1034+
name: &str,
1035+
) -> Result<Option<ProviderEnvironmentRecord>, Status> {
1036+
let Some(record) = store
1037+
.get_by_name(Provider::object_type(), workspace, name)
1038+
.await
1039+
.map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))?
1040+
else {
1041+
return Ok(None);
1042+
};
1043+
let provider = Provider::decode(record.payload.as_slice())
1044+
.map_err(|e| Status::internal(format!("failed to decode provider '{name}': {e}")))?;
1045+
let refresh_states =
1046+
crate::provider_refresh::list_refresh_states_for_provider(store, &record.id).await?;
1047+
Ok(Some(ProviderEnvironmentRecord {
1048+
name: name.to_string(),
1049+
object_id: record.id,
1050+
resource_version: record.resource_version,
1051+
provider,
1052+
refresh_states,
1053+
}))
1054+
}
1055+
1056+
/// Loads providers concurrently, in `provider_names` order; a missing
1057+
/// provider is `None`. Store errors short-circuit.
1058+
async fn load_optional_provider_environment_records(
1059+
store: &Store,
1060+
workspace: &str,
1061+
provider_names: &[String],
1062+
) -> Result<Vec<Option<ProviderEnvironmentRecord>>, Status> {
1063+
// Providers are independent, so load them concurrently.
1064+
futures::future::try_join_all(
1065+
provider_names
1066+
.iter()
1067+
.map(|name| load_provider_environment_record(store, workspace, name)),
1068+
)
1069+
.await
1070+
}
1071+
10301072
pub(super) async fn load_provider_environment_records(
10311073
store: &Store,
10321074
workspace: &str,
10331075
provider_names: &[String],
10341076
) -> Result<Vec<ProviderEnvironmentRecord>, Status> {
1035-
// Providers are independent, so load them concurrently.
1036-
futures::future::try_join_all(provider_names.iter().map(|name| async move {
1037-
let record = store
1038-
.get_by_name(Provider::object_type(), workspace, name)
1039-
.await
1040-
.map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))?
1041-
.ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))?;
1042-
let provider = Provider::decode(record.payload.as_slice())
1043-
.map_err(|e| Status::internal(format!("failed to decode provider '{name}': {e}")))?;
1044-
let refresh_states =
1045-
crate::provider_refresh::list_refresh_states_for_provider(store, &record.id).await?;
1046-
Ok(ProviderEnvironmentRecord {
1047-
name: name.clone(),
1048-
object_id: record.id,
1049-
resource_version: record.resource_version,
1050-
provider,
1051-
refresh_states,
1077+
load_optional_provider_environment_records(store, workspace, provider_names)
1078+
.await?
1079+
.into_iter()
1080+
.zip(provider_names)
1081+
.map(|(record, name)| {
1082+
record
1083+
.ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))
10521084
})
1053-
}))
1054-
.await
1085+
.collect()
1086+
}
1087+
1088+
/// Like [`load_provider_environment_records`], but returns the names of
1089+
/// missing providers instead of failing.
1090+
pub(super) async fn load_existing_provider_environment_records<'a>(
1091+
store: &Store,
1092+
workspace: &str,
1093+
provider_names: &'a [String],
1094+
) -> Result<(Vec<ProviderEnvironmentRecord>, Vec<&'a str>), Status> {
1095+
let mut records = Vec::with_capacity(provider_names.len());
1096+
let mut missing = Vec::new();
1097+
for (record, name) in
1098+
load_optional_provider_environment_records(store, workspace, provider_names)
1099+
.await?
1100+
.into_iter()
1101+
.zip(provider_names)
1102+
{
1103+
match record {
1104+
Some(record) => records.push(record),
1105+
None => missing.push(name.as_str()),
1106+
}
1107+
}
1108+
Ok((records, missing))
10551109
}
10561110

10571111
#[cfg(test)]

0 commit comments

Comments
 (0)