Add AUTH/AUTH2 options to CLUSTER MIGRATESLOTS#3538
Add AUTH/AUTH2 options to CLUSTER MIGRATESLOTS#3538nemtsv wants to merge 4 commits intovalkey-io:unstablefrom
Conversation
Signed-off-by: Vasily Nemtsov <13330571+nemtsv@users.noreply.github.com>
efe6bc7 to
b1c821b
Compare
Signed-off-by: Vasily Nemtsov <13330571+nemtsv@users.noreply.github.com>
b1c821b to
426ae50
Compare
madolson
left a comment
There was a problem hiding this comment.
Implementation seems reasonable. One thing to consider is whether we need both AUTH and AUTH2. The new HELLO command requires username and password, so maybe we should just require username and password here and skip the AUTH/AUTH2 split. One blocker around binary safety.
| lens[argc] = strlen(server.primary_user); | ||
| if (user) { | ||
| args[argc] = (char *)user; | ||
| lens[argc] = strlen(user); |
There was a problem hiding this comment.
blocker: strlen(user) and strlen(pass) are not binary safe. The old code used sdslen(server.primary_auth) for the password, which correctly handles embedded null bytes. This PR changes it to strlen(pass), which truncates at the first \0.
This regresses all three call sites, not just the new migration path — the two existing replication callers also pass server.primary_auth (an sds) through strlen now.
For comparison, MIGRATE uses sdslen(username) and sdslen(password) (cluster.c ~line 538-540).
Fix: add length parameters to the signature:
sds replicationSendAuth(connection *conn, const char *user, size_t user_len, const char *pass, size_t pass_len);Callers pass sdslen() for sds values and strlen() for char * values. This also makes the binary-safety contract explicit at each call site.
| serverAssert(job->type == SLOT_MIGRATION_EXPORT); | ||
| serverAssert(server.primary_auth); | ||
| sds user = job->auth_user ? job->auth_user : server.primary_user; | ||
| sds pass = job->auth_password ? job->auth_password : server.primary_auth; |
There was a problem hiding this comment.
minor: server.primary_user is char *, not sds. Assigning it to an sds variable works (both are char *) but is misleading. Use const char * for both locals, which also matches the replicationSendAuth signature.
| @@ -1249,6 +1280,13 @@ void clusterCommandMigrateSlots(client *c) { | |||
| cleanup: | |||
There was a problem hiding this comment.
nit: The if guard is unnecessary — sdsfree handles NULL. The if (auth_pass) guard is needed for the memset, but this one is just noise. Same in freeSlotMigrationJob at line 2211. Optional to clean up.
Fixes #2392.
CLUSTER MIGRATESLOTScurrently authenticates to target nodes using thesource node's
masteruser/masterauth. This adds optionalper-target AUTH/AUTH2 support, matching the existing
MIGRATEcommand syntax.New syntax:
When AUTH/AUTH2 is omitted, behavior is unchanged (falls back to masteruser/masterauth).
Changes:
auth_user/auth_passwordfields toslotMigrationJobslotMigrationJobSendAuthto prefer per-job credentials over globalsTests:
TODO: