Merging to release-5.3: [TT-12417] Do not delete keys on synchronization (#6642)

buger commented 1 month ago

User description

TT-12417 Do not delete keys on synchronization (#6642)

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

PR Type

Bug fix, Enhancement

Description

Introduced a singleton pattern for the SyncForcer to ensure a single instance is used throughout the application.
Added methods SetFirstConnection and GetIsFirstConnection to manage the initial connection state.
Enhanced error handling in RPCStorageHandler by initializing SyncForcer on unexpected errors.
Modified synchronization logic to avoid key deletion during synchronization, addressing the issue of lost API keys when keyspace sync is interrupted.

Changes walkthrough 📝

Relevant files

Enhancement

rpc_storage_handler.go `Initialize SyncForcer on RPC reload check errors` gateway/rpc_storage_handler.go Added a new `SyncForcer` instance creation with `SetFirstConnection` set to true. Improved error handling by initializing the `SyncForcer` on unexpected errors.	+2/-0
synchronization_forcer.go `Implement singleton pattern and enhance SyncForcer logic` rpc/synchronization_forcer.go Implemented singleton pattern for `SyncForcer` instance creation. Added `SetFirstConnection` and `GetIsFirstConnection` methods. Modified `GroupLoginCallback` to use `isFirstConnection` for force sync logic.	+33/-8

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

github-actions[bot] commented 1 month ago

API Changes

--- prev.txt    2024-10-24 14:55:14.112053711 +0000
+++ current.txt 2024-10-24 14:55:11.080028306 +0000
@@ -10802,11 +10802,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"

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis ✅** **[6642](https://github.com/TykTechnologies/tyk/issues/6642) - Fully compliant** Fully compliant requirements: - Ensure that API keys are not deleted during synchronization interruptions. - Maintain consistency of key data between master and slave Redis instances.

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Error Handling
The error handling logic might cause recursive calls without proper exit conditions, potentially leading to stack overflow. Singleton Implementation
The singleton pattern implementation for SyncronizerForcer might not be thread-safe due to the check outside the synchronized block.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Best practice	Handle errors from the store connection attempt to ensure robustness ___ Consider handling the error returned by `sf.store.Connect()` to ensure that the connection is established successfully before proceeding. [rpc/synchronization_forcer.go [27]](https://github.com/TykTechnologies/tyk/pull/6667/files#diff-97417011065a292f63eeb6fb031afbcfffa75cb3fc7073f8431add277b250c98R27-R27) ```diff -sf.store.Connect() +if err := sf.store.Connect(); err != nil { + return nil, err +} ``` Suggestion importance[1-10]: 9 Why: Handling the error from `sf.store.Connect()` is crucial for ensuring the connection is established successfully. This suggestion improves robustness by preventing further operations if the connection fails.	9
Possible bug	Prevent potential infinite recursion in the `CheckForReload` method ___ Avoid potential infinite recursion by implementing a maximum recursion depth or a different mechanism to prevent `CheckForReload` from calling itself indefinitely. [gateway/rpc_storage_handler.go [797-798]](https://github.com/TykTechnologies/tyk/pull/6667/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021R797-R798) ```diff -if rpc.Login() { +if rpc.Login() && recursionDepth < maxRecursionDepth { r.CheckForReload(orgId) } ``` Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential infinite recursion issue by proposing a mechanism to limit recursion depth, which is important for preventing stack overflow errors.	8
Performance	Implement a backoff strategy to optimize resource usage during retries ___ Consider adding a backoff strategy for the retry mechanism in `CheckForReload` to avoid potential rapid resource exhaustion. [gateway/rpc_storage_handler.go [806]](https://github.com/TykTechnologies/tyk/pull/6667/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021R806-R806) ```diff -time.Sleep(1 * time.Second) +time.Sleep(backoffDuration) ``` Suggestion importance[1-10]: 7 Why: Introducing a backoff strategy can help prevent rapid resource exhaustion during retries, enhancing the performance and stability of the retry mechanism.	7