Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FQDN memory leak #17432

Merged
merged 2 commits into from
Sep 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions daemon/cmd/fqdn.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ func (d *Daemon) bootstrapFQDN(possibleEndpoints map[uint16]*endpoint.Endpoint,
Cache: fqdn.NewDNSCache(option.Config.ToFQDNsMinTTL),
UpdateSelectors: d.updateSelectors,
}
// Disable cleanup tracking on the default DNS cache. This cache simply
// tracks which api.FQDNSelector are present in policy which apply to
// locally running endpoints.
cfg.Cache.DisableCleanupTrack()
aditighag marked this conversation as resolved.
Show resolved Hide resolved

rg := fqdn.NewNameManager(cfg)
d.policy.GetSelectorCache().SetLocalIdentityNotifier(rg)
Expand Down
18 changes: 9 additions & 9 deletions pkg/fqdn/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,12 @@ func NewDNSCacheWithLimit(minTTL int, limit int) *DNSCache {
return c
}

func (c *DNSCache) DisableCleanupTrack() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (c *DNSCache) DisableCleanupTrack() {
func (c *DNSCache) DisableCleanupTracking() {

c.Lock()
defer c.Unlock()
c.cleanup = nil
}

// Update inserts a new entry into the cache.
// After insertion cache entries for name are expired and redundant entries
// evicted. This is O(number of new IPs) for eviction, and O(number of IPs for
Expand Down Expand Up @@ -213,6 +219,9 @@ func (c *DNSCache) updateWithEntry(entry *cacheEntry) bool {
// delete the entry from the policy when it expires.
// Need to be called with a write lock
func (c *DNSCache) addNameToCleanup(entry *cacheEntry) {
if c.cleanup == nil {
return
}
if c.lastCleanup.IsZero() || entry.ExpirationTime.Before(c.lastCleanup) {
c.lastCleanup = entry.ExpirationTime
}
Expand Down Expand Up @@ -565,15 +574,6 @@ func (c *DNSCache) ForceExpire(expireLookupsBefore time.Time, nameMatch *regexp.
return KeepUniqueNames(namesAffected)
}

// ForceExpireByNames is the same function as ForceExpire but uses the exact
// names to delete the entries.
func (c *DNSCache) ForceExpireByNames(expireLookupsBefore time.Time, names []string) (namesAffected []string) {
c.Lock()
defer c.Unlock()

return c.forceExpireByNames(expireLookupsBefore, names)
}

func (c *DNSCache) forceExpireByNames(expireLookupsBefore time.Time, names []string) (namesAffected []string) {
for _, name := range names {
entries, exists := c.forward[name]
Expand Down
8 changes: 4 additions & 4 deletions pkg/fqdn/cache_test.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2018 Authors of Cilium
// Copyright 2018-2021 Authors of Cilium

//go:build !privileged_tests
// +build !privileged_tests
Expand Down Expand Up @@ -131,7 +131,7 @@ func (ds *DNSCacheTestSuite) TestDelete(c *C) {
c.Assert(len(dump), Equals, 0, Commentf("Returned cache entries from cache dump after the cache was fully cleared: %v", dump))
}

func (ds *DNSCacheTestSuite) TestForceExpiredByNames(c *C) {
func (ds *DNSCacheTestSuite) Test_forceExpiredByNames(c *C) {
names := []string{"test1.com", "test2.com"}
cache := NewDNSCache(0)
for i := 1; i < 4; i++ {
Expand All @@ -143,12 +143,12 @@ func (ds *DNSCacheTestSuite) TestForceExpiredByNames(c *C) {
}

c.Assert(cache.forward, HasLen, 3)
result := cache.ForceExpireByNames(time.Now(), names)
result := cache.forceExpireByNames(time.Now(), names)
c.Assert(result, checker.DeepEquals, names)
c.Assert(result, HasLen, 2)
c.Assert(cache.forward["test3.com"], Not(IsNil))

invalidName := cache.ForceExpireByNames(now, []string{"invalid.name"})
invalidName := cache.forceExpireByNames(now, []string{"invalid.name"})
c.Assert(invalidName, HasLen, 0)
}

Expand Down