fix: fix user ERR NOAUTH Authentication required #2939
Conversation
WalkthroughThe pull request introduces changes across multiple files, focusing on configuration updates, code formatting, and enhancements to user management. A new configuration parameter Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
include/pika_client_conn.h (1)
87-87: Consider documenting the authentication flow.The authentication mechanism spans multiple methods (InitUser, DoAuth, UnAuth, IsAuthed) and maintains state via
authenticated_anduser_members. Consider adding documentation to clarify:
- The role of each authentication method
- The state transitions between authenticated/unauthenticated
- The interaction with the ACL system
Also applies to: 129-130
conf/pika.conf (1)
694-694: Document the new configuration parameter.The newly added
best-delete-min-ratioparameter appears to be related to the OBD_Compact strategy, but lacks documentation explaining its purpose and impact.Consider adding a comment block above the parameter explaining:
- Its purpose and relationship with OBD_Compact
- Valid value range and default value
- Impact on compaction behavior
- Relationship with other compaction parameters like
force-compact-min-delete-ratio+ # The minimum ratio of deleted entries required for considering a file + # as a candidate for compaction in the OBD_Compact strategy. + # Valid range: [1-100], Default: 10 best-delete-min-ratio : 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
conf/pika.conf(1 hunks)include/acl.h(3 hunks)include/pika_client_conn.h(3 hunks)src/acl.cc(1 hunks)src/pika_client_conn.cc(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- include/acl.h
🔇 Additional comments (8)
include/pika_client_conn.h (2)
25-33: LGTM! Improved readability of TimeStat methods.
The formatting changes make these simple getter methods more concise while maintaining functionality.
87-87: LGTM! InitUser method addition aligns with authentication fix.
The new InitUser() method centralizes user initialization logic, working alongside existing authentication methods (DoAuth, UnAuth, IsAuthed) to fix the ERR NOAUTH issues when upgrading from v3.5.2 to v3.5.5.
Let's verify the relationship between authentication methods:
✅ Verification successful
InitUser() implementation correctly integrates with existing auth flow
The verification confirms that InitUser() is properly integrated into the authentication system:
- It's called in the constructor to initialize user state
- Sets up initial authentication state based on password requirements and user flags
- Works in conjunction with other auth methods:
DoAuth(): Sets authenticated state after successful authUnAuth(): Resets auth state and userAuthRequired(): Uses authenticated state set by InitUser/DoAuthIsAuthed(): Returns the authenticated state
The implementation shows proper initialization of both user and authentication state, with correct handling of default users and password requirements. The method fits well into the existing authentication flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the relationship between auth-related methods
# Look for calls to InitUser and its interaction with other auth methods
# Search for InitUser implementation and usage
echo "=== InitUser implementation and usage ==="
rg "InitUser.*\(" -A 10
# Search for auth-related method interactions
echo "=== Auth method interactions ==="
rg "DoAuth|UnAuth|IsAuthed|AuthRequired" -A 5
Length of output: 9664
src/pika_client_conn.cc (4)
33-33: LGTM: Improved user initialization in constructor.
The change centralizes user initialization logic in a dedicated method, making it more maintainable.
291-293: LGTM: Clear documentation of pipeline batch handling.
The comment clearly explains that pipeline batches are not intercepted by the cache.
358-368: LGTM: Comprehensive ACL check implementation.
The ACL check properly handles all permission cases (CMD, KEY, CHANNEL, NO_SUB_CMD, NO_AUTH).
554-560: LGTM: Clear and correct authentication check logic.
The authentication check properly handles all cases in the correct order:
- Disabled users always require authentication
- Users with NO_PASS flag don't require authentication
- Other users require explicit authentication
src/acl.cc (2)
492-496: LGTM: Blacklist processing for existing limit user.
The implementation correctly processes blacklisted commands by trimming whitespace and disabling them for the limit user.
509-513: LGTM: Password handling logic fixes the authentication issue.
The implementation correctly sets either:
nopasswhen the password is empty, allowing ordinary users to connect without a password- The provided password when one is configured
This change directly addresses the PR objective of fixing the ERR NOAUTH Authentication required issue.
src/pika_client_conn.cc
Outdated
| void PikaClientConn::InitUser() { | ||
| if (g_pika_conf->requirepass().empty()) { | ||
| user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser); | ||
| } | ||
| user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser); | ||
| authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) && | ||
| !user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED)); | ||
| } |
There was a problem hiding this comment.
Fix logical error in InitUser implementation.
There's a critical bug in the control flow. The requirepass check is ineffective because user_ is always set to DefaultLimitUser regardless of the condition.
Apply this fix to correct the logic:
void PikaClientConn::InitUser() {
if (g_pika_conf->requirepass().empty()) {
user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser);
+ } else {
+ user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser);
}
- user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser);
authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) &&
!user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void PikaClientConn::InitUser() { | |
| if (g_pika_conf->requirepass().empty()) { | |
| user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser); | |
| } | |
| user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser); | |
| authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) && | |
| !user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED)); | |
| } | |
| void PikaClientConn::InitUser() { | |
| if (g_pika_conf->requirepass().empty()) { | |
| user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultUser); | |
| } else { | |
| user_ = g_pika_server->Acl()->GetUserLock(Acl::DefaultLimitUser); | |
| } | |
| authenticated_ = user_->HasFlags(static_cast<uint32_t>(AclUserFlag::NO_PASS)) && | |
| !user_->HasFlags(static_cast<uint32_t>(AclUserFlag::DISABLED)); | |
| } |
There was a problem hiding this comment.
@buzhimingyonghu 谢谢您的修正！🐰🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
src/acl.cc
Outdated
| for (auto& cmd : blacklist) { | ||
| cmd = pstd::StringTrim(cmd, " "); | ||
| u->SetUser("-" + cmd); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicated blacklist processing logic.
The blacklist processing logic is duplicated between the two loops. Consider extracting this into a helper method.
+void ProcessBlacklist(const std::vector<std::string>& blacklist, std::shared_ptr<User>& user) {
+ for (auto& cmd : blacklist) {
+ cmd = pstd::StringTrim(cmd, " ");
+ user->SetUser("-" + cmd);
+ }
+}
void Acl::InitLimitUser(const std::string& bl, bool limit_exist) {
auto pass = g_pika_conf->userpass();
std::vector<std::string> blacklist;
pstd::StringSplit(bl, ',', blacklist);
std::unique_lock wl(mutex_);
auto u = GetUser(DefaultLimitUser);
if (limit_exist) {
if (!bl.empty()) {
- for (auto& cmd : blacklist) {
- cmd = pstd::StringTrim(cmd, " ");
- u->SetUser("-" + cmd);
- }
+ ProcessBlacklist(blacklist, u);
u->SetUser("on");
}
} else {
u->SetUser("on");
u->SetUser("+@all");
u->SetUser("~*");
u->SetUser("&*");
- for (auto& cmd : blacklist) {
- cmd = pstd::StringTrim(cmd, " ");
- u->SetUser("-" + cmd);
- }
+ ProcessBlacklist(blacklist, u);
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/pika_client_conn.cc (2)
261-261: Enhance comment clarity for RTC interception.The current comment could be more descriptive about why only these commands are intercepted.
- // currently we only Intercept: Get, HGet + // Only Get and HGet commands are intercepted for RTC cache optimization + // as they are the most common read operations
358-368: Add descriptive comment for ACL check logic.The ACL check implementation would benefit from a clear comment explaining the permission verification process.
+ // Verify user permissions before cache read: + // - Check command level permissions + // - Check key level access + // - Check channel permissions + // - Verify sub-command validity + // - Confirm authentication status int8_t subCmdIndex = -1; std::string errKey; auto checkRes = user_->CheckUserPermission(c_ptr, argv, subCmdIndex, &errKey);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/pika_client_conn.cc(5 hunks)
🔇 Additional comments (2)
src/pika_client_conn.cc (2)
543-551: LGTM! Implementation correctly fixes the authentication issue.
The InitUser implementation properly addresses the ERR NOAUTH issue by:
- Using DefaultUser for passwordless access
- Using DefaultLimitUser when password is required
- Correctly setting the authenticated state
554-561: LGTM! Improved authentication check logic.
The new AuthRequired implementation follows a clear and logical flow for determining authentication requirements, making the code more maintainable and robust.
| // we don't intercept pipeline batch (argvs.size() > 1) | ||
| if (g_pika_conf->rtc_cache_read_enabled() && argvs.size() == 1 && IsInterceptedByRTC(opt) && | ||
| PIKA_CACHE_NONE != g_pika_conf->cache_mode() && !IsInTxn()) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider splitting complex condition for better readability.
The multiple conditions could be split into separate checks with meaningful variable names for better maintainability.
- if (g_pika_conf->rtc_cache_read_enabled() && argvs.size() == 1 && IsInterceptedByRTC(opt) &&
- PIKA_CACHE_NONE != g_pika_conf->cache_mode() && !IsInTxn()) {
+ bool is_cache_enabled = g_pika_conf->rtc_cache_read_enabled();
+ bool is_single_command = argvs.size() == 1;
+ bool is_cacheable_command = IsInterceptedByRTC(opt);
+ bool has_valid_cache_mode = PIKA_CACHE_NONE != g_pika_conf->cache_mode();
+ bool not_in_transaction = !IsInTxn();
+
+ if (is_cache_enabled && is_single_command && is_cacheable_command &&
+ has_valid_cache_mode && not_in_transaction) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // we don't intercept pipeline batch (argvs.size() > 1) | |
| if (g_pika_conf->rtc_cache_read_enabled() && argvs.size() == 1 && IsInterceptedByRTC(opt) && | |
| PIKA_CACHE_NONE != g_pika_conf->cache_mode() && !IsInTxn()) { | |
| // we don't intercept pipeline batch (argvs.size() > 1) | |
| bool is_cache_enabled = g_pika_conf->rtc_cache_read_enabled(); | |
| bool is_single_command = argvs.size() == 1; | |
| bool is_cacheable_command = IsInterceptedByRTC(opt); | |
| bool has_valid_cache_mode = PIKA_CACHE_NONE != g_pika_conf->cache_mode(); | |
| bool not_in_transaction = !IsInTxn(); | |
| if (is_cache_enabled && is_single_command && is_cacheable_command && | |
| has_valid_cache_mode && not_in_transaction) { |
Mixficsol
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/pika_client_conn.cc (1)
291-293: Consider splitting complex condition for better readability.The multiple conditions could be split into separate checks with meaningful variable names for better maintainability.
- if (g_pika_conf->rtc_cache_read_enabled() && argvs.size() == 1 && IsInterceptedByRTC(opt) && - PIKA_CACHE_NONE != g_pika_conf->cache_mode() && !IsInTxn()) { + bool is_cache_enabled = g_pika_conf->rtc_cache_read_enabled(); + bool is_single_command = argvs.size() == 1; + bool is_cacheable_command = IsInterceptedByRTC(opt); + bool has_valid_cache_mode = PIKA_CACHE_NONE != g_pika_conf->cache_mode(); + bool not_in_transaction = !IsInTxn(); + + if (is_cache_enabled && is_single_command && is_cacheable_command && + has_valid_cache_mode && not_in_transaction) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/acl.cc(1 hunks)src/pika_client_conn.cc(5 hunks)
🔇 Additional comments (4)
src/pika_client_conn.cc (3)
543-551: LGTM! Clean implementation of InitUser.
The implementation correctly initializes the user based on the blacklist configuration and sets the authentication state based on user flags.
554-564: LGTM! Well-structured authentication checks.
The AuthRequired method has been improved with clear, early returns making the authentication logic more readable and maintainable.
358-365: LGTM! Proper ACL permission checks added.
The implementation correctly checks ACL permissions and returns early if the checks fail, maintaining security.
src/acl.cc (1)
499-499: LGTM: Password handling is consistent.
The password handling logic is consistent between both paths (existing and new limit user). The code correctly sets the password using the > prefix to indicate a plaintext password that should be hashed.
Also applies to: 505-505
| for (auto& cmd : blacklist) { | ||
| cmd = pstd::StringTrim(cmd, " "); | ||
| u->SetUser("-" + cmd); | ||
| } | ||
| u->SetUser("on"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated blacklist processing logic.
The blacklist processing logic is duplicated between the two code blocks. Consider extracting this into a helper method to improve maintainability and reduce code duplication.
Apply this diff to extract the common logic:
+void ProcessBlacklist(std::shared_ptr<User>& user, const std::vector<std::string>& blacklist) {
+ for (auto& cmd : blacklist) {
+ cmd = pstd::StringTrim(cmd, " ");
+ user->SetUser("-" + cmd);
+ }
+}
void Acl::InitLimitUser(const std::string& bl, bool limit_exist) {
auto pass = g_pika_conf->userpass();
std::vector<std::string> blacklist;
pstd::StringSplit(bl, ',', blacklist);
std::unique_lock wl(mutex_);
auto u = GetUser(DefaultLimitUser);
if (limit_exist) {
if (!bl.empty()) {
- for (auto& cmd : blacklist) {
- cmd = pstd::StringTrim(cmd, " ");
- u->SetUser("-" + cmd);
- }
+ ProcessBlacklist(u, blacklist);
u->SetUser("on");
}
if (!pass.empty()) {
u->SetUser(">" + pass);
}
} else {
if (pass.empty()) {
u->SetUser("nopass");
} else {
u->SetUser(">" + pass);
}
u->SetUser("on");
u->SetUser("+@all");
u->SetUser("~*");
u->SetUser("&*");
- for (auto& cmd : blacklist) {
- cmd = pstd::StringTrim(cmd, " ");
- u->SetUser("-" + cmd);
- }
+ ProcessBlacklist(u, blacklist);
}
}Also applies to: 512-515
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
tools/pika_exporter/exporter/metrics/parser.go (4)
33-57: OptimizeCheckContainsEmptyValuemethod inVersionChecker336To improve the performance of the
CheckContainsEmptyValuemethod, consider using amap[string]struct{}for theEmptyValueNamefield. This change reduces the lookup time from O(n) to O(1).Apply this diff to optimize the lookup:
type VersionChecker336 struct { - EmptyValueName []string + EmptyValueName map[string]struct{} } func (v *VersionChecker336) CheckContainsEmptyValue(key string) bool { if v.EmptyValueName == nil { - v.EmptyValueName = []string{ + v.EmptyValueName = map[string]struct{}{ - "instantaneous_output_repl_kbps", + "instantaneous_output_repl_kbps": {}, "total_net_output_bytes": {}, "cache_db_num": {}, "hits_per_sec": {}, "cache_status": {}, "total_net_input_bytes": {}, "instantaneous_output_kbps": {}, "instantaneous_input_kbps": {}, "total_net_repl_input_bytes": {}, "instantaneous_input_repl_kbps": {}, "slow_logs_count": {}, "total_net_repl_output_bytes": {}, "cache_memory": {}, } } - for _, str := range v.EmptyValueName { - if str == key { + if _, exists := v.EmptyValueName[key]; exists { return true } return false }
63-78: OptimizeCheckContainsEmptyValuemethod inVersionChecker355Similar to
VersionChecker336, using amap[string]struct{}forEmptyValueNamewill enhance performance by providing constant-time lookups.Apply this diff to optimize the method:
type VersionChecker355 struct { - EmptyValueName []string + EmptyValueName map[string]struct{} } func (v *VersionChecker355) CheckContainsEmptyValue(key string) bool { if v.EmptyValueName == nil { - v.EmptyValueName = []string{ + v.EmptyValueName = map[string]struct{}{ - "cache_db_num", + "cache_db_num": {}, "cache_status": {}, "cache_memory": {}, "hits_per_sec": {}, } } - for _, str := range v.EmptyValueName { - if str == key { + if _, exists := v.EmptyValueName[key]; exists { return true } return false }
84-99: OptimizeCheckContainsEmptyValuemethod inVersionChecker350Again, implementing a
map[string]struct{}forEmptyValueNameenhances lookup efficiency.Apply this diff to optimize the method:
type VersionChecker350 struct { - EmptyValueName []string + EmptyValueName map[string]struct{} } func (v *VersionChecker350) CheckContainsEmptyValue(key string) bool { if v.EmptyValueName == nil { - v.EmptyValueName = []string{ + v.EmptyValueName = map[string]struct{}{ - "cache_db_num", + "cache_db_num": {}, "cache_status": {}, "cache_memory": {}, "hits_per_sec": {}, "slow_logs_count": {}, } } - for _, str := range v.EmptyValueName { - if str == key { + if _, exists := v.EmptyValueName[key]; exists { return true } return false }
309-309: Redundant empty lineThe added empty line at line 309 is unnecessary and can be removed to maintain code consistency and readability.
tools/pika_exporter/exporter/pika.go (2)
267-282: Enhance version handling inselectversionfunctionThe
selectversionfunction currently handles specific version strings explicitly. This approach may not scale well with future versions and could lead to maintenance challenges. Consider using semantic version parsing to handle version comparisons more robustly, supporting version ranges and defaulting to the appropriateVersionCheckerimplementations.Apply the following changes to improve version handling:
+import ( + "github.com/blang/semver" +) func selectversion(versionStr string) metrics.VersionChecker { - var v metrics.VersionChecker - - switch version { - case "3.3.6": - v = &metrics.VersionChecker336{} - case "3.5.5": - v = &metrics.VersionChecker355{} - case "3.5.0": - v = &metrics.VersionChecker350{} - default: - return nil - } - return v + version, err := semver.Parse(versionStr) + if err != nil { + log.Warnf("Invalid version format: %s", versionStr) + return nil + } + + if version.Major == 3 { + if version.Minor == 3 && version.Patch >= 6 { + return &metrics.VersionChecker336{} + } else if version.Minor == 5 { + if version.Patch >= 5 { + return &metrics.VersionChecker355{} + } else if version.Patch >= 0 { + return &metrics.VersionChecker350{} + } + } + } + + log.Warnf("Unsupported Pika version: %s", versionStr) + return nil }This refactor:
- Utilizes the
semverpackage for semantic version parsing.- Supports version ranges, allowing for easier future maintenance.
- Logs a warning when an unsupported or invalid version is encountered.
Remember to update your
go.modfile to include thegithub.com/blang/semverpackage.
267-282: Use consistent naming conventions for function namesIn Go, the convention is to use mixedCaps (camelCase) for function names. The function
selectversionshould be renamed toselectVersionto follow Go naming conventions, improving code readability and consistency.Apply this diff:
-func selectversion(versionStr string) metrics.VersionChecker { +func selectVersion(versionStr string) metrics.VersionChecker { // function body }Also, update all references to this function:
parseOpt := metrics.ParseOption{ Version: version, Extracts: extracts, Info: info, - CurrentVersion: selectversion(version.Original()), + CurrentVersion: selectVersion(version.Original()), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tools/pika_exporter/exporter/metrics/parser.go(4 hunks)tools/pika_exporter/exporter/pika.go(2 hunks)
🔇 Additional comments (3)
tools/pika_exporter/exporter/metrics/parser.go (3)
20-25: Addition of CurrentVersion field in ParseOption struct
The introduction of the CurrentVersion field to the ParseOption struct allows for version-specific parsing logic, enhancing the flexibility of the parser.
26-28: Implementation of VersionChecker interface
Defining the VersionChecker interface provides a clean abstraction for version-specific checks, improving code maintainability.
191-193: Conditional logging in regexParser
The conditional logging appropriately checks if the CurrentVersion is nil or if it does not contain the empty value, ensuring that warnings are logged only when necessary.
| type VersionChecker336 struct { | ||
| EmptyValueName []string | ||
| } | ||
|
|
||
| func (v *VersionChecker336) CheckContainsEmptyValue(key string) bool { | ||
| if v.EmptyValueName == nil { | ||
| v.EmptyValueName = []string{ | ||
| "instantaneous_output_repl_kbps", | ||
| "total_net_output_bytes", | ||
| "cache_db_num", | ||
| "hits_per_sec", | ||
| "cache_status", | ||
| "total_net_input_bytes", | ||
| "instantaneous_output_kbps", | ||
| "instantaneous_input_kbps", | ||
| "total_net_repl_input_bytes", | ||
| "instantaneous_input_repl_kbps", | ||
| "slow_logs_count", | ||
| "total_net_repl_output_bytes", | ||
| "cache_memory", | ||
| } | ||
| } | ||
| for _, str := range v.EmptyValueName { | ||
| if str == key { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| type VersionChecker355 struct { | ||
| EmptyValueName []string | ||
| } | ||
|
|
||
| func (v *VersionChecker355) CheckContainsEmptyValue(key string) bool { | ||
| if v.EmptyValueName == nil { | ||
| v.EmptyValueName = []string{ | ||
| "cache_db_num", | ||
| "cache_status", | ||
| "cache_memory", | ||
| "hits_per_sec", | ||
| } | ||
| } | ||
| for _, str := range v.EmptyValueName { | ||
| if str == key { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| type VersionChecker350 struct { | ||
| EmptyValueName []string | ||
| } | ||
|
|
||
| func (v *VersionChecker350) CheckContainsEmptyValue(key string) bool { | ||
| if v.EmptyValueName == nil { | ||
| v.EmptyValueName = []string{ | ||
| "cache_db_num", | ||
| "cache_status", | ||
| "cache_memory", | ||
| "hits_per_sec", | ||
| "slow_logs_count", | ||
| } | ||
| } | ||
| for _, str := range v.EmptyValueName { | ||
| if str == key { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication among VersionChecker implementations
The three VersionChecker types have similar structures and methods. Consider refactoring to a single VersionChecker type with parameterized empty value names to reduce code duplication and improve maintainability.
Here's how you might refactor the code:
type VersionChecker struct {
EmptyValueName map[string]struct{}
}
func NewVersionChecker(emptyValueNames []string) *VersionChecker {
emptyValueMap := make(map[string]struct{})
for _, name := range emptyValueNames {
emptyValueMap[name] = struct{}{}
}
return &VersionChecker{EmptyValueName: emptyValueMap}
}
func (v *VersionChecker) CheckContainsEmptyValue(key string) bool {
if v.EmptyValueName == nil {
return false
}
_, exists := v.EmptyValueName[key]
return exists
}
// Initialize VersionCheckers for different versions
var VersionChecker336 = NewVersionChecker([]string{
"instantaneous_output_repl_kbps",
"total_net_output_bytes",
// ... other names
})
var VersionChecker355 = NewVersionChecker([]string{
"cache_db_num",
"cache_status",
// ... other names
})
var VersionChecker350 = NewVersionChecker([]string{
"cache_db_num",
"cache_status",
// ... other names
})| if opt.CurrentVersion == nil || !opt.CurrentVersion.CheckContainsEmptyValue(m.ValueName) { | ||
| log.Warnf("normalParser::Parse not found value. metricName:%s valueName:%s", m.Name, m.ValueName) | ||
| } | ||
| return | ||
|
|
There was a problem hiding this comment.
Unintended early return in normalParser
The unconditional return statement at line 255 may prevent further parsing after logging the warning. If the intention is to skip processing only when the value is genuinely not found, consider moving the return inside the conditional block.
Apply this diff to adjust the control flow:
if m.ValueName != "" {
if v, ok := findInMap(m.ValueName, opt.Extracts); !ok {
if opt.CurrentVersion == nil || !opt.CurrentVersion.CheckContainsEmptyValue(m.ValueName) {
log.Warnf("normalParser::Parse not found value. metricName:%s valueName:%s", m.Name, m.ValueName)
}
+ return
} else {
metric.Value = convertToFloat64(v)
}
}Committable suggestion skipped: line range outside the PR's diff.
tools/pika_exporter/exporter/pika.go
Outdated
| Version: version, | ||
| Extracts: extracts, | ||
| Info: info, | ||
| CurrentVersion: selectversion(version.Original()), |
There was a problem hiding this comment.
Potential nil dereference due to unhandled nil CurrentVersion
In the collectInfo function, the ParseOption struct now includes CurrentVersion, which is assigned via selectversion(version.Original()). If selectversion returns nil (which happens when an unsupported version is encountered), this could lead to a nil pointer dereference when CurrentVersion methods are called later in the code. To prevent this, ensure that you check for nil and handle unsupported versions appropriately.
Apply the following changes to handle the nil case:
func (e *exporter) collectInfo(c *client, ch chan<- prometheus.Metric) error {
// update info config
if err := LoadConfig(); err != nil {
log.Errorln("load config failed:", err)
return err
}
info, err := c.GetInfo()
if err != nil {
return err
}
version, extracts, err := parseInfo(info)
if err != nil {
return err
}
extracts[metrics.LabelNameAddr] = c.Addr()
extracts[metrics.LabelNameAlias] = c.Alias()
extracts[metrics.LabelInstanceMode], err = c.InstanceModeInfo()
if err != nil {
return err
}
extracts[metrics.LabelConsensusLevel], err = c.LabelConsensusLevelInfo()
if err != nil {
return err
}
collector := metrics.CollectFunc(func(m metrics.Metric) error {
promMetric, err := prometheus.NewConstMetric(
prometheus.NewDesc(prometheus.BuildFQName(e.namespace, "", m.Name), m.Help, m.Labels, nil),
m.MetricsType(), m.Value, m.LabelValues...)
if err != nil {
return err
}
ch <- promMetric
return nil
})
parseOpt := metrics.ParseOption{
Version: version,
Extracts: extracts,
Info: info,
CurrentVersion: selectversion(version.Original()),
}
+ if parseOpt.CurrentVersion == nil {
+ log.Errorf("Unsupported Pika version: %s", version.Original())
+ return fmt.Errorf("unsupported Pika version: %s", version.Original())
+ }
for _, m := range metrics.MetricConfigs {
m.Parse(m, collector, parseOpt)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Version: version, | |
| Extracts: extracts, | |
| Info: info, | |
| CurrentVersion: selectversion(version.Original()), | |
| Version: version, | |
| Extracts: extracts, | |
| Info: info, | |
| CurrentVersion: selectversion(version.Original()), | |
| } | |
| if parseOpt.CurrentVersion == nil { | |
| log.Errorf("Unsupported Pika version: %s", version.Original()) | |
| return fmt.Errorf("unsupported Pika version: %s", version.Original()) | |
| } |
buzhimingyonghu
force-pushed
the
unstable
branch
from
93e6e6d to
19c1c8b
Compare
| } | ||
| } else { | ||
| if (pass.empty()) { | ||
| u->SetUser("nopass"); | ||
| } else { | ||
| u->SetUser(">"+pass); | ||
| u->SetUser(">" + pass); |
There was a problem hiding this comment.
别的地方没啥问题, 就是这里和Acl::UpdateDefaultUserPassword这个函数里,要多次判断和设置密码
Issue
(#2920)
描述
配置如下：
requirepass:xxx(管理员密码)userpass:userblacklist:FLUSHALL设置了管理员密码，然后将
userpass设置为空，这样普通用户无需密码即可使用。对于管理命令，如FLUSHALL等，需要管理员密码登录才可使用。在 3.5.2 版本中，工作正常，升级到 3.5.5 后，普通用户无法连接 Pika，提示：ERR NOAUTH Authentication required。需要去除
requirepass才可以正常连接使用，如下：requirepass:userpass:userblacklist:FLUSHALL只有两项均为空，才可以无需密码连接，无法单独设置管理员密码。
Issue 背景
在 3.5.5 后，添加了 ACL 认证，改变了传统的认证方式
auth_stat_.IsAuthed(c_ptr)。每一个PikaClientConn都拥有一个用户，里面存储了相关的 flag、password、limit 等信息。每来一个请求，PikaClientConn都会做一次 Check authed，即检查当前PikaClientConn是否有密码、是否被禁用、是否需要认证。问题
由于
PikaClientConn在初始化时一直使用的是DefaultUser，所以在配置文件中无论是否配置密码或 blacklist，均使用的是DefaultUser。解决方案
需要根据配置文件，去调整用户信息。在
PikaClientConn初始化时，获取g_pika_conf的requirepass()是否为空；如果为空，则使用默认的用户；如果设置了密码，则使用 limit 的用户。经过测试可以实现普通用户无密码登陆
有密码登陆也正常
limit也正常
Summary by CodeRabbit
New Features
best-delete-min-ratiofor improved compaction strategy.InitUser()to enhance user initialization in the Pika client connection.Bug Fixes
Style
Documentation