kuzzleio / kuzzle-plugin-cluster

Kuzzle cluster mode

Apache License 2.0

7 stars 2 forks source link

KZL-998- persist strategies in redis #74

Closed benoitvidis closed 5 years ago

benoitvidis commented 5 years ago

What does this PR do ?

This PR fixes the case where some strategies could be lost across a cluster of Kuzzle nodes if one or more of them came to not respond by persisting the strategies in Redis.

How should this be manually tested?

Add this small plugin to your cluster: https://gist.github.com/benoitvidis/a5d9bd592a261d7b903209e73abed59d
Check the dummy strategy does not exist by trying to log in
Call http://localhost:7512/_plugin/test/do/bar
Check the dummy strategy is known on all nodes
Call http://localhost:7512/_plugin/test/do/foo
Check the dummy strategy has been deleted from all nodes

codecov-io commented 5 years ago

Codecov Report

Merging #74 into 3-dev will increase coverage by 0.87%. The diff coverage is 82.6%.

@@            Coverage Diff             @@
##            3-dev      #74      +/-   ##
==========================================
+ Coverage   57.03%   57.91%   +0.87%     
==========================================
  Files           4        4              
  Lines         533      537       +4     
==========================================
+ Hits          304      311       +7     
+ Misses        229      226       -3

Impacted Files	Coverage Δ
lib/redis/manager.js	`33.33% <0%> (+2.18%)`	:arrow_up:
lib/index.js	`49.44% <100%> (+0.37%)`	:arrow_up:
lib/node.js	`77.72% <93.75%> (+0.17%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5cd1285...9dc742e. Read the comment docs.

benoitvidis commented 5 years ago

@scottinet not really. Some cleaning is done when a node is exiting but only for realtime subscriptions.

I see at least 2 cases where this fix is needed:

the one described in the original linked issue, which we already encountered twice on production
if you scale a cluster up.

scottinet commented 5 years ago

I don't question the fix, I see clearly why it's needed.

I only wonder what happens if persisted strategy information gets obsoleted when a cluster restarts (e.g. an auth plugin is uninstalled).

From the top of my head, I don't think that this pose a security risk, as Kuzzle will believe that the strategy exists at first, but an error will be issued when it'll try to use it. So credentials should never be leaked. But this may warrant a check just to be sure. What do you think?

benoitvidis commented 5 years ago

Ah yes, you're right. I will check what happens in this case.

benoitvidis commented 5 years ago

@scottinet Just checked and the strategy is rejected on the login attempt stating the strategy is unknown. So yes, we properly get an error in this situation.