Skip to content
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

Merged
merged 3 commits into from Mar 4, 2024

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented Feb 28, 2024

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

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh skyzh force-pushed the skyzh/check-error-before-zenith branch from c4f12e2 to 37b631e Compare February 28, 2024 16:39
@skyzh skyzh changed the title compute_ctl: only try zenith_admin if could not auth compute_ctl: only try zenith_admin if could not authenticate Feb 28, 2024
Copy link
Contributor

@Bodobolero Bodobolero left a 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?

compute_tools/src/compute.rs Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Feb 28, 2024

2472 tests run: 2350 passed, 0 failed, 122 skipped (full report)


Code coverage* (full report)

  • functions: 28.8% (6931 of 24085 functions)
  • lines: 47.4% (42553 of 89855 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
0b202f1 at 2024-03-01T20:24:45.415Z :recycle:

@skyzh skyzh marked this pull request as ready for review March 1, 2024 19:36
@skyzh skyzh requested review from a team as code owners March 1, 2024 19:36
Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh skyzh force-pushed the skyzh/check-error-before-zenith branch from 459c35b to 25f5d4a Compare March 1, 2024 19:37
@skyzh
Copy link
Member Author

skyzh commented Mar 1, 2024

@Bodobolero Just manually run some tests.

  • Change page server default cloud admin user to zenith_admin. This creates a basebackup with zenith_admin as the admin.
  • Start the compute node. It will only proceed to the new role creation when invalid credential is detected.
  • Produce some other error type. The compute node will not start and directly exit with a fatal error, which indicates that all errors except authentication error will be rejected and cause startup failure.

The error code for user not found is INVALID_AUTHORIZATION_SPECIFICATION. I added both of INVALID_PASSWORD and INVALID_AUTHORIZATION_SPECIFICATION to the match branch.

@skyzh
Copy link
Member Author

skyzh commented Mar 1, 2024

Given the default admin user is hard-coded in pageserver/src/config.rs.

pub const DEFAULT_SUPERUSER: &str = "cloud_admin";

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>
@skyzh
Copy link
Member Author

skyzh commented Mar 1, 2024

Also added more error context to hint people reading the log that a re-authentication happened.

@skyzh skyzh enabled auto-merge (squash) March 1, 2024 21:57
@skyzh skyzh requested review from save-buffer and removed request for conradludgate March 1, 2024 22:12
@skyzh skyzh disabled auto-merge March 1, 2024 22:13
@skyzh
Copy link
Member Author

skyzh commented Mar 4, 2024

needs someone from the compute team to approve due to code owner setting 😅

@skyzh skyzh merged commit b7db912 into main Mar 4, 2024
53 checks passed
@skyzh skyzh deleted the skyzh/check-error-before-zenith branch March 4, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants