[TT-12417] Do not delete keys on synchronization

[mativm02]

TT-12417

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.

Related Issue

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

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]

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]

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] -_, 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] -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] -_, 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]

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

Your PR title and story title look slightly different. Just checking in to know if it was intentional!

Story Title	Api keys are lost in worker gateways when keyspace sync interrupted
PR Title	[TT-12417] Do not delete keys on synchronization

Check out this guide to learn more about PR best-practices.

[github-actions]

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"

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[mativm02]

/release to release-5.3

[tykbot]

Working on it! Note that it can take a few minutes.

[tykbot]

@mativm02 Succesfully merged PR