From 4083faed866e01c63ea92614dbc162d15c19ee14 Mon Sep 17 00:00:00 2001 From: Paul Lorenz Date: Tue, 9 Jun 2026 16:35:32 -0400 Subject: [PATCH] Refcount shared intercept hostnames so iptables rules are installed for every service (#3868) * Refcount shared intercept hostnames so iptables rules are installed for every service. Fixes #3868 - modifies getDnsIp to invoke addrCB and register a cleanup action when the hostname already has an allocated IP, so each service sharing the hostname gets its per-service iptables rule installed - adds a reference count on hostname allocations so the resolver entry is removed and the CGNAT IP recycled only when the last service using the hostname is cleaned up - canonicalizes the refcount key by lowercasing so case variants of the same hostname share one allocation, matching the resolver's case-insensitive view - adds tests covering shared-hostname rule installation, refcounted cleanup, out-of-order cleanup, and case-insensitive sharing * Address review comments * Refcount wildcard-allocated intercept hostnames. Fixes #3957 - moves hostname registration into allocateDnsIp, under the allocation mutex, so every allocation (fresh or reused) takes one reference through the refcounting resolver and closes the lookup/registration race between wildcard DNS queries and service updates - removes the direct AddHostname call in getAddress, which bypassed the refcounting layer and let one service's cleanup remove a hostname still used by an overlapping wildcard or literal intercept - documents the AddDomain callback contract: the callback registers the hostname mapping itself - updates tests to match the production wiring and adds coverage for overlapping wildcard/literal intercepts in both removal orders - adds a dns package test pinning the getAddress callback contract - adds local ai tooling files to .gitignore --- .gitignore | 6 + tunnel/dns/refcount.go | 51 +++-- tunnel/dns/resolver.go | 5 + tunnel/dns/server.go | 6 +- tunnel/dns/server_test.go | 70 ++++++ tunnel/intercept/iputils.go | 70 ++++-- tunnel/intercept/iputils_test.go | 368 +++++++++++++++++++++++++++++++ 7 files changed, 536 insertions(+), 40 deletions(-) create mode 100644 tunnel/dns/server_test.go create mode 100644 tunnel/intercept/iputils_test.go diff --git a/.gitignore b/.gitignore index 497345338..d2744e6b6 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,12 @@ release/ # Dependency directories (remove the comment below to include it) # vendor/ +# ai agents +.claude +.mercurius/ +mercurius.yaml +.mcp.json + # goland .idea github_deploy_key diff --git a/tunnel/dns/refcount.go b/tunnel/dns/refcount.go index 68f085b3d..8944ed877 100644 --- a/tunnel/dns/refcount.go +++ b/tunnel/dns/refcount.go @@ -1,19 +1,28 @@ package dns import ( - cmap "github.com/orcaman/concurrent-map/v2" "net" + "strings" + "sync" ) +// NewRefCountingResolver wraps resolver so the underlying hostname is only +// removed once the last caller releases it. Successive AddHostname calls for +// the same hostname bump a reference count; RemoveHostname decrements it and +// only forwards to the wrapped resolver when the count reaches zero. func NewRefCountingResolver(resolver Resolver) Resolver { return &RefCountingResolver{ - names: cmap.New[int](), + names: map[string]int{}, wrapped: resolver, } } +// RefCountingResolver reference counts AddHostname/RemoveHostname calls per +// hostname. The lock is held across the wrapped resolver calls so the count +// and the wrapped resolver's state can't diverge under concurrent use. type RefCountingResolver struct { - names cmap.ConcurrentMap[string, int] + lock sync.Mutex + names map[string]int wrapped Resolver } @@ -34,31 +43,33 @@ func (self *RefCountingResolver) RemoveDomain(name string) { } func (self *RefCountingResolver) AddHostname(s string, ip net.IP) error { + self.lock.Lock() + defer self.lock.Unlock() + err := self.wrapped.AddHostname(s, ip) - if err != nil { - self.names.Upsert(s, 1, func(exist bool, valueInMap int, newValue int) int { - if exist { - return valueInMap + 1 - } - return 1 - }) + if err == nil { + // canonicalize so different-case spellings of the same hostname share + // one count, matching the wrapped resolver's case-insensitive view + self.names[strings.ToLower(s)]++ } return err } func (self *RefCountingResolver) RemoveHostname(s string) net.IP { - val := self.names.Upsert(s, 1, func(exist bool, valueInMap int, newValue int) int { - if exist { - return valueInMap - 1 - } - return 0 - }) + self.lock.Lock() + defer self.lock.Unlock() - if val == 0 { - self.names.Remove(s) - return self.wrapped.RemoveHostname(s) + key := strings.ToLower(s) + if count := self.names[key]; count > 1 { + self.names[key] = count - 1 + return nil } - return nil + + // count <= 1 covers both the last reference and, defensively, hostnames + // never added through this layer (e.g. when the wrapped AddHostname + // failed, so the count was never incremented) + delete(self.names, key) + return self.wrapped.RemoveHostname(s) } func (self *RefCountingResolver) Cleanup() error { diff --git a/tunnel/dns/resolver.go b/tunnel/dns/resolver.go index 0722b7228..90159567f 100644 --- a/tunnel/dns/resolver.go +++ b/tunnel/dns/resolver.go @@ -18,8 +18,13 @@ package dns import "net" +// Resolver maps hostnames to intercept IPs for the tunneler's internal DNS. type Resolver interface { AddHostname(string, net.IP) error + // AddDomain registers a wildcard domain (e.g. *.ziti). The callback is + // invoked to produce an IP for each previously unknown hostname matching + // the domain, and must register the resulting hostname -> IP mapping via + // AddHostname; the resolver does not register it implicitly. AddDomain(string, func(string) (net.IP, error)) error Lookup(net.IP) (string, error) LookupIP(string) (net.IP, bool) diff --git a/tunnel/dns/server.go b/tunnel/dns/server.go index da3eb8a6d..b82af4b47 100644 --- a/tunnel/dns/server.go +++ b/tunnel/dns/server.go @@ -121,7 +121,11 @@ func (r *resolver) getAddress(name string) (net.IP, error) { return nil, err } log.Debugf("assigned %v => %v", name, ip) - _ = r.AddHostname(name, ip) // this resolver impl never returns an error + // the getIP callback registers the hostname -> IP mapping (see + // Resolver.AddDomain), so later queries short-circuit via the + // LookupIP check above. Registering it here directly would bypass + // the refcounting layer wrapping this resolver and let a cleanup + // of one service remove a hostname other services still use. return ip, err } } diff --git a/tunnel/dns/server_test.go b/tunnel/dns/server_test.go new file mode 100644 index 000000000..d7d80486e --- /dev/null +++ b/tunnel/dns/server_test.go @@ -0,0 +1,70 @@ +/* + Copyright NetFoundry Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package dns + +import ( + "net" + "testing" + + "github.com/stretchr/testify/require" +) + +// newTestResolver builds a resolver with no listening DNS server, suitable +// for exercising getAddress directly. +func newTestResolver() *resolver { + return &resolver{ + names: map[string]net.IP{}, + ips: map[string]string{}, + domains: map[string]*domainEntry{}, + } +} + +// Test_GetAddress_DomainCallbackRegistersHostname pins the AddDomain callback +// contract: the callback registers the hostname -> IP mapping itself (through +// the refcounting layer in production), and getAddress does not add it to the +// wrapped resolver directly. A direct add would bypass the refcount and let +// one service's cleanup remove a hostname other services still use. +func Test_GetAddress_DomainCallbackRegistersHostname(t *testing.T) { + req := require.New(t) + + wrapped := newTestResolver() + wrapper := NewRefCountingResolver(wrapped) + + ip := net.IP{100, 64, 0, 1} + calls := 0 + req.NoError(wrapper.AddDomain("*.example.com", func(host string) (net.IP, error) { + calls++ + req.NoError(wrapper.AddHostname(host, ip)) + return ip, nil + })) + + got, err := wrapped.getAddress("test.example.com.") + req.NoError(err) + req.True(ip.Equal(got)) + req.Equal(1, calls, "first query must invoke the domain callback") + + got, err = wrapped.getAddress("test.example.com.") + req.NoError(err) + req.True(ip.Equal(got)) + req.Equal(1, calls, "registered hostname must short-circuit; callback must not run again") + + // the callback registered through the refcounting layer, so a paired + // release removes the mapping from the wrapped resolver + req.NotNil(wrapper.RemoveHostname("test.example.com")) + _, found := wrapped.LookupIP("test.example.com.") + req.False(found, "hostname must be removed once its only reference is released") +} diff --git a/tunnel/intercept/iputils.go b/tunnel/intercept/iputils.go index e3f3a1e84..aca6702b1 100644 --- a/tunnel/intercept/iputils.go +++ b/tunnel/intercept/iputils.go @@ -64,8 +64,12 @@ func GetDnsInterceptIpRange() *net.IPNet { } } +// cleanUpFunc returns the per-service cleanup action registered when a hostname +// is intercepted. The wrapping RefCountingResolver only forwards +// RemoveHostname to the underlying resolver on the last release, so this +// cleanup recycles the CGNAT IP only when the returned IP is non-nil. func cleanUpFunc(hostname string, resolver dns.Resolver) func() { - f := func() { + return func() { ip := resolver.RemoveHostname(hostname) if ip != nil { dnsCurrentIpMtx.Lock() @@ -74,19 +78,51 @@ func cleanUpFunc(hostname string, resolver dns.Resolver) func() { dnsRecycledIps.PushBack(addr) } } - return f +} + +// hostMask returns a host-route mask (/32 or /128) for the given IP. +func hostMask(ip net.IP) net.IPMask { + bits := len(ip) * 8 + return net.CIDRMask(bits, bits) } func getDnsIp(host string, addrCB func(*net.IPNet, bool), svc *entities.Service, resolver dns.Resolver) (net.IP, error) { + addr, cleanup, err := allocateDnsIp(host, resolver) + if err != nil { + return nil, err + } + // addrCB and AddCleanupAction must run outside dnsCurrentIpMtx: addrCB can + // touch interceptor state, and AddCleanupAction takes service.lock which + // is acquired in the reverse order by RunCleanupActions -> cleanUpFunc. + addrCB(addr, false) // no route is needed because the dns cidr was added to "lo" at startup + svc.AddCleanupAction(cleanup) + return addr.IP, nil +} + +// allocateDnsIp returns the IP mapped to host, allocating one from the +// intercept range if the hostname is unknown, and registers the hostname -> IP +// mapping with the resolver. Lookup and registration both happen under +// dnsCurrentIpMtx so concurrent allocations of the same hostname (e.g. a +// wildcard-domain DNS query racing a service update) can't both miss the +// lookup and allocate distinct IPs. Every call takes one reference on the +// hostname (see dns.NewRefCountingResolver); the returned cleanup releases it. +func allocateDnsIp(host string, resolver dns.Resolver) (*net.IPNet, func(), error) { dnsCurrentIpMtx.Lock() defer dnsCurrentIpMtx.Unlock() - var ip netip.Addr - foundIP, found := resolver.LookupIP(host + ".") - if found { - return foundIP, nil + // If the hostname already has an allocated IP, reuse it, taking an + // additional reference. Even though no new IP is allocated, the caller + // still must invoke addrCB so the interceptor installs its per-service + // iptables rule; otherwise the tproxy listener exists with no kernel rule + // pointing at it and the service is silently unreachable. + if foundIP, found := resolver.LookupIP(host + "."); found { + if err := resolver.AddHostname(host, foundIP); err != nil { + pfxlog.Logger().WithError(err).Errorf("failed to add host/ip mapping to resolver: %v -> %v", host, foundIP) + } + return &net.IPNet{IP: foundIP, Mask: hostMask(foundIP)}, cleanUpFunc(host, resolver), nil } + var ip netip.Addr // look for returned IPs first if dnsRecycledIps.Len() > 0 { e := dnsRecycledIps.Front() @@ -98,19 +134,18 @@ func getDnsIp(host string, addrCB func(*net.IPNet, bool), svc *entities.Service, if ip.IsValid() && dnsPrefix.Contains(ip) { dnsCurrentIp = ip } else { - return nil, fmt.Errorf("cannot allocate ip address: ip range exhausted") + return nil, nil, fmt.Errorf("cannot allocate ip address: ip range exhausted") } } - addr := &net.IPNet{IP: ip.AsSlice(), Mask: net.CIDRMask(ip.BitLen(), ip.BitLen())} - addrCB(addr, false) // no route is needed because the dns cidr was added to "lo" at startup - svc.AddCleanupAction(cleanUpFunc(host, resolver)) - return ip.AsSlice(), nil + ipBytes := ip.AsSlice() + if err := resolver.AddHostname(host, ipBytes); err != nil { + pfxlog.Logger().WithError(err).Errorf("failed to add host/ip mapping to resolver: %v -> %v", host, ip) + } + return &net.IPNet{IP: ipBytes, Mask: hostMask(ipBytes)}, cleanUpFunc(host, resolver), nil } func getInterceptIP(svc *entities.Service, hostname string, resolver dns.Resolver, addrCB func(*net.IPNet, bool)) error { - logger := pfxlog.Logger() - // handle wildcard domain - IPs will be allocated when matching hostnames are queried if hostname[0] == '*' { err := resolver.AddDomain(hostname, func(host string) (net.IP, error) { @@ -129,14 +164,11 @@ func getInterceptIP(svc *entities.Service, hostname string, resolver dns.Resolve return err } - // handle hostnames - ip, err := getDnsIp(hostname, addrCB, svc, resolver) - if err != nil { + // handle hostnames. getDnsIp registers the hostname -> IP mapping with + // the resolver as part of allocation. + if _, err = getDnsIp(hostname, addrCB, svc, resolver); err != nil { return fmt.Errorf("invalid IP address or unresolvable hostname: %s", hostname) } - if err = resolver.AddHostname(hostname, ip); err != nil { - logger.WithError(err).Errorf("failed to add host/ip mapping to resolver: %v -> %v", hostname, ip) - } return nil } diff --git a/tunnel/intercept/iputils_test.go b/tunnel/intercept/iputils_test.go new file mode 100644 index 000000000..387ac1ca2 --- /dev/null +++ b/tunnel/intercept/iputils_test.go @@ -0,0 +1,368 @@ +/* + Copyright NetFoundry Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package intercept + +import ( + "net" + "strings" + "testing" + + "github.com/openziti/edge-api/rest_model" + "github.com/openziti/ziti/tunnel/dns" + "github.com/openziti/ziti/tunnel/entities" + "github.com/stretchr/testify/require" +) + +// fakeResolver is a minimal in-memory dns.Resolver. Only the methods the +// tests exercise are implemented; the rest panic so an accidental dependency +// on more behavior fails loudly. In tests this is wrapped with +// dns.NewRefCountingResolver so the refcounting layer is exercised end to end. +type fakeResolver struct { + names map[string]net.IP +} + +func newFakeResolver() *fakeResolver { + return &fakeResolver{names: map[string]net.IP{}} +} + +func (r *fakeResolver) LookupIP(name string) (net.IP, bool) { + ip, ok := r.names[strings.ToLower(name)] + return ip, ok +} + +func (r *fakeResolver) AddHostname(hostname string, ip net.IP) error { + r.names[strings.ToLower(hostname)+"."] = ip + return nil +} + +func (r *fakeResolver) RemoveHostname(hostname string) net.IP { + key := strings.ToLower(hostname) + "." + ip, ok := r.names[key] + if !ok { + return nil + } + delete(r.names, key) + return ip +} + +func (r *fakeResolver) AddDomain(string, func(string) (net.IP, error)) error { + panic("unused") +} +func (r *fakeResolver) Lookup(net.IP) (string, error) { panic("unused") } +func (r *fakeResolver) RemoveDomain(string) { panic("unused") } +func (r *fakeResolver) Cleanup() error { panic("unused") } + +// resetDnsState resets the package-level allocation state so each test starts +// from a known baseline. The intercept package keeps allocator state in +// globals, and other tests in the package may run first. +func resetDnsState(t *testing.T) { + t.Helper() + require.NoError(t, SetDnsInterceptIpRange("100.64.0.1/10")) +} + +// addInterceptHostname simulates getInterceptIP's handling of a direct +// hostname by calling getDnsIp, which allocates (or reuses) the IP and +// registers the hostname -> IP mapping with the resolver as part of +// allocation. +func addInterceptHostname(t *testing.T, host string, addrCB func(*net.IPNet, bool), svc *entities.Service, resolver dns.Resolver) net.IP { + t.Helper() + ip, err := getDnsIp(host, addrCB, svc, resolver) + require.NoError(t, err) + return ip +} + +// Test_GetDnsIp_SharedHostnameInstallsRule asserts that when two services share +// an intercept hostname, getDnsIp invokes addrCB for both services so each one +// gets its iptables rule installed (the regression in #3867). Without the fix +// the second service silently has no rule programmed and is unreachable. +func Test_GetDnsIp_SharedHostnameInstallsRule(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + svcA := &entities.Service{} + svcB := &entities.Service{} + + var callsA, callsB []*net.IPNet + cbA := func(ipNet *net.IPNet, _ bool) { callsA = append(callsA, ipNet) } + cbB := func(ipNet *net.IPNet, _ bool) { callsB = append(callsB, ipNet) } + + ipA := addInterceptHostname(t, "shared.example", cbA, svcA, resolver) + req.Len(callsA, 1, "addrCB must fire for the first service") + req.True(ipA.Equal(callsA[0].IP), "addrCB receives the allocated IP") + + ipB := addInterceptHostname(t, "shared.example", cbB, svcB, resolver) + req.True(ipA.Equal(ipB), "second service must reuse the first service's IP") + req.Len(callsB, 1, "addrCB must fire for the second service so its iptables rule is installed") + req.True(ipB.Equal(callsB[0].IP), "addrCB for second service receives the shared IP") +} + +// Test_GetDnsIp_RefCountedCleanup asserts that the hostname/IP allocation is +// reference counted so a service cleanup does not pull the rug out from under +// other services still using the same hostname. +func Test_GetDnsIp_RefCountedCleanup(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + svcA := &entities.Service{} + svcB := &entities.Service{} + noopCB := func(*net.IPNet, bool) {} + + addInterceptHostname(t, "shared.example", noopCB, svcA, resolver) + addInterceptHostname(t, "shared.example", noopCB, svcB, resolver) + + // Cleaning up the second service must not remove the hostname or recycle + // the IP -- the first service still depends on both. + svcB.RunCleanupActions() + _, found := resolver.LookupIP("shared.example.") + req.True(found, "hostname must remain while another service still uses it") + req.Equal(0, dnsRecycledIps.Len(), "IP must not be recycled while another service uses it") + + // Cleaning up the last service must remove the hostname and recycle the IP. + svcA.RunCleanupActions() + _, found = resolver.LookupIP("shared.example.") + req.False(found, "hostname must be removed when the last service cleans up") + req.Equal(1, dnsRecycledIps.Len(), "IP must be recycled when the last service cleans up") +} + +// Test_GetDnsIp_SharedHostnameDifferentCase asserts that two services whose +// hostnames differ only in case share a single allocation, matching the +// resolver's case-insensitive view. +func Test_GetDnsIp_SharedHostnameDifferentCase(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + svcA := &entities.Service{} + svcB := &entities.Service{} + noopCB := func(*net.IPNet, bool) {} + + ipA := addInterceptHostname(t, "Shared.Example", noopCB, svcA, resolver) + ipB := addInterceptHostname(t, "shared.example", noopCB, svcB, resolver) + req.True(ipA.Equal(ipB), "case variants must resolve to the same IP") + + // First cleanup must not recycle: both services share the allocation. + svcA.RunCleanupActions() + req.Equal(0, dnsRecycledIps.Len(), "IP must not be recycled while a case variant still holds it") + + // Final cleanup recycles. + svcB.RunCleanupActions() + req.Equal(1, dnsRecycledIps.Len(), "IP must be recycled once the last case variant releases it") +} + +// Test_GetDnsIp_CleanupOutOfOrder asserts that cleaning up the +// first-registered service does not strand other services still using the +// same hostname. Before the fix, the first service's cleanUpFunc was the +// only one registered for a shared hostname and would unconditionally remove +// the resolver entry and recycle the IP. +func Test_GetDnsIp_CleanupOutOfOrder(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + svcA := &entities.Service{} + svcB := &entities.Service{} + noopCB := func(*net.IPNet, bool) {} + + addInterceptHostname(t, "shared.example", noopCB, svcA, resolver) + addInterceptHostname(t, "shared.example", noopCB, svcB, resolver) + + svcA.RunCleanupActions() + _, found := resolver.LookupIP("shared.example.") + req.True(found, "hostname must remain when an earlier-registered service cleans up while others still use it") + req.Equal(0, dnsRecycledIps.Len(), "IP must not be recycled while another service uses it") +} + +// Test_GetDnsIp_FromWildcardLambda asserts that getDnsIp works the same way +// when invoked from the closure registered via resolver.AddDomain for a +// wildcard intercept -- the lambda path getInterceptIP uses for hostnames +// starting with '*'. The closure must allocate an IP, register the hostname +// through the refcounting layer, install the per-service iptables rule via +// addrCB, and register a refcount-aware cleanup so reusing the same name from +// another service shares the allocation. This also covers the +// wildcard-then-literal overlap with the wildcard service removed first: +// before allocation took a reference, the wildcard cleanup stole the literal +// service's reference, removing the hostname while it was still intercepted. +func Test_GetDnsIp_FromWildcardLambda(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + svcA := &entities.Service{} + svcB := &entities.Service{} + + var callsA, callsB []*net.IPNet + cbA := func(ipNet *net.IPNet, _ bool) { callsA = append(callsA, ipNet) } + cbB := func(ipNet *net.IPNet, _ bool) { callsB = append(callsB, ipNet) } + + // Mimic the closure getInterceptIP registers via resolver.AddDomain. + wildcardLambdaA := func(host string) (net.IP, error) { + return getDnsIp(host, cbA, svcA, resolver) + } + + ipA, err := wildcardLambdaA("host.example") + req.NoError(err) + req.Len(callsA, 1, "wildcard lambda must invoke addrCB for the first service") + + // A second service joining the same hostname directly must reuse the + // allocation and still get its own addrCB call. + ipB := addInterceptHostname(t, "host.example", cbB, svcB, resolver) + req.True(ipA.Equal(ipB), "second service must reuse the allocation from the wildcard lambda") + req.Len(callsB, 1, "addrCB must fire so the joining service gets its iptables rule") + + // First cleanup must not recycle while another service still holds the hostname. + svcA.RunCleanupActions() + req.Equal(0, dnsRecycledIps.Len(), "IP must not be recycled while another service uses it") + _, found := resolver.LookupIP("host.example.") + req.True(found, "hostname must remain while another service still uses it") + + // Last cleanup releases the IP. + svcB.RunCleanupActions() + req.Equal(1, dnsRecycledIps.Len(), "IP must be recycled when the last service cleans up") +} + +// Test_GetDnsIp_WildcardThenLiteral_RemoveLiteralFirst covers the other +// removal order for overlapping wildcard and literal intercepts: a hostname +// (test.example.com) is first allocated through a wildcard domain's callback +// (*.example.com), then a service intercepting the literal hostname joins. +// Removing the literal service must not remove the resolver entry or recycle +// the IP while the wildcard service's iptables rule still points at it. +// Before the fix the wildcard allocation held no reference, so the literal +// service's cleanup tore down the shared entry and recycled the IP. +func Test_GetDnsIp_WildcardThenLiteral_RemoveLiteralFirst(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + wildcardSvc := &entities.Service{} + literalSvc := &entities.Service{} + noopCB := func(*net.IPNet, bool) {} + + // wildcard path: a DNS query for test.example.com matches *.example.com + // and the domain callback allocates and registers the hostname + wildcardIP, err := getDnsIp("test.example.com", noopCB, wildcardSvc, resolver) + req.NoError(err) + + // literal path: a service intercepting test.example.com directly joins + literalIP := addInterceptHostname(t, "test.example.com", noopCB, literalSvc, resolver) + req.True(wildcardIP.Equal(literalIP), "literal service must reuse the wildcard allocation") + + literalSvc.RunCleanupActions() + _, found := resolver.LookupIP("test.example.com.") + req.True(found, "hostname must remain while the wildcard service still uses it") + req.Equal(0, dnsRecycledIps.Len(), "IP must not be recycled while the wildcard service uses it") + + wildcardSvc.RunCleanupActions() + _, found = resolver.LookupIP("test.example.com.") + req.False(found, "hostname must be removed when the last user cleans up") + req.Equal(1, dnsRecycledIps.Len(), "IP must be recycled when the last user cleans up") +} + +// recordingAddrCB collects the InterceptAddress values produced by +// GetInterceptAddresses so tests can assert per-service expansion. +type recordingAddrCB struct { + calls []*InterceptAddress +} + +func (r *recordingAddrCB) Apply(a *InterceptAddress) { r.calls = append(r.calls, a) } + +// newSharedHostnameService builds a minimally populated entities.Service that +// intercepts the given hostname on a single TCP port. Only the fields +// GetInterceptAddresses actually reads are set. +func newSharedHostnameService(name, hostname string, port uint16) *entities.Service { + svc := &entities.Service{} + svc.Name = &name + svc.InterceptV1Config = &entities.InterceptV1Config{ + Addresses: []string{hostname}, + PortRanges: []*entities.PortRange{{Low: port, High: port}}, + } + return svc +} + +// Test_GetInterceptAddresses_SharedHostnameProducesRulePerService is the +// integration-level guard for #3867. GetInterceptAddresses is the actual entry +// point tproxy_linux.go calls per service; each Apply call eventually becomes +// an iptables rule. Before the fix the second service's callback never fired, +// so this test would observe zero InterceptAddress instances for svcB. It also +// guards against future drift in getInterceptIP's wiring that bypasses our +// finer-grained getDnsIp tests. +func Test_GetInterceptAddresses_SharedHostnameProducesRulePerService(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + svcA := newSharedHostnameService("svcA", "shared.example", 80) + svcB := newSharedHostnameService("svcB", "shared.example", 80) + + cbA := &recordingAddrCB{} + cbB := &recordingAddrCB{} + + req.NoError(GetInterceptAddresses(svcA, []string{"tcp"}, resolver, cbA)) + req.NoError(GetInterceptAddresses(svcB, []string{"tcp"}, resolver, cbB)) + + req.Len(cbA.calls, 1, "svcA must receive its InterceptAddress") + req.Len(cbB.calls, 1, "svcB must receive its InterceptAddress (the #3867 regression)") + + req.True(cbA.calls[0].IpNet().IP.Equal(cbB.calls[0].IpNet().IP), + "both services must see the same allocated IP") + req.Equal(uint16(80), cbA.calls[0].LowPort()) + req.Equal(uint16(80), cbB.calls[0].LowPort()) + req.Equal("tcp", cbA.calls[0].Proto()) + req.Equal("tcp", cbB.calls[0].Proto()) +} + +// Test_GetInterceptAddresses_SharedHostnameMultiPortProtocol checks that the +// per-service expansion of protocols x port ranges still fires correctly for +// the reuse path. With N services, M protocols, and K port ranges we expect +// each service to produce M*K InterceptAddress instances, all sharing the +// allocated IP. +func Test_GetInterceptAddresses_SharedHostnameMultiPortProtocol(t *testing.T) { + req := require.New(t) + resetDnsState(t) + + resolver := dns.NewRefCountingResolver(newFakeResolver()) + + makeSvc := func(name string) *entities.Service { + n := name + return &entities.Service{ + ServiceDetail: rest_model.ServiceDetail{ + BaseEntity: rest_model.BaseEntity{}, + Name: &n, + }, + InterceptV1Config: &entities.InterceptV1Config{ + Addresses: []string{"multi.example"}, + PortRanges: []*entities.PortRange{{Low: 80, High: 80}, {Low: 443, High: 443}}, + }, + } + } + + svcA, svcB := makeSvc("svcA"), makeSvc("svcB") + cbA, cbB := &recordingAddrCB{}, &recordingAddrCB{} + protocols := []string{"tcp", "udp"} + + req.NoError(GetInterceptAddresses(svcA, protocols, resolver, cbA)) + req.NoError(GetInterceptAddresses(svcB, protocols, resolver, cbB)) + + // 2 protocols x 2 port ranges = 4 InterceptAddress per service. + req.Len(cbA.calls, 4) + req.Len(cbB.calls, 4, "reuse path must still expand protocols x port ranges for the joining service") + + for _, c := range cbB.calls { + req.True(cbA.calls[0].IpNet().IP.Equal(c.IpNet().IP), "svcB calls must all use the shared IP") + } +}