From 94055637194c984502e17cd259a7a982af6ee561 Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Tue, 5 Dec 2023 13:25:17 -0800 Subject: [PATCH 1/9] Adds initial approach for region fan-out --- src/api.rs | 101 +++++++++++++++++++++++++---- src/commands/organizations/list.rs | 20 +++++- src/config.rs | 16 +++-- 3 files changed, 117 insertions(+), 20 deletions(-) diff --git a/src/api.rs b/src/api.rs index 302af75438..4d86bd1a7a 100644 --- a/src/api.rs +++ b/src/api.rs @@ -252,6 +252,8 @@ pub enum ApiErrorKind { RequestFailed, #[error("could not compress data")] CompressionFailed, + #[error("region prefixes cannot be applied to absolute urls")] + InvalidRegionRequest, } #[derive(Debug, thiserror::Error)] @@ -391,21 +393,41 @@ impl Api { /// URL is just a path then it's relative to the configured API host /// and authentication is automatically enabled. pub fn request(&self, method: Method, url: &str) -> ApiResult { - let mut handle = self.pool.get().unwrap(); - handle.reset(); - if !self.config.allow_keepalive() { - handle.forbid_reuse(true).ok(); - } - let mut ssl_opts = curl::easy::SslOpt::new(); - if self.config.disable_ssl_revocation_check() { - ssl_opts.no_revoke(true); + let (url, auth) = self.resolve_base_url_and_auth(url, None)?; + self.construct_api_request(method, &url, auth) + } + + /// Like `request`, but constructs a new `ApiRequest` using the base URL + /// plus a region prefix for requests that must be routed to a region. + pub fn region_request( + &self, + method: Method, + url: &str, + region: &Region, + ) -> ApiResult { + let (url, auth) = self.resolve_base_url_and_auth(url, Some(region))?; + self.construct_api_request(method, &url, auth) + } + + fn resolve_base_url_and_auth( + &self, + url: &str, + region: Option<&Region>, + ) -> ApiResult<(String, Option<&Auth>)> { + if is_absolute_url(url) && region.is_some() { + return Err(ApiErrorKind::InvalidRegionRequest.into()); } - handle.ssl_options(&ssl_opts)?; + let (url, auth) = if is_absolute_url(url) { (Cow::Borrowed(url), None) } else { + let host_override = match region { + Some(region_config) => Some(region_config.url.as_str()), + None => None, + }; + ( - Cow::Owned(match self.config.get_api_endpoint(url) { + Cow::Owned(match self.config.get_api_endpoint(url, host_override) { Ok(rv) => rv, Err(err) => return Err(ApiError::with_source(ApiErrorKind::BadApiUrl, err)), }), @@ -413,6 +435,26 @@ impl Api { ) }; + Ok((url.into_owned(), auth)) + } + + fn construct_api_request( + &self, + method: Method, + url: &str, + auth: Option<&Auth>, + ) -> ApiResult { + let mut handle = self.pool.get().unwrap(); + handle.reset(); + if !self.config.allow_keepalive() { + handle.forbid_reuse(true).ok(); + } + let mut ssl_opts = curl::easy::SslOpt::new(); + if self.config.disable_ssl_revocation_check() { + ssl_opts.no_revoke(true); + } + handle.ssl_options(&ssl_opts)?; + if let Some(proxy_url) = self.config.get_proxy_url() { handle.proxy(&proxy_url)?; } @@ -431,7 +473,7 @@ impl Api { let env = self.config.get_pipeline_env(); let headers = self.config.get_headers(); - ApiRequest::create(handle, &method, &url, auth, env, headers) + ApiRequest::create(handle, &method, url, auth, env, headers) } /// Convenience method that performs a request using DSN as authentication method. @@ -445,7 +487,7 @@ impl Api { // We resolve an absolute URL to skip default authentication flow. let url = self .config - .get_api_endpoint(url) + .get_api_endpoint(url, None) .map_err(|err| ApiError::with_source(ApiErrorKind::BadApiUrl, err))?; let mut request = self @@ -1443,11 +1485,19 @@ impl Api { } /// List all organizations associated with the authenticated token - pub fn list_organizations(&self) -> ApiResult> { + /// in the given `Region`. If no `Region` is provided, we assume + /// we're issuing a request to a monolith deployment. + pub fn list_organizations(&self, region: Option<&Region>) -> ApiResult> { let mut rv = vec![]; let mut cursor = "".to_string(); loop { - let resp = self.get(&format!("/organizations/?cursor={}", QueryArg(&cursor)))?; + let current_path = &format!("/organizations/?cursor={}", QueryArg(&cursor)); + let resp = if let Some(rg) = region { + self.region_request(Method::Get, current_path, rg)?.send()? + } else { + self.get(current_path)? + }; + if resp.status() == 404 || (resp.status() == 400 && !cursor.is_empty()) { if rv.is_empty() { return Err(ApiErrorKind::ResourceNotFound.into()); @@ -1466,6 +1516,16 @@ impl Api { Ok(rv) } + pub fn list_available_regions(&self) -> ApiResult> { + let resp = self.get("/users/me/regions/")?; + if resp.status() == 404 || resp.status == 400 { + return Err(ApiErrorKind::ResourceNotFound.into()); + } + + let region_response = resp.convert::()?; + Ok(region_response.regions) + } + /// List all monitors associated with an organization pub fn list_organization_monitors(&self, org: &str) -> ApiResult> { let mut rv = vec![]; @@ -2947,3 +3007,16 @@ impl fmt::Display for ProcessedEventTag { Ok(()) } } + +#[derive(Clone, Debug, Deserialize)] +pub struct Region { + pub name: String, + pub url: String, +} + +#[derive(Clone, Debug, Deserialize)] +pub struct RegionResponse { + pub regions: Vec +} + + diff --git a/src/commands/organizations/list.rs b/src/commands/organizations/list.rs index 89f16e186b..276770be07 100644 --- a/src/commands/organizations/list.rs +++ b/src/commands/organizations/list.rs @@ -1,7 +1,8 @@ use anyhow::Result; use clap::{ArgMatches, Command}; +use log::debug; -use crate::api::Api; +use crate::api::{Api, Organization}; use crate::utils::formatting::Table; pub fn make_command(command: Command) -> Command { @@ -10,7 +11,22 @@ pub fn make_command(command: Command) -> Command { pub fn execute(_matches: &ArgMatches) -> Result<()> { let api = Api::current(); - let mut organizations = api.list_organizations()?; + + // Query regions available to the current CLI user + let regions = api.list_available_regions()?; + + let mut organizations: Vec = vec![]; + debug!("Available regions: {:?}", regions); + + // Self-hosted instances won't have a region instance or prefix, so we + // need to check before fanning out. + if regions.len() > 1 { + for region in regions { + organizations.append(&mut api.list_organizations(Some(®ion))?) + } + } else { + organizations.append(&mut api.list_organizations(None)?) + } organizations.sort_by_key(|o| o.name.clone().to_lowercase()); diff --git a/src/config.rs b/src/config.rs index 110ce08b76..421cf7ef28 100644 --- a/src/config.rs +++ b/src/config.rs @@ -270,10 +270,11 @@ impl Config { } /// Returns the API URL for a path - pub fn get_api_endpoint(&self, path: &str) -> Result { - let base = self.get_base_url()?; + pub fn get_api_endpoint(&self, path: &str, base_url_override: Option<&str>) -> Result { + let base: &str = base_url_override.unwrap_or(self.get_base_url()?); let path = path.trim_start_matches('/'); let path = path.trim_start_matches("api/0/"); + Ok(format!("{}/api/0/{}", base, path)) } @@ -788,16 +789,23 @@ mod tests { assert_eq!( config - .get_api_endpoint("/organizations/test-org/chunk-upload/") + .get_api_endpoint("/organizations/test-org/chunk-upload/", None) .unwrap(), "https://sentry.io/api/0/organizations/test-org/chunk-upload/" ); assert_eq!( config - .get_api_endpoint("/api/0/organizations/test-org/chunk-upload/") + .get_api_endpoint("/api/0/organizations/test-org/chunk-upload/", None) .unwrap(), "https://sentry.io/api/0/organizations/test-org/chunk-upload/" ); + + assert_eq!( + config + .get_api_endpoint("/api/0/organizations/test-org/chunk-upload/", Some("https://us.sentry.io/")) + .unwrap(), + "https://us.sentry.io/api/0/organizations/test-org/chunk-upload/" + ); } } From db9b6b8f1e7a7efbb53dd32b9845eb389ccbb376 Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Wed, 6 Dec 2023 12:32:44 -0800 Subject: [PATCH 2/9] Runs cargo fmt --- src/api.rs | 6 ++---- src/config.rs | 5 ++++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/api.rs b/src/api.rs index 4d86bd1a7a..d253cf0c8a 100644 --- a/src/api.rs +++ b/src/api.rs @@ -1497,7 +1497,7 @@ impl Api { } else { self.get(current_path)? }; - + if resp.status() == 404 || (resp.status() == 400 && !cursor.is_empty()) { if rv.is_empty() { return Err(ApiErrorKind::ResourceNotFound.into()); @@ -3016,7 +3016,5 @@ pub struct Region { #[derive(Clone, Debug, Deserialize)] pub struct RegionResponse { - pub regions: Vec + pub regions: Vec, } - - diff --git a/src/config.rs b/src/config.rs index 421cf7ef28..696874c27e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -803,7 +803,10 @@ mod tests { assert_eq!( config - .get_api_endpoint("/api/0/organizations/test-org/chunk-upload/", Some("https://us.sentry.io/")) + .get_api_endpoint( + "/api/0/organizations/test-org/chunk-upload/", + Some("https://us.sentry.io/") + ) .unwrap(), "https://us.sentry.io/api/0/organizations/test-org/chunk-upload/" ); From ed3f5d3746420499d3e43958822a4dce1b2b64fa Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Wed, 6 Dec 2023 12:36:00 -0800 Subject: [PATCH 3/9] Replaces region url extraction match with map --- src/api.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/api.rs b/src/api.rs index d253cf0c8a..6ae07acc3f 100644 --- a/src/api.rs +++ b/src/api.rs @@ -421,10 +421,7 @@ impl Api { let (url, auth) = if is_absolute_url(url) { (Cow::Borrowed(url), None) } else { - let host_override = match region { - Some(region_config) => Some(region_config.url.as_str()), - None => None, - }; + let host_override = region.map(|rg| rg.url.as_str()); ( Cow::Owned(match self.config.get_api_endpoint(url, host_override) { From c97eda780cc6d8f0457fc87af932342e00c17580 Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Wed, 6 Dec 2023 12:39:52 -0800 Subject: [PATCH 4/9] Trims extraneous slash from end of path --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index 696874c27e..8f7c98b2ae 100644 --- a/src/config.rs +++ b/src/config.rs @@ -271,7 +271,7 @@ impl Config { /// Returns the API URL for a path pub fn get_api_endpoint(&self, path: &str, base_url_override: Option<&str>) -> Result { - let base: &str = base_url_override.unwrap_or(self.get_base_url()?); + let base: &str = base_url_override.unwrap_or(self.get_base_url()?).trim_end_matches('/'); let path = path.trim_start_matches('/'); let path = path.trim_start_matches("api/0/"); From 2d13951d7a29f869b7892de936a3ec5500ee13bc Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Wed, 6 Dec 2023 16:14:27 -0800 Subject: [PATCH 5/9] Adds mock response for region --- src/api.rs | 2 +- src/config.rs | 4 +++- tests/integration/organizations/list.rs | 18 ++++++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/api.rs b/src/api.rs index 6ae07acc3f..a12295038f 100644 --- a/src/api.rs +++ b/src/api.rs @@ -252,7 +252,7 @@ pub enum ApiErrorKind { RequestFailed, #[error("could not compress data")] CompressionFailed, - #[error("region prefixes cannot be applied to absolute urls")] + #[error("region overrides cannot be applied to absolute urls")] InvalidRegionRequest, } diff --git a/src/config.rs b/src/config.rs index 8f7c98b2ae..fabf3e269e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -271,7 +271,9 @@ impl Config { /// Returns the API URL for a path pub fn get_api_endpoint(&self, path: &str, base_url_override: Option<&str>) -> Result { - let base: &str = base_url_override.unwrap_or(self.get_base_url()?).trim_end_matches('/'); + let base: &str = base_url_override + .unwrap_or(self.get_base_url()?) + .trim_end_matches('/'); let path = path.trim_start_matches('/'); let path = path.trim_start_matches("api/0/"); diff --git a/tests/integration/organizations/list.rs b/tests/integration/organizations/list.rs index b01c0d0e93..1a9f879be7 100644 --- a/tests/integration/organizations/list.rs +++ b/tests/integration/organizations/list.rs @@ -1,3 +1,5 @@ +use mockito::server_url; + use crate::integration::{mock_endpoint, register_test, EndpointOptions}; #[test] @@ -6,6 +8,22 @@ fn command_organizations_list() { EndpointOptions::new("GET", "/api/0/organizations/?cursor=", 200) .with_response_file("organizations/get-organizations.json"), ); + + let region_response = format!( + "{{ + \"regions\": [{{ + \"name\": \"monolith\", + \"url\": \"{}\" + }}] + }}", + server_url(), + ); + + let _mock_regions = mock_endpoint( + EndpointOptions::new("GET", "/api/0/users/me/regions/", 200).with_response_body( + region_response + ) + ); register_test("organizations/organizations-list.trycmd"); } From b565445448a48c900a36c0e0141fe4f56f0af6ee Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Wed, 6 Dec 2023 16:16:17 -0800 Subject: [PATCH 6/9] formats test file --- tests/integration/organizations/list.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/integration/organizations/list.rs b/tests/integration/organizations/list.rs index 1a9f879be7..bbcd23278e 100644 --- a/tests/integration/organizations/list.rs +++ b/tests/integration/organizations/list.rs @@ -20,9 +20,8 @@ fn command_organizations_list() { ); let _mock_regions = mock_endpoint( - EndpointOptions::new("GET", "/api/0/users/me/regions/", 200).with_response_body( - region_response - ) + EndpointOptions::new("GET", "/api/0/users/me/regions/", 200) + .with_response_body(region_response), ); register_test("organizations/organizations-list.trycmd"); } From ac5a799b27a84922df2386b579088ade4e825513 Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Wed, 6 Dec 2023 16:20:13 -0800 Subject: [PATCH 7/9] Adds different default behavior for region API 404s --- src/api.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/api.rs b/src/api.rs index a12295038f..137bf1bc5b 100644 --- a/src/api.rs +++ b/src/api.rs @@ -1515,7 +1515,13 @@ impl Api { pub fn list_available_regions(&self) -> ApiResult> { let resp = self.get("/users/me/regions/")?; - if resp.status() == 404 || resp.status == 400 { + if resp.status() == 404 { + // This endpoint may not exist for self-hosted users, so + // returning a default of [] seems appropriate. + return Ok(vec![]); + } + + if resp.status == 400 { return Err(ApiErrorKind::ResourceNotFound.into()); } From d0bc7086518cf6b480b5fd09ebe59b731e82e0c9 Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Fri, 8 Dec 2023 09:18:05 -0800 Subject: [PATCH 8/9] Fixes typo with status --- src/api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api.rs b/src/api.rs index 137bf1bc5b..f3b52dd50c 100644 --- a/src/api.rs +++ b/src/api.rs @@ -1521,7 +1521,7 @@ impl Api { return Ok(vec![]); } - if resp.status == 400 { + if resp.status() == 400 { return Err(ApiErrorKind::ResourceNotFound.into()); } From 136bb12e830b0391ea221401ebf562fa1f315e10 Mon Sep 17 00:00:00 2001 From: Gabe Villalobos Date: Tue, 12 Dec 2023 09:20:32 -0800 Subject: [PATCH 9/9] Use nicer raw string syntax. Co-authored-by: Sebastian Zivota --- tests/integration/organizations/list.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/integration/organizations/list.rs b/tests/integration/organizations/list.rs index bbcd23278e..1cbbaa676c 100644 --- a/tests/integration/organizations/list.rs +++ b/tests/integration/organizations/list.rs @@ -10,12 +10,12 @@ fn command_organizations_list() { ); let region_response = format!( - "{{ - \"regions\": [{{ - \"name\": \"monolith\", - \"url\": \"{}\" + r#"{{ + "regions": [{{ + "name": "monolith", + "url": "{}" }}] - }}", + }}"#, server_url(), );