Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
109 changes: 105 additions & 4 deletions crates/openshell-server/src/grpc/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2541,14 +2541,23 @@ async fn validate_provider_composition_for_existing_sandboxes(
.expect("catalog was inserted for sandbox workspace");
let base_policy =
current_base_policy_for_sandbox(state.store.as_ref(), &sandbox).await?;
let provider_layers = provider_policy_context_with_catalog(
// A provider deleted under a sandbox leaves a dangling reference. The

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.

This helper also backs the global-policy delete in UpdateConfig (around L4398), so that RPC stops failing on a dangling reference too. I think that's the right call since the sandbox fails closed either way, but it deserves a line in the PR description, and maybe a test next to deleting_global_policy_rejects_reactivated_ambiguous_provider_binding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a line to the description and a global policy delete test.

// sandbox already fails closed, so warn and validate the rest.
let (records, missing) = super::provider::load_existing_provider_environment_records(
state.store.as_ref(),
catalog,
&workspace,
provider_names,
)
.await?
.layers;
.await?;
if !missing.is_empty() {
warn!(
workspace = %workspace,
sandbox = %sandbox.object_name(),
providers = %missing.join(","),
"sandbox references providers that no longer exist"
);
}
let provider_layers = provider_policy_context_from_records(catalog, &records).layers;
validate_candidate_effective_policy(&base_policy, &provider_layers).map_err(|error| {
Status::failed_precondition(format!(
"cannot activate provider policy composition: sandbox '{}/{}' has an invalid effective policy: {}",
Expand Down Expand Up @@ -22582,6 +22591,98 @@ mod tests {
assert!(error.message().contains("invalid effective policy"));
}

async fn install_dangling_provider_sandbox(state: &Arc<ServerState>) {
state
.store
.put_message(&test_sandbox(
"sandbox-dangling",
"sandbox-dangling",
test_policy_with_rule("base", "api.example.com"),
vec!["deleted-provider".to_string()],
))
.await
.unwrap();
}

#[tokio::test]
async fn startup_preflight_tolerates_dangling_provider_reference() {
let state = test_server_state().await;
install_dangling_provider_sandbox(&state).await;

validate_provider_composition_startup_preflight(&state)
.await
.expect("a dangling provider reference must not block startup");
}

#[tokio::test]
async fn startup_preflight_still_rejects_invalid_policy_beside_dangling_reference() {
let state = test_server_state().await;
install_dangling_provider_sandbox(&state).await;
// Sorts after `sandbox-dangling`, so the dangling sandbox is checked first.
install_ambiguous_provider_binding(&state, "z-beside-dangling").await;

let error = validate_provider_composition_startup_preflight(&state)
.await
.expect_err("an invalid composed policy must stay fatal");

assert_eq!(error.code(), Code::FailedPrecondition);
assert!(error.message().contains("sandbox-z-beside-dangling"));
}

#[tokio::test]
async fn startup_preflight_validates_providers_beside_dangling_reference_on_one_sandbox() {
let state = test_server_state().await;
install_ambiguous_provider_binding(&state, "mixed").await;
state
.store
.put_message(&test_sandbox(
"sandbox-mixed",
"sandbox-mixed",
test_policy_with_rule("base", "api.example.com"),
vec!["deleted-provider".to_string(), "provider-mixed".to_string()],
))
.await
.unwrap();

let error = validate_provider_composition_startup_preflight(&state)
.await
.expect_err("a dangling reference must not skip the sandbox's other providers");

assert_eq!(error.code(), Code::FailedPrecondition);
assert!(error.message().contains("sandbox-mixed"));
}

#[tokio::test]
async fn deleting_global_policy_tolerates_dangling_provider_reference() {
let state = test_server_state().await;
install_dangling_provider_sandbox(&state).await;

handle_update_config(
&state,
with_user(Request::new(UpdateConfigRequest {
global: true,
policy: Some(test_policy_with_rule("global", "global.example.com")),
..Default::default()
})),
)
.await
.expect("global policy should suppress provider composition");
handle_update_config(
&state,
with_user(Request::new(UpdateConfigRequest {
global: true,
setting_key: POLICY_SETTING_KEY.to_string(),
delete_setting: true,
..Default::default()
})),
)
.await
.expect("a dangling provider reference must not block global policy deletion");

let settings = load_global_settings(state.store.as_ref()).await.unwrap();
assert!(!settings.settings.contains_key(POLICY_SETTING_KEY));
}

#[test]
fn merge_effective_settings_global_overrides_sandbox_key() {
let global = StoredSettings {
Expand Down
92 changes: 73 additions & 19 deletions crates/openshell-server/src/grpc/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1027,31 +1027,85 @@ pub(super) async fn resolve_provider_environment_with_credentials(
resolve_provider_environment_from_records_with_credentials(catalog, &records, credentials).await
}

/// Loads one provider; `Ok(None)` means it does not exist.
async fn load_provider_environment_record(
store: &Store,
workspace: &str,
name: &str,
) -> Result<Option<ProviderEnvironmentRecord>, Status> {
let Some(record) = store
.get_by_name(Provider::object_type(), workspace, name)
.await
.map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))?
else {
return Ok(None);
};
let provider = Provider::decode(record.payload.as_slice())
.map_err(|e| Status::internal(format!("failed to decode provider '{name}': {e}")))?;
let refresh_states =
crate::provider_refresh::list_refresh_states_for_provider(store, &record.id).await?;
Ok(Some(ProviderEnvironmentRecord {
name: name.to_string(),
object_id: record.id,
resource_version: record.resource_version,
provider,
refresh_states,
}))
}

/// Loads providers concurrently, in `provider_names` order; a missing
/// provider is `None`. Store errors short-circuit.
async fn load_optional_provider_environment_records(
store: &Store,
workspace: &str,
provider_names: &[String],
) -> Result<Vec<Option<ProviderEnvironmentRecord>>, Status> {
// Providers are independent, so load them concurrently.
futures::future::try_join_all(
provider_names
.iter()
.map(|name| load_provider_environment_record(store, workspace, name)),
)
.await
}

pub(super) async fn load_provider_environment_records(
store: &Store,
workspace: &str,
provider_names: &[String],
) -> Result<Vec<ProviderEnvironmentRecord>, Status> {
// Providers are independent, so load them concurrently.
futures::future::try_join_all(provider_names.iter().map(|name| async move {
let record = store
.get_by_name(Provider::object_type(), workspace, name)
.await
.map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))?
.ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))?;
let provider = Provider::decode(record.payload.as_slice())
.map_err(|e| Status::internal(format!("failed to decode provider '{name}': {e}")))?;
let refresh_states =
crate::provider_refresh::list_refresh_states_for_provider(store, &record.id).await?;
Ok(ProviderEnvironmentRecord {
name: name.clone(),
object_id: record.id,
resource_version: record.resource_version,
provider,
refresh_states,
load_optional_provider_environment_records(store, workspace, provider_names)
.await?
.into_iter()
.zip(provider_names)
.map(|(record, name)| {
record
.ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))
})
}))
.await
.collect()
}

/// Like [`load_provider_environment_records`], but returns the names of
/// missing providers instead of failing.
pub(super) async fn load_existing_provider_environment_records<'a>(
store: &Store,
workspace: &str,
provider_names: &'a [String],
) -> Result<(Vec<ProviderEnvironmentRecord>, Vec<&'a str>), Status> {
let mut records = Vec::with_capacity(provider_names.len());
let mut missing = Vec::new();
for (record, name) in
load_optional_provider_environment_records(store, workspace, provider_names)
.await?
.into_iter()
.zip(provider_names)
{
match record {
Some(record) => records.push(record),
None => missing.push(name.as_str()),
}
}
Ok((records, missing))
}

#[cfg(test)]
Expand Down
Loading