Skip to content

Commit 927cc49

Browse files
probelabs[bot]Tyk Botburaksezer
authored
Merging to release-5.11: [TT-16468] Using a JWKS URL causes memory leak in gateway 5.11 (#7703) (#7707)
Cherry-pick of `60367d7374d7c3fe1d2eb0d65f1439be77a42533` from `master` to `release-5.11` requires manual resolution. **Conflicts detected:** 3 - gateway/mw_jwt_test.go Tips: - Check out this branch locally and run: `git cherry-pick -x 60367d7` - Resolve conflicts (including submodules if any), then push back to this branch. Original commit: 60367d7 --------- Co-authored-by: Tyk Bot <bot@tyk.io> Co-authored-by: Burak Sezer <burak.sezer.developer@gmail.com>
1 parent d64d8aa commit 927cc49

File tree

5 files changed

+102
-20
lines changed

5 files changed

+102
-20
lines changed

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,6 @@ To write your own test please use this guide [https://github.com/TykTechnologies
193193

194194
## Contributing
195195

196-
For more information about contributing PRs and issues, see [CONTRIBUTING.md](https://github.com/TykTechnologies/tyk/blob/master/CONTRIBUTING.md).
196+
For more information about contributing PRs and issues, see [CONTRIBUTING.md](https://github.com/TykTechnologies/tyk/blob/master/CONTRIBUTING.md)
197197

198198

gateway/mw_jwt.go

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ func (k *JWTMiddleware) Init() {
7676
go func() {
7777
k.Logger().Debug("Pre-fetching JWKs asynchronously")
7878
// Drop the previous cache for the API ID
79-
JWKCaches.Delete(k.Spec.APIID)
79+
deleteJWKCacheByAPIID(k.Spec.APIID)
80+
8081
jwkCache := loadOrCreateJWKCacheByApiID(k.Spec.APIID)
8182

8283
// Create client factory for JWK fetching
@@ -114,8 +115,16 @@ func (k *JWTMiddleware) Unload() {
114115
// Init tries to clean the cache for the API ID when it starts
115116
spec := k.Gw.getApiSpec(k.Spec.APIID)
116117
if spec == nil {
117-
// Drop the cache for API ID if it's removed from Tyk
118-
JWKCaches.Delete(k.Spec.APIID)
118+
// delete the cache from the global map and stop its janitor.
119+
deleteJWKCacheByAPIID(k.Spec.APIID)
120+
}
121+
}
122+
123+
func deleteJWKCacheByAPIID(apiID string) {
124+
if existing, ok := JWKCaches.LoadAndDelete(apiID); ok {
125+
if repo, ok := existing.(cache.Repository); ok {
126+
repo.Close()
127+
}
119128
}
120129
}
121130

@@ -128,10 +137,22 @@ func (k *JWTMiddleware) loadOrCreateJWKCache() cache.Repository {
128137
}
129138

130139
func loadOrCreateJWKCacheByApiID(apiID string) cache.Repository {
131-
raw, _ := JWKCaches.LoadOrStore(apiID, cache.New(240, 30))
140+
if raw, ok := JWKCaches.Load(apiID); ok {
141+
if jwkCache, ok := raw.(cache.Repository); ok {
142+
return jwkCache
143+
}
144+
}
145+
146+
newCache := cache.New(240, 30)
147+
raw, loaded := JWKCaches.LoadOrStore(apiID, newCache)
148+
149+
// If another goroutine won the race, close the unused cache
150+
if loaded {
151+
newCache.Close()
152+
}
153+
132154
jwkCache, ok := raw.(cache.Repository)
133155
if !ok {
134-
// This should not be possible
135156
panic("JWKCache instance must implement cache.Repository")
136157
}
137158
return jwkCache
@@ -1594,15 +1615,8 @@ func getUserIDFromClaim(claims jwt.MapClaims, identityBaseField string, shouldFa
15941615
}
15951616

15961617
func invalidateJWKSCacheByAPIID(apiID string) {
1597-
raw, ok := JWKCaches.Load(apiID)
1598-
if ok {
1599-
jwkCache, interfaceOK := raw.(cache.Repository)
1600-
if !interfaceOK {
1601-
panic("JWKCache must implement cache.Repository")
1602-
}
1603-
jwkCache.Flush()
1604-
mainLog.Debugf("JWKS cache for API: %s has been flushed", apiID)
1605-
}
1618+
deleteJWKCacheByAPIID(apiID)
1619+
mainLog.Debugf("JWKS cache for API: %s has been invalidated", apiID)
16061620
}
16071621

16081622
func (gw *Gateway) invalidateJWKSCacheForAPIID(w http.ResponseWriter, r *http.Request) {
@@ -1614,7 +1628,13 @@ func (gw *Gateway) invalidateJWKSCacheForAPIID(w http.ResponseWriter, r *http.Re
16141628
}
16151629

16161630
func (gw *Gateway) invalidateJWKSCacheForAllAPIs(w http.ResponseWriter, _ *http.Request) {
1617-
JWKCaches.Clear()
1631+
JWKCaches.Range(func(key, _ any) bool {
1632+
apiID, ok := key.(string)
1633+
if ok {
1634+
deleteJWKCacheByAPIID(apiID)
1635+
}
1636+
return true
1637+
})
16181638

16191639
doJSONWrite(w, http.StatusOK, apiOk("cache invalidated"))
16201640
}

gateway/mw_jwt_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5022,3 +5022,23 @@ func TestJWT_TraditionalAuth_ExistingSessionNoPolicyInToken(t *testing.T) {
50225022
})
50235023
})
50245024
}
5025+
5026+
func TestDeleteJWKCacheByAPIID(t *testing.T) {
5027+
apiID := "test-api-" + uuid.NewHex()
5028+
5029+
// Create and populate a cache
5030+
jwkCache := loadOrCreateJWKCacheByApiID(apiID)
5031+
jwkCache.Set("test-key", "test-value", 0)
5032+
5033+
// Verify the cache has items before deletion
5034+
assert.Equal(t, 1, jwkCache.Count())
5035+
5036+
deleteJWKCacheByAPIID(apiID)
5037+
5038+
// Verify cache is removed from the JWKCaches map
5039+
_, exists := JWKCaches.Load(apiID)
5040+
assert.False(t, exists, "cache should be removed from JWKCaches")
5041+
5042+
// Verify cache contents are flushed (Close calls Flush)
5043+
assert.Equal(t, 0, jwkCache.Count(), "cache items should be flushed after Close()")
5044+
}

internal/cache/janitor.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,22 @@
11
package cache
22

33
import (
4+
"sync"
45
"time"
56
)
67

78
// Janitor is responsible for performing periodic cleanup operations.
89
type Janitor struct {
910
Interval time.Duration
10-
stop chan bool
11+
stop chan struct{}
12+
once sync.Once
1113
}
1214

1315
// NewJanitor returns a new Janitor that performs cleanup at the specified interval.
1416
func NewJanitor(interval time.Duration, cleanup func()) *Janitor {
1517
janitor := &Janitor{
1618
Interval: interval,
17-
stop: make(chan bool, 1),
19+
stop: make(chan struct{}),
1820
}
1921

2022
go janitor.Run(cleanup)
@@ -32,13 +34,15 @@ func (j *Janitor) Run(cleanup func()) {
3234
case <-ticker.C:
3335
cleanup()
3436
case <-j.stop:
35-
close(j.stop)
3637
return
3738
}
3839
}
3940
}
4041

4142
// Close stops the janitor from performing further cleanup operations.
43+
// It is safe to call Close multiple times.
4244
func (j *Janitor) Close() {
43-
j.stop <- true
45+
j.once.Do(func() {
46+
close(j.stop)
47+
})
4448
}

internal/cache/janitor_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package cache
22

33
import (
4+
"sync"
45
"sync/atomic"
56
"testing"
67
"time"
@@ -31,3 +32,40 @@ func TestJanitor(t *testing.T) {
3132

3233
assert.NotEqual(t, int32(0), finalCount, "Expected cleanup() called")
3334
}
35+
36+
func TestJanitor_MultipleClose(t *testing.T) {
37+
cleanup := func() {}
38+
39+
t.Run("Multiple sequential calls to Close should not panic or block.", func(t *testing.T) {
40+
janitor := NewJanitor(10*time.Millisecond, cleanup)
41+
42+
janitor.Close()
43+
janitor.Close()
44+
janitor.Close()
45+
})
46+
47+
t.Run("Multiple concurrent calls to Close should not panic or block", func(t *testing.T) {
48+
janitor := NewJanitor(10*time.Millisecond, cleanup)
49+
50+
var wg sync.WaitGroup
51+
for i := 0; i < 10; i++ {
52+
wg.Add(1)
53+
go func() {
54+
defer wg.Done()
55+
janitor.Close()
56+
}()
57+
}
58+
59+
done := make(chan struct{})
60+
go func() {
61+
wg.Wait()
62+
close(done)
63+
}()
64+
65+
select {
66+
case <-done:
67+
case <-time.After(time.Second):
68+
assert.Fail(t, "Close() blocked or deadlocked")
69+
}
70+
})
71+
}

0 commit comments

Comments
 (0)