feat(auth): AWS sourced external accounts#4790
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4790 +/- ##
==========================================
- Coverage 95.01% 94.98% -0.03%
==========================================
Files 204 205 +1
Lines 7877 8054 +177
==========================================
+ Hits 7484 7650 +166
- Misses 393 404 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Successfully tested using our internal go/3pi-sdk-testing process. use google_cloud_auth::credentials::Builder;
use google_cloud_storage::client::StorageControl;
const BUCKET_ID: &str = "REDACTED";
#[tokio::main]
async fn main() -> anyhow::Result<()> {
println!("Building Credentials...");
// Will use ADC with the environment variable set
let creds = Builder::default().build()?;
println!("Credentials built: {:?}", creds);
let client = StorageControl::builder()
.with_credentials(creds)
.build()
.await?;
let bucket = client
.get_bucket()
.set_name(format!("projects/_/buckets/{BUCKET_ID}"))
.send()
.await?;
println!("successfully obtained bucket metadata {bucket:?}");
Ok(())
}output: |
coryan
left a comment
There was a problem hiding this comment.
Pretty good, a few nits and suggestions. Try to refactor the code making HTTP requests so the error handling is all in one place.
src/auth/src/credentials/external_account_sources/aws_sourced.rs
Outdated
Show resolved
Hide resolved
src/auth/src/credentials/external_account_sources/aws_sourced.rs
Outdated
Show resolved
Hide resolved
| let sts_url = if sts_url.starts_with("http") { | ||
| sts_url | ||
| } else { | ||
| format!("https://{sts_url}") | ||
| }; | ||
| let url = url::Url::parse(&sts_url) |
There was a problem hiding this comment.
If you are already going to use url::Url::parse() shouldn't that be able to handle the missing scheme?
There was a problem hiding this comment.
url::Url::parse() return Err with relative URL without a base if scheme is missing
There was a problem hiding this comment.
SG, though I think I assume you could do something like:
let url = match url::Url::parse(sts_url) {
Ok(u) => Ok(u),
Err(ParseError::RelativeUrlWithoutBase) => url::Url::parse(format!("https://{sts_url}")),
Err(e) => Err(e),
};
let url = url.map_err(|e| ....)?;
| fn parse_region_from_zone(zone: &str) -> Option<String> { | ||
| let zone = zone.trim(); | ||
| if zone.is_empty() { | ||
| return None; | ||
| } | ||
| if let Some(last_char) = zone.chars().last() { | ||
| if last_char.is_ascii_alphabetic() && zone.len() > 1 { | ||
| let potential_region = &zone[..zone.len() - 1]; | ||
| if potential_region | ||
| .chars() | ||
| .last() | ||
| .is_some_and(|c| c.is_ascii_digit()) | ||
| { | ||
| return Some(potential_region.to_string()); | ||
| } | ||
| } | ||
| } | ||
| Some(zone.to_string()) | ||
| } |
There was a problem hiding this comment.
Consider:
| fn parse_region_from_zone(zone: &str) -> Option<String> { | |
| let zone = zone.trim(); | |
| if zone.is_empty() { | |
| return None; | |
| } | |
| if let Some(last_char) = zone.chars().last() { | |
| if last_char.is_ascii_alphabetic() && zone.len() > 1 { | |
| let potential_region = &zone[..zone.len() - 1]; | |
| if potential_region | |
| .chars() | |
| .last() | |
| .is_some_and(|c| c.is_ascii_digit()) | |
| { | |
| return Some(potential_region.to_string()); | |
| } | |
| } | |
| } | |
| Some(zone.to_string()) | |
| } | |
| fn parse_region_from_zone(zone: &str) -> Option<&str> { | |
| let zone = zone.trim(); | |
| let parts: Vec<&str> = id.split('-').collect(); | |
| match &parts[..] { | |
| [geo, region, letter] if !letter.is_empty() && !geo.is_empty() => Some(region), | |
| _ => None, | |
| } | |
| } |
If you push me, we can save the memory allocation using:
fn parse_region_from_zone(zone: &str) -> Option<&str> {
let zone = zone.trim();
let parts = id.split('-');
match (parts.next(), parts.next(), parts.next(), parts.next()) {
(Some(geo), Some(region), Some(z), None] if !z.is_empty() && !geo.is_empty() => Some(region),
_ => None,
}
}
There was a problem hiding this comment.
Ah, I see in the tests that AWS zones can have four parts: just add a branch to the match case.
There was a problem hiding this comment.
This function is about converting from a zone like "us-east-1d" to a region "us-east-1", which is mostly just about removing the final d letter at the end. There is just this special case where some gov zones already don't have the letter at the end, so I check that if the last one is a letter or not. Returning just the region is not the would return east instead of us-east
src/auth/src/credentials/external_account_sources/aws_sourced.rs
Outdated
Show resolved
Hide resolved
src/auth/src/credentials/external_account_sources/aws_sourced.rs
Outdated
Show resolved
Hide resolved
| if !response.status().is_success() { | ||
| return Err(errors::from_http_response( | ||
| response, | ||
| "could not resolve AWS credentials", | ||
| ) | ||
| .await); | ||
| } |
src/auth/src/credentials/external_account_sources/aws_sourced.rs
Outdated
Show resolved
Hide resolved
alvarowolfx
left a comment
There was a problem hiding this comment.
addressed some of the review comments
| let sts_url = if sts_url.starts_with("http") { | ||
| sts_url | ||
| } else { | ||
| format!("https://{sts_url}") | ||
| }; | ||
| let url = url::Url::parse(&sts_url) |
There was a problem hiding this comment.
url::Url::parse() return Err with relative URL without a base if scheme is missing
| fn parse_region_from_zone(zone: &str) -> Option<String> { | ||
| let zone = zone.trim(); | ||
| if zone.is_empty() { | ||
| return None; | ||
| } | ||
| if let Some(last_char) = zone.chars().last() { | ||
| if last_char.is_ascii_alphabetic() && zone.len() > 1 { | ||
| let potential_region = &zone[..zone.len() - 1]; | ||
| if potential_region | ||
| .chars() | ||
| .last() | ||
| .is_some_and(|c| c.is_ascii_digit()) | ||
| { | ||
| return Some(potential_region.to_string()); | ||
| } | ||
| } | ||
| } | ||
| Some(zone.to_string()) | ||
| } |
There was a problem hiding this comment.
This function is about converting from a zone like "us-east-1d" to a region "us-east-1", which is mostly just about removing the final d letter at the end. There is just this special case where some gov zones already don't have the letter at the end, so I check that if the last one is a letter or not. Returning just the region is not the would return east instead of us-east
src/auth/src/credentials/external_account_sources/aws_sourced.rs
Outdated
Show resolved
Hide resolved
| let sts_url = if sts_url.starts_with("http") { | ||
| sts_url | ||
| } else { | ||
| format!("https://{sts_url}") | ||
| }; | ||
| let url = url::Url::parse(&sts_url) |
There was a problem hiding this comment.
SG, though I think I assume you could do something like:
let url = match url::Url::parse(sts_url) {
Ok(u) => Ok(u),
Err(ParseError::RelativeUrlWithoutBase) => url::Url::parse(format!("https://{sts_url}")),
Err(e) => Err(e),
};
let url = url.map_err(|e| ....)?;
Towards #3644