fix(secret_manager): garbage collect locational secrets in integration tests#4433
Conversation
…n tests Fixes googleapis#4349 The stale secrets were not correctly removed since wrong arguments were passed into `delete_secret_by_project_and_location_and_secret`. This fix parses the secret name to properly delete stale secrets.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4433 +/- ##
==========================================
- Coverage 94.99% 94.98% -0.02%
==========================================
Files 193 193
Lines 7294 7294
==========================================
- Hits 6929 6928 -1
- Misses 365 366 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coryan
left a comment
There was a problem hiding this comment.
Good find! I think we can improve the parsing and prevent some panics.
| let project = parts[1]; | ||
| let location = parts[3]; | ||
| let secret = parts[5]; |
There was a problem hiding this comment.
This will panic if the name is not in the right format:
https://doc.rust-lang.org/std/vec/struct.Vec.html#indexing
You may try something like:
Which validates all the stuff and never panics.
Prevent panics from invalid input by returning a descriptive error.
| Ok(()) | ||
| } | ||
|
|
||
| fn extract(name: &str) -> Result<(&str, &str, &str)> { |
There was a problem hiding this comment.
I was maybe too succinct in my playground example:
| fn extract(name: &str) -> Result<(&str, &str, &str)> { | |
| fn parse_secret_name(name: &str) -> Result<(&str, &str, &str)> { |
| #[test] | ||
| fn test_extract_invalid_name() { | ||
| let name = "invalid/format"; | ||
| assert!(extract(name).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_extract_missing_parts() { | ||
| assert!(extract("projects/only-one").is_err()); | ||
| } |
There was a problem hiding this comment.
Consider:
| #[test] | |
| fn test_extract_invalid_name() { | |
| let name = "invalid/format"; | |
| assert!(extract(name).is_err()); | |
| } | |
| #[test] | |
| fn test_extract_missing_parts() { | |
| assert!(extract("projects/only-one").is_err()); | |
| } | |
| #[test_case("invalid/format")] | |
| #[test_case("projects/only-one")] | |
| #[test_case("projects/p/locations/l/bad/s")] | |
| #[test_case("projects/p/bad/l/secrets/s")] | |
| #[test_case("bad/p/locations/l/secrets/s")] | |
| fn test_extract_invalid_name(input: &str) { | |
| let result = parse_secret_name(input); | |
| assert!(result.is_err(), "{result:?}); | |
| } |
| .iter() | ||
| .map(|secret_id| { | ||
| .map(|name| { | ||
| let (project, location, secret) = extract(name).unwrap(); |
There was a problem hiding this comment.
This still panics, no? Maybe something like (but move the function definition to the right place):
async fn delete_by_name(client: smo::client::SecretManagerService, &name: &str) -> anyhow::Result<()> {
let (project, location, secret) = parse_secret_name(name)?;
client.delete_secret_by_project_and_location_and_secret()
.set_project(project)
.set_location(location)
.set_secret(secret)
.with_idempotency(true)
.send()
.await?;
Ok(())
}
let pending = stale_secrets.iter().map(|name| delete_by_name(name));There was a problem hiding this comment.
Hmmm... now that I think about it, we don't need a function, I think an expression like this should work to:
let pending = stale_secrets.iter().map(|name| -> anyhow::Result<()> {
let (project, location, secret) = parse_secret_name(name)?;
client.delete_secret_by_project_and_location_and_secret()
.set_project(project)
.set_location(location)
.set_secret(secret)
.with_idempotency(true)
.send()
.await?;
Ok(())
});Your call.
dbolduc
left a comment
There was a problem hiding this comment.
Looks great!
The build failure is unrelated... I think it will magically get fixed after
(1) we publish the google-cloud-storage crate at version 1.7.0
(2) we rerun the semver checks build
Fixes #4349
The stale secrets were not correctly removed since wrong arguments were passed into
delete_secret_by_project_and_location_and_secret. This fix parses the secret name to properly delete stale secrets.