TT-6968 GroupLogin synchronization force #4605

tbuchaillot · 2022-12-30T20:10:04Z

[changelog]
fixed: force synchronization for the edge group when slave_options.synchroniser_enabled is set to true.

Description

This PR adds a new groupLogin callback function for RPC connections when slave_options.synchroniser_enabled=true. This changes the groupLogin function, adding an extra param to force the group synchronization in MDCB.
It also adds a sync group key in Redis for the group to avoid multiple synchronizations force.

This is complementary to the work done in https://github.com/TykTechnologies/tyk-sink/pull/307

Related Issue

https://tyktech.atlassian.net/browse/TT-6968

Motivation and Context

https://tyktech.atlassian.net/browse/TT-6968

How This Has Been Tested

Added unit tests

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

Tyk-ITS · 2022-12-30T20:55:45Z

API tests result: success ✅
Branch used: refs/pull/4605/merge
Commit: ea0dcca
Triggered by: pull_request (@tbuchaillot)
Execution page

Tyk-ITS · 2022-12-30T21:52:07Z

API tests result: success ✅
Branch used: refs/pull/4605/merge
Commit: c197a65
Triggered by: pull_request (@tbuchaillot)
Execution page

Tyk-ITS · 2022-12-30T22:09:38Z

API tests result: success ✅
Branch used: refs/pull/4605/merge
Commit: 680de0c
Triggered by: pull_request (@tbuchaillot)
Execution page

sredxny

Added some comments, requested some information. Also, lets manually test with older versions od MDCB to check compatibility

rpc/synchronization_forcer.go

sredxny · 2023-01-02T13:51:20Z

rpc/synchronization_forcer.go

+func NewSyncForcer(redisController *storage.RedisController) *SyncronizerForcer {
+	sf := &SyncronizerForcer{}
+
+	sf.store = &storage.RedisCluster{KeyPrefix: "synchronizer-group-", RedisController: redisController}
+	sf.store.Connect()
+
+	return sf
+}


Could you explain why do we need a separate object and functions to force a synchronization? is not enough only sending the param to MDCB?

@sredxny the force param is going to change depending if the key exists or not in the Redis.
The RPC Connect function receives a function getGroupLoginFunc so I thought having a separated structure with its own groupLoginCallback it's the cleanest way to do it, without modifying the current behavior since this new callback func is only going to work when the synchronizer is enabled (slave_options.synchroniser_enabled).

Does it makes sense?

mativm02

lgtm!

sonarcloud · 2023-01-03T13:52:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

92.0% Coverage
0.0% Duplication

Tyk-ITS · 2023-01-03T13:56:02Z

API tests result: success ✅
Branch used: refs/pull/4605/merge
Commit: c374351
Triggered by: pull_request (@tbuchaillot)
Execution page

Tyk-ITS · 2023-01-03T13:59:05Z

API tests result: success ✅
Branch used: refs/pull/4605/merge
Commit: 08479fd
Triggered by: pull_request (@tbuchaillot)
Execution page

tbuchaillot · 2023-01-03T16:21:59Z

Added some comments, requested some information. Also, lets manually test with older versions od MDCB to check compatibility

I manually tested it with MDCB v2.0.3 and everything was working as expected.

sredxny

lgtm

tbuchaillot · 2023-01-03T19:33:35Z

/release to release-4

tykbot · 2023-01-03T19:33:51Z

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

[changelog] fixed: force synchronization for the edge group when `slave_options.synchroniser_enabled` is set to true. (cherry picked from commit 178511b)

tykbot · 2023-01-03T19:34:11Z

@tbuchaillot Succesfully merged PR

tbuchaillot · 2023-01-03T19:41:56Z

/release to release-4.3

tykbot · 2023-01-03T19:42:10Z

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

[changelog] fixed: force synchronization for the edge group when `slave_options.synchroniser_enabled` is set to true. (cherry picked from commit 178511b)

tykbot · 2023-01-03T19:42:32Z

@tbuchaillot Succesfully merged PR

…) (#4610) TT-6968 GroupLogin synchronization force (#4605) [changelog] fixed: force synchronization for the edge group when `slave_options.synchroniser_enabled` is set to true. Co-authored-by: Tomas Buchaillot <tombuchaillot89@gmail.com>

#4609) TT-6968 GroupLogin synchronization force (#4605) [changelog] fixed: force synchronization for the edge group when `slave_options.synchroniser_enabled` is set to true. Co-authored-by: Tomas Buchaillot <tombuchaillot89@gmail.com>

tbuchaillot added 4 commits December 29, 2022 15:55

adding ForceSync into GroupLoginRequest

85b44ba

adding synchronization forcer

75fb104

Merge branch 'master' into bugfix/TT-6968

52f3b3f

adding rpc_storage_handler tests + goimports

ea0dcca

linting

c197a65

tbuchaillot changed the title ~~TT-6968~~ TT-6968 GroupLogin synchronization force Dec 30, 2022

TykTechnologies deleted a comment from github-actions bot Dec 30, 2022

TykTechnologies deleted a comment from Tyk-ITS Dec 30, 2022

TykTechnologies deleted a comment from github-actions bot Dec 30, 2022

fixing tests linting

680de0c

TykTechnologies deleted a comment from sonarcloud bot Dec 30, 2022

tbuchaillot requested review from sredxny and mativm02 December 30, 2022 22:05

tbuchaillot enabled auto-merge (squash) December 30, 2022 22:08

tbuchaillot disabled auto-merge December 30, 2022 22:08

tbuchaillot enabled auto-merge (squash) December 30, 2022 22:08

sredxny requested changes Jan 2, 2023

View reviewed changes

mativm02 approved these changes Jan 2, 2023

View reviewed changes

tbuchaillot added 2 commits January 3, 2023 14:37

fixing typo + adding comments

c374351

Merge branch 'master' into bugfix/TT-6968

08479fd

tbuchaillot requested a review from sredxny January 3, 2023 13:38

sredxny approved these changes Jan 3, 2023

View reviewed changes

tbuchaillot merged commit 178511b into master Jan 3, 2023

tbuchaillot deleted the bugfix/TT-6968 branch January 3, 2023 19:08

buger mentioned this pull request Jan 3, 2023

Merging to release-4: TT-6968 GroupLogin synchronization force (#4605) #4609

Merged

tykbot bot pushed a commit that referenced this pull request Jan 3, 2023

TT-6968 GroupLogin synchronization force (#4605)

4c0aa56

[changelog] fixed: force synchronization for the edge group when `slave_options.synchroniser_enabled` is set to true. (cherry picked from commit 178511b)

buger mentioned this pull request Jan 3, 2023

Merging to release-4.3: TT-6968 GroupLogin synchronization force (#4605) #4610

Merged

tykbot bot pushed a commit that referenced this pull request Jan 3, 2023

TT-6968 GroupLogin synchronization force (#4605)

d8bf996

[changelog] fixed: force synchronization for the edge group when `slave_options.synchroniser_enabled` is set to true. (cherry picked from commit 178511b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TT-6968 GroupLogin synchronization force #4605

TT-6968 GroupLogin synchronization force #4605

tbuchaillot commented Dec 30, 2022 •

edited

Tyk-ITS commented Dec 30, 2022

Tyk-ITS commented Dec 30, 2022

Tyk-ITS commented Dec 30, 2022

sredxny left a comment

sredxny Jan 2, 2023

tbuchaillot Jan 3, 2023

mativm02 left a comment

sonarcloud bot commented Jan 3, 2023

Tyk-ITS commented Jan 3, 2023

Tyk-ITS commented Jan 3, 2023

tbuchaillot commented Jan 3, 2023

sredxny left a comment

tbuchaillot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

tbuchaillot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

TT-6968 GroupLogin synchronization force #4605

TT-6968 GroupLogin synchronization force #4605

Conversation

tbuchaillot commented Dec 30, 2022 • edited

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

Tyk-ITS commented Dec 30, 2022

Tyk-ITS commented Dec 30, 2022

Tyk-ITS commented Dec 30, 2022

sredxny left a comment

Choose a reason for hiding this comment

sredxny Jan 2, 2023

Choose a reason for hiding this comment

tbuchaillot Jan 3, 2023

Choose a reason for hiding this comment

mativm02 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 3, 2023

Tyk-ITS commented Jan 3, 2023

Tyk-ITS commented Jan 3, 2023

tbuchaillot commented Jan 3, 2023

sredxny left a comment

Choose a reason for hiding this comment

tbuchaillot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

tbuchaillot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

tykbot bot commented Jan 3, 2023

tbuchaillot commented Dec 30, 2022 •

edited