diff --git a/architecture/gateway.md b/architecture/gateway.md index 7afec0767..32d86f14b 100644 --- a/architecture/gateway.md +++ b/architecture/gateway.md @@ -216,7 +216,8 @@ modes: mutation. If they diverge it returns `Conflict` without attempting the write. Client-facing operations that carry an `expected_resource_version` field use this mode: `AttachSandboxProvider`, `DetachSandboxProvider`, - `UpdateProvider`, and `UpdateConfig` (policy backfill path). + `UpdateProvider`, `UpdateProviderProfiles`, and `UpdateConfig` (policy + backfill path). **Lists.** The `list_messages` and `list_messages_with_selector` helpers decode protobuf payloads from list results and hydrate `resource_version` from the @@ -235,7 +236,7 @@ coverage: |---|---|---|---| | Sandbox | `MustCreate` | `update_message_cas` | `list_messages` | | Provider | `MustCreate` | `update_message_cas` | `list_messages` | -| ProviderProfile | `MustCreate` | (immutable) | `list_messages` | +| ProviderProfile | `MustCreate` | `MatchResourceVersion` | `list_messages` | | InferenceRoute | `MustCreate` | `update_message_cas` | `list_messages` | | SandboxPolicy | scoped versioning | scoped versioning | scoped query | | Settings | `Mutex`-guarded | `Mutex`-guarded | single-row | @@ -247,7 +248,20 @@ gateways, the Mutex alone would be insufficient. Sandbox-scoped settings rely entirely on CAS without a Mutex. The `resource_version` is surfaced to clients through `ObjectMeta` in proto -responses. Database migrations backfill existing rows with version 1. +responses. Provider profiles are the exception: custom profile get/list/export +responses copy the stored version onto the profile payload so exported YAML can +carry the expected version for safe single-profile updates. Profile update +requests also carry an explicit target profile ID; the payload ID must match the +target so an edited export cannot overwrite a different profile. Database +migrations backfill existing rows with version 1. + +Provider profile imports, updates, and deletes hold the sandbox synchronization +guard while checking attached-sandbox dynamic token grant ambiguity or in-use +state and writing the profile record. Sandbox creation with initial providers and +sandbox provider attach/detach use the same guard, so one gateway process cannot +interleave a profile mutation with a sandbox provider-set mutation that would +leave an ambiguous final dynamic-token state or a deleted custom profile that is +still referenced by a sandbox. Policy and runtime settings are delivered together through the effective sandbox config path. A gateway-global policy can override sandbox-scoped policy. The diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 9b80f1914..d70e9b9da 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -930,6 +930,17 @@ enum ProviderProfileCommands { from: Option, }, + /// Update an existing custom provider profile from a file. + #[command(help_template = LEAF_HELP_TEMPLATE, next_help_heading = "FLAGS")] + Update { + /// Existing provider profile id to update. + id: String, + + /// Profile file to update. + #[arg(short = 'f', long = "file", value_hint = ValueHint::FilePath)] + file: PathBuf, + }, + /// Validate provider profile files without registering them. #[command(group = clap::ArgGroup::new("source").required(true).args(["file", "from"]), help_template = LEAF_HELP_TEMPLATE, next_help_heading = "FLAGS")] Lint { @@ -2927,6 +2938,9 @@ async fn main() -> Result<()> { ) .await?; } + ProviderProfileCommands::Update { id, file } => { + run::provider_profile_update(endpoint, &id, &file, &tls).await?; + } ProviderProfileCommands::Lint { file, from } => { run::provider_profile_lint( endpoint, @@ -3765,6 +3779,26 @@ mod tests { }) )); + let update = Cli::try_parse_from([ + "openshell", + "provider", + "profile", + "update", + "custom-api", + "-f", + "./profiles/custom-api.yaml", + ]) + .expect("provider profile update should parse"); + assert!(matches!( + update.command, + Some(Commands::Provider { + command: Some(ProviderCommands::Profile(ProviderProfileCommands::Update { + id, + file: _ + })) + }) if id == "custom-api" + )); + let delete = Cli::try_parse_from(["openshell", "provider", "profile", "delete", "custom-api"]) .expect("provider profile delete should parse"); diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 6f5520755..a825fcccd 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -51,8 +51,8 @@ use openshell_core::proto::{ RevokeSshSessionRequest, RotateProviderCredentialRequest, Sandbox, SandboxPhase, SandboxPolicy, SandboxSpec, SandboxTemplate, ServiceEndpointResponse, SetClusterInferenceRequest, SettingScope, SettingValue, TcpForwardFrame, TcpForwardInit, TcpRelayTarget, - UpdateConfigRequest, UpdateProviderRequest, WatchSandboxRequest, exec_sandbox_event, - setting_value, tcp_forward_init, + UpdateConfigRequest, UpdateProviderProfilesRequest, UpdateProviderRequest, WatchSandboxRequest, + exec_sandbox_event, setting_value, tcp_forward_init, }; use openshell_core::settings::{self, SettingValueKind}; use openshell_core::{ObjectId, ObjectName}; @@ -4955,6 +4955,21 @@ pub async fn provider_profile_export( output: &str, tls: &TlsOptions, ) -> Result<()> { + let rendered = provider_profile_export_text(server, id, output, tls).await?; + if output == "json" { + println!("{rendered}"); + } else { + print!("{rendered}"); + } + Ok(()) +} + +pub async fn provider_profile_export_text( + server: &str, + id: &str, + output: &str, + tls: &TlsOptions, +) -> Result { let mut client = grpc_client(server, tls).await?; let response = client .get_provider_profile(GetProviderProfileRequest { id: id.to_string() }) @@ -4966,16 +4981,14 @@ pub async fn provider_profile_export( .ok_or_else(|| miette!("provider profile '{id}' not found"))?; let profile = ProviderTypeProfile::from_proto(&profile); - if !crate::output::print_output_direct( - output, - || profile_to_json(&profile).into_diagnostic(), - || profile_to_yaml(&profile).into_diagnostic(), - )? { - return Err(miette!( + match output { + "json" => profile_to_json(&profile).into_diagnostic(), + "yaml" => profile_to_yaml(&profile).into_diagnostic(), + "table" => Err(miette!( "profile export supports '-o yaml' and '-o json'; table output is not supported" - )); + )), + _ => Err(miette!("unsupported output format: {output}")), } - Ok(()) } pub async fn provider_profile_import( @@ -5019,6 +5032,47 @@ pub async fn provider_profile_import( Err(miette!("provider profile import failed")) } +pub async fn provider_profile_update( + server: &str, + id: &str, + file: &Path, + tls: &TlsOptions, +) -> Result<()> { + let (mut items, mut diagnostics) = load_profile_import_items(Some(file), None)?; + if items.is_empty() && diagnostics.is_empty() { + return Err(miette!("no provider profile files found")); + } + if profile_diagnostics_have_errors(&diagnostics) { + print_profile_diagnostics(&diagnostics); + return Err(miette!("provider profile update failed")); + } + + let mut client = grpc_client(server, tls).await?; + if let Some(item) = items.pop() { + let expected_resource_version = item + .profile + .as_ref() + .map_or(0, |profile| profile.resource_version); + let response = client + .update_provider_profiles(UpdateProviderProfilesRequest { + profile: Some(item), + expected_resource_version, + id: id.to_string(), + }) + .await + .into_diagnostic()? + .into_inner(); + diagnostics.extend(response.diagnostics); + if response.updated { + println!("Updated provider profile."); + return Ok(()); + } + } + + print_profile_diagnostics(&diagnostics); + Err(miette!("provider profile update failed")) +} + pub async fn provider_profile_lint( server: &str, file: Option<&Path>, diff --git a/crates/openshell-cli/tests/ensure_providers_integration.rs b/crates/openshell-cli/tests/ensure_providers_integration.rs index ea2d5a465..7bf8612b4 100644 --- a/crates/openshell-cli/tests/ensure_providers_integration.rs +++ b/crates/openshell-cli/tests/ensure_providers_integration.rs @@ -276,6 +276,13 @@ impl OpenShell for TestOpenShell { Err(Status::unimplemented("not implemented in test")) } + async fn update_provider_profiles( + &self, + _request: tonic::Request, + ) -> Result, Status> { + Err(Status::unimplemented("not implemented in test")) + } + async fn lint_provider_profiles( &self, _request: tonic::Request, diff --git a/crates/openshell-cli/tests/mtls_integration.rs b/crates/openshell-cli/tests/mtls_integration.rs index 7cb9e1e76..28a4e6c9c 100644 --- a/crates/openshell-cli/tests/mtls_integration.rs +++ b/crates/openshell-cli/tests/mtls_integration.rs @@ -226,6 +226,13 @@ impl OpenShell for TestOpenShell { Err(Status::unimplemented("not implemented in test")) } + async fn update_provider_profiles( + &self, + _request: tonic::Request, + ) -> Result, Status> { + Err(Status::unimplemented("not implemented in test")) + } + async fn lint_provider_profiles( &self, _request: tonic::Request, diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index 1450b99d4..951ae3bfa 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -455,6 +455,10 @@ impl OpenShell for TestOpenShell { .profiles .into_iter() .filter_map(|item| item.profile) + .map(|mut profile| { + profile.resource_version = 1; + profile + }) .inspect(|profile| { profiles.insert(profile.id.clone(), profile.clone()); }) @@ -468,6 +472,72 @@ impl OpenShell for TestOpenShell { )) } + async fn update_provider_profiles( + &self, + request: tonic::Request, + ) -> Result, Status> { + let mut profiles = self.state.profiles.lock().await; + let request = request.into_inner(); + let mut profile = request + .profile + .and_then(|item| item.profile) + .ok_or_else(|| Status::invalid_argument("provider profile is required"))?; + let target_id = request.id; + if target_id != profile.id { + return Ok(Response::new( + openshell_core::proto::UpdateProviderProfilesResponse { + diagnostics: vec![openshell_core::proto::ProviderProfileDiagnostic { + source: target_id.clone(), + profile_id: profile.id.clone(), + field: "id".to_string(), + message: format!( + "provider profile update target '{}' does not match payload id '{}'", + target_id, profile.id + ), + severity: "error".to_string(), + }], + profile: None, + updated: false, + }, + )); + } + let Some(current) = profiles.get(&target_id) else { + return Ok(Response::new( + openshell_core::proto::UpdateProviderProfilesResponse { + diagnostics: vec![openshell_core::proto::ProviderProfileDiagnostic { + source: target_id.clone(), + profile_id: target_id.clone(), + field: "id".to_string(), + message: format!("custom provider profile '{target_id}' does not exist"), + severity: "error".to_string(), + }], + profile: None, + updated: false, + }, + )); + }; + let expected_resource_version = if request.expected_resource_version != 0 { + request.expected_resource_version + } else { + profile.resource_version + }; + if expected_resource_version == 0 || expected_resource_version != current.resource_version { + return Err(Status::aborted(format!( + "provider profile was modified concurrently (current resource_version: {})", + current.resource_version + ))); + } + profile.resource_version = current.resource_version + 1; + profiles.insert(profile.id.clone(), profile.clone()); + Ok(Response::new( + openshell_core::proto::UpdateProviderProfilesResponse { + diagnostics: Vec::new(), + profile: Some(profile), + updated: true, + }, + )) + } + async fn lint_provider_profiles( &self, _request: tonic::Request, @@ -1366,6 +1436,31 @@ binaries: [/usr/bin/custom] run::provider_profile_import(&ts.endpoint, Some(&profile_path), None, &ts.tls) .await .expect("profile import"); + let exported_yaml = + run::provider_profile_export_text(&ts.endpoint, "custom-api", "yaml", &ts.tls) + .await + .expect("profile export text"); + assert!(exported_yaml.contains("resource_version: 1")); + let updated_yaml = exported_yaml + .replace( + "display_name: Custom API", + "display_name: Custom API Updated", + ) + .replace("host: api.custom.example", "host: api.updated.example"); + std::fs::write(&profile_path, updated_yaml).unwrap(); + run::provider_profile_update(&ts.endpoint, "custom-api", &profile_path, &ts.tls) + .await + .expect("profile update"); + assert_eq!( + ts.state + .profiles + .lock() + .await + .get("custom-api") + .and_then(|profile| profile.endpoints.first()) + .map(|endpoint| endpoint.host.as_str()), + Some("api.updated.example") + ); run::provider_profile_export(&ts.endpoint, "custom-api", "yaml", &ts.tls) .await .expect("profile export"); diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 7061614cb..254f1fd31 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -277,6 +277,13 @@ impl OpenShell for TestOpenShell { Err(Status::unimplemented("not implemented in test")) } + async fn update_provider_profiles( + &self, + _request: tonic::Request, + ) -> Result, Status> { + Err(Status::unimplemented("not implemented in test")) + } + async fn lint_provider_profiles( &self, _request: tonic::Request, diff --git a/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs b/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs index 5e753eff9..d4052ff68 100644 --- a/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs +++ b/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs @@ -262,6 +262,13 @@ impl OpenShell for TestOpenShell { Err(Status::unimplemented("not implemented in test")) } + async fn update_provider_profiles( + &self, + _request: tonic::Request, + ) -> Result, Status> { + Err(Status::unimplemented("not implemented in test")) + } + async fn lint_provider_profiles( &self, _request: tonic::Request, diff --git a/crates/openshell-providers/src/discovery.rs b/crates/openshell-providers/src/discovery.rs index 97a6a911b..ebe75e434 100644 --- a/crates/openshell-providers/src/discovery.rs +++ b/crates/openshell-providers/src/discovery.rs @@ -83,6 +83,7 @@ mod tests { fn profile() -> ProviderTypeProfile { ProviderTypeProfile { id: "custom".to_string(), + resource_version: 0, display_name: "Custom".to_string(), description: String::new(), category: openshell_core::proto::ProviderProfileCategory::Other, diff --git a/crates/openshell-providers/src/profiles.rs b/crates/openshell-providers/src/profiles.rs index d2a35ca80..f66168708 100644 --- a/crates/openshell-providers/src/profiles.rs +++ b/crates/openshell-providers/src/profiles.rs @@ -273,6 +273,8 @@ pub struct BinaryProfile { #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] pub struct ProviderTypeProfile { pub id: String, + #[serde(default, skip_serializing_if = "is_u64_zero")] + pub resource_version: u64, pub display_name: String, #[serde(default)] pub description: String, @@ -303,6 +305,7 @@ impl ProviderTypeProfile { pub fn from_proto(profile: &ProviderProfile) -> Self { Self { id: profile.id.clone(), + resource_version: profile.resource_version, display_name: profile.display_name.clone(), description: profile.description.clone(), category: ProviderProfileCategory::try_from(profile.category) @@ -373,6 +376,7 @@ impl ProviderTypeProfile { pub fn to_proto(&self) -> ProviderProfile { ProviderProfile { id: self.id.clone(), + resource_version: self.resource_version, display_name: self.display_name.clone(), description: self.description.clone(), category: self.category as i32, @@ -410,6 +414,11 @@ impl ProviderTypeProfile { } } +#[allow(clippy::trivially_copy_pass_by_ref)] +fn is_u64_zero(value: &u64) -> bool { + *value == 0 +} + impl CredentialProfile { #[must_use] pub fn is_runtime_resolvable(&self) -> bool { @@ -2395,6 +2404,7 @@ binaries: ["", /usr/bin/broken] "space.yaml".to_string(), ProviderTypeProfile { id: " alex-api ".to_string(), + resource_version: 0, display_name: "Space".to_string(), description: String::new(), category: ProviderProfileCategory::Other, @@ -2409,6 +2419,7 @@ binaries: ["", /usr/bin/broken] "underscore.yaml".to_string(), ProviderTypeProfile { id: "alex_api".to_string(), + resource_version: 0, display_name: "Underscore".to_string(), description: String::new(), category: ProviderProfileCategory::Other, @@ -2423,6 +2434,7 @@ binaries: ["", /usr/bin/broken] "case.yaml".to_string(), ProviderTypeProfile { id: "Alex-API".to_string(), + resource_version: 0, display_name: "Case".to_string(), description: String::new(), category: ProviderProfileCategory::Other, diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 6d687fb7c..c8d76977c 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -282,10 +282,10 @@ impl ComputeRuntime { }) } - /// Serializes sandbox object read-modify-write operations within this - /// gateway process. + /// Serializes sandbox/provider-profile invariant checks and object writes + /// within this gateway process. /// - /// This is a temporary single-gateway guard for full-object sandbox writes. + /// This is a temporary single-gateway guard for cross-object invariants. /// It is not HA-safe; replace it with DB-backed CAS/resource-version writes /// tracked by #1255 before enabling multiple gateway writers. pub(crate) async fn sandbox_sync_guard(&self) -> tokio::sync::OwnedMutexGuard<()> { diff --git a/crates/openshell-server/src/grpc/mod.rs b/crates/openshell-server/src/grpc/mod.rs index 5947bb334..fe2eb331c 100644 --- a/crates/openshell-server/src/grpc/mod.rs +++ b/crates/openshell-server/src/grpc/mod.rs @@ -40,7 +40,8 @@ use openshell_core::proto::{ RotateProviderCredentialRequest, RotateProviderCredentialResponse, SandboxResponse, SandboxStreamEvent, ServiceEndpointResponse, ServiceStatus, SubmitPolicyAnalysisRequest, SubmitPolicyAnalysisResponse, SupervisorMessage, TcpForwardFrame, UndoDraftChunkRequest, - UndoDraftChunkResponse, UpdateConfigRequest, UpdateConfigResponse, UpdateProviderRequest, + UndoDraftChunkResponse, UpdateConfigRequest, UpdateConfigResponse, + UpdateProviderProfilesRequest, UpdateProviderProfilesResponse, UpdateProviderRequest, WatchSandboxRequest, open_shell_server::OpenShell, }; use serde::{Deserialize, Serialize}; @@ -404,6 +405,14 @@ impl OpenShell for OpenShellService { provider::handle_import_provider_profiles(&self.state, request).await } + #[rpc_auth(auth = "bearer", scope = "provider:write", role = "admin")] + async fn update_provider_profiles( + &self, + request: Request, + ) -> Result, Status> { + provider::handle_update_provider_profiles(&self.state, request).await + } + #[rpc_auth(auth = "bearer", scope = "provider:read", role = "user")] async fn lint_provider_profiles( &self, diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 2e2210f44..770bb71cc 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -4445,6 +4445,7 @@ mod tests { }), profile: Some(openshell_core::proto::ProviderProfile { id: "generic".to_string(), + resource_version: 0, display_name: "Generic Override".to_string(), description: String::new(), category: openshell_core::proto::ProviderProfileCategory::Other as i32, @@ -4488,6 +4489,7 @@ mod tests { }), profile: Some(openshell_core::proto::ProviderProfile { id: "custom-api".to_string(), + resource_version: 0, display_name: "Custom API".to_string(), description: String::new(), category: openshell_core::proto::ProviderProfileCategory::Other as i32, @@ -4552,6 +4554,7 @@ mod tests { }), profile: Some(openshell_core::proto::ProviderProfile { id: "custom-api".to_string(), + resource_version: 0, display_name: "Custom API".to_string(), description: String::new(), category: openshell_core::proto::ProviderProfileCategory::Other as i32, @@ -4798,6 +4801,142 @@ mod tests { ); } + #[tokio::test] + async fn sandbox_config_uses_updated_custom_provider_profile_without_rewriting_provider() { + use crate::grpc::provider::handle_update_provider_profiles; + use openshell_core::proto::{ + ProviderProfile, ProviderProfileCategory, ProviderProfileImportItem, + StoredProviderProfile, UpdateProviderProfilesRequest, + }; + + fn stored_profile(host: &str) -> StoredProviderProfile { + StoredProviderProfile { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: "profile-custom-policy".to_string(), + name: "custom-policy".to_string(), + created_at_ms: 1_000_000, + labels: HashMap::new(), + resource_version: 0, + }), + profile: Some(ProviderProfile { + id: "custom-policy".to_string(), + resource_version: 0, + display_name: "Custom Policy".to_string(), + description: String::new(), + category: ProviderProfileCategory::Other as i32, + credentials: Vec::new(), + endpoints: vec![NetworkEndpoint { + host: host.to_string(), + port: 443, + ..Default::default() + }], + binaries: Vec::new(), + inference_capable: false, + discovery: None, + }), + } + } + + let state = test_server_state().await; + enable_providers_v2(&state).await; + state + .store + .put_message(&stored_profile("api.before.example")) + .await + .unwrap(); + let provider = test_provider("work-custom", "custom-policy"); + state.store.put_message(&provider).await.unwrap(); + state + .store + .put_message(&test_sandbox( + "sb-custom-policy-update", + "custom-policy-update", + test_policy_with_rule("sandbox_only", "sandbox.example.com"), + vec!["work-custom".to_string()], + )) + .await + .unwrap(); + + let before_policy = get_sandbox_policy(&state, "sb-custom-policy-update").await; + assert!( + before_policy.network_policies["_provider_work_custom"] + .endpoints + .iter() + .any(|endpoint| endpoint.host == "api.before.example") + ); + + let mut updated_profile = stored_profile("api.after.example").profile.unwrap(); + updated_profile.resource_version = state + .store + .get_message_by_name::("custom-policy") + .await + .unwrap() + .unwrap() + .metadata + .as_ref() + .unwrap() + .resource_version; + let response = handle_update_provider_profiles( + &state, + with_user(Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(updated_profile), + source: "custom-policy.yaml".to_string(), + }), + expected_resource_version: 0, + id: "custom-policy".to_string(), + })), + ) + .await + .unwrap() + .into_inner(); + assert!(response.updated); + + let after_policy = get_sandbox_policy(&state, "sb-custom-policy-update").await; + let provider_rule = &after_policy.network_policies["_provider_work_custom"]; + assert!( + provider_rule + .endpoints + .iter() + .any(|endpoint| endpoint.host == "api.after.example") + ); + assert!( + !provider_rule + .endpoints + .iter() + .any(|endpoint| endpoint.host == "api.before.example") + ); + + let persisted_provider: Provider = state + .store + .get_message_by_name("work-custom") + .await + .unwrap() + .unwrap(); + assert_eq!(persisted_provider.r#type, provider.r#type); + assert_eq!(persisted_provider.credentials, provider.credentials); + + let persisted_policy = state + .store + .get_latest_policy("sb-custom-policy-update") + .await + .unwrap() + .expect("sandbox policy should be lazily backfilled"); + let persisted_policy = + ProtoSandboxPolicy::decode(persisted_policy.policy_payload.as_slice()) + .expect("persisted sandbox policy should decode"); + assert!( + persisted_policy + .network_policies + .contains_key("sandbox_only") + ); + assert!( + !persisted_policy + .network_policies + .contains_key("_provider_work_custom") + ); + } + #[tokio::test] async fn sandbox_config_preserves_overlapping_user_and_provider_rules() { let state = test_server_state().await; @@ -4946,9 +5085,11 @@ mod tests { #[tokio::test] async fn provider_env_revision_changes_when_custom_profile_token_grant_changes() { + use crate::grpc::provider::handle_update_provider_profiles; use openshell_core::proto::{ ProviderCredentialTokenGrant, ProviderProfile, ProviderProfileCategory, - ProviderProfileCredential, StoredProviderProfile, + ProviderProfileCredential, ProviderProfileImportItem, StoredProviderProfile, + UpdateProviderProfilesRequest, }; use std::time::Duration; @@ -4963,6 +5104,7 @@ mod tests { }), profile: Some(ProviderProfile { id: "custom-token".to_string(), + resource_version: 0, display_name: "Custom Token".to_string(), description: String::new(), category: ProviderProfileCategory::Other as i32, @@ -5007,13 +5149,32 @@ mod tests { .unwrap(); tokio::time::sleep(Duration::from_millis(2)).await; - state + let mut rotated_profile = token_grant_profile("https://auth.example.com/rotated-token") + .profile + .unwrap(); + rotated_profile.resource_version = state .store - .put_message(&token_grant_profile( - "https://auth.example.com/rotated-token", - )) + .get_message_by_name::("custom-token") .await - .unwrap(); + .unwrap() + .unwrap() + .metadata + .as_ref() + .unwrap() + .resource_version; + handle_update_provider_profiles( + &state, + with_user(Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(rotated_profile), + source: "custom-token.yaml".to_string(), + }), + expected_resource_version: 0, + id: "custom-token".to_string(), + })), + ) + .await + .unwrap(); let second = compute_provider_env_revision(state.store.as_ref(), &["work-custom-token".to_string()]) @@ -5163,6 +5324,7 @@ mod tests { source: "custom-api.yaml".to_string(), profile: Some(ProviderProfile { id: "custom-api".to_string(), + resource_version: 0, display_name: "Custom API".to_string(), description: String::new(), category: ProviderProfileCategory::Other as i32, @@ -7172,6 +7334,7 @@ mod tests { }), profile: Some(ProviderProfile { id: "custom-api".to_string(), + resource_version: 0, display_name: "Custom API".to_string(), description: String::new(), category: ProviderProfileCategory::Other as i32, diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 641118206..7e84384c4 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -1181,7 +1181,8 @@ use openshell_core::proto::{ ListProviderProfilesResponse, ListProvidersRequest, ListProvidersResponse, ProviderCredentialRefreshStrategy, ProviderProfileDiagnostic, ProviderProfileImportItem, ProviderProfileResponse, ProviderResponse, RotateProviderCredentialRequest, - RotateProviderCredentialResponse, StoredProviderProfile, UpdateProviderRequest, + RotateProviderCredentialResponse, StoredProviderProfile, UpdateProviderProfilesRequest, + UpdateProviderProfilesResponse, UpdateProviderRequest, }; use openshell_providers::{ CredentialRefreshProfile, ProfileValidationDiagnostic, ProviderTypeProfile, default_profiles, @@ -1298,11 +1299,12 @@ pub(super) async fn handle_import_provider_profiles( let request = request.into_inner(); let (profiles, mut diagnostics) = profiles_from_import_items(&request.profiles); add_empty_profile_set_diagnostic(&profiles, &mut diagnostics); + let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await; diagnostics.extend(profile_conflict_diagnostics(state.store.as_ref(), &profiles).await?); diagnostics.extend(validate_profile_set(&profiles)); if !has_errors(&diagnostics) { diagnostics.extend( - profile_import_attached_sandbox_diagnostics(state.store.as_ref(), &profiles).await?, + profile_attached_sandbox_diagnostics(state.store.as_ref(), &profiles, "import").await?, ); } @@ -1316,8 +1318,8 @@ pub(super) async fn handle_import_provider_profiles( let mut imported = Vec::with_capacity(profiles.len()); for (_, profile) in profiles { - let stored = stored_provider_profile(profile.to_proto()); - state + let mut stored = stored_provider_profile(profile.to_proto()); + let result = state .store .put_if( StoredProviderProfile::object_type(), @@ -1329,7 +1331,14 @@ pub(super) async fn handle_import_provider_profiles( ) .await .map_err(|e| Status::internal(format!("persist provider profile failed: {e}")))?; - imported.push(stored.profile.unwrap_or_default()); + if let Some(metadata) = stored.metadata.as_mut() { + metadata.resource_version = result.resource_version; + } + let resource_version = stored_profile_resource_version(&stored); + imported.push(profile_response_payload( + stored.profile.unwrap_or_default(), + resource_version, + )); } Ok(Response::new(ImportProviderProfilesResponse { @@ -1339,6 +1348,111 @@ pub(super) async fn handle_import_provider_profiles( })) } +pub(super) async fn handle_update_provider_profiles( + state: &Arc, + request: Request, +) -> Result, Status> { + let request = request.into_inner(); + let items = request.profile.into_iter().collect::>(); + let (profiles, mut diagnostics) = profiles_from_import_items(&items); + add_empty_profile_set_diagnostic(&profiles, &mut diagnostics); + let target_id = normalize_profile_id_request(&request.id)?; + diagnostics.extend( + profile_update_target_diagnostics(state.store.as_ref(), &profiles, &target_id).await?, + ); + diagnostics.extend(validate_profile_set(&profiles)); + let expected_resource_version = if request.expected_resource_version != 0 { + Some(request.expected_resource_version) + } else { + profiles + .first() + .map(|(_, profile)| profile.resource_version) + .filter(|version| *version != 0) + }; + if expected_resource_version.is_none() && !profiles.is_empty() { + let (source, profile) = &profiles[0]; + diagnostics.push(ProfileValidationDiagnostic { + source: source.clone(), + profile_id: profile.id.clone(), + field: "resource_version".to_string(), + message: "custom provider profile update requires a non-zero resource_version; export the current profile before editing it".to_string(), + severity: "error".to_string(), + }); + } + let _sandbox_sync_guard = if has_errors(&diagnostics) { + None + } else { + Some(state.compute.sandbox_sync_guard().await) + }; + if !has_errors(&diagnostics) { + diagnostics.extend( + profile_attached_sandbox_diagnostics(state.store.as_ref(), &profiles, "update").await?, + ); + } + + if has_errors(&diagnostics) { + return Ok(Response::new(UpdateProviderProfilesResponse { + diagnostics: diagnostics.into_iter().map(proto_diagnostic).collect(), + profile: None, + updated: false, + })); + } + + let expected_resource_version = expected_resource_version.unwrap_or_default(); + let (_, profile) = profiles + .into_iter() + .next() + .ok_or_else(|| Status::internal("validated provider profile update is missing"))?; + let mut stored = state + .store + .get_message_by_name::(&target_id) + .await + .map_err(|e| Status::internal(format!("fetch provider profile failed: {e}")))? + .ok_or_else(|| Status::not_found("provider profile not found"))?; + let current_version = stored + .metadata + .as_ref() + .map_or(0, |metadata| metadata.resource_version); + if current_version != expected_resource_version { + return Err(Status::aborted(format!( + "provider profile was modified concurrently (current resource_version: {current_version})" + ))); + } + + stored.profile = Some(profile_storage_payload(profile.to_proto())); + let labels_json = stored + .object_labels() + .filter(|labels| !labels.is_empty()) + .map(|labels| { + serde_json::to_string(&labels) + .map_err(|e| Status::internal(format!("serialize labels failed: {e}"))) + }) + .transpose()?; + let result = state + .store + .put_if( + StoredProviderProfile::object_type(), + stored.object_id(), + stored.object_name(), + &stored.encode_to_vec(), + labels_json.as_deref(), + WriteCondition::MatchResourceVersion(expected_resource_version), + ) + .await + .map_err(|e| super::persistence_error_to_status(e, "update provider profile"))?; + if let Some(metadata) = stored.metadata.as_mut() { + metadata.resource_version = result.resource_version; + } + let resource_version = stored_profile_resource_version(&stored); + let profile = profile_response_payload(stored.profile.unwrap_or_default(), resource_version); + + Ok(Response::new(UpdateProviderProfilesResponse { + diagnostics: Vec::new(), + profile: Some(profile), + updated: true, + })) +} + pub(super) async fn handle_lint_provider_profiles( state: &Arc, request: Request, @@ -1368,6 +1482,7 @@ pub(super) async fn handle_delete_provider_profile( )); } + let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await; let existing = state .store .get_message_by_name::(&id) @@ -1408,8 +1523,15 @@ pub(super) async fn get_provider_type_profile( .get_message_by_name::(&id) .await .map_err(|e| Status::internal(format!("fetch provider profile failed: {e}")))? - .and_then(|stored| stored.profile) - .map(|profile| ProviderTypeProfile::from_proto(&profile)); + .and_then(|stored| { + let resource_version = stored_profile_resource_version(&stored); + stored.profile.map(|profile| { + ProviderTypeProfile::from_proto(&profile_response_payload( + profile, + resource_version, + )) + }) + }); Ok(profile) } @@ -1475,8 +1597,15 @@ async fn merged_provider_profiles(store: &Store) -> Result Result, Status> { + let mut diagnostics = Vec::new(); + if profiles.len() == 1 { + let (source, profile) = &profiles[0]; + if let Some(payload_id) = normalize_profile_id(&profile.id) + && payload_id != target_id + { + diagnostics.push(ProfileValidationDiagnostic { + source: source.clone(), + profile_id: profile.id.clone(), + field: "id".to_string(), + message: format!( + "provider profile update target '{target_id}' does not match payload id '{payload_id}'" + ), + severity: "error".to_string(), + }); + } + } + if get_default_profile(target_id).is_some() { + diagnostics.push(ProfileValidationDiagnostic { + source: target_id.to_string(), + profile_id: target_id.to_string(), + field: "id".to_string(), + message: format!("provider profile '{target_id}' is built-in and cannot be updated"), + severity: "error".to_string(), + }); + return Ok(diagnostics); + } + if store + .get_message_by_name::(target_id) + .await + .map_err(|e| Status::internal(format!("fetch provider profile failed: {e}")))? + .is_none() + { + diagnostics.push(ProfileValidationDiagnostic { + source: target_id.to_string(), + profile_id: target_id.to_string(), + field: "id".to_string(), + message: format!("custom provider profile '{target_id}' does not exist"), + severity: "error".to_string(), + }); + } + for (source, profile) in profiles { + let Some(id) = normalize_profile_id(&profile.id) else { + continue; + }; + if get_default_profile(&id).is_some() { + diagnostics.push(ProfileValidationDiagnostic { + source: source.clone(), + profile_id: id.clone(), + field: "id".to_string(), + message: format!("provider profile '{id}' is built-in and cannot be updated"), + severity: "error".to_string(), + }); + } + } + Ok(diagnostics) +} + +async fn profile_attached_sandbox_diagnostics( store: &Store, profiles: &[(String, ProviderTypeProfile)], + operation: &str, ) -> Result, Status> { let mut candidate_profiles = std::collections::HashMap::::new(); @@ -1640,7 +1833,7 @@ async fn profile_import_attached_sandbox_diagnostics( profile_id: profile_id.clone(), field: "credentials.token_grant.audience_overrides".to_string(), message: format!( - "import would create ambiguous dynamic token grants on sandbox '{sandbox_name}': {}", + "{operation} would create ambiguous dynamic token grants on sandbox '{sandbox_name}': {}", err.message() ), severity: "error".to_string(), @@ -1655,6 +1848,7 @@ async fn profile_import_attached_sandbox_diagnostics( fn stored_provider_profile(profile: ProviderProfile) -> StoredProviderProfile { use crate::persistence::current_time_ms; let now_ms = current_time_ms(); + let profile = profile_storage_payload(profile); StoredProviderProfile { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: uuid::Uuid::new_v4().to_string(), @@ -1667,6 +1861,26 @@ fn stored_provider_profile(profile: ProviderProfile) -> StoredProviderProfile { } } +fn profile_storage_payload(mut profile: ProviderProfile) -> ProviderProfile { + profile.resource_version = 0; + profile +} + +fn profile_response_payload( + mut profile: ProviderProfile, + resource_version: u64, +) -> ProviderProfile { + profile.resource_version = resource_version; + profile +} + +fn stored_profile_resource_version(stored: &StoredProviderProfile) -> u64 { + stored + .metadata + .as_ref() + .map_or(0, |metadata| metadata.resource_version) +} + fn proto_diagnostic(diagnostic: ProfileValidationDiagnostic) -> ProviderProfileDiagnostic { ProviderProfileDiagnostic { source: diagnostic.source, @@ -2174,7 +2388,8 @@ mod tests { NetworkEndpoint, ProviderCredentialRefresh, ProviderCredentialRefreshMaterial, ProviderCredentialTokenGrant, ProviderCredentialTokenGrantAudienceOverride, ProviderProfile, ProviderProfileCategory, ProviderProfileCredential, - ProviderProfileImportItem, Sandbox, SandboxSpec, + ProviderProfileImportItem, Sandbox, SandboxSpec, StoredProviderProfile, + UpdateProviderProfilesRequest, }; use openshell_core::{ObjectId, ObjectName}; use std::collections::HashMap; @@ -2280,6 +2495,7 @@ mod tests { }; let profile = ProviderProfile { id: "keycloak-sso".to_string(), + resource_version: 0, display_name: "Keycloak SSO".to_string(), description: String::new(), category: ProviderProfileCategory::Other as i32, @@ -2473,6 +2689,332 @@ mod tests { })); } + #[tokio::test] + async fn import_provider_profile_waits_for_sandbox_sync_guard() { + let state = test_server_state().await; + let guard = state.compute.sandbox_sync_guard().await; + let task_state = state.clone(); + let task = tokio::spawn(async move { + handle_import_provider_profiles( + &task_state, + Request::new(ImportProviderProfilesRequest { + profiles: vec![ProviderProfileImportItem { + profile: Some(custom_profile("guarded-import")), + source: "guarded-import.yaml".to_string(), + }], + }), + ) + .await + }); + + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + assert!( + !task.is_finished(), + "profile import should wait for sandbox sync guard" + ); + drop(guard); + + let response = tokio::time::timeout(std::time::Duration::from_secs(5), task) + .await + .expect("import should finish after guard release") + .expect("join import task") + .expect("import should succeed") + .into_inner(); + assert!(response.imported); + } + + #[tokio::test] + async fn update_provider_profile_replaces_custom_profile_and_preserves_metadata() { + let state = test_server_state().await; + let mut original = custom_profile("custom-api"); + original.display_name = "Original API".to_string(); + let mut stored = stored_provider_profile(original); + stored.metadata.as_mut().unwrap().labels = + HashMap::from([("team".to_string(), "platform".to_string())]); + state.store.put_message(&stored).await.unwrap(); + let before: StoredProviderProfile = state + .store + .get_message_by_name("custom-api") + .await + .unwrap() + .unwrap(); + + let mut updated_profile = custom_profile("custom-api"); + updated_profile.resource_version = before.metadata.as_ref().unwrap().resource_version; + updated_profile.display_name = "Updated API".to_string(); + updated_profile.endpoints = vec![NetworkEndpoint { + host: "api.updated.example".to_string(), + port: 443, + ..Default::default() + }]; + let response = handle_update_provider_profiles( + &state, + Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(updated_profile.clone()), + source: "custom-api.yaml".to_string(), + }), + expected_resource_version: 0, + id: "custom-api".to_string(), + }), + ) + .await + .unwrap() + .into_inner(); + + assert!(response.updated); + assert_eq!(response.profile.as_ref().unwrap().id, updated_profile.id); + assert_eq!( + response.profile.as_ref().unwrap().display_name, + updated_profile.display_name + ); + let after: StoredProviderProfile = state + .store + .get_message_by_name("custom-api") + .await + .unwrap() + .unwrap(); + let before_meta = before.metadata.unwrap(); + let after_meta = after.metadata.unwrap(); + assert_eq!(after_meta.id, before_meta.id); + assert_eq!(after_meta.name, before_meta.name); + assert_eq!(after_meta.created_at_ms, before_meta.created_at_ms); + assert_eq!(after_meta.labels, before_meta.labels); + assert!(after_meta.resource_version > before_meta.resource_version); + assert_eq!( + after.profile.unwrap().endpoints[0].host, + "api.updated.example" + ); + } + + #[tokio::test] + async fn update_provider_profile_rejects_built_in_and_missing_profiles() { + let state = test_server_state().await; + + let built_in = handle_update_provider_profiles( + &state, + Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(custom_profile("github")), + source: "github.yaml".to_string(), + }), + expected_resource_version: 0, + id: "github".to_string(), + }), + ) + .await + .unwrap() + .into_inner(); + assert!(!built_in.updated); + assert!(built_in.diagnostics.iter().any(|diagnostic| { + diagnostic + .message + .contains("built-in and cannot be updated") + })); + + let missing = handle_update_provider_profiles( + &state, + Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(custom_profile("missing-custom")), + source: "missing-custom.yaml".to_string(), + }), + expected_resource_version: 0, + id: "missing-custom".to_string(), + }), + ) + .await + .unwrap() + .into_inner(); + assert!(!missing.updated); + assert!(missing.diagnostics.iter().any(|diagnostic| { + diagnostic + .message + .contains("custom provider profile 'missing-custom' does not exist") + })); + } + + #[tokio::test] + async fn update_provider_profile_requires_current_resource_version() { + let state = test_server_state().await; + state + .store + .put_message(&stored_provider_profile(custom_profile("custom-api"))) + .await + .unwrap(); + + let missing_version = handle_update_provider_profiles( + &state, + Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(custom_profile("custom-api")), + source: "custom-api.yaml".to_string(), + }), + expected_resource_version: 0, + id: "custom-api".to_string(), + }), + ) + .await + .unwrap() + .into_inner(); + assert!(!missing_version.updated); + assert!(missing_version.diagnostics.iter().any(|diagnostic| { + diagnostic.field == "resource_version" + && diagnostic.message.contains("non-zero resource_version") + })); + + let mut stale_profile = custom_profile("custom-api"); + stale_profile.resource_version = 99; + let stale_error = handle_update_provider_profiles( + &state, + Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(stale_profile), + source: "custom-api.yaml".to_string(), + }), + expected_resource_version: 0, + id: "custom-api".to_string(), + }), + ) + .await + .unwrap_err(); + assert_eq!(stale_error.code(), Code::Aborted); + assert!(stale_error.message().contains("resource_version")); + } + + #[tokio::test] + async fn update_provider_profile_rejects_payload_id_change() { + let state = test_server_state().await; + let mut profile_a = custom_profile("profile-a"); + profile_a.display_name = "Profile A".to_string(); + let mut profile_b = custom_profile("profile-b"); + profile_b.display_name = "Profile B".to_string(); + state + .store + .put_message(&stored_provider_profile(profile_a)) + .await + .unwrap(); + state + .store + .put_message(&stored_provider_profile(profile_b)) + .await + .unwrap(); + let profile_a_version = state + .store + .get_message_by_name::("profile-a") + .await + .unwrap() + .unwrap() + .metadata + .as_ref() + .unwrap() + .resource_version; + + let mut edited_payload = custom_profile("profile-b"); + edited_payload.resource_version = profile_a_version; + edited_payload.display_name = "Wrong overwrite".to_string(); + let response = handle_update_provider_profiles( + &state, + Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(edited_payload), + source: "profile-a.yaml".to_string(), + }), + expected_resource_version: 0, + id: "profile-a".to_string(), + }), + ) + .await + .unwrap() + .into_inner(); + + assert!(!response.updated); + assert!(response.diagnostics.iter().any(|diagnostic| { + diagnostic.field == "id" + && diagnostic + .message + .contains("does not match payload id 'profile-b'") + })); + let stored_b: StoredProviderProfile = state + .store + .get_message_by_name("profile-b") + .await + .unwrap() + .unwrap(); + assert_eq!(stored_b.profile.unwrap().display_name, "Profile B"); + } + + #[tokio::test] + async fn update_provider_profile_rejects_attached_dynamic_binding_ambiguity() { + let state = test_server_state().await; + let store = state.store.as_ref(); + import_token_grant_profile(&state, "grant-existing", "api.example.com", 443, "/v1/**") + .await; + import_token_grant_profile(&state, "grant-updated", "api.example.com", 443, "/v2/**").await; + create_empty_token_grant_provider(store, "provider-existing", "grant-existing").await; + create_empty_token_grant_provider(store, "provider-updated", "grant-updated").await; + store + .put_message(&Sandbox { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: "sandbox-update-ambiguity-id".to_string(), + name: "sandbox-update-ambiguity".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, + }), + spec: Some(SandboxSpec { + providers: vec![ + "provider-existing".to_string(), + "provider-updated".to_string(), + ], + ..Default::default() + }), + ..Default::default() + }) + .await + .unwrap(); + + let mut profile = custom_profile("grant-updated"); + profile.resource_version = store + .get_message_by_name::("grant-updated") + .await + .unwrap() + .unwrap() + .metadata + .as_ref() + .unwrap() + .resource_version; + profile.credentials = vec![token_grant_credential("access_token")]; + profile.endpoints = vec![NetworkEndpoint { + host: "api.example.com".to_string(), + port: 443, + path: "/v1/**".to_string(), + protocol: "rest".to_string(), + ..Default::default() + }]; + let response = handle_update_provider_profiles( + &state, + Request::new(UpdateProviderProfilesRequest { + profile: Some(ProviderProfileImportItem { + profile: Some(profile), + source: "grant-updated.yaml".to_string(), + }), + expected_resource_version: 0, + id: "grant-updated".to_string(), + }), + ) + .await + .unwrap() + .into_inner(); + + assert!(!response.updated); + assert!(response.diagnostics.iter().any(|diagnostic| { + diagnostic + .message + .contains("update would create ambiguous dynamic token grants") + })); + } + fn provider_with_values(name: &str, provider_type: &str) -> Provider { Provider { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { @@ -2502,6 +3044,7 @@ mod tests { fn custom_profile(id: &str) -> ProviderProfile { ProviderProfile { id: id.to_string(), + resource_version: 0, display_name: format!("{id} Profile"), description: String::new(), category: ProviderProfileCategory::Other as i32, @@ -2937,6 +3480,7 @@ mod tests { profiles: vec![ProviderProfileImportItem { profile: Some(ProviderProfile { id: "advanced-api".to_string(), + resource_version: 0, display_name: "Advanced API".to_string(), description: String::new(), category: ProviderProfileCategory::Other as i32, @@ -3716,6 +4260,43 @@ mod tests { assert_eq!(missing.code(), Code::NotFound); } + #[tokio::test] + async fn delete_provider_profile_waits_for_sandbox_sync_guard() { + let state = test_server_state().await; + state + .store + .put_message(&stored_provider_profile(custom_profile("guarded-delete"))) + .await + .unwrap(); + + let guard = state.compute.sandbox_sync_guard().await; + let task_state = state.clone(); + let task = tokio::spawn(async move { + handle_delete_provider_profile( + &task_state, + Request::new(DeleteProviderProfileRequest { + id: "guarded-delete".to_string(), + }), + ) + .await + }); + + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + assert!( + !task.is_finished(), + "profile delete should wait for sandbox sync guard" + ); + drop(guard); + + let response = tokio::time::timeout(std::time::Duration::from_secs(5), task) + .await + .expect("delete should finish after guard release") + .expect("join delete task") + .expect("delete should succeed") + .into_inner(); + assert!(response.deleted); + } + #[tokio::test] async fn provider_crud_round_trip_and_semantics() { let store = test_store().await; @@ -4014,6 +4595,7 @@ mod tests { profiles: vec![ProviderProfileImportItem { profile: Some(ProviderProfile { id: "delegated-refresh-api".to_string(), + resource_version: 0, display_name: "Delegated Refresh API".to_string(), description: String::new(), category: ProviderProfileCategory::Messaging as i32, diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index e60ce3995..7a1482e59 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -132,6 +132,12 @@ async fn handle_create_sandbox_inner( crate::grpc::validation::validate_label_value(value)?; } + let _sandbox_sync_guard = if spec.providers.is_empty() { + None + } else { + Some(state.compute.sandbox_sync_guard().await) + }; + // Validate provider names exist (fail fast). for name in &spec.providers { state @@ -2588,6 +2594,51 @@ mod tests { assert!(err.message().contains("provider-b")); } + #[tokio::test] + async fn create_sandbox_with_providers_waits_for_sandbox_sync_guard() { + let state = test_server_state().await; + state + .store + .put_message(&test_provider("work-github", "github")) + .await + .unwrap(); + + let guard = state.compute.sandbox_sync_guard().await; + let task_state = state.clone(); + let task = tokio::spawn(async move { + handle_create_sandbox( + &task_state, + Request::new(CreateSandboxRequest { + name: "guarded-create".to_string(), + spec: Some(openshell_core::proto::SandboxSpec { + providers: vec!["work-github".to_string()], + ..Default::default() + }), + labels: HashMap::new(), + }), + ) + .await + }); + + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + assert!( + !task.is_finished(), + "sandbox create with initial providers should wait for sandbox sync guard" + ); + drop(guard); + + let response = tokio::time::timeout(std::time::Duration::from_secs(5), task) + .await + .expect("create should finish after guard release") + .expect("join create task") + .expect("create should succeed") + .into_inner(); + assert_eq!( + response.sandbox.unwrap().spec.unwrap().providers, + vec!["work-github".to_string()] + ); + } + #[tokio::test] async fn attach_sandbox_provider_rejects_credential_key_collisions() { let state = test_server_state().await; diff --git a/crates/openshell-server/tests/common/mod.rs b/crates/openshell-server/tests/common/mod.rs index 00228b043..0f01a0503 100644 --- a/crates/openshell-server/tests/common/mod.rs +++ b/crates/openshell-server/tests/common/mod.rs @@ -232,6 +232,13 @@ impl OpenShell for TestOpenShell { Err(Status::unimplemented("not implemented in test")) } + async fn update_provider_profiles( + &self, + _request: tonic::Request, + ) -> Result, Status> { + Err(Status::unimplemented("not implemented in test")) + } + async fn lint_provider_profiles( &self, _request: tonic::Request, diff --git a/crates/openshell-server/tests/supervisor_relay_integration.rs b/crates/openshell-server/tests/supervisor_relay_integration.rs index dadb8b384..bd94d151e 100644 --- a/crates/openshell-server/tests/supervisor_relay_integration.rs +++ b/crates/openshell-server/tests/supervisor_relay_integration.rs @@ -257,6 +257,13 @@ impl OpenShell for RelayGateway { Err(Status::unimplemented("unused")) } + async fn update_provider_profiles( + &self, + _: tonic::Request, + ) -> Result, Status> { + Err(Status::unimplemented("unused")) + } + async fn lint_provider_profiles( &self, _: tonic::Request, diff --git a/docs/sandboxes/manage-providers.mdx b/docs/sandboxes/manage-providers.mdx index 67fa8f23a..42daab920 100644 --- a/docs/sandboxes/manage-providers.mdx +++ b/docs/sandboxes/manage-providers.mdx @@ -75,6 +75,21 @@ have a built-in or imported provider profile with a `discovery` section. If no matching profile exists, the CLI returns an error instead of falling back to legacy discovery. +Update a custom provider profile after exporting it, editing its endpoints, +binaries, or credential metadata, and preserving the exported `resource_version`: + +```shell +openshell provider profile export my-api -o yaml > my-api-profile.yaml +openshell provider profile update my-api -f my-api-profile.yaml +``` + +Import remains create-only and fails if the profile ID already exists. Use +`provider profile update ` for existing custom profiles. Built-in profiles are +read-only. The target ID must match the profile ID in the file. Update accepts +one file at a time and rejects stale resource versions. When +`providers_v2_enabled=true`, updated profile policy applies to all provider +instances of that type on the next sandbox config sync. + ## Manage Providers List, inspect, update, and delete providers from the active gateway. diff --git a/docs/sandboxes/providers-v2.mdx b/docs/sandboxes/providers-v2.mdx index 1456c5cfa..8360df96f 100644 --- a/docs/sandboxes/providers-v2.mdx +++ b/docs/sandboxes/providers-v2.mdx @@ -53,7 +53,7 @@ Providers v2 currently includes these user-facing features: - Built-in provider profiles stored in the `providers/` directory of the GitHub repository. - `openshell provider list-profiles` with table, YAML, and JSON output. -- `openshell provider profile export`, `import`, `lint`, and `delete` for custom profiles. +- `openshell provider profile export`, `import`, `update`, `lint`, and `delete` for custom profiles. - Provider instances created from built-in or imported profile IDs with `openshell provider create --type `. - Profile-backed credential discovery for explicit `openshell provider create --from-existing` and `openshell provider update --from-existing` flows. The built-in `google-vertex-ai` profile also supplements discovery with Vertex config env vars such as `VERTEX_AI_PROJECT_ID` and `VERTEX_AI_REGION`. - Just-in-time effective policy composition from sandbox policy plus attached provider profiles. @@ -126,7 +126,18 @@ Import all non-recursive `*.yaml`, `*.yml`, and `*.json` files from a directory: openshell provider profile import --from ./provider-profiles ``` -Custom profile IDs must use lowercase kebab-case with `a-z`, `0-9`, and `-`. Built-in profile IDs and legacy provider aliases are reserved. Built-in profiles are read-only, and OpenShell rejects deleting a custom profile while a sandbox-attached provider uses it. +Import is create-only. It fails if a custom profile with the same ID already exists. + +Update an existing custom profile by exporting the current custom profile, editing the file, and submitting the edited file back: + +```shell +openshell provider profile export github-profile -o yaml > github-profile.yaml +openshell provider profile update github-profile -f github-profile.yaml +``` + +Exported custom profiles include `resource_version`. OpenShell requires that version during update so stale files cannot silently overwrite newer profile definitions. The target ID in the command must match the profile ID in the file. Update accepts one file at a time. If an update would make dynamic token grants ambiguous for an attached sandbox, OpenShell rejects it before changing the profile. + +Custom profile IDs must use lowercase kebab-case with `a-z`, `0-9`, and `-`. Built-in profile IDs and legacy provider aliases are reserved. Built-in profiles are read-only, and OpenShell rejects updating or deleting a built-in profile. OpenShell also rejects deleting a custom profile while a sandbox-attached provider uses it. ### Category Enum @@ -148,6 +159,8 @@ Provider profile YAML and JSON use this shape. Treat this as a field map, not a ```yaml wordWrap showLineNumbers={false} id: custom-api +# Present on exported custom profiles; preserve it when updating. +resource_version: 1 display_name: Custom API description: Custom API access for sandbox agents category: data @@ -535,6 +548,8 @@ openshell sandbox create \ When `providers_v2_enabled=true`, each attached provider with a matching profile contributes a provider policy layer to the sandbox effective policy. When the setting is disabled, the sandbox receives provider credentials but not provider-derived policy entries. +Updating a custom provider profile affects every provider instance whose `type` matches that profile ID. Provider instances are not rewritten, and sandbox-authored policies are not modified. Running sandboxes observe the updated provider-derived policy on their next config sync. If a gateway-global policy is active, provider-derived policy layers remain suppressed. + Providers v2 does not infer or auto-attach providers from the sandbox command. Attach providers explicitly with `--provider` during sandbox creation, or use `openshell sandbox provider attach` after creation. diff --git a/proto/openshell.proto b/proto/openshell.proto index d701956d3..1e33e4bd2 100644 --- a/proto/openshell.proto +++ b/proto/openshell.proto @@ -95,6 +95,10 @@ service OpenShell { rpc ImportProviderProfiles(ImportProviderProfilesRequest) returns (ImportProviderProfilesResponse); + // Update an existing custom provider type profile. + rpc UpdateProviderProfiles(UpdateProviderProfilesRequest) + returns (UpdateProviderProfilesResponse); + // Validate provider type profiles without registering them. rpc LintProviderProfiles(LintProviderProfilesRequest) returns (LintProviderProfilesResponse); @@ -1075,6 +1079,10 @@ message ProviderProfile { repeated openshell.sandbox.v1.NetworkBinary binaries = 7; bool inference_capable = 8; ProviderProfileDiscovery discovery = 9; + // Storage resource version for custom profiles. Built-in profiles and new + // profile files use 0. Gateway responses set this for stored custom profiles. + // Update calls use this for optimistic concurrency. + uint64 resource_version = 10; } // Stored custom provider profile object. @@ -1105,6 +1113,25 @@ message ImportProviderProfilesResponse { bool imported = 3; } +// Update one custom provider profile request. +message UpdateProviderProfilesRequest { + ProviderProfileImportItem profile = 1; + // Expected storage resource version for optimistic concurrency control. + // If 0, the server uses the resource_version embedded in profile.profile. + // Updates without a non-zero version are rejected to prevent stale files from + // silently overwriting newer profile definitions. + uint64 expected_resource_version = 2; + // Existing custom provider profile ID to update. The payload ID must match. + string id = 3; +} + +// Update one custom provider profile response. +message UpdateProviderProfilesResponse { + repeated ProviderProfileDiagnostic diagnostics = 1; + ProviderProfile profile = 2; + bool updated = 3; +} + // Lint provider profiles request. message LintProviderProfilesRequest { repeated ProviderProfileImportItem profiles = 1;