diff --git a/python/python/lancedb/functions.py b/python/python/lancedb/functions.py index a04dffcfc..ca14b8460 100644 --- a/python/python/lancedb/functions.py +++ b/python/python/lancedb/functions.py @@ -559,7 +559,6 @@ class RefreshColumnResult(_RemoteValue): _FUNCTION_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_.-]*$") -_DECLARED_SECRET = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") _FUNCTION_BLOB_V2_TYPE = "blob_v2" _ARROW_EXTENSION_NAME_KEY = "ARROW:extension:name" diff --git a/python/python/tests/test_first_class_function_slice2.py b/python/python/tests/test_first_class_function_slice2.py index 3eca11ded..753c71b70 100644 --- a/python/python/tests/test_first_class_function_slice2.py +++ b/python/python/tests/test_first_class_function_slice2.py @@ -1528,6 +1528,11 @@ def _mock_remote_function_catalog(): if len(parts) == 4 and parts[0] == "v1" and parts[1] == "function" else (None, None) ) + secret_action = ( + (urllib.parse.unquote(parts[2]), parts[3]) + if len(parts) == 4 and parts[0] == "v1" and parts[1] == "secret" + else (None, None) + ) if function_action[1] == "create": state["version"] = { "name": function_action[0], @@ -1559,20 +1564,14 @@ def _mock_remote_function_catalog(): elif function_action[1] == "drop": assert body == {"version": "fv_exact"} response = {"dropped": True} - elif self.path in ("/v1/secrets/create", "/v1/secrets/alter"): - assert set(body) == {"name", "value"} + elif secret_action[1] in ("create", "alter"): + # The Secret is the path identifier, so the body is the value. + assert set(body) == {"value"} + assert secret_action[0] == "openai-prod" response = {} - elif self.path == "/v1/secrets/list": - if "page_token" not in body: - response = { - "secrets": [{"name": "openai-prod"}], - "page_token": "next", - } - else: - assert body["page_token"] == "next" - response = {"secrets": [{"name": "hf-prod"}]} - elif self.path == "/v1/secrets/drop": - assert body == {"name": "openai-prod"} + elif secret_action[1] == "drop": + assert secret_action[0] == "openai-prod" + assert body == {} response = {} else: status = 404 @@ -1586,6 +1585,16 @@ def _mock_remote_function_catalog(): for key, values in urllib.parse.parse_qs(url.query).items() } state["requests"].append((url.path, query)) + if url.path == "/v1/namespace/$/secret/list": + if "page_token" not in query: + self._write_response( + 200, + {"secrets": [{"name": "openai-prod"}], "page_token": "next"}, + ) + else: + assert query["page_token"] == "next" + self._write_response(200, {"secrets": [{"name": "hf-prod"}]}) + return if url.path != "/v1/namespace/$/function/list": self._write_response(404, {"error": "not found"}) return @@ -1698,15 +1707,18 @@ def test_remote_secret_verbs_round_trip(): routes = [path for path, _ in state["requests"]] assert routes == [ - "/v1/secrets/create", - "/v1/secrets/alter", - "/v1/secrets/list", - "/v1/secrets/list", - "/v1/secrets/drop", + "/v1/secret/openai-prod/create", + "/v1/secret/openai-prod/alter", + "/v1/namespace/$/secret/list", + "/v1/namespace/$/secret/list", + "/v1/secret/openai-prod/drop", ] - assert state["requests"][0][1] == {"name": "openai-prod", "value": "sk-live-0001"} - # The listing returns names, and the client has no way to ask for more. + # The Secret is the path identifier, so the body is the value alone. + assert state["requests"][0][1] == {"value": "sk-live-0001"} + # Listing is a GET: the first page asks for nothing, the second resumes on + # the token the server handed back, and neither carries a body. assert state["requests"][2][1] == {} + assert state["requests"][3][1] == {"page_token": "next"} def test_building_a_binding_contacts_no_server(): diff --git a/rust/lancedb/src/remote/db.rs b/rust/lancedb/src/remote/db.rs index 5891e99f3..0e0582ec3 100644 --- a/rust/lancedb/src/remote/db.rs +++ b/rust/lancedb/src/remote/db.rs @@ -282,22 +282,6 @@ pub struct RemoteHostOverrides { pub sql: Option, } -/// Attach a namespace path to a Secret request body. -/// -/// A root path is omitted rather than sent empty, so a root request is byte -/// identical to one from a client that predates namespace addressing. -fn add_namespace_path(body: &mut serde_json::Value, namespace_path: &[String]) { - if namespace_path.is_empty() { - return; - } - body["namespace_path"] = serde_json::Value::Array( - namespace_path - .iter() - .map(|segment| serde_json::Value::String(segment.clone())) - .collect(), - ); -} - impl RemoteDatabase { pub(crate) fn try_new( uri: &str, @@ -381,6 +365,17 @@ impl RemoteDatabase { /// /// The value is a request field and never a path segment or query /// parameter, which keeps it out of access logs and proxy traces. + /// The path segment addressing one Secret. + /// + /// Written into the path as-is, the way a table identifier is. The charset + /// a Secret name and a namespace segment admit excludes everything that + /// would need escaping, and a `/v1/secret/prod$openai/drop` that reads like + /// the table and Function routes beside it is worth more than encoding + /// against a charset that does not exist yet. + fn secret_id(&self, name: &str, namespace_path: &[String]) -> String { + build_secret_identifier(name, namespace_path, &self.client.id_delimiter) + } + async fn post_secret_write(&self, route: &str, body: &T) -> Result<()> { let req = self.client.post(route).json(body); // This call is what says the body is a credential. Nothing downstream @@ -538,6 +533,16 @@ fn build_table_identifier(name: &str, namespace: &[String], delimiter: &str) -> } } +/// A Secret's path identifier: its namespace path and name joined the way every +/// other object's is. +/// +/// Secrets are not tables, but the identifier grammar belongs to the namespace +/// spec rather than to any one object type, and having two would mean a caller +/// had to know which kind of name it was holding. +fn build_secret_identifier(name: &str, namespace: &[String], delimiter: &str) -> String { + build_table_identifier(name, namespace, delimiter) +} + fn build_namespace_identifier(namespace: &[String], delimiter: &str) -> String { if namespace.is_empty() { // According to the namespace spec, use delimiter to represent root namespace @@ -634,36 +639,17 @@ struct RemoteCreateFunctionRequest<'a> { /// today: they are different operations to the service -- one refuses an /// existing name, the other requires it -- and either may grow a field the /// other has no meaning for. +/// +/// The name and its namespace are the path identifier, so neither appears here. #[derive(serde::Serialize)] struct RemoteCreateSecretRequest<'a> { - name: &'a str, value: &'a str, - /// Omitted at the root, so a request from a client that predates namespaces - /// is byte-identical to one that does not use them. - #[serde(skip_serializing_if = "<[String]>::is_empty")] - namespace_path: &'a [String], } /// Replace the credential behind a Secret the database already holds. #[derive(serde::Serialize)] struct RemoteAlterSecretRequest<'a> { - name: &'a str, value: &'a str, - #[serde(skip_serializing_if = "<[String]>::is_empty")] - namespace_path: &'a [String], -} - -/// One page of a Secret listing. A struct rather than an inline object so the -/// request and the response are declared the same way -- a reader of one finds -/// the other. -#[derive(serde::Serialize)] -struct RemoteListSecretsRequest<'a> { - #[serde(skip_serializing_if = "Option::is_none")] - page_token: Option, - /// Omitted at the root, so a listing from a client that predates namespaces - /// is byte-identical to one that does not use them. - #[serde(skip_serializing_if = "<[String]>::is_empty")] - namespace_path: &'a [String], } #[derive(serde::Deserialize)] @@ -922,39 +908,34 @@ impl Database for RemoteDatabase { value: &str, namespace_path: &[String], ) -> Result<()> { + let secret_id = self.secret_id(name, namespace_path); self.post_secret_write( - "/v1/secrets/create", - &RemoteCreateSecretRequest { - name, - value, - namespace_path, - }, + &format!("/v1/secret/{secret_id}/create"), + &RemoteCreateSecretRequest { value }, ) .await } async fn alter_secret(&self, name: &str, value: &str, namespace_path: &[String]) -> Result<()> { + let secret_id = self.secret_id(name, namespace_path); self.post_secret_write( - "/v1/secrets/alter", - &RemoteAlterSecretRequest { - name, - value, - namespace_path, - }, + &format!("/v1/secret/{secret_id}/alter"), + &RemoteAlterSecretRequest { value }, ) .await } async fn list_secrets(&self, namespace_path: &[String]) -> Result> { + let namespace_id = build_namespace_identifier(namespace_path, &self.client.id_delimiter); + let path = format!("/v1/namespace/{namespace_id}/secret/list"); let mut names = Vec::new(); let mut page_token: Option = None; let mut seen_page_tokens = HashSet::new(); loop { - let body = RemoteListSecretsRequest { - page_token: page_token.clone(), - namespace_path, - }; - let req = self.client.post("/v1/secrets/list").json(&body); + let mut req = self.client.get(&path); + if let Some(token) = &page_token { + req = req.query(&[("page_token", token)]); + } let (request_id, response) = self.client.send(req).await?; let response = self.client.check_response(&request_id, response).await?; let status = response.status(); @@ -978,18 +959,16 @@ impl Database for RemoteDatabase { } async fn drop_secret(&self, name: &str, namespace_path: &[String]) -> Result<()> { - let mut body = serde_json::json!({ "name": name }); - add_namespace_path(&mut body, namespace_path); - let req = self.client.post("/v1/secrets/drop").json(&body); + let secret_id = self.secret_id(name, namespace_path); + let req = self.client.post(&format!("/v1/secret/{secret_id}/drop")); let (request_id, response) = self.client.send(req).await?; self.client.check_response(&request_id, response).await?; Ok(()) } async fn describe_secret(&self, name: &str, namespace_path: &[String]) -> Result { - let mut body = serde_json::json!({ "name": name }); - add_namespace_path(&mut body, namespace_path); - let req = self.client.post("/v1/secrets/describe").json(&body); + let secret_id = self.secret_id(name, namespace_path); + let req = self.client.post(&format!("/v1/secret/{secret_id}/describe")); let (request_id, response) = self.client.send(req).await?; let response = self.client.check_response(&request_id, response).await?; response.json().await.err_to_http(request_id) @@ -3246,7 +3225,10 @@ mod tests { #[tokio::test] async fn test_create_and_alter_secret_send_the_value_in_the_request_body() { - for (route, call) in [("/v1/secrets/create", true), ("/v1/secrets/alter", false)] { + for (route, call) in [ + ("/v1/secret/openai-prod/create", true), + ("/v1/secret/openai-prod/alter", false), + ] { let conn = Connection::new_with_handler(move |request| { assert_eq!(request.method(), &reqwest::Method::POST); assert_eq!(request.url().path(), route); @@ -3255,8 +3237,8 @@ mod tests { assert!(request.url().query().is_none(), "{:?}", request.url()); let body: serde_json::Value = serde_json::from_slice(request.body().unwrap().as_bytes().unwrap()).unwrap(); - assert_eq!(body["name"], "openai-prod"); - assert_eq!(body["value"], "sk-live-0001"); + // The name is the path identifier, so the body is the value alone. + assert_eq!(body, serde_json::json!({ "value": "sk-live-0001" })); http::Response::builder().status(200).body("{}").unwrap() }); if call { @@ -3274,11 +3256,14 @@ mod tests { #[tokio::test] async fn test_list_secrets_walks_pages_and_returns_names_only() { let conn = Connection::new_with_handler(|request| { - assert_eq!(request.url().path(), "/v1/secrets/list"); - let body: serde_json::Value = - serde_json::from_slice(request.body().unwrap().as_bytes().unwrap()).unwrap(); - let page = body.get("page_token").and_then(|token| token.as_str()); - let body = match page { + assert_eq!(request.method(), &reqwest::Method::GET); + assert_eq!(request.url().path(), "/v1/namespace/$/secret/list"); + let page = request + .url() + .query_pairs() + .find(|(key, _)| key == "page_token") + .map(|(_, value)| value.into_owned()); + let body = match page.as_deref() { None => r#"{"secrets":[{"name":"openai-prod"}],"page_token":"p2"}"#, Some("p2") => r#"{"secrets":[{"name":"hf-prod"}]}"#, Some(other) => panic!("unexpected page token: {other}"), @@ -3309,32 +3294,37 @@ mod tests { } #[tokio::test] - async fn test_drop_secret_posts_the_name_alone() { + async fn test_drop_and_describe_address_the_secret_in_the_path() { let conn = Connection::new_with_handler(|request| { - assert_eq!(request.url().path(), "/v1/secrets/drop"); - let body: serde_json::Value = - serde_json::from_slice(request.body().unwrap().as_bytes().unwrap()).unwrap(); - assert_eq!(body, serde_json::json!({"name": "openai-prod"})); + assert_eq!(request.url().path(), "/v1/secret/openai-prod/drop"); + // Nothing is left to say once the path names the Secret. + assert!(request.body().is_none(), "{:?}", request.body()); http::Response::builder().status(200).body("{}").unwrap() }); conn.drop_secret("openai-prod", &[]).await.unwrap(); + + let conn = Connection::new_with_handler(|request| { + assert_eq!(request.url().path(), "/v1/secret/openai-prod/describe"); + assert!(request.body().is_none(), "{:?}", request.body()); + http::Response::builder() + .status(200) + .body(r#"{"name":"openai-prod","created_at_millis":1,"updated_at_millis":2}"#) + .unwrap() + }); + let info = conn.describe_secret("openai-prod", &[]).await.unwrap(); + assert_eq!(info.name, "openai-prod"); } - /// A namespace path is sent when there is one and omitted when there is - /// not, so a root request stays byte identical to one from a client that - /// predates namespace addressing -- which is what lets the parameter ship - /// before every server implements it. + /// The namespace is part of the identifier the path addresses, joined with + /// the client's configured delimiter the way every other object's is. A + /// root Secret is therefore addressed by its bare name, and a namespaced + /// one by the joined path -- there is no body field either way. #[tokio::test] - async fn test_a_namespace_path_is_sent_only_when_it_is_not_root() { + async fn test_a_namespace_path_is_addressed_in_the_path() { let conn = Connection::new_with_handler(|request| { - let body: serde_json::Value = - serde_json::from_slice(request.body().unwrap().as_bytes().unwrap()).unwrap(); assert_eq!( - body, - serde_json::json!({ - "name": "openai-prod", - "namespace_path": ["prod", "vision"], - }) + request.url().path(), + "/v1/secret/prod$vision$openai-prod/drop" ); http::Response::builder().status(200).body("{}").unwrap() }); @@ -3343,12 +3333,25 @@ mod tests { .unwrap(); let conn = Connection::new_with_handler(|request| { - let body: serde_json::Value = - serde_json::from_slice(request.body().unwrap().as_bytes().unwrap()).unwrap(); - assert!(body.get("namespace_path").is_none(), "{body}"); + assert_eq!(request.url().path(), "/v1/secret/openai-prod/drop"); http::Response::builder().status(200).body("{}").unwrap() }); conn.drop_secret("openai-prod", &[]).await.unwrap(); + + // Listing is namespace-scoped, so the namespace is the whole identifier. + let conn = Connection::new_with_handler(|request| { + assert_eq!( + request.url().path(), + "/v1/namespace/prod$vision/secret/list" + ); + http::Response::builder() + .status(200) + .body(r#"{"secrets":[]}"#) + .unwrap() + }); + conn.list_secrets(&["prod".to_string(), "vision".to_string()]) + .await + .unwrap(); } #[tokio::test]