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.

Sign up for GitHub

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

Jump to bottom

QUIT is a command, HOST: and POST are not #9798

Merged
merged 4 commits into from
Nov 23, 2021
Merged

QUIT is a command, HOST: and POST are not #9798

merged 4 commits into from
Nov 23, 2021

Conversation

guybe7
Copy link
Collaborator

@guybe7 guybe7 commented Nov 17, 2021 •
edited by oranagra
Loading

Some people complain that QUIT is missing from help/command table.
Not appearing in COMMAND command, command stats, ACL, etc.
and instead, there's a hack in processCommand with a comment that looks outdated.
Note that it is documented

At the same time, HOST: and POST are there in the command table although these are not real commands.
They would appear in the COMMAND command, and even in commandstats.

Other changes:

  1. Initialize the static logged_time static var in securityWarningCommand
  2. add no-auth flag to RESET so it can always be executed.

Fix #9793

oranagra
oranagra approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redis/core-team please approve promoting the QUIT to a real command, and demoting HOST: and PORT.

no real change on behavior, but does affect COMMAND introspection and INFO commandstats.

@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Nov 17, 2021
@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label Nov 17, 2021
yossigo
yossigo approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@yossigo yossigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I want to mention that QUIT is actually an anti-pattern for TCP based protocols, where we'll always prefer the client to initiate the connection termination and handle TIME_WAIT. Luckily, I don't think too many clients choose to actually use it.

@soloestoy
Copy link
Collaborator

I agree with this PR, but have some questions:

  1. after this PR, QUIT needs auth and ACL permission, that means QUIT would not always return OK reply.
  2. after this PR, if redis is in CLIENT_PAUSE_ALL status, then QUIT command could be also blocked.

not sure if these changes have real effects.

@oranagra
Copy link
Member

good catch, let's add no-auth to QUIT and RESET?

regarding CLIENT_PAUSE_ALL, i think this is ok.

@yoav-steinberg
Copy link
Contributor

@guybe7 Please take a look at #2982 it's a dup of this and might have some more info in to.

@oranagra
Copy link
Member

@guybe7 Please take a look at #2982 it's a dup of this and might have some more info in to.

thanks. actually i remembered that i saw it somewhere, so i should have search for it myself.
anyway, that other PR only adds a command so it's visible in COMMAND command, but it's still not a proper command (will not show up in commandstats, and be filtered by ACL, etc).
i don't think that neither here nor there approach is good.. i think we wanna solve the remaining problems like we do for other commands (they're probably applicable for RESET and others anyway).

@oranagra oranagra mentioned this pull request Nov 18, 2021
Closed
@soloestoy
Copy link
Collaborator

good catch, let's add no-auth to QUIT and RESET?

Agree

@oranagra oranagra requested a review from madolson November 22, 2021 08:56
@oranagra oranagra merged commit b161cff into redis:unstable Nov 23, 2021
hwware pushed a commit to hwware/redis that referenced this pull request Dec 20, 2021
@guybe7 @hwware
QUIT is a command, HOST: and POST are not (redis#9798)
522196c 
Some people complain that QUIT is missing from help/command table.
Not appearing in COMMAND command, command stats, ACL, etc.
and instead, there's a hack in processCommand with a comment that looks outdated.
Note that it is [documented](https://redis.io/commands/quit)

At the same time, HOST: and POST are there in the command table although these are not real commands.
They would appear in the COMMAND command, and even in commandstats.

Other changes:
1. Initialize the static logged_time static var in securityWarningCommand
2. add `no-auth` flag to RESET so it can always be executed.
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Mar 4, 2022
@enjoy-binbin
In redis-cli interactive mode, QUIT is a command
c2e1111 
In redis#9798, QUIT became a command. In redis-cli intercative
mode, we will exit directly if we match quit. Instead of
sending the QUIT command to redis server.

Now in this PR, we will send the real QUIT command to the
redis sever. Execute it as a real command.
@enjoy-binbin enjoy-binbin mentioned this pull request Mar 4, 2022
Closed
@oranagra oranagra mentioned this pull request Jul 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yossigo yossigo yossigo approved these changes

@itamarhaber itamarhaber itamarhaber approved these changes

@oranagra oranagra oranagra approved these changes

@soloestoy soloestoy soloestoy approved these changes

@madolson madolson madolson approved these changes

Assignees
No one assigned
Labels
approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes
Projects
Redis 7.0
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add QUIT to command table, remove HOST: and POST
7 participants
@guybe7 @soloestoy @oranagra @yoav-steinberg @yossigo @itamarhaber @madolson