Add admin/superuser error handling and cleanup #8559
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added a defer function to clean up the admin user if an error occurs during creation. - Added tests for creating an admin user, verifying successful creation, attempting to create another admin user, deleting the admin user, creating an admin user without a secret access key, and re-adding the admin user. - Included a new test `TestAddAdminUser` to verify the behavior of the `AddAdminUser` function in various scenarios.
nopcoder
added
area/auth
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
yonipeleg33
approved these changes
Jan 28, 2025
There was a problem hiding this comment.
TLDR: Thanks, LGTM!
I have two drawbacks here:
- This solution isn't bulletproof, as the process might die before executing the
deferstatement. That being said, it is probably good enough for the majority of cases. - I'd like to implement this suggested solution to re-validate the user intent with a special flag passed to the command.
It is orthogonal to this PR, so it's not a blocker (obviously), but I believe making the command more human-error-proof can be even more valuable than the rollback implemented in this PR.
But nonetheless, this PR is net-positive, so LGTM! Thanks for working on this
| @@ -174,6 +174,12 @@ func AddAdminUser(ctx context.Context, authService auth.Service, user *model.Sup | |||
| if err != nil { | |||
| return nil, fmt.Errorf("create user - %w", err) | |||
| } | |||
| defer func() { | |||
| // delete admin on any error to avoid partial setup | |||
There was a problem hiding this comment.
Please replace the comment with an informative log message (info/warn imo)
N-o-Z
added a commit
that referenced
this pull request
Jan 30, 2025
# This is the 1st commit message: StorageConfigList response changes # The commit message #2 will be skipped: # Fix newConfig # The commit message #3 will be skipped: # More fixes # The commit message #4 will be skipped: # More fixes # The commit message #5 will be skipped: # Add admin/superuser error handling and cleanup (#8559) # # * Add admin/superuser error handling and cleanup # # - Added a defer function to clean up the admin user if an error occurs during creation. # - Added tests for creating an admin user, verifying successful creation, attempting to create another admin user, deleting the admin user, creating an admin user without a secret access key, and re-adding the admin user. # - Included a new test `TestAddAdminUser` to verify the behavior of the `AddAdminUser` function in various scenarios. # # * Added a logger to log the error message and warn about deleting the user # The commit message #6 will be skipped: # Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace (#8551) # # * Add StorageID to adapter metadata functions # # * Fix tests # # * Add error handling # # * Fix tests # # * Fix lint # # * Revert "Fix lint" # # This reverts commit 850f17b. # # * Revert "Fix tests" # # This reverts commit 7bf9acc. # # * Revert "Add error handling" # # This reverts commit 9d3f0a2. # # * Revert "Fix tests" # # This reverts commit a2a4ab5. # # * Revert "Add StorageID to adapter metadata functions" # # This reverts commit 06b32ee. # # * Add storageID to metadata functions # # * Fix tests # # * Fix tests # # * Remove storageID from BlockstoreMetadata
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TestAddAdminUserto verify the behavior of theAddAdminUserfunction in various scenarios.Closes #8490