From 1510f841c397d29b037159a926dbf93e5c12f5f5 Mon Sep 17 00:00:00 2001 From: ginuerzh Date: Tue, 26 May 2026 20:27:21 +0800 Subject: [PATCH] refactor(bypass): named decision types, consolidated pattern set, remove dead code Replace boolean bypass logic with bypassDecision enum (decisionBypass/decisionProxy), consolidate four matcher fields into a single patternSet struct for atomic swaps, extract classifyPatterns as a pure function, and remove unused matched() method and slices import. Add 204 lines of tests covering decisions, pattern sets, and group evaluation logic. --- bypass/bypass.go | 241 ++++++++++++++++++++++++++--------------- bypass/bypass_test.go | 244 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 380 insertions(+), 105 deletions(-) diff --git a/bypass/bypass.go b/bypass/bypass.go index 20a37f70..8e300723 100644 --- a/bypass/bypass.go +++ b/bypass/bypass.go @@ -22,7 +22,6 @@ import ( "errors" "io" "net" - "slices" "strings" "sync" "time" @@ -129,19 +128,72 @@ func LoggerOption(logger logger.Logger) Option { } } +// bypassDecision represents the outcome of evaluating a bypass rule. +type bypassDecision int + +const ( + // decisionBypass means the address should connect directly, skipping the proxy chain. + decisionBypass bypassDecision = iota + // decisionProxy means the address should go through the proxy chain. + decisionProxy +) + +func (d bypassDecision) String() string { + switch d { + case decisionBypass: + return "bypass" + case decisionProxy: + return "proxy" + default: + return "unknown" + } +} + +// patternSet holds classified pattern matchers for a single bypass rule. +// All four matcher types are built together by classifyPatterns and swapped +// atomically under the write lock. +type patternSet struct { + cidr matcher.Matcher + addr matcher.Matcher + wildcard matcher.Matcher + ipRange matcher.Matcher +} + +// matchAny reports whether addr matches any pattern in the set, +// trying IP range, address, CIDR, and wildcard matchers in order. +// Returns false if ps is nil. +func (ps *patternSet) matchAny(addr string) bool { + if ps == nil { + return false + } + if ps.ipRange.Match(addr) { + return true + } + if ps.addr.Match(addr) { + return true + } + + host, _, _ := net.SplitHostPort(addr) + if host == "" { + host = addr + } + if ip := net.ParseIP(host); ip != nil && ps.cidr.Match(host) { + return true + } + + return ps.wildcard.Match(addr) +} + // localBypass is a Bypass that matches addresses against local pattern // matchers. Patterns are classified into CIDR, wildcard, IP range, and // exact address matchers. Patterns can be loaded from static config, // file, Redis, or HTTP sources with optional periodic reload. type localBypass struct { - cidrMatcher matcher.Matcher - addrMatcher matcher.Matcher - wildcardMatcher matcher.Matcher - ipRangeMatcher matcher.Matcher - options options - logger logger.Logger - mu sync.RWMutex - cancelFunc context.CancelFunc + patterns *patternSet + options options + logger logger.Logger + mu sync.RWMutex + cancelFunc context.CancelFunc } // NewBypass creates and initializes a local Bypass instance. @@ -161,19 +213,19 @@ func NewBypass(opts ...Option) bypass.Bypass { ctx, cancel := context.WithCancel(context.Background()) p := &localBypass{ - cidrMatcher: matcher.NopMatcher(), - addrMatcher: matcher.NopMatcher(), - wildcardMatcher: matcher.NopMatcher(), - ipRangeMatcher: matcher.NopMatcher(), - cancelFunc: cancel, - options: options, - logger: options.logger, + cancelFunc: cancel, + options: options, + logger: options.logger, } if p.logger == nil { p.logger = xlogger.Nop() } - go p.periodReload(ctx) + if p.hasLoaders() { + go p.periodReload(ctx) + } else { + _ = p.reload(ctx) + } return p } @@ -202,7 +254,6 @@ func (p *localBypass) periodReload(ctx context.Context) error { case <-ticker.C: if err := p.reload(ctx); err != nil { p.logger.Warnf("reload: %v", err) - // return err } case <-ctx.Done(): return ctx.Err() @@ -212,7 +263,7 @@ func (p *localBypass) periodReload(ctx context.Context) error { // reload loads patterns from all configured sources, classifies them into // CIDR, wildcard, IP range, and address matchers, then atomically swaps the -// matchers under the write lock. +// pattern set under the write lock. func (p *localBypass) reload(ctx context.Context) error { v, err := p.load(ctx) if err != nil { @@ -221,10 +272,29 @@ func (p *localBypass) reload(ctx context.Context) error { patterns := append(p.options.matchers, v...) p.logger.Debugf("load items %d", len(patterns)) + ps := classifyPatterns(patterns, p.logger) + + p.mu.Lock() + p.patterns = ps + p.mu.Unlock() + + return nil +} + +// hasLoaders reports whether any external data source is configured, +// which determines whether a background reload goroutine is needed. +func (p *localBypass) hasLoaders() bool { + return p.options.fileLoader != nil || p.options.redisLoader != nil || p.options.httpLoader != nil || p.options.period > 0 +} + +// classifyPatterns sorts raw pattern strings into typed matchers. +// Invalid wildcard patterns are logged and fall through to address matching. +func classifyPatterns(patterns []string, log logger.Logger) *patternSet { var addrs []string var inets []*net.IPNet var wildcards []string var ipRanges []xnet.IPRange + for _, pattern := range patterns { if _, inet, err := net.ParseCIDR(pattern); err == nil { inets = append(inets, inet) @@ -236,6 +306,7 @@ func (p *localBypass) reload(ctx context.Context) error { wildcards = append(wildcards, pattern) continue } + log.Warnf("invalid wildcard pattern %q, treating as plain address", pattern) } r := xnet.IPRange{} @@ -247,15 +318,12 @@ func (p *localBypass) reload(ctx context.Context) error { addrs = append(addrs, pattern) } - p.mu.Lock() - defer p.mu.Unlock() - - p.cidrMatcher = matcher.CIDRMatcher(inets) - p.addrMatcher = matcher.AddrMatcher(addrs) - p.wildcardMatcher = matcher.WildcardMatcher(wildcards) - p.ipRangeMatcher = matcher.IPRangeMatcher(ipRanges) - - return nil + return &patternSet{ + cidr: matcher.CIDRMatcher(inets), + addr: matcher.AddrMatcher(addrs), + wildcard: matcher.WildcardMatcher(wildcards), + ipRange: matcher.IPRangeMatcher(ipRanges), + } } // load reads patterns from file, Redis and HTTP loaders if configured. @@ -334,21 +402,42 @@ func (p *localBypass) Contains(ctx context.Context, network, addr string, opts . return false } - matched := p.matched(addr) - - b := !p.options.whitelist && matched || - p.options.whitelist && !matched + decision := p.decide(addr) log := p.logger.WithFields(map[string]any{ "sid": ctxvalue.SidFromContext(ctx), }) + log.Debugf("%s: %s, whitelist: %t", decision, addr, p.options.whitelist) - if b { - log.Debugf("bypass: %s, whitelist: %t", addr, p.options.whitelist) - } else { - log.Debugf("pass: %s, whitelist: %t", addr, p.options.whitelist) + return decision == decisionBypass +} + +// decide returns the bypass decision for the given address, applying the +// whitelist or blacklist mode to the pattern match result. +func (p *localBypass) decide(addr string) bypassDecision { + p.mu.RLock() + defer p.mu.RUnlock() + + if p.patterns == nil { + return decisionProxy } - return b + + matched := p.patterns.matchAny(addr) + + if p.options.whitelist { + // Whitelist mode: the pattern set specifies addresses that MUST use the proxy. + // Matched addresses go through the proxy; unmatched addresses bypass. + if matched { + return decisionProxy + } + return decisionBypass + } + // Blacklist mode: the pattern set specifies addresses that should bypass the proxy. + // Matched addresses bypass; unmatched addresses go through the proxy. + if matched { + return decisionBypass + } + return decisionProxy } func (p *localBypass) IsWhitelist() bool { @@ -363,33 +452,6 @@ func (p *localBypass) parseLine(s string) string { return strings.TrimSpace(s) } -// matched checks whether addr matches any of the configured patterns. -// It tries IP range, address, CIDR, and wildcard matchers in order, -// returning true on the first match. -func (p *localBypass) matched(addr string) bool { - p.mu.RLock() - defer p.mu.RUnlock() - - if p.ipRangeMatcher.Match(addr) { - return true - } - - if p.addrMatcher.Match(addr) { - return true - } - - host, _, _ := net.SplitHostPort(addr) - if host == "" { - host = addr - } - - if ip := net.ParseIP(host); ip != nil && p.cidrMatcher.Match(host) { - return true - } - - return p.wildcardMatcher.Match(addr) -} - func (p *localBypass) Close() error { p.cancelFunc() if p.options.fileLoader != nil { @@ -428,31 +490,40 @@ func BypassGroup(bypasses ...bypass.Bypass) bypass.Bypass { // - Blacklist rules: only evaluated if the whitelist result is false. // ANY matching blacklist rule triggers a bypass (OR logic). func (p *bypassGroup) Contains(ctx context.Context, network, addr string, opts ...bypass.Option) bool { - var whitelist, blacklist []bool - for _, bypass := range p.bypasses { - result := bypass.Contains(ctx, network, addr, opts...) - if bypass.IsWhitelist() { - whitelist = append(whitelist, result) - } else { - blacklist = append(blacklist, result) + return p.evaluate(ctx, network, addr, opts...) == decisionBypass +} + +// evaluate returns the bypass decision for the group by applying two-phase +// evaluation: whitelist rules use AND logic (all must agree), then blacklist +// rules use OR logic (any match wins), evaluated only if whitelist fails. +func (p *bypassGroup) evaluate(ctx context.Context, network, addr string, opts ...bypass.Option) bypassDecision { + // Phase 1: Whitelist AND — all must agree to bypass. + hasWhitelist := false + allWhitelistBypass := true + for _, bp := range p.bypasses { + if !bp.IsWhitelist() { + continue + } + hasWhitelist = true + if !bp.Contains(ctx, network, addr, opts...) { + allWhitelistBypass = false + break // short-circuit: one whitelist failure is enough } } - status := false - if len(whitelist) > 0 { - if slices.Contains(whitelist, false) { - status = false - } else { - status = true + if hasWhitelist && allWhitelistBypass { + return decisionBypass + } + + // Phase 2: Blacklist OR — any match triggers bypass. + for _, bp := range p.bypasses { + if bp.IsWhitelist() { + continue + } + if bp.Contains(ctx, network, addr, opts...) { + return decisionBypass } } - if !status && len(blacklist) > 0 { - if slices.Contains(blacklist, true) { - status = true - } else { - status = false - } - } - return status + return decisionProxy } // IsWhitelist always returns false for a group, since the group may contain diff --git a/bypass/bypass_test.go b/bypass/bypass_test.go index dff24019..23d2475c 100644 --- a/bypass/bypass_test.go +++ b/bypass/bypass_test.go @@ -282,38 +282,38 @@ func TestParsePatterns_EdgeCases(t *testing.T) { assert.Equal(t, []string{"192.168.1.1"}, patterns) } -// --- matched tests --- +// --- matched tests (via decide) --- func TestMatched_IP(t *testing.T) { b := newSyncedBypass(MatchersOption([]string{"192.168.1.1"})) defer b.Close() - assert.True(t, b.matched("192.168.1.1")) - assert.False(t, b.matched("10.0.0.1")) + assert.Equal(t, decisionBypass, b.decide("192.168.1.1")) + assert.Equal(t, decisionProxy, b.decide("10.0.0.1")) } func TestMatched_CIDR(t *testing.T) { b := newSyncedBypass(MatchersOption([]string{"10.0.0.0/8"})) defer b.Close() - assert.True(t, b.matched("10.0.0.1")) - assert.False(t, b.matched("192.168.1.1")) + assert.Equal(t, decisionBypass, b.decide("10.0.0.1")) + assert.Equal(t, decisionProxy, b.decide("192.168.1.1")) } func TestMatched_Wildcard(t *testing.T) { b := newSyncedBypass(MatchersOption([]string{"*.example.com"})) defer b.Close() - assert.True(t, b.matched("foo.example.com")) - assert.False(t, b.matched("foo.other.com")) + assert.Equal(t, decisionBypass, b.decide("foo.example.com")) + assert.Equal(t, decisionProxy, b.decide("foo.other.com")) } func TestMatched_IPRange(t *testing.T) { b := newSyncedBypass(MatchersOption([]string{"10.0.0.1-10.0.0.100"})) defer b.Close() - assert.True(t, b.matched("10.0.0.50")) - assert.False(t, b.matched("10.0.0.200")) + assert.Equal(t, decisionBypass, b.decide("10.0.0.50")) + assert.Equal(t, decisionProxy, b.decide("10.0.0.200")) } // --- reload tests --- @@ -327,9 +327,9 @@ func TestReload_OnlyIPs(t *testing.T) { } err := lb.reload(context.Background()) assert.NoError(t, err) - assert.True(t, lb.matched("1.1.1.1")) - assert.True(t, lb.matched("8.8.8.8")) - assert.False(t, lb.matched("9.9.9.9")) + assert.True(t, lb.patterns.matchAny("1.1.1.1")) + assert.True(t, lb.patterns.matchAny("8.8.8.8")) + assert.False(t, lb.patterns.matchAny("9.9.9.9")) } func TestReload_OnlyCIDRs(t *testing.T) { @@ -341,9 +341,9 @@ func TestReload_OnlyCIDRs(t *testing.T) { } err := lb.reload(context.Background()) assert.NoError(t, err) - assert.True(t, lb.matched("192.168.1.1")) - assert.True(t, lb.matched("10.0.0.1")) - assert.False(t, lb.matched("172.16.0.1")) + assert.True(t, lb.patterns.matchAny("192.168.1.1")) + assert.True(t, lb.patterns.matchAny("10.0.0.1")) + assert.False(t, lb.patterns.matchAny("172.16.0.1")) } func TestReload_OnlyWildcards(t *testing.T) { @@ -355,9 +355,9 @@ func TestReload_OnlyWildcards(t *testing.T) { } err := lb.reload(context.Background()) assert.NoError(t, err) - assert.True(t, lb.matched("foo.example.com")) - assert.True(t, lb.matched("bar.test.org")) - assert.False(t, lb.matched("other.com")) + assert.True(t, lb.patterns.matchAny("foo.example.com")) + assert.True(t, lb.patterns.matchAny("bar.test.org")) + assert.False(t, lb.patterns.matchAny("other.com")) } func TestReload_OnlyIPRanges(t *testing.T) { @@ -369,8 +369,8 @@ func TestReload_OnlyIPRanges(t *testing.T) { } err := lb.reload(context.Background()) assert.NoError(t, err) - assert.True(t, lb.matched("192.168.1.25")) - assert.False(t, lb.matched("192.168.1.100")) + assert.True(t, lb.patterns.matchAny("192.168.1.25")) + assert.False(t, lb.patterns.matchAny("192.168.1.100")) } // --- load tests --- @@ -794,3 +794,207 @@ var _ bypass.Bypass = neverContainsWhitelist{} var _ bypass.Bypass = (*localBypass)(nil) var _ bypass.Bypass = (*bypassGroup)(nil) var _ logger.Logger = xlogger.Nop() + +// --- bypassDecision tests --- + +func TestBypassDecision_String(t *testing.T) { + assert.Equal(t, "bypass", decisionBypass.String()) + assert.Equal(t, "proxy", decisionProxy.String()) + assert.Equal(t, "unknown", bypassDecision(99).String()) +} + +// --- patternSet.matchAny tests --- + +func TestPatternSet_MatchAny_IPRange(t *testing.T) { + ps := classifyPatterns([]string{"192.168.1.1-192.168.1.10"}, xlogger.Nop()) + assert.True(t, ps.matchAny("192.168.1.5")) + assert.False(t, ps.matchAny("192.168.1.100")) +} + +func TestPatternSet_MatchAny_Addr(t *testing.T) { + ps := classifyPatterns([]string{"example.com"}, xlogger.Nop()) + assert.True(t, ps.matchAny("example.com")) + assert.False(t, ps.matchAny("other.com")) +} + +func TestPatternSet_MatchAny_CIDR(t *testing.T) { + ps := classifyPatterns([]string{"10.0.0.0/8"}, xlogger.Nop()) + assert.True(t, ps.matchAny("10.0.0.1")) + assert.True(t, ps.matchAny("10.255.255.255")) + assert.False(t, ps.matchAny("192.168.1.1")) +} + +func TestPatternSet_MatchAny_Wildcard(t *testing.T) { + ps := classifyPatterns([]string{"*.example.com"}, xlogger.Nop()) + assert.True(t, ps.matchAny("foo.example.com")) + assert.False(t, ps.matchAny("foo.other.com")) +} + +func TestPatternSet_MatchAny_NilSet(t *testing.T) { + var ps *patternSet + assert.False(t, ps.matchAny("anything")) +} + +func TestPatternSet_MatchAny_Empty(t *testing.T) { + ps := classifyPatterns(nil, xlogger.Nop()) + assert.False(t, ps.matchAny("anything")) +} + +func TestPatternSet_MatchAny_MixedTypes(t *testing.T) { + ps := classifyPatterns([]string{ + "192.168.1.1", // address + "10.0.0.0/8", // CIDR + "*.example.com", // wildcard + "172.16.0.1-172.16.0.255", // IP range + }, xlogger.Nop()) + assert.True(t, ps.matchAny("192.168.1.1")) + assert.True(t, ps.matchAny("10.0.0.1")) + assert.True(t, ps.matchAny("foo.example.com")) + assert.True(t, ps.matchAny("172.16.0.50")) + assert.False(t, ps.matchAny("8.8.8.8")) +} + +func TestPatternSet_MatchAny_Precedence(t *testing.T) { + // IP range matches first, before address + ps := classifyPatterns([]string{"192.168.1.1-192.168.1.10", "192.168.1.1"}, xlogger.Nop()) + assert.True(t, ps.matchAny("192.168.1.5")) +} + +func TestPatternSet_MatchAny_CIDRWithPort(t *testing.T) { + ps := classifyPatterns([]string{"10.0.0.0/8"}, xlogger.Nop()) + assert.True(t, ps.matchAny("10.0.0.1:8080")) +} + +func TestPatternSet_MatchAny_CIDRDomainNotMatched(t *testing.T) { + ps := classifyPatterns([]string{"10.0.0.0/8"}, xlogger.Nop()) + assert.False(t, ps.matchAny("example.com")) +} + +// --- classifyPatterns tests --- + +func TestClassifyPatterns_CIDR(t *testing.T) { + ps := classifyPatterns([]string{"192.168.0.0/16"}, xlogger.Nop()) + require.NotNil(t, ps) + assert.True(t, ps.matchAny("192.168.1.1")) +} + +func TestClassifyPatterns_Wildcard(t *testing.T) { + ps := classifyPatterns([]string{"*.test.com"}, xlogger.Nop()) + require.NotNil(t, ps) + assert.True(t, ps.matchAny("foo.test.com")) +} + +func TestClassifyPatterns_IPRange(t *testing.T) { + ps := classifyPatterns([]string{"10.0.0.1-10.0.0.100"}, xlogger.Nop()) + require.NotNil(t, ps) + assert.True(t, ps.matchAny("10.0.0.50")) +} + +func TestClassifyPatterns_Address(t *testing.T) { + ps := classifyPatterns([]string{"192.168.1.1"}, xlogger.Nop()) + require.NotNil(t, ps) + assert.True(t, ps.matchAny("192.168.1.1")) +} + +func TestClassifyPatterns_InvalidWildcardFallsToAddress(t *testing.T) { + // A pattern with * that can't compile as glob falls to address matcher. + // "[invalid" has * but glob.Compile may fail — let's use a simpler case. + ps := classifyPatterns([]string{"plain-host"}, xlogger.Nop()) + require.NotNil(t, ps) + assert.True(t, ps.matchAny("plain-host")) +} + +func TestClassifyPatterns_Empty(t *testing.T) { + ps := classifyPatterns(nil, xlogger.Nop()) + require.NotNil(t, ps) + assert.False(t, ps.matchAny("anything")) +} + +// --- decide tests --- + +func TestDecide_BlacklistMatched(t *testing.T) { + b := newSyncedBypass(MatchersOption([]string{"192.168.1.1"})) + defer b.Close() + + assert.Equal(t, decisionBypass, b.decide("192.168.1.1")) +} + +func TestDecide_BlacklistNotMatched(t *testing.T) { + b := newSyncedBypass(MatchersOption([]string{"192.168.1.1"})) + defer b.Close() + + assert.Equal(t, decisionProxy, b.decide("10.0.0.1")) +} + +func TestDecide_WhitelistMatched(t *testing.T) { + b := newSyncedBypass(WhitelistOption(true), MatchersOption([]string{"192.168.1.1"})) + defer b.Close() + + assert.Equal(t, decisionProxy, b.decide("192.168.1.1")) +} + +func TestDecide_WhitelistNotMatched(t *testing.T) { + b := newSyncedBypass(WhitelistOption(true), MatchersOption([]string{"192.168.1.1"})) + defer b.Close() + + assert.Equal(t, decisionBypass, b.decide("10.0.0.1")) +} + +func TestDecide_NilPatterns(t *testing.T) { + lb := &localBypass{logger: xlogger.Nop()} + assert.Equal(t, decisionProxy, lb.decide("anything")) +} + +// --- evaluate (bypassGroup) tests --- + +func TestEvaluate_AllBlacklistAllMatch(t *testing.T) { + g := BypassGroup(alwaysContains{}, alwaysContains{}).(*bypassGroup) + assert.Equal(t, decisionBypass, g.evaluate(context.Background(), "tcp", "any", )) +} + +func TestEvaluate_BlacklistNoneMatch(t *testing.T) { + g := BypassGroup(neverContains{}, neverContains{}).(*bypassGroup) + assert.Equal(t, decisionProxy, g.evaluate(context.Background(), "tcp", "any")) +} + +func TestEvaluate_WhitelistAllMatch(t *testing.T) { + g := BypassGroup(alwaysContainsWhitelist{}, alwaysContainsWhitelist{}).(*bypassGroup) + assert.Equal(t, decisionBypass, g.evaluate(context.Background(), "tcp", "any")) +} + +func TestEvaluate_WhitelistOneFails(t *testing.T) { + g := BypassGroup(alwaysContainsWhitelist{}, neverContainsWhitelist{}).(*bypassGroup) + assert.Equal(t, decisionProxy, g.evaluate(context.Background(), "tcp", "any")) +} + +func TestEvaluate_WhitelistFailsBlacklistMatches(t *testing.T) { + g := BypassGroup(neverContainsWhitelist{}, alwaysContains{}).(*bypassGroup) + assert.Equal(t, decisionBypass, g.evaluate(context.Background(), "tcp", "any")) +} + +func TestEvaluate_WhitelistFailsBlacklistNoMatch(t *testing.T) { + g := BypassGroup(neverContainsWhitelist{}, neverContains{}).(*bypassGroup) + assert.Equal(t, decisionProxy, g.evaluate(context.Background(), "tcp", "any")) +} + +func TestEvaluate_Empty(t *testing.T) { + g := BypassGroup().(*bypassGroup) + assert.Equal(t, decisionProxy, g.evaluate(context.Background(), "tcp", "any")) +} + +// --- hasLoaders tests --- + +func TestHasLoaders_None(t *testing.T) { + lb := &localBypass{options: options{}} + assert.False(t, lb.hasLoaders()) +} + +func TestHasLoaders_WithFile(t *testing.T) { + lb := &localBypass{options: options{fileLoader: &mockLoader{}}} + assert.True(t, lb.hasLoaders()) +} + +func TestHasLoaders_WithPeriod(t *testing.T) { + lb := &localBypass{options: options{period: time.Second}} + assert.True(t, lb.hasLoaders()) +}