-
Notifications
You must be signed in to change notification settings - Fork 393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
compute_ctl: only try zenith_admin if could not authenticate #6955
Conversation
Signed-off-by: Alex Chi Z <chi@neon.tech>
c4f12e2
to
37b631e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I didn't test however that "invalid user" also does return
pub const INVALID_PASSWORD: SqlState = SqlState(Inner::E28P01);
did you use a a project still defined with zenith_admin to test? - or do you want to add a test case to verify the new code works correctly to migrate from zenith_admin to cloud_admin?
2472 tests run: 2350 passed, 0 failed, 122 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
0b202f1 at 2024-03-01T20:24:45.415Z :recycle: |
Signed-off-by: Alex Chi Z <chi@neon.tech>
459c35b
to
25f5d4a
Compare
@Bodobolero Just manually run some tests.
The error code for user not found is |
Given the default admin user is hard-coded in
There does not seem to have an easy way of testing it automatically. So I prefer manual testing in this case. |
Signed-off-by: Alex Chi Z <chi@neon.tech>
Also added more error context to hint people reading the log that a re-authentication happened. |
needs someone from the compute team to approve due to code owner setting 😅 |
Problem
Fix #6498
Summary of changes
Only re-authenticate with zenith_admin if authentication fails. Otherwise, directly return the error message.
Checklist before requesting a review
Checklist before merging