Skip to content

Conversation

@oranagra
Copy link
Member

@oranagra oranagra commented Apr 18, 2021

DRAFT (to be folded into #8794):

In the initial release of Redis 6.2 setting a user to only allow pubsub access to
a specific, and doing ACL SAVE, resulted in an assertion when ACL LOAD was used.
this was later changed by #8723 (not yet released), but still not properly resolved.

The problem is that the server that generates an ACL file, doesn't know what
would be the setting of the acl-pubsub-default config in the server that will load it.
so it needs to always start with resetchannels directive.

This should still be compatible with old acl files (from redis 6.0), and ones from earlier
versions of 6.2 that didn't mess with channels.

@huangzhw
Copy link
Contributor

huangzhw commented Apr 19, 2021

I think the more severe is when we I have a user like user hello &abc.
The server will refuse to start.

@oranagra
Copy link
Member Author

@huangzhw but why would such a thing happen?

  • The new ACL SAVE will never generate such a thing.
  • The old ACL SAVE would, but that would have lead to a crash in earlier versions, so i can conclude users aren't facing this scenario.
  • Someone may write that line manually into his config or ACL file, and that would work with one config of acl-pubsub-default, and not with the other. but if that's a manual edit, it's the same as many other invalid configuration combinations.

bottom line:

  • i don't mind failing an ACL command, an interactive user session will realize what he did wrong and issue it again.
  • i don't mind failing to start redis right after a manual edit to the acl or config file.
  • i do mind not being able to restart after ACL SAVE or CONFIG REWRITE
  • i do mind not supporting old config files that used to work in the past.

i think this simple one line change fits all of these..

Co-authored-by: Yossi Gottlieb <yossigo@gmail.com>
@huangzhw
Copy link
Contributor

If you have a old server, execute config rewrite and stop server, then replace binary, start will fail.

@oranagra
Copy link
Member Author

If you have a old server, execute config rewrite and stop server, then replace binary, start will fail.

right. but if you do the same without replacing the binary, start will fail too.
that CONFIG REWRITE was broken, and i think it's safe to assume no one used it this way (if they did they would have already noticed and fixed it).

point is, we're not breaking compatibility with (a valid) old version of the config.

@huangzhw
Copy link
Contributor

Agree. You are right. I think this fix is enough.

@oranagra
Copy link
Member Author

folded into #8794

@oranagra oranagra closed this Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants