fix(export): fix the policy and service account export #665
Conversation
|
|
There was a problem hiding this comment.
Pull Request Overview
Fixes bugs in exporting policies and service accounts by correcting policy version validation, adjusting IAM import behavior, and fixing a service account check.
- Validate Policy and BucketPolicy using the version field (not id).
- Change IAM import to aggregate per-policy errors and remove “removed” from ImportIAMResult.
- Fix service account verification logic in IamSys.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rustfs/src/admin/handlers/user.rs | Changes import flow to set policies and collect failures instead of failing fast; removes “removed” entities from the import result. |
| crates/policy/src/policy/policy.rs | Validates Policy and BucketPolicy against DEFAULT_VERSION using the version field. |
| crates/madmin/src/user.rs | Removes removed from ImportIAMResult and updates doc for added. |
| crates/iam/src/sys.rs | Corrects service account check to reject non-service accounts. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /// imported entries. We dont fail hard in this case and | ||
| pub skipped: IAMEntities, | ||
|
|
||
| /// Removed entries - this mostly happens for policies | ||
| /// where empty might be getting imported and that's invalid | ||
| pub removed: IAMEntities, | ||
|
|
||
| /// Newly added entries | ||
| /// Newly added or updated entries | ||
| pub added: IAMEntities, | ||
|
|
||
| /// Failed entries while import. This would have details of |
There was a problem hiding this comment.
Removing the removed field from ImportIAMResult is a breaking change for any external consumers of the madmin API. To preserve backward compatibility, consider keeping removed as an optional, deprecated field with serde defaults (e.g., #[serde(default, skip_serializing_if = "Option::is_none")] pub removed: Option) or explicitly bump the crate’s major version and note the API change.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| .map_err(|e| S3Error::with_message(S3ErrorCode::InternalError, e.to_string()))?; | ||
| for (name, policy) in policies { | ||
| if policy.id.is_empty() { | ||
| if policy.is_empty() { |
There was a problem hiding this comment.
Policy does not expose an is_empty() method; this will not compile. Based on the related changes switching from id to version, this condition should check the version field instead (e.g., policy.version.is_empty()).
| if policy.is_empty() { | |
| if policy.version.is_empty() { |
| let res = iam_store.delete_policy(&name, true).await; | ||
| removed.policies.push(name.clone()); | ||
| if let Err(e) = res { |
There was a problem hiding this comment.
The removal is recorded before confirming delete_policy succeeded, which can produce inaccurate export results. Move removed.policies.push(name.clone()) after a successful deletion (e.g., inside the Ok branch) so failures are not reported as removed.
4 participants
Type of Change
Related Issues
#643
Summary of Changes
fix bugs in exporting policies and service account
Checklist
make pre-commitImpact
Additional Notes
Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.