[I] Potential Vulnerability in Update Sequence During Slot Migration [kvrocks-controller]

["RiversJin (via GitHub)" <gi...@apache.org>]

RiversJin opened a new issue, #268:
URL: https://github.com/apache/kvrocks-controller/issues/268

   In the `tryUpdateMigrationStatus` logic, after a successful migration, the controller first updates the topology in the kvrocks cluster via `SetSlot`, then updates the etcd data using `clusterStore.UpdateCluster`. However, there appears to be a potential vulnerability here:  
   - If `SetSlot` succeeds but a network failure prevents `UpdateCluster` from completing (or worse, if `SetSlot` partially succeeds with some nodes updated and others failing), this could leave the kvrocks cluster version **higher** than the cluster version stored in etcd.  
   - Even if another controller takes over from the failed one, it would be unable to execute new operations because subsequent "versions" issued by the controller would no longer exceed the version already in kvrocks.  
   
   To address this, I suggest reversing the order: **update etcd first**, then propagate the new topology to kvrocks.  
   As a side note, if this approach is adopted, the `clusterx set slot` command in kvrocks might become redundant, as we could fully update the post-migration topology using `setnodes` instead.  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Potential Vulnerability in Update Sequence During Slot Migration [kvrocks-controller]

["Kaustubh1204 (via GitHub)" <gi...@apache.org>]

Kaustubh1204 commented on issue #268:
URL: https://github.com/apache/kvrocks-controller/issues/268#issuecomment-3892268749

   Hi @RiversJin and @git-hulk,
   I would like to take ownership of this issue.
   From my understanding, the core problem lies in the non-atomic update sequence between Kvrocks (data plane) and etcd (control plane). If SetSlot succeeds but UpdateCluster fails, the version in Kvrocks can advance beyond the version stored in etcd, leading to a divergence that may block subsequent controller operations. This creates a potential deadlock scenario where future version increments are rejected because they are no longer strictly greater than the already-applied version.
   Reversing the update order (etcd first, then Kvrocks) can mitigate this specific failure mode, but I believe we should also clearly define version authority and consider reconciliation logic to handle cases where Kvrocks is already ahead. Ensuring idempotency and convergence under partial failures will be critical.
   My plan is to:
   Trace the full slot migration flow and version bump logic.
   Identify all failure points between SetSlot and UpdateCluster.
   Propose a safe update sequence with proper CAS/version checks.
   Evaluate whether reconciliation logic is needed when Kvrocks reports a higher version.
   I’ll share a more concrete design proposal before implementing changes to ensure alignment.
   Please let me know if you have any specific constraints or considerations I should keep in mind.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Potential Vulnerability in Update Sequence During Slot Migration [kvrocks-controller]

["git-hulk (via GitHub)" <gi...@apache.org>]

git-hulk commented on issue #268:
URL: https://github.com/apache/kvrocks-controller/issues/268#issuecomment-2668115191

   @RiversJin Thanks for your investigation, we did have this issue, and changing the order could mitigate it. Another enhancement would be allowed to update the store(ETCD) once the version in Kvrocks is higher than the controller.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org