-
Notifications
You must be signed in to change notification settings - Fork 5.1k
fix(core): add warning when config admin password is ignored #22709
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR addresses a common user confusion by adding a warning message when the harbor_admin_password configuration setting is ignored during Harbor startup. The admin password setting only applies during initial installation, and changes after the first run are silently ignored, leading to login errors that are documented but frequently misunderstood.
Key Changes:
- Refactored password initialization logic to support dependency injection for testability
- Added warning log when config admin password is set but won't be applied due to existing database password
- Implemented comprehensive unit tests covering all password initialization scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/core/main.go | Extracted updateInitPasswordWithMgr function with dependency injection and added conditional warning when config password is ignored |
| src/core/main_test.go | Added four unit tests covering new user password initialization, existing user skip behavior, warning presence/absence scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22709 +/- ##
===========================================
+ Coverage 45.36% 68.62% +23.26%
===========================================
Files 244 821 +577
Lines 13333 101831 +88498
Branches 2719 0 -2719
===========================================
+ Hits 6049 69886 +63837
- Misses 6983 28057 +21074
- Partials 301 3888 +3587
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
TL;DR: Kindly requesting a re-run of the failed Dear maintainers, Thank you very much for taking the time to review this pull request. I have carefully analyzed the CI failures and would like to respectfully share my findings. Regarding The test failure shows I would be most grateful if you could kindly re-run the failed job at your convenience. Regarding This PR requires a Would you be so kind as to add this label when you have a moment? I sincerely apologize for any inconvenience this request may cause. Please do not hesitate to let me know if there is anything I should modify or if you have any concerns about the implementation. Thank you very much for your time and consideration. Best regards |
| } else { | ||
| log.Infof("User id: %d already has its encrypted password.", userID) | ||
| if password != "" { | ||
| log.Warningf("The admin password in configuration (HARBOR_ADMIN_PASSWORD) will not be applied " + |
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.
How can we determine whether the user is a Harbor admin? At the very least, should we check the user ID?
da76ad9 to
c095506
Compare
The harbor_admin_password setting in harbor.yml only applies during initial installation. When an admin password already exists in the database, the config value is silently ignored, causing confusion when users try to change the password via configuration. This change adds a warning log when the config password is set but will not be applied because a password already exists. The warning guides users to change the password via Harbor UI or API instead. Refactored updateInitPassword to accept a user manager parameter for testability. Added unit tests covering: - New user password initialization - Existing user password skip - Warning when config password differs - No warning when config password is empty Fixes goharbor#22704 Signed-off-by: thc1006 <[email protected]>
- Remove updateInitPasswordWithMgr wrapper function per reviewer request - Add userID == adminUserID check before showing admin-specific warning - Remove unit tests that depended on the removed DI pattern Signed-off-by: thc1006 <[email protected]>
c095506 to
8d3b21d
Compare
Summary
harbor_admin_passwordin config is ignored because admin password already exists in databaseupdateInitPasswordfor testability with dependency injectionBackground
The
harbor_admin_passwordsetting only applies during initial installation. When users modify this value after Harbor is running, the change is silently ignored, leading to "username or password is not correct" errors when they try to login with the new password.This is documented behavior but causes recurring confusion (see related issues below).
Changes
src/core/main.go
updateInitPasswordWithMgrto accept user manager as parametersrc/core/main_test.go (new)
Test Plan
Related Issues
Fixes #22704
Related: #21981, #17122, #12639, #11600