ACL LOAD returns error with non-default auth users about dragonfly HOT 6 CLOSED

Hi @kostasrim,

Thank you, I changed the title of the issue accordingly. Makes sense, then this is something I can ignore in my code for now. Also I didn't think of disconnection after ACL modification.

FWIW, I think there's a better design about the connection management when ACLs are modified: why does DF have to abort all connections given there's a way to select the relevant connections and abort only the modified ones? Overall this would then lead to client disconnect whenever user adds, deletes, modifies a user in the ACL file and it doesn't sound desirable to me for large clusters(with many users) with demanding uptime requirements for example.

from dragonfly.

I also fixed both of the issues you created so you won't see those incompatibility issues in the next release :)

from dragonfly.

Hi @safa-topal,

I don't think this is a bug because of ACL LOAD semantics: regardless of the user the server must kill all the currently open connections and then load the new users. This is for security reasons, since we can't really know priory which of the users exist on the new acl file and therefore all connections must be killed.

I remember testing this with redis back when I implemented ACL but I double checked again today. The only difference I see, is that ACL LOAD in redis does not print directly in redis-cli that the server closed the connection. This is because we kill the connections and never return to the client an OK message which I agree is misleading and I shall patch this soon but if you check with ACL LIST the acl file was loaded corrrectly. For redis-server, when I try to issue a new command in redis-cli after an acl-load it will complain with: Error: Server closed the connection.

All in all,

1. I shall fix that we don't return OK to the client.
2. Plz ignore this for now since there are no really functional changes other than the missing OK message

from dragonfly.

FWIW, I think there's a better design about the connection management when ACLs are modified: why does DF have to abort all connections given there's a way to select the relevant connections and abort only the modified ones?

Yes we do have a mechanism to filter out connections and I did think of this approach initially but that would:

1. Break the spec and the compatibility with ACL in Redis. When a user is deleted or the aclfile is reloaded, all users are killed.
2. Maybe it degrades the system because now for each user and connection you need to re-verify the passwords, so if I have 1k connections then I would need to check 1k passwords! Moreover, redis supports multiple passwords per user (which currently DF doesn't but we might extend) and that would also add an extra layer of checks.

Overall this would then lead to client disconnect whenever user adds, deletes, modifies a user in the ACL file and it doesn't sound desirable to me for large clusters(with many users) with demanding uptime requirements for example.

This is not true, if we update a user they won't disconnect. So for example, if I am user Kostas and you decide as admin that my admin tag should be revoked then you issue an ACL SETUSER -@admin and voila, Kostas is no longer an admin (and my connections are not dropped). However, if I delete you or ACL LOAD then the connection will die. So ACL LOAD act as ACL DELUSER for all users.

I am not saying this is optimal but it's there for compatibility reasons.

from dragonfly.

Overall this would then lead to client disconnect whenever user adds, deletes, modifies a user in the ACL file and it doesn't sound desirable to me for large clusters(with many users) with demanding uptime requirements for example.

This is not true, if we update a user they won't disconnect. So for example, if I am user Kostas and you decide as admin that my admin tag should be revoked then you issue an ACL SETUSER -@admin and voila, Kostas is no longer an admin (and my connections are not dropped). However, if I delete you or ACL LOAD then the connection will die. So ACL LOAD act as ACL DELUSER for all users.

OK I got your point, however it's still true for our use-case because we don't use SETUSER, DELUSER etc, we update the acl file and then issue ACL LOAD command.

Overall, Redis way doing ACL LOAD is not that good when it comes to connection management I think and I understand the concern to stay compatible with Redis behavior.

I also fixed both of the issues you created so you won't see those incompatibility issues in the next release :)

Much appreciated 💯

from dragonfly.

Overall, Redis way doing ACL LOAD is not that good when it comes to connection management I think and I understand the concern to stay compatible with Redis behavior.

Yep and I would be hesitant to provide an extension unless it's an absolute must (for obvious reasons, if we start extending commands for use cases we are drifting away from)

Anyway, let me know if something comes up :)

Best regards!

from dragonfly.