[TT-13041] [release-5.3] backport (#6499)

### **User description** Backport of issue on master, two conflicts resolved: 1. session.Touch doesn't exist on 5.3 (api compatibility) 2. some internal gateway function renames (initSystem) ___ ### **PR Type** Bug fix, Enhancement, Tests ___ ### **Description** - Refactored the `RedisQuotaExceeded` function to improve quota handling logic, including better management of key expiration and renewal. - Enhanced logging with additional context to aid in debugging and monitoring of quota operations. - Removed the distributed lock during quota increment and added it during quota reset to optimize performance. - Updated test cases in `middleware_test.go` to use `t.Run` for better organization and added additional test cases for comprehensive coverage. - Introduced `test.Exclusive` function to limit parallelism in tests that use `DeleteAllKeys`. - Replaced `initialiseSystem` with `initSystem` and added a global mutex for synchronization. - Enhanced CI workflow by adding steps to print CPU info and Go environment. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>4 files</summary><table> <tr> <td> <details> <summary><strong>session_manager.go</strong><dd><code>Refactor and improve quota handling logic</code>                                </dd></summary> <hr> gateway/session_manager.go <li>Refactored <code>RedisQuotaExceeded</code> function to improve quota handling.<br> <li> Enhanced logging for better debugging and monitoring.<br> <li> Removed distributed lock during quota increment and added it during <br>quota reset.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-e6b40a285464cd86736e970c4c0b320b44c75b18b363d38c200e9a9d36cdabb6">+81/-50</a>  </td> </tr> <tr> <td> <details> <summary><strong>server.go</strong><dd><code>Synchronize system initialization with global mutex</code>            </dd></summary> <hr> gateway/server.go <li>Replaced <code>initialiseSystem</code> with <code>initSystem</code>.<br> <li> Added a global mutex for synchronization.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-4652d1bf175a0be8f5e61ef7177c9666f23e077d8626b73ac9d13358fa8b525b">+6/-4</a>      </td> </tr> <tr> <td> <details> <summary><strong>testutil.go</strong><dd><code>Update test utility for system initialization</code>                        </dd></summary> <hr> gateway/testutil.go - Replaced `initialiseSystem` with `initSystem`. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-7aaf6ae49fb8f58a8c99d337fedd15b3e430dd928ed547e425ef429b10d28ce8">+1/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>util.go</strong><dd><code>Add exclusive test execution utility</code>                                          </dd></summary> <hr> test/util.go - Introduced `Exclusive` function to gate single test execution. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-042018ccb72a158622dc1cc3307e54dc21761f6ab59e5635854810127e4f48e6">+15/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>6 files</summary><table> <tr> <td> <details> <summary><strong>middleware_test.go</strong><dd><code>Refactor and extend test cases for session limiter</code>              </dd></summary> <hr> gateway/middleware_test.go <li>Added <code>test.Exclusive</code> to limit parallelism in tests.<br> <li> Refactored test cases to use <code>t.Run</code> for better organization.<br> <li> Added additional test cases for comprehensive coverage.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-6a09a08e3f82cc5e9d8c6b5c8426d75ea1e5d85e15ab008fca1f512e7c49c1e6">+27/-24</a>  </td> </tr> <tr> <td> <details> <summary><strong>api_test.go</strong><dd><code>Add exclusive test handling for API tests</code>                                </dd></summary> <hr> gateway/api_test.go <li>Added <code>test.Exclusive</code> to limit parallelism in tests.<br> <li> Added comments to <code>DeleteAllKeys</code> for clarity.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-10b4a3d7bdd8d98e48b288d27fd46d9ee436617806c46913fdf7942c0e4a992e">+11/-3</a>    </td> </tr> <tr> <td> <details> <summary><strong>gateway_test.go</strong><dd><code>Add exclusive test handling for quota tests</code>                            </dd></summary> <hr> gateway/gateway_test.go - Added `test.Exclusive` to limit parallelism in quota tests. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-d34c7069ce5e81d45082b19eb3e869ee1a086e185dcd6630e75e3ed0d368b546">+2/-0</a>      </td> </tr> <tr> <td> <details> <summary><strong>mw_external_oauth_test.go</strong><dd><code>Add exclusive test handling for OAuth middleware tests</code>      </dd></summary> <hr> gateway/mw_external_oauth_test.go <li>Added <code>test.Exclusive</code> to limit parallelism in OAuth middleware tests.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-3bde461a83fa23b503cf80349641129e4c69e6ae1afb7b628a882fada3a54566">+4/-2</a>      </td> </tr> <tr> <td> <details> <summary><strong>mw_rate_limiting_test.go</strong><dd><code>Add exclusive test handling for rate limiting tests</code>            </dd></summary> <hr> gateway/mw_rate_limiting_test.go <li>Added <code>test.Exclusive</code> to limit parallelism in rate limiting tests.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-7cf2199231924147d538ba7ad576a48a3c0e691852077e147c9b2d86ba9b7c4d">+4/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>rpc_storage_handler_test.go</strong><dd><code>Add exclusive test handling for RPC storage handler tests</code></dd></summary> <hr> gateway/rpc_storage_handler_test.go <li>Added <code>test.Exclusive</code> to limit parallelism in RPC storage handler <br>tests.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-69de989a02b3bc32ae376c514ee84633c609200db22385c0e16c361d6ea74cd6">+11/-6</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>1 files</summary><table> <tr> <td> <details> <summary><strong>ci-tests.yml</strong><dd><code>Enhance CI workflow with additional environment information</code></dd></summary> <hr> .github/workflows/ci-tests.yml - Added steps to print CPU info and Go environment in CI workflow. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6499/files#diff-03609cb60b0c6e92fb771eb8787d6722b8c31ca4c03eabc788e147acd8c6fb43">+6/-0</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Tit Petric <[email protected]> Co-authored-by: Esteban Ricardo Mirizio <[email protected]>
TykTechnologies · Sep 11, 2024 · e445be3 · e445be3
1 parent b510966
commit e445be3
Show file tree

Hide file tree

Showing 11 changed files with 168 additions and 91 deletions.
diff --git a/.github/workflows/ci-tests.yml b/.github/workflows/ci-tests.yml
@@ -108,6 +108,12 @@ jobs:
         if: ${{ github.event_name == 'pull_request' }}
         run: git fetch origin ${{ github.base_ref }}
 
+      - name: Print CPU info
+        run: grep '^model name' /proc/cpuinfo
+
+      - name: Print Go env
+        run: go env
+
       - name: Run Gateway Tests
         id: ci-tests
         run: |

diff --git a/gateway/api_test.go b/gateway/api_test.go
@@ -219,6 +219,8 @@ func TestApiHandlerPostDupPath(t *testing.T) {
 }
 
 func TestKeyHandler(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
+
 	ts := StartTest(nil)
 	defer ts.Close()
 
@@ -338,7 +340,7 @@ func TestKeyHandler(t *testing.T) {
 			},
 		}...)
 
-		ts.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+		ts.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 	})
 
 	_, knownKey := ts.CreateSession(func(s *user.SessionState) {
@@ -644,6 +646,8 @@ func BenchmarkKeyHandler_CreateKeyHandler(b *testing.B) {
 }
 
 func TestKeyHandler_DeleteKeyWithQuota(t *testing.T) {
+	test.Exclusive(t) // Uses quota, need to limit parallelism due to DeleteAllKeys.
+
 	const testAPIID = "testAPIID"
 	const orgId = "default"
 
@@ -826,6 +830,8 @@ func TestUpdateKeyWithCert(t *testing.T) {
 }
 
 func TestKeyHandler_CheckKeysNotDuplicateOnUpdate(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
+
 	ts := StartTest(nil)
 	defer ts.Close()
 
@@ -878,7 +884,7 @@ func TestKeyHandler_CheckKeysNotDuplicateOnUpdate(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Run(tc.Name, func(t *testing.T) {
-			ts.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+			ts.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 			session := CreateStandardSession()
 			session.AccessRights = map[string]user.AccessDefinition{"test": {
 				APIID: "test", Versions: []string{"v1"},
@@ -906,6 +912,8 @@ func TestKeyHandler_CheckKeysNotDuplicateOnUpdate(t *testing.T) {
 }
 
 func TestHashKeyHandler(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
+
 	conf := func(globalConf *config.Config) {
 		// make it to use hashes for Redis keys
 		globalConf.HashKeys = true
@@ -932,7 +940,7 @@ func TestHashKeyHandler(t *testing.T) {
 		gwConf := ts.Gw.GetConfig()
 		gwConf.HashKeyFunction = tc.hashFunction
 		ts.Gw.SetConfig(gwConf)
-		ok := ts.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+		ok := ts.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 		assert.True(t, ok)
 
 		t.Run(fmt.Sprintf("%sHash fn: %s", tc.desc, tc.hashFunction), func(t *testing.T) {

diff --git a/gateway/gateway_test.go b/gateway/gateway_test.go
@@ -323,6 +323,8 @@ func TestSkipTargetPassEscapingOffWithSkipURLCleaningTrue(t *testing.T) {
 }
 
 func TestQuota(t *testing.T) {
+	test.Exclusive(t) // Uses quota, need to limit parallelism due to DeleteAllKeys.
+
 	ts := StartTest(nil)
 	defer ts.Close()
 

diff --git a/gateway/middleware_test.go b/gateway/middleware_test.go
@@ -12,6 +12,7 @@ import (
 	"github.com/TykTechnologies/tyk/apidef"
 	headers2 "github.com/TykTechnologies/tyk/header"
 	"github.com/TykTechnologies/tyk/internal/cache"
+	"github.com/TykTechnologies/tyk/internal/uuid"
 	"github.com/TykTechnologies/tyk/test"
 
 	"github.com/TykTechnologies/tyk/config"
@@ -258,24 +259,20 @@ func TestBaseMiddleware_getAuthToken(t *testing.T) {
 }
 
 func TestSessionLimiter_RedisQuotaExceeded_PerAPI(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
+
 	g := StartTest(nil)
 	defer g.Close()
-	g.Gw.GlobalSessionManager.Store().DeleteAllKeys()
-	defer g.Gw.GlobalSessionManager.Store().DeleteAllKeys()
 
-	apis := BuildAPI(func(spec *APISpec) {
-		spec.APIID = "api1"
-		spec.UseKeylessAccess = false
-		spec.Proxy.ListenPath = "/api1/"
-	}, func(spec *APISpec) {
-		spec.APIID = "api2"
-		spec.UseKeylessAccess = false
-		spec.Proxy.ListenPath = "/api2/"
-	}, func(spec *APISpec) {
-		spec.APIID = "api3"
+	g.Gw.GlobalSessionManager.Store().DeleteAllKeys()       // exclusive
+	defer g.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
+
+	api := func(spec *APISpec) {
+		spec.APIID = uuid.New()
 		spec.UseKeylessAccess = false
-		spec.Proxy.ListenPath = "/api3/"
-	})
+		spec.Proxy.ListenPath = fmt.Sprintf("/%s/", spec.APIID)
+	}
+	apis := BuildAPI(api, api, api)
 
 	g.Gw.LoadAPI(apis...)
 
@@ -330,18 +327,24 @@ func TestSessionLimiter_RedisQuotaExceeded_PerAPI(t *testing.T) {
 		}
 	}
 
-	// for api1 - per api
-	sendReqAndCheckQuota(t, apis[0].APIID, 9, true)
-	sendReqAndCheckQuota(t, apis[0].APIID, 8, true)
-	sendReqAndCheckQuota(t, apis[0].APIID, 7, true)
+	t.Run("For api1 - per api", func(t *testing.T) {
+		sendReqAndCheckQuota(t, apis[0].APIID, 9, true)
+		sendReqAndCheckQuota(t, apis[0].APIID, 8, true)
+		sendReqAndCheckQuota(t, apis[0].APIID, 7, true)
+	})
 
-	// for api2 - per api
-	sendReqAndCheckQuota(t, apis[1].APIID, 1, true)
-	sendReqAndCheckQuota(t, apis[1].APIID, 0, true)
+	t.Run("For api2 - per api", func(t *testing.T) {
+		sendReqAndCheckQuota(t, apis[1].APIID, 1, true)
+		sendReqAndCheckQuota(t, apis[1].APIID, 0, true)
+	})
 
-	// for api3 - global
-	sendReqAndCheckQuota(t, apis[2].APIID, 24, false)
-	sendReqAndCheckQuota(t, apis[2].APIID, 23, false)
+	t.Run("For api3 - global", func(t *testing.T) {
+		sendReqAndCheckQuota(t, apis[2].APIID, 24, false)
+		sendReqAndCheckQuota(t, apis[2].APIID, 23, false)
+		sendReqAndCheckQuota(t, apis[2].APIID, 22, false)
+		sendReqAndCheckQuota(t, apis[2].APIID, 21, false)
+		sendReqAndCheckQuota(t, apis[2].APIID, 20, false)
+	})
 }
 
 func TestCopyAllowedURLs(t *testing.T) {

diff --git a/gateway/mw_external_oauth_test.go b/gateway/mw_external_oauth_test.go
@@ -367,6 +367,8 @@ func TestExternalOAuthMiddleware_introspection(t *testing.T) {
 	}...)
 
 	t.Run("cache", func(t *testing.T) {
+		test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
+
 		api.ExternalOAuth.Providers[0].Introspection.Cache.Enabled = true
 		api.ExternalOAuth.Providers[0].Introspection.Cache.Timeout = 0
 		ts.Gw.LoadAPI(api)
@@ -382,13 +384,13 @@ func TestExternalOAuthMiddleware_introspection(t *testing.T) {
 		}...)
 
 		// invalidate cache
-		externalOAuthIntrospectionCache.DeleteAllKeys()
+		externalOAuthIntrospectionCache.DeleteAllKeys() // exclusive
 		_, _ = ts.Run(t, []test.TestCase{
 			{Path: "/get", Headers: headers, BodyMatch: "access token is not valid", Code: http.StatusUnauthorized},
 		}...)
 
 		t.Run("expired", func(t *testing.T) {
-			externalOAuthIntrospectionCache.DeleteAllKeys()
+			externalOAuthIntrospectionCache.DeleteAllKeys() // exclusive
 
 			// normally for expired token, the introspection returns active false
 			// this is to get rid of putting delay to wait until expiration

diff --git a/gateway/mw_rate_limiting_test.go b/gateway/mw_rate_limiting_test.go
@@ -66,6 +66,7 @@ func TestRateLimit_Unlimited(t *testing.T) {
 }
 
 func TestNeverRenewQuota(t *testing.T) {
+	test.Exclusive(t) // Uses quota, need to limit parallelism due to DeleteAllKeys.
 
 	g := StartTest(nil)
 	defer g.Close()
@@ -192,6 +193,8 @@ func TestMwRateLimiting_DepthLimit(t *testing.T) {
 }
 
 func providerCustomRatelimitKey(t *testing.T, limiter string) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
+
 	t.Helper()
 
 	tcs := []struct {
@@ -246,7 +249,7 @@ func providerCustomRatelimitKey(t *testing.T, limiter string) {
 
 			ts.Gw.SetConfig(globalConf)
 
-			ok := ts.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+			ok := ts.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 			assert.True(t, ok)
 
 			customRateLimitKey := "portal-developer-1" + tc.hashAlgo + limiter

diff --git a/gateway/rpc_storage_handler_test.go b/gateway/rpc_storage_handler_test.go
@@ -59,6 +59,7 @@ func getRefreshToken(td tokenData) string {
 }
 
 func TestProcessKeySpaceChangesForOauth(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
 
 	cases := []struct {
 		TestName string
@@ -135,7 +136,7 @@ func TestProcessKeySpaceChangesForOauth(t *testing.T) {
 				}
 			} else {
 				getKeyFromStore = ts.Gw.GlobalSessionManager.Store().GetKey
-				ts.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+				ts.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 				err := ts.Gw.GlobalSessionManager.Store().SetRawKey(token, token, 100)
 				assert.NoError(t, err)
 				_, err = ts.Gw.GlobalSessionManager.Store().GetRawKey(token)
@@ -155,6 +156,7 @@ func TestProcessKeySpaceChangesForOauth(t *testing.T) {
 }
 
 func TestProcessKeySpaceChanges_ResetQuota(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
 
 	g := StartTest(nil)
 	defer g.Close()
@@ -166,8 +168,8 @@ func TestProcessKeySpaceChanges_ResetQuota(t *testing.T) {
 		Gw:               g.Gw,
 	}
 
-	g.Gw.GlobalSessionManager.Store().DeleteAllKeys()
-	defer g.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+	g.Gw.GlobalSessionManager.Store().DeleteAllKeys()       // exclusive
+	defer g.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 
 	api := g.Gw.BuildAndLoadAPI(func(spec *APISpec) {
 		spec.UseKeylessAccess = false
@@ -217,6 +219,7 @@ func TestProcessKeySpaceChanges_ResetQuota(t *testing.T) {
 
 // TestRPCUpdateKey check that on update key event the key still exist in worker redis
 func TestRPCUpdateKey(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
 
 	cases := []struct {
 		TestName     string
@@ -248,8 +251,8 @@ func TestRPCUpdateKey(t *testing.T) {
 				Gw:               g.Gw,
 			}
 
-			g.Gw.GlobalSessionManager.Store().DeleteAllKeys()
-			defer g.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+			g.Gw.GlobalSessionManager.Store().DeleteAllKeys()       // exclusive
+			defer g.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 
 			api := g.Gw.BuildAndLoadAPI(func(spec *APISpec) {
 				spec.UseKeylessAccess = false
@@ -289,6 +292,8 @@ func TestRPCUpdateKey(t *testing.T) {
 }
 
 func TestGetGroupLoginCallback(t *testing.T) {
+	test.Exclusive(t) // Uses DeleteAllKeys, need to limit parallelism.
+
 	tcs := []struct {
 		testName                 string
 		syncEnabled              bool
@@ -318,7 +323,7 @@ func TestGetGroupLoginCallback(t *testing.T) {
 				globalConf.SlaveOptions.SynchroniserEnabled = tc.syncEnabled
 			})
 			defer ts.Close()
-			defer ts.Gw.GlobalSessionManager.Store().DeleteAllKeys()
+			defer ts.Gw.GlobalSessionManager.Store().DeleteAllKeys() // exclusive
 
 			rpcListener := RPCStorageHandler{
 				KeyPrefix:        "rpc.listener.",

diff --git a/gateway/server.go b/gateway/server.go
@@ -68,6 +68,8 @@ import (
 )
 
 var (
+	globalMu sync.Mutex
+
 	log       = logger.Get()
 	mainLog   = log.WithField("prefix", "main")
 	pubSubLog = log.WithField("prefix", "pub-sub")
@@ -334,8 +336,8 @@ func (gw *Gateway) apisByIDLen() int {
 
 // Create all globals and init connection handlers
 func (gw *Gateway) setupGlobals() {
-	gw.reloadMu.Lock()
-	defer gw.reloadMu.Unlock()
+	globalMu.Lock()
+	defer globalMu.Unlock()
 
 	defaultTykErrors()
 
@@ -1211,7 +1213,7 @@ func (gw *Gateway) setupLogger() {
 	}
 }
 
-func (gw *Gateway) initialiseSystem() error {
+func (gw *Gateway) initSystem() error {
 	if gw.isRunningTests() && os.Getenv("TYK_LOGLEVEL") == "" {
 		// `go test` without TYK_LOGLEVEL set defaults to no log
 		// output
@@ -1621,7 +1623,7 @@ func Start() {
 
 	gw := NewGateway(gwConfig, ctx)
 
-	if err := gw.initialiseSystem(); err != nil {
+	if err := gw.initSystem(); err != nil {
 		mainLog.Fatalf("Error initialising system: %v", err)
 	}