From f8bfcfc8857243625467d929fa42b697cc497f1d Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 5 Sep 2025 10:08:00 -0400 Subject: [PATCH] Drop utiliptables.NewDualStack() The semantics (sometimes it returns an error that is really just a warning) are too confusing, and it turns out that we really only need it in one place (platformCheckSupported()); after that we've already figured out what IP families are supported, so we could just use utiliptables.NewBestEffort() instead, knowing we want exactly what it returns. So we can just expand the semantics of the old NewDualStack() inline in the one place we care, without hiding any of it behind a too-complicated return value. --- cmd/kube-proxy/app/server_linux.go | 20 +++++++++++-------- pkg/proxy/iptables/proxier.go | 2 +- pkg/proxy/ipvs/proxier.go | 2 +- pkg/util/iptables/iptables.go | 31 +++++------------------------- pkg/util/iptables/iptables_test.go | 8 +------- 5 files changed, 20 insertions(+), 43 deletions(-) diff --git a/cmd/kube-proxy/app/server_linux.go b/cmd/kube-proxy/app/server_linux.go index d355d23746e..d8ef32359c9 100644 --- a/cmd/kube-proxy/app/server_linux.go +++ b/cmd/kube-proxy/app/server_linux.go @@ -90,17 +90,21 @@ func (s *ProxyServer) platformCheckSupported(ctx context.Context) (ipv4Supported if isIPTablesBased(s.Config.Mode) { // Check for the iptables and ip6tables binaries. - ipts, errDS := utiliptables.NewDualStack() + errv4 := utiliptables.New(utiliptables.ProtocolIPv4).Present() + errv6 := utiliptables.New(utiliptables.ProtocolIPv6).Present() - ipv4Supported = ipts[v1.IPv4Protocol] != nil - ipv6Supported = ipts[v1.IPv6Protocol] != nil + ipv4Supported = errv4 == nil + ipv6Supported = errv6 == nil if !ipv4Supported && !ipv6Supported { - err = fmt.Errorf("iptables is not available on this host : %w", errDS) + // errv4 and errv6 are almost certainly the same underlying error + // ("iptables isn't installed" or "kernel modules not available") + // so it doesn't make sense to try to combine them. + err = fmt.Errorf("iptables is not available on this host : %w", errv4) } else if !ipv4Supported { - logger.Info("No iptables support for family", "ipFamily", v1.IPv4Protocol, "error", errDS) + logger.Info("No iptables support for family", "ipFamily", v1.IPv4Protocol, "error", errv4) } else if !ipv6Supported { - logger.Info("No iptables support for family", "ipFamily", v1.IPv6Protocol, "error", errDS) + logger.Info("No iptables support for family", "ipFamily", v1.IPv6Protocol, "error", errv6) } } else { // The nft CLI always supports both families. @@ -130,7 +134,7 @@ func (s *ProxyServer) createProxier(ctx context.Context, config *proxyconfigapi. if config.Mode == proxyconfigapi.ProxyModeIPTables { logger.Info("Using iptables Proxier") - ipts, _ := utiliptables.NewDualStack() + ipts := utiliptables.NewBestEffort() if dualStack { // TODO this has side effects that should only happen when Run() is invoked. @@ -184,7 +188,7 @@ func (s *ProxyServer) createProxier(ctx context.Context, config *proxyconfigapi. if err := ipvs.CanUseIPVSProxier(ctx, ipvsInterface, ipsetInterface, config.IPVS.Scheduler); err != nil { return nil, fmt.Errorf("can't use the IPVS proxier: %v", err) } - ipts, _ := utiliptables.NewDualStack() + ipts := utiliptables.NewBestEffort() logger.Info("Using ipvs Proxier") if dualStack { diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index c599c2b0015..d4f8cf13dba 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -404,7 +404,7 @@ var iptablesCleanupOnlyChains = []iptablesJumpChain{} // CleanupLeftovers removes all iptables rules and chains created by the Proxier // It returns true if an error was encountered. Errors are logged. func CleanupLeftovers(ctx context.Context) (encounteredError bool) { - ipts, _ := utiliptables.NewDualStack() + ipts := utiliptables.NewBestEffort() for _, ipt := range ipts { encounteredError = cleanupLeftoversForFamily(ctx, ipt) || encounteredError } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 8fc01b29798..aa4b4dd2c6c 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -706,7 +706,7 @@ func CleanupLeftovers(ctx context.Context) (encounteredError bool) { return false } - ipts, _ := utiliptables.NewDualStack() + ipts := utiliptables.NewBestEffort() ipsetInterface := utilipset.New() ipvsInterface := utilipvs.New() diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index dd6cd005d5b..8476c1ca78d 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -247,37 +247,17 @@ func New(protocol Protocol) Interface { return newInternal(utilexec.New(), protocol, "", "") } -func newDualStackInternal(exec utilexec.Interface) (map[v1.IPFamily]Interface, error) { - var err error +func newDualStackInternal(exec utilexec.Interface) map[v1.IPFamily]Interface { interfaces := map[v1.IPFamily]Interface{} - iptv4 := newInternal(exec, ProtocolIPv4, "", "") - if presentErr := iptv4.Present(); presentErr != nil { - err = presentErr - } else { + if presentErr := iptv4.Present(); presentErr == nil { interfaces[v1.IPv4Protocol] = iptv4 } iptv6 := newInternal(exec, ProtocolIPv6, "", "") - if presentErr := iptv6.Present(); presentErr != nil { - // If we get an error for both IPv4 and IPv6 Present() calls, it's virtually guaranteed that - // they're going to be the same error. We ignore the error for IPv6 if IPv4 has already failed. - if err == nil { - err = presentErr - } - } else { + if presentErr := iptv6.Present(); presentErr == nil { interfaces[v1.IPv6Protocol] = iptv6 } - - return interfaces, err -} - -// NewDualStack returns a map containing an IPv4 Interface (if IPv4 iptables is supported) -// and an IPv6 Interface (if IPv6 iptables is supported). If only one family is supported, -// it will return a map with one Interface *and* an error (indicating the problem with the -// other family). If neither family is supported, it will return an empty map and an -// error. -func NewDualStack() (map[v1.IPFamily]Interface, error) { - return newDualStackInternal(utilexec.New()) + return interfaces } // NewBestEffort returns a map containing an IPv4 Interface (if IPv4 iptables is @@ -286,8 +266,7 @@ func NewDualStack() (map[v1.IPFamily]Interface, error) { // simple for callers that just want "best-effort" iptables support, where neither partial // nor complete lack of iptables support is considered an error. func NewBestEffort() map[v1.IPFamily]Interface { - ipts, _ := newDualStackInternal(utilexec.New()) - return ipts + return newDualStackInternal(utilexec.New()) } // EnsureChain is part of Interface. diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index 18475526213..e98abff487a 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -329,7 +329,7 @@ func TestNewDualStack(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { fexec := fakeExecForCommands(tc.commands) - runners, err := newDualStackInternal(fexec) + runners := newDualStackInternal(fexec) if tc.ipv4 && runners[v1.IPv4Protocol] == nil { t.Errorf("Expected ipv4 runner, got nil") @@ -341,12 +341,6 @@ func TestNewDualStack(t *testing.T) { } else if !tc.ipv6 && runners[v1.IPv6Protocol] != nil { t.Errorf("Expected no ipv6 runner, got one") } - - if len(runners) == 2 && err != nil { - t.Errorf("Got 2 runners but also an error (%v)", err) - } else if len(runners) != 2 && err == nil { - t.Errorf("Got %d runners but no error", len(runners)) - } }) } }