Skip to content

Remove ipRulesProvider.get's skipRule parameter #11610

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.

Sign up for GitHub

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

Merged
iain-macdonald merged 2 commits into from
Mar 18, 2026
+31 −27
Merged

Remove ipRulesProvider.get's skipRule parameter #11610

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be24d7a
Get rid of ipRulesProvider.get's skipRule parameter
iain-macdonald Mar 18, 2026
e6e11c7
Potential fix for pull request finding
iain-macdonald Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 31 additions & 27 deletions enterprise/server/ip_rules_enforcer/ip_rules_enforcer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,43 +36,47 @@ const (
cacheSize = 100_000
)

type ipRule struct {
id string
allowed *net.IPNet
}

type ipRuleCache interface {
Copy link
Contributor

@vanja-p vanja-p Mar 18, 2026

Choose a reason for hiding this comment

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

This type has some weird stuff going on with exports. It's not exported, but it's method are, but the method operate on the unexported ipRule type.

Copy link
Contributor Author

@iain-macdonald iain-macdonald Mar 18, 2026

Choose a reason for hiding this comment

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

I'd like to just use an lru.LRU as an implementation, so the methods need to be public, unless I'm missing something.

Add(groupID string, allowed []*net.IPNet) bool
Add(groupID string, allowed []ipRule) bool
Remove(groupID string) bool
Get(groupID string) ([]*net.IPNet, bool)
Get(groupID string) ([]ipRule, bool)
}

type noopIpRuleCache struct {
}

func (c *noopIpRuleCache) Add(groupID string, allowed []*net.IPNet) bool {
func (c *noopIpRuleCache) Add(groupID string, allowed []ipRule) bool {
return false
}

func (c *noopIpRuleCache) Remove(groupID string) bool {
return false
}

func (c *noopIpRuleCache) Get(groupID string) ([]*net.IPNet, bool) {
func (c *noopIpRuleCache) Get(groupID string) ([]ipRule, bool) {
return nil, false
}

func newIpRuleCache() (ipRuleCache, error) {
if *cacheTTL == 0 {
return &noopIpRuleCache{}, nil
}
return lru.New(&lru.Config[[]*net.IPNet]{
return lru.New(&lru.Config[[]ipRule]{
TTL: *cacheTTL,
MaxSize: cacheSize,
SizeFn: func(v []*net.IPNet) int64 { return int64(len(v)) },
SizeFn: func(v []ipRule) int64 { return int64(len(v)) },
ThreadSafe: true,
})
}

// An abstraction for retrieving IP rules from a source of truth.
type ipRulesProvider interface {
// TODO(iain): get rid of skipRuleID.
get(ctx context.Context, groupID string, skipRuleID string) ([]*net.IPNet, error)
get(ctx context.Context, groupID string) ([]ipRule, error)
invalidate(ctx context.Context, groupID string)
startRefresher(env environment.Env) error
}
Expand Down Expand Up @@ -104,29 +108,29 @@ func (p *dbIPRulesProvider) loadRulesFromDB(ctx context.Context, groupID string)
return rules, nil
}

func (p *dbIPRulesProvider) loadParsedRulesFromDB(ctx context.Context, groupID string, skipRuleID string) ([]*net.IPNet, error) {
func (p *dbIPRulesProvider) loadParsedRulesFromDB(ctx context.Context, groupID string) ([]ipRule, error) {
rs, err := p.loadRulesFromDB(ctx, groupID)
if err != nil {
return nil, err
}

var allowed []*net.IPNet
var allowed []ipRule
for _, r := range rs {
if r.IPRuleID == skipRuleID {
continue
}
_, ipNet, err := net.ParseCIDR(r.CIDR)
if err != nil {
alert.UnexpectedEvent("unparsable CIDR rule", "rule %q", r.CIDR)
continue
}
allowed = append(allowed, ipNet)
allowed = append(allowed, ipRule{
id: r.IPRuleID,
allowed: ipNet,
})
}
return allowed, nil
}

func (p *dbIPRulesProvider) refreshRules(ctx context.Context, groupID string) error {
pr, err := p.loadParsedRulesFromDB(ctx, groupID, "" /*=skipRuleId*/)
pr, err := p.loadParsedRulesFromDB(ctx, groupID)
if err != nil {
return err
}
Expand All @@ -135,17 +139,14 @@ func (p *dbIPRulesProvider) refreshRules(ctx context.Context, groupID string) er
return nil
}

func (p *dbIPRulesProvider) get(ctx context.Context, groupID string, skipRuleID string) ([]*net.IPNet, error) {
func (p *dbIPRulesProvider) get(ctx context.Context, groupID string) ([]ipRule, error) {
allowed, ok := p.cache.Get(groupID)
if !ok {
pr, err := p.loadParsedRulesFromDB(ctx, groupID, skipRuleID)
pr, err := p.loadParsedRulesFromDB(ctx, groupID)
if err != nil {
return nil, err
}
// if skipRuleID is set, the retrieved rule list may be incomplete.
if skipRuleID == "" {
p.cache.Add(groupID, pr)
}
p.cache.Add(groupID, pr)
allowed = pr
}
return allowed, nil
Expand Down Expand Up @@ -240,7 +241,7 @@ func (n *NoOpEnforcer) AuthorizeHTTPRequest(ctx context.Context, r *http.Request
func (n *NoOpEnforcer) InvalidateCache(ctx context.Context, groupID string) {
}

func (n *NoOpEnforcer) Check(ctx context.Context, groupID string, skipRuleID string) error {
func (n *NoOpEnforcer) Check(ctx context.Context, groupID, skipRuleID string) error {
return nil
}

Expand Down Expand Up @@ -272,21 +273,24 @@ func Register(env *real_environment.RealEnv) error {
return nil
}

func (s *Enforcer) Check(ctx context.Context, groupID string, skipRuleID string) error {
func (s *Enforcer) Check(ctx context.Context, groupID, skipRuleID string) error {
rawClientIP := clientip.Get(ctx)
clientIP := net.ParseIP(rawClientIP)
// Client IP is not parsable.
if clientIP == nil {
return status.FailedPreconditionErrorf("client IP %q is not valid", rawClientIP)
}

allowed, err := s.rulesProvider.get(ctx, groupID, skipRuleID)
rules, err := s.rulesProvider.get(ctx, groupID)
if err != nil {
return err
}

for _, a := range allowed {
if a.Contains(clientIP) {
for _, rule := range rules {
if rule.id == skipRuleID {
continue
}
if rule.allowed.Contains(clientIP) {
return nil
}
}
Expand All @@ -296,7 +300,7 @@ func (s *Enforcer) Check(ctx context.Context, groupID string, skipRuleID string)

func (s *Enforcer) authorize(ctx context.Context, groupID string) error {
start := time.Now()
err := s.Check(ctx, groupID, "" /*skipRuleID*/)
err := s.Check(ctx, groupID, "" /*=skipRuleID*/)
metrics.IPRulesCheckLatencyUsec.With(
prometheus.Labels{metrics.StatusHumanReadableLabel: status.MetricsLabel(err)},
).Observe(float64(time.Since(start).Microseconds()))
Expand Down
Loading