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

migrate command with password #2507

Merged
merged 9 commits into from Jan 9, 2018

Conversation

Projects
None yet
8 participants
@AlexStocks
Copy link
Contributor

AlexStocks commented Apr 8, 2015

#The format of migrate command is "MIGRATE host port key destination-db timeout [COPY] [REPLACE]". If the destination server is protected by a paassword, it will not work. This obstacle has confused so many people and so long time (for example:http://redis.io/commands/migrate, the question asked by Marc-Olivier LaBarre "Does it work if the destination redis instance is protected by a password?") that I decided to add this feature by myself.

@@ -4573,6 +4576,9 @@ void migrateCommand(redisClient *c) {
copy = 1;
} else if (!strcasecmp(c->argv[j]->ptr,"replace")) {
replace = 1;
} else if (!auth) {

This comment has been minimized.

@badboy

badboy Apr 8, 2015

Contributor

This always enables auth for any additional argument.

This comment has been minimized.

@AlexStocks

AlexStocks Apr 8, 2015

Contributor

it meas that if you use migrate command like this "migrate 127.0.0.1 40000 hash_resume 0 1000000 copy replace password1 abcdefg" and then the key word "password1" is considered as a password but the word "abcdefg" is a wrong argument.

void migrateCommand(redisClient *c) {
migrateCachedSocket *cs;
int copy, replace, j;
int copy, replace, auth, j;
char* password;

This comment has been minimized.

@badboy

badboy Apr 8, 2015

Contributor

Small style issue: make it char *password (star right-aligned)

// addReplyErrorFormat(c,"Target instance replied with error: %s",
// (auth && buf1[0] == '-') ? buf1+1 : ((select && buf2[0] == '-') ? buf2+1 : buf3+1));
// goto socket_rd_err;
// }

This comment has been minimized.

@badboy

badboy Apr 8, 2015

Contributor

If this is to be merged, we can reduce the comment to a small summary, no need to retain old code here.

@badboy

This comment has been minimized.

Copy link
Contributor

badboy commented Apr 8, 2015

Some comments inline. Final decision is up to @antirez anyway.

One more suggestion: We probably want MIGRATE host port key dbid timeout [COPY | REPLACE | AUTH password], where password is the actual password and AUTH just a keyword to signal that intent, that would allow for any password (with the current approach COPY and REPLACE are not possible as password).

// goto socket_rd_err;
// }
if ((select && buf2[0] == '-') || buf3[0] == '-') {
if (!auth) {

This comment has been minimized.

@badboy

badboy Apr 8, 2015

Contributor

Why only if no auth is given?

@AlexStocks

This comment has been minimized.

Copy link
Contributor

AlexStocks commented Apr 8, 2015

@badboy Hey, good guy. I have changed migrate command format again as your suggestion. Please check it again. Hope you will like it and give more suggestions.

@anthonator

This comment has been minimized.

Copy link

anthonator commented May 11, 2016

I'm not sure I agree with this implementation but we need this feature.

Would you be able to use both AUTH and COPY, REPLACE or KEYS? I'm not very familiar with C but it doesn't look like it. The syntax also communicates it's not possible (at least to me).

I think something like this makes sense:

MIGRATE host password port key dbid timeout [COPY | REPLACE | KEYS]

For the sake of backwards compatibility you might also be able to tack it on to the host param separated by a : or something.

Example:

MIGRATE mypassword:my.host 6379 "" 0 5000 KEYS mykey

You could also introduce a MIGRATE AUTH command with the syntax I proposed in my first example.

Would love to have the ability to use a password.

@anthonator anthonator referenced this pull request May 11, 2016

Open

Support Redis 3.x #12

@anthonator

This comment has been minimized.

Copy link

anthonator commented May 14, 2016

@antirez curious to hear your take on this. I know this is an old PR but it would be good to know if adding password to MIGRATE would be something you'd accept.

@AlexStocks

This comment has been minimized.

Copy link
Contributor

AlexStocks commented May 16, 2016

@anthonator I am very happy that hear your reply.Pls check my last commit . According to the advice of @badboy, I have changed the format of command MIGRATE as follows:
MIGRATE host port key dbid timeout [COPY | REPLACE | AUTH password]

As your suggestion, @antirez would be better like your migrate command format (MIGRATE host password port key dbid timeout [COPY | REPLACE | KEYS]) ? If sure, I will modify my codes of course.

@AlexStocks AlexStocks changed the title modify migrate command with veify destination server password feature modify migrate command with verify destination server password feature Aug 29, 2016

@chester89

This comment has been minimized.

Copy link

chester89 commented Jan 9, 2017

@AlexStocks any updates on this?

@spccold

This comment has been minimized.

Copy link

spccold commented Apr 19, 2017

any updates on this?

@AlexStocks

This comment has been minimized.

Copy link
Contributor

AlexStocks commented Apr 28, 2017

@chester89 @spccold I do not know why @antirez does not accept this branch.

@allenbaoj

This comment has been minimized.

Copy link

allenbaoj commented Jul 17, 2017

@antirez , could you pay close attention to this? this is very userful fou us. hope you can accept this branch. thanks.

@AlexStocks

This comment has been minimized.

Copy link
Contributor

AlexStocks commented Aug 15, 2017

@badboy @anthonator @chester89 @spccold @allenbaoj I have a good news for all of U. I have updated my branch codes with the antirez/redis unstable branch latest codes now[until 2017/08/15]. Before @antirez accept this pull request, U can use this this feature by copy the file cluster.c to your local redis code directory and compile it to get your own redis binary. Hope u like it.

The latest "migrate" command format is as follows:

  • MIGRATE host port key dbid timeout [COPY | REPLACE | AUTH password]
  • On in the multiple keys form:
  • MIGRATE host port "" dbid timeout [COPY | REPLACE | AUTH password] KEYS key1 key2 ... keyN

@AlexStocks AlexStocks changed the title modify migrate command with verify destination server password feature migrate command with password Aug 15, 2017

@artix75
Copy link
Contributor

artix75 left a comment

The implementation seems ok to me. Anyway I should talk with @antirez about the appropriateness of changing the command's syntax in order to handle the issue you're trying to solve.

Just a note:

In case of wrong password you're directly replying with the standard AUTH error (shared.noautherr) and I think this could bring some confusion since it's not clear if the auth went wrong on the source instance or if it went wrong on the target instance.
I think it would be better to prefix the error with ""Target instance replied with error: " as it's already done for other target errors.

@AlexStocks

This comment has been minimized.

Copy link
Contributor

AlexStocks commented Dec 20, 2017

@artix75 Thank you. I have changed the error alert as your suggestion to "ERR Target instance replied with error: NOAUTH Authentication required.". And also I have merged the latest unstable branch(until 2017/12/20) into this branch.

@artix75

artix75 approved these changes Jan 2, 2018

@antirez antirez merged commit cc4d67a into antirez:unstable Jan 9, 2018

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Jan 9, 2018

Merged, but we identified errors that we are fixing. Thanks.

antirez added a commit that referenced this pull request Jan 9, 2018

Several fixes to MIGRATE AUTH option.
See PR #2507. The PR was merged but there were different errors in the
implementation, so this is kinda of a rewrite in order to use the
existing error handling mechanisms.

antirez added a commit that referenced this pull request Jan 9, 2018

antirez added a commit that referenced this pull request Jan 9, 2018

Rewrite MIGRATE AUTH option.
See PR #2507. This is a reimplementation of the fix that contained
different problems.

antirez added a commit that referenced this pull request Jan 9, 2018

antirez added a commit that referenced this pull request Jan 11, 2018

Rewrite MIGRATE AUTH option.
See PR #2507. This is a reimplementation of the fix that contained
different problems.

antirez added a commit that referenced this pull request Jan 11, 2018

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Jan 13, 2018

Rewrite MIGRATE AUTH option.
See PR antirez#2507. This is a reimplementation of the fix that contained
different problems.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Jan 13, 2018

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Jan 13, 2018

Rewrite MIGRATE AUTH option.
See PR antirez#2507. This is a reimplementation of the fix that contained
different problems.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Jan 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment