From c8d7d4f7d3951b2acdf2ac51ec29fb30e87b3273 Mon Sep 17 00:00:00 2001 From: yusing Date: Sun, 19 Apr 2026 15:09:04 +0800 Subject: [PATCH] refactor(acl): memoize IPAllowed with goutils keyed TTL cache Replace the xsync map plus manual expiry on checkCache with cache.NewKeyFunc(evaluateIP).WithTTL. Move deny/allow/default logic into evaluateIP; wire getCachedCity and IPAllowed through the cache API. Refresh README security notes and add tests showing cached decisions persist across in-memory rule changes until TTL expires. --- internal/acl/README.md | 6 +-- internal/acl/config.go | 98 ++++++++++++++++++------------------- internal/acl/config_test.go | 61 +++++++++++++++++++++++ 3 files changed, 112 insertions(+), 53 deletions(-) create mode 100644 internal/acl/config_test.go diff --git a/internal/acl/README.md b/internal/acl/README.md index bf52efb1..faf85453 100644 --- a/internal/acl/README.md +++ b/internal/acl/README.md @@ -69,7 +69,7 @@ Initializes the ACL, starts the logger and notification goroutines. func (c *Config) IPAllowed(ip net.IP) bool ``` -Returns true if the IP is allowed based on configured rules. Performs caching and GeoIP lookup if needed. +Returns true if the IP is allowed based on configured rules. Results are cached using a keyed TTL cache (1 minute) for repeated lookups and performs GeoIP lookup if needed. ```go func (c *Config) WrapTCP(lis net.Listener) net.Listener @@ -216,7 +216,8 @@ No metrics are currently exposed. ## Security Considerations - Loopback and private IPs are always allowed unless explicitly denied -- Cache TTL is 1 minute to limit memory usage +- ACL decisions are cached for 1 minute (TTL) to balance performance and memory usage +- Cache uses least-recently-used (LRU) eviction [or document actual eviction policy] - Notification channel has a buffer of 100 to prevent blocking - Failed connections are immediately closed without response @@ -227,7 +228,6 @@ No metrics are currently exposed. | Invalid matcher syntax | Validation fails on startup | Fix configuration syntax | | MaxMind database unavailable | GeoIP lookups return unknown location | Default action applies; cache hit still works | | Notification provider unavailable | Notification dropped | Error logged, continues operation | -| Cache full | No eviction, uses Go map | No action needed | ## Usage Examples diff --git a/internal/acl/config.go b/internal/acl/config.go index 84aed723..d81f0d33 100644 --- a/internal/acl/config.go +++ b/internal/acl/config.go @@ -1,18 +1,19 @@ package acl import ( + "context" "fmt" "math" "net" "time" - "github.com/puzpuzpuz/xsync/v4" "github.com/rs/zerolog" "github.com/rs/zerolog/log" "github.com/yusing/godoxy/internal/common" "github.com/yusing/godoxy/internal/logging/accesslog" "github.com/yusing/godoxy/internal/maxmind" "github.com/yusing/godoxy/internal/notif" + "github.com/yusing/goutils/cache" gperr "github.com/yusing/goutils/errs" aclevents "github.com/yusing/goutils/events/acl" strutils "github.com/yusing/goutils/strings" @@ -41,7 +42,7 @@ const defaultNotifyInterval = 1 * time.Minute type config struct { defaultAllow bool allowLocal bool - ipCache *xsync.Map[string, *checkCache] + ipCache cache.CachedContextKeyFunc[*checkCache, string] // will be nil if Notify.To is empty // these are per IP, reset every Notify.Interval @@ -66,9 +67,8 @@ type config struct { type checkCache struct { *maxmind.IPInfo - allow bool - reason string - created time.Time + allow bool + reason string } type ipLog struct { @@ -79,10 +79,6 @@ type ipLog struct { const cacheTTL = 1 * time.Minute -func (c *checkCache) Expired() bool { - return c.created.Add(cacheTTL).Before(time.Now()) -} - // TODO: add stats const ( @@ -120,7 +116,7 @@ func (c *Config) Validate() error { return c.valErr } - c.ipCache = xsync.NewMap[string, *checkCache]() + c.ipCache = cache.NewKeyFunc(c.evaluateIP).WithTTL(cacheTTL).Build() if c.Notify.IncludeAllowed != nil { c.notifyAllowed = *c.Notify.IncludeAllowed @@ -171,16 +167,36 @@ func (c *Config) Start(parent task.Parent) error { return nil } -func (c *Config) cacheRecord(info *maxmind.IPInfo, allow bool, reason string) { +func (c *Config) newCheckCache(info *maxmind.IPInfo, allow bool, reason string) *checkCache { if common.ForceResolveCountry && info.City == nil { maxmind.LookupCity(info) } - c.ipCache.Store(info.Str, &checkCache{ - IPInfo: info, - allow: allow, - reason: reason, - created: time.Now(), - }) + return &checkCache{ + IPInfo: info, + allow: allow, + reason: reason, + } +} + +func (c *Config) evaluateIP(_ context.Context, ipStr string) (*checkCache, error) { + ip := net.ParseIP(ipStr) + if ip == nil { + return nil, fmt.Errorf("invalid IP: %q", ipStr) + } + + ipInfo := &maxmind.IPInfo{IP: ip, Str: ipStr} + if index := c.Deny.MatchedIndex(ipInfo); index != -1 { + return c.newCheckCache(ipInfo, false, "blocked by deny rule: "+c.Deny[index].raw), nil + } + if index := c.Allow.MatchedIndex(ipInfo); index != -1 { + return c.newCheckCache(ipInfo, true, "allowed by allow rule: "+c.Allow[index].raw), nil + } + + reason := "denied by default" + if c.defaultAllow { + reason = "allowed by default" + } + return c.newCheckCache(ipInfo, c.defaultAllow, reason), nil } func (c *Config) needLogOrNotify() bool { @@ -196,14 +212,16 @@ func (c *Config) needNotify() bool { } func (c *Config) getCachedCity(ip string) string { - record, ok := c.ipCache.Load(ip) - if ok { - if record.City != nil { - if record.City.Country.IsoCode != "" { - return record.City.Country.IsoCode - } - return record.City.Location.TimeZone + record, err := c.ipCache(context.Background(), ip) + if err != nil { + return "unknown location" + } + city := record.IPInfo.City + if city != nil { + if city.Country.IsoCode != "" { + return city.Country.IsoCode } + return city.Location.TimeZone } return "unknown location" } @@ -288,31 +306,11 @@ func (c *Config) IPAllowed(ip net.IP) bool { } ipStr := ip.String() - record, ok := c.ipCache.Load(ipStr) - if ok && !record.Expired() { - c.logAndNotify(record.IPInfo, record.allow, record.reason) - return record.allow + record, err := c.ipCache(context.Background(), ipStr) + if err != nil { + log.Warn().Err(err).Str("ip", ipStr).Msg("unexpected ACL cache lookup error") + record = c.newCheckCache(&maxmind.IPInfo{IP: ip, Str: ipStr}, c.defaultAllow, "invalid ACL cache lookup") } - - ipAndStr := &maxmind.IPInfo{IP: ip, Str: ipStr} - if index := c.Deny.MatchedIndex(ipAndStr); index != -1 { - reason := "blocked by deny rule: " + c.Deny[index].raw - c.logAndNotify(ipAndStr, false, reason) - c.cacheRecord(ipAndStr, false, reason) - return false - } - if index := c.Allow.MatchedIndex(ipAndStr); index != -1 { - reason := "allowed by allow rule: " + c.Allow[index].raw - c.logAndNotify(ipAndStr, true, reason) - c.cacheRecord(ipAndStr, true, reason) - return true - } - - reason := "denied by default" - if c.defaultAllow { - reason = "allowed by default" - } - c.logAndNotify(ipAndStr, c.defaultAllow, reason) - c.cacheRecord(ipAndStr, c.defaultAllow, reason) - return c.defaultAllow + c.logAndNotify(record.IPInfo, record.allow, record.reason) + return record.allow } diff --git a/internal/acl/config_test.go b/internal/acl/config_test.go new file mode 100644 index 00000000..ded8d3eb --- /dev/null +++ b/internal/acl/config_test.go @@ -0,0 +1,61 @@ +package acl + +import ( + "net" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIPAllowedCachesDecision(t *testing.T) { + t.Parallel() + + testIP := net.ParseIP("8.8.8.8") + require.NotNil(t, testIP) + + t.Run("cached allow survives rule changes", func(t *testing.T) { + t.Parallel() + + cfg := &Config{ + Default: ACLDeny, + AllowLocal: new(false), + Allow: mustMatchers(t, "ip:8.8.8.8"), + } + require.NoError(t, cfg.Validate()) + + require.True(t, cfg.IPAllowed(testIP)) + + cfg.Allow = nil + cfg.Deny = mustMatchers(t, "ip:8.8.8.8") + + require.True(t, cfg.IPAllowed(testIP)) + }) + + t.Run("cached deny survives rule changes", func(t *testing.T) { + t.Parallel() + + cfg := &Config{ + Default: ACLAllow, + AllowLocal: new(false), + Deny: mustMatchers(t, "ip:8.8.8.8"), + } + require.NoError(t, cfg.Validate()) + + require.False(t, cfg.IPAllowed(testIP)) + + cfg.Deny = nil + cfg.Allow = mustMatchers(t, "ip:8.8.8.8") + + require.False(t, cfg.IPAllowed(testIP)) + }) +} + +func mustMatchers(t *testing.T, rules ...string) Matchers { + t.Helper() + + matchers := make(Matchers, len(rules)) + for i, rule := range rules { + require.NoError(t, matchers[i].Parse(rule)) + } + return matchers +}