[TT-12417] Do not delete keys on synchronization

mativm02 commented 1 month ago

Summary	Api keys are lost in worker gateways when keyspace sync interrupted
Type	Bug
Status	In Dev
Points	N/A
Labels	customer_bug, jira_escalated

Description

Avoiding key deletion when synchronizing. This will avoid having inconsistent key data between master and slave Redis.

Motivation and Context

https://tyktech.atlassian.net/browse/TT-12417?atlOrigin=eyJpIjoiYWNiZTdlNmYwODY5NDI1ZDkzYmQ1MWFlZjM5NGQ3ZTgiLCJwIjoiaiJ9

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

[ ] I ensured that the documentation is up to date
[ ] I explained why this PR updates go.mod in detail with reasoning why it's required
[ ] I would like a code coverage CI quality gate exception and have explained why

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Error Handling
The error handling for the key retrieval process might be insufficient. The debug log in line 1139 only logs the error but does not handle it, potentially leading to unmanaged errors that could affect the flow. Logging Level
The use of `log.Debugf` for a potentially critical error related to key retrieval might be inappropriate. Consider using a higher log level such as `Error` or `Warn` to ensure visibility in production environments.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Add error handling for session detail retrieval to prevent unnoticed failures ___ Ensure that the session details are properly handled for both hashed and non-hashed keys by checking the return values and handling potential errors. [gateway/rpc_storage_handler.go [1128-1134]](https://github.com/TykTechnologies/tyk/pull/6642/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021R1128-R1134) ```diff -_, found = r.Gw.GlobalSessionManager.SessionDetail(orgId, key, true) -_, found = r.Gw.GlobalSessionManager.SessionDetail(orgId, key, false) +_, found, err = r.Gw.GlobalSessionManager.SessionDetail(orgId, key, true) +if err != nil { + log.Errorf("Error retrieving session details: %v", err) +} +_, found, err = r.Gw.GlobalSessionManager.SessionDetail(orgId, key, false) +if err != nil { + log.Errorf("Error retrieving session details: %v", err) +} ``` Suggestion importance[1-10]: 9 Why: This suggestion adds necessary error handling for session detail retrieval, which is crucial for preventing silent failures and ensuring robustness in the code.	9
Enhancement	Improve error logging for better error visibility and troubleshooting ___ Replace the debug log statement with an error log if the key ID cannot be found, to ensure visibility of critical errors. [gateway/rpc_storage_handler.go [1139]](https://github.com/TykTechnologies/tyk/pull/6642/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021R1139-R1139) ```diff -log.Debugf("cannot found key with id: %v, error: %v", id, err) +log.Errorf("cannot found key with id: %v, error: %v", id, err) ``` Suggestion importance[1-10]: 8 Why: Changing the log level from debug to error is a significant improvement for visibility of critical issues, ensuring that missing keys are not overlooked in production environments.	8
Maintainability	Refactor session detail checking into a method to enhance code maintainability ___ Refactor the session detail retrieval and key not found logic into a separate method to improve code readability and maintainability. [gateway/rpc_storage_handler.go [1128-1141]](https://github.com/TykTechnologies/tyk/pull/6642/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021R1128-R1141) ```diff -_, found = r.Gw.GlobalSessionManager.SessionDetail(orgId, key, true) -if !found { - id, err := storage.TokenID(key) - if err != nil { - log.Debugf("cannot found key with id: %v, error: %v", id, err) - } - _, found = r.Gw.GlobalSessionManager.SessionDetail(orgId, id, false) -} +found = r.checkSessionDetails(orgId, key) ``` Suggestion importance[1-10]: 7 Why: Refactoring the logic into a separate method can improve code readability and maintainability, making it easier to manage and understand the codebase.	7

buger commented 1 month ago

Let's make that PR title a 💯 shall we? 💪

<p>
Your <em>PR title</em> and <em>story title</em> look <strong>slightly different</strong>. Just checking in to know if it was intentional!
</p>
<table>
  <tr>
    <th>Story Title</th>
    <td>Api keys are lost in worker gateways when keyspace sync interrupted</td>
  </tr>
  <tr>
      <th>PR Title</th>
      <td>[TT-12417] Do not delete keys on synchronization</td>
    </tr>
</table>
<p>
  Check out this <a href="https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests">guide</a> to learn more about PR best-practices.
</p>

github-actions[bot] commented 1 month ago

API Changes

--- prev.txt    2024-10-24 12:00:40.271485679 +0000
+++ current.txt 2024-10-24 12:00:33.947525669 +0000
@@ -11837,11 +11837,15 @@
     NewSyncForcer returns a new syncforcer with a connected redis with a key
     prefix synchronizer-group- for group synchronization control.

+func (sf *SyncronizerForcer) GetIsFirstConnection() bool
+
 func (sf *SyncronizerForcer) GroupLoginCallback(userKey string, groupID string) interface{}
     GroupLoginCallback checks if the groupID key exists in the storage to turn
     on/off ForceSync param. If the the key doesn't exists in the storage,
     it creates it and set ForceSync to true

+func (sf *SyncronizerForcer) SetFirstConnection(isFirstConnection bool)
+
 # Package: ./signature_validator

 package signature_validator // import "github.com/TykTechnologies/tyk/signature_validator"