Move IP Rule fetching, caching, and refreshing logic into in interface in ip_rules_enforcer#11595
Move IP Rule fetching, caching, and refreshing logic into in interface in ip_rules_enforcer#11595iain-macdonald merged 1 commit intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the enterprise IP rules enforcer to fetch/cache parsed rules (including rule IDs) via a provider abstraction, and adds a notification-driven cache refresher that shuts down cleanly with the server lifecycle.
Changes:
- Introduces
ipRulesProvider(dbIPRulesProvider) to centralize DB loading + in-memory caching of parsed CIDRs with rule IDs. - Adds a server-notification-driven refresher loop with HealthChecker-managed shutdown.
- Adds a unit test ensuring the refresher stops on shutdown and no longer refreshes the cache afterward.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| enterprise/server/ip_rules_enforcer/ip_rules_enforcer.go | Introduces provider abstraction + notification refresher; updates cache to store ipRule (CIDR + rule ID) and updates enforcement logic accordingly. |
| enterprise/server/ip_rules_enforcer/ip_rules_enforcer_test.go | Adds a fake notification service and a shutdown behavior test for the refresher. |
| enterprise/server/ip_rules_enforcer/BUILD | Adds proto/protobuf deps needed by the new test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
iain-macdonald
force-pushed
the
im/093fd44aff1
branch
4 times, most recently
from
1bb3849 to
905f657
Compare
iain-macdonald
force-pushed
the
im/093fd44aff1
branch
from
905f657 to
9230799
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the IP rules enforcer to separate “how rules are fetched/refreshed” from “how rules are enforced”, by introducing an internal provider abstraction backed by the DB + cache. This keeps Enforcer focused on authorization logic while the provider owns caching and refresh behavior.
Changes:
- Introduced an internal
ipRulesProviderinterface to abstract IP rule retrieval and refresh. - Added
dbIPRulesProviderto encapsulate DB access + caching + notification-driven refresh. - Updated
Enforcerto depend onipRulesProviderand delegate rule loading/caching to it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| for msg := range sns.Subscribe(&snpb.InvalidateIPRulesCache{}) { | ||
| ic, ok := msg.(*snpb.InvalidateIPRulesCache) | ||
| if !ok { | ||
| alert.UnexpectedEvent("iprules_invalid_proto_type", "received proto type %T", msg) | ||
| continue | ||
| } | ||
| if err := p.refreshRules(env.GetServerContext(), ic.GetGroupId()); err != nil { | ||
| log.Warningf("could not refresh IP rules for group %q: %s", ic.GetGroupId(), err) | ||
| } |
There was a problem hiding this comment.
Gonna do this next 🙂
iain-macdonald
force-pushed
the
im/093fd44aff1
branch
from
9230799 to
d381a59
Compare
| } | ||
|
|
||
| func (p *dbIPRulesProvider) get(ctx context.Context, groupID string, skipCache bool, skipRuleID string) ([]*net.IPNet, error) { | ||
| allowed, ok := p.cache.Get(groupID) |
There was a problem hiding this comment.
if we get the result from the cache, skipRuleID is ignored. That seems weird.
There was a problem hiding this comment.
That's the way it currently works and I'd like to get rid of these parameters, so I'm gonna leave it as is. But yeah, it's confusing 😛
This change decouples the logic for fetching, caching, and refreshing IP rules from the logic that enforces those rules via an interface in ip_rules_enforcer. It's pretty mechanical, just moving some logic around without any change in functionality. I still want to do more cleanup before adding the remote version, like cleaning up the interface and correctly halting the background goroutine, but wanted to keep this PR reasonable. After cleaning up the interface a little bit more, I'll add a remote IP rules provider that fetches IP rules from the app for use in the proxy.
Related issues: https://github.com/buildbuddy-io/buildbuddy-internal/issues/6797