From ae8b2d39eccd6acbc484cb2efd7bbbf68bc9c6cd Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 27 Oct 2026 11:48:54 +0100 Subject: [PATCH] swarm: merge a re-create into the stored agent permissions instead of overwriting them --- docs/process/conventions.md | 10 +- swarm-controller-api/src/agents.rs | 10 +- swarm-controller/src/agent_permissions.rs | 110 +++++++++++++++++++--- 3 files changed, 108 insertions(+), 22 deletions(-) diff --git a/docs/process/conventions.md b/docs/process/conventions.md index 912475e7..9739928b 100644 --- a/docs/process/conventions.md +++ b/docs/process/conventions.md @@ -314,10 +314,12 @@ none. swarm-controller logs every refused `PUT` or `DELETE` on that route at `wa **Internet flag** — the record also carries `unrestricted_internet`, a boolean that isn't a `Permission`. An absent value means on, so every agent that existed before the field keeps the open access it had. `POST /api/agents` -always writes the record with an explicit value, `false` unless the request -names it. A `PUT` that leaves the field out and a `DELETE` (`swarmctl agent -permissions clear`) keep the stored value; a `DELETE` on an agent that has one -rewrites the record to just that value. Read it with +writes `false` for an agent that has no record, unless the request names a +value. Over an existing record (a re-create) it keeps the stored list and flag, +an absent flag included, and a value in the request wins. A `PUT` that leaves +the field out and a `DELETE` (`swarmctl agent permissions clear`) also keep the +stored value; a `DELETE` on an agent that has one rewrites the record to just +that value. Read it with `swarm_queue_client::agent_permissions::unrestricted`. Nothing acts on it yet. **How a change applies** — diff --git a/swarm-controller-api/src/agents.rs b/swarm-controller-api/src/agents.rs index 3d2d7eb5..4b3d5550 100644 --- a/swarm-controller-api/src/agents.rs +++ b/swarm-controller-api/src/agents.rs @@ -76,13 +76,15 @@ pub struct CreateAgentRequest { /// /api/agents/{name}/inference` takes them. Absent as `limits`. #[serde(default, skip_serializing_if = "Option::is_none")] pub inference: Option, - /// The agent's permissions, stored before the job is queued. Left out, the - /// agent holds the default set. + /// The agent's permissions, stored before the job is queued. Left out, an + /// existing record keeps its list and otherwise the agent holds the default + /// set. #[serde(default, skip_serializing_if = "Option::is_none")] #[cfg_attr(feature = "schema", schema(value_type = Option>))] pub permissions: Option>, - /// Whether the agent's traffic leaves the host unfiltered. Left out, the - /// agent is created with `false`. + /// Whether the agent's traffic leaves the host unfiltered. Left out, an + /// agent with no stored value is created with `false`, and one that has a + /// record (a re-create) keeps its value. #[serde(default, skip_serializing_if = "Option::is_none")] pub unrestricted_internet: Option, /// The archetype to build the agent from, one `GET /api/archetypes` diff --git a/swarm-controller/src/agent_permissions.rs b/swarm-controller/src/agent_permissions.rs index 94b99110..7aa6d95f 100644 --- a/swarm-controller/src/agent_permissions.rs +++ b/swarm-controller/src/agent_permissions.rs @@ -240,7 +240,8 @@ pub(crate) async fn delete_agent_permissions( } /// Delete a destroyed agent's key. A name is free again once its agent is -/// destroyed, so it starts with no record. +/// destroyed, and a create keeps the list and flag of a record it finds, so a +/// key left here would carry over to the next agent of that name. /// /// Like the credential revoke it runs beside, a failure is logged and does /// not stop the destroy; declaring the agent destroyed again re-runs it. @@ -260,8 +261,8 @@ pub(crate) async fn clear_on_destroy(state: &AppState, hive: &str, agent: &str) hive, agent, error = %format!("{e:#}"), - "clearing this agent's permissions FAILED; it was destroyed anyway, so a read \ - of that name finds the old record until a create writes a new one. \ + "clearing this agent's permissions FAILED; it was destroyed anyway, so a new \ + agent of the same name created later keeps this record's list and flag. \ Declaring the agent destroyed again re-runs this." ), } @@ -291,9 +292,9 @@ pub(crate) async fn get_permission_catalogue() -> Json { }) } -/// The value `POST /api/agents` stores, or why it is refused. `perms` left -/// out keeps the default set; `unrestricted_internet` left out is `false`, so -/// every new agent has an explicit value and only older records read as on. +/// The value `POST /api/agents` asks [`store`] to merge in, or why it is +/// refused. A field the request leaves out stays unset here: [`store`] keeps +/// the stored value, and gives an agent with no stored flag `false`. /// The agent is not declared on its hive yet, so the "declared by a hive" /// check of [`put_agent_permissions`] does not apply. pub(crate) fn at_create( @@ -305,14 +306,37 @@ pub(crate) fn at_create( } Ok(AgentPermissions { permissions: perms, - unrestricted_internet: Some(unrestricted_internet.unwrap_or(false)), + unrestricted_internet, }) } +/// What a create leaves in the key: the request's fields win, and a field it +/// leaves out keeps the stored value. Only an agent with no record at all +/// (`held` false) gets `unrestricted_internet: false`, so re-creating an +/// existing agent keeps its list and its flag, a legacy record's absent flag +/// included. +fn merged_at_create( + value: &AgentPermissions, + current: &AgentPermissions, + held: bool, +) -> AgentPermissions { + AgentPermissions { + permissions: value + .permissions + .clone() + .or_else(|| current.permissions.clone()), + unrestricted_internet: value + .unrestricted_internet + .or(current.unrestricted_internet) + .or_else(|| (!held).then_some(false)), + } +} + /// What [`store`] replaced, for [`restore`]; `None` when it wrote nothing. pub(crate) type Replaced = Option>; -/// Replace `agent`'s stored permissions with `value`. +/// Merge `value` into `agent`'s stored permissions, as [`merged_at_create`] +/// says. pub(crate) async fn store( client: &async_nats::Client, agent: &str, @@ -323,17 +347,18 @@ pub(crate) async fn store( let (current, revision, undecodable) = read_or_default::(&store, agent) .await .map_err(|e| queue_problem(&e))?; - if current == *value && !undecodable { - tracing::info!(%agent, ?value, "set agent permissions at create"); - return Ok(None); - } // A deleted key reads as present at its marker's revision, so ask - // whether it holds a value before keeping one to put back. + // whether it holds a value. let held = store .get(agent) .await .map_err(|e| queue_problem(&anyhow::Error::new(e)))? .is_some(); + let value = &merged_at_create(value, ¤t, held); + if current == *value && !undecodable { + tracing::info!(%agent, ?value, "set agent permissions at create"); + return Ok(None); + } if let Some(written) = write_revision(&store, agent, value, revision) .await .map_err(|e| queue_problem(&e))? @@ -697,7 +722,10 @@ mod tests { // Control: an agent that never had the flag has nothing left after a // clear, and a `PUT` does not invent one. - crate::agent_permissions::store(&client, "atlas", &super::AgentPermissions::default()) + swarm_queue_client::agent_permissions::open_or_create(&client) + .await + .expect("bucket") + .put("atlas", "{}".into()) .await .expect("an older record"); let _ = put(&state, "atlas", granted()).await.expect("set"); @@ -709,6 +737,60 @@ mod tests { assert_eq!(published(&client, "atlas").await, None); } + /// A create over an existing record merges into it: a list and a flag + /// that were set stay, and a record without a flag (an older agent) + /// stays without one. Control: with no record, the flag is `false`. + #[tokio::test] + async fn a_re_create_keeps_the_record_it_finds() { + let nats = TestNats::start(); + let client = nats.client().await; + let state = crate::agent_limits::tests::declared(&client).await; + let create = |permissions, unrestricted_internet| { + crate::routes::agents::create_agent( + State(state.clone()), + axum::Json(swarm_controller_api::agents::CreateAgentRequest { + name: "atlas".to_owned(), + hive: "pr1ma".to_owned(), + limits: None, + inference: None, + permissions, + unrestricted_internet, + archetype: None, + agents_md: None, + }), + ) + }; + + let on = Permissions { + unrestricted_internet: Some(true), + ..granted() + }; + let _ = put(&state, "atlas", on).await.expect("set"); + let _ = create(None, None).await.expect("re-created"); + assert_eq!( + published(&client, "atlas").await.as_deref(), + Some(r#"{"permissions":["manage_root_agent"],"unrestricted_internet":true}"#) + ); + + // A value in the request wins over the stored one. + let _ = create(None, Some(false)).await.expect("re-created"); + assert_eq!( + published(&client, "atlas").await.as_deref(), + Some(r#"{"permissions":["manage_root_agent"],"unrestricted_internet":false}"#) + ); + + // A record from before the flag existed stays without one. + let older = r#"{"permissions":["manage_root_agent"]}"#; + swarm_queue_client::agent_permissions::open_or_create(&client) + .await + .expect("bucket") + .put("atlas", older.into()) + .await + .expect("an older record"); + let _ = create(None, None).await.expect("re-created"); + assert_eq!(published(&client, "atlas").await.as_deref(), Some(older)); + } + /// A name no hive declares, or a name that is not an identifier, is a 400 /// and writes nothing. #[tokio::test]