From e35961d051f2da55291bfc602c9d18e7bd1327e0 Mon Sep 17 00:00:00 2001 From: Dmitry Sharshakov Date: Mon, 30 Dec 2024 20:21:32 +0100 Subject: [PATCH] fix(NSC): only set rp_filter to 2 if it is 1 Setting rp_filter to 2 when it is 0 will override its status to be always enabled (in the loose mode). This behavior could break some networking solutions as it made packet admission rules more strict. --- .../proxy/network_services_controller.go | 29 +++++++--- pkg/utils/sysctl.go | 57 +++++++++++++++---- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/pkg/controllers/proxy/network_services_controller.go b/pkg/controllers/proxy/network_services_controller.go index aaf8015fc..728b62e90 100644 --- a/pkg/controllers/proxy/network_services_controller.go +++ b/pkg/controllers/proxy/network_services_controller.go @@ -225,6 +225,15 @@ type endpointSliceInfo struct { // map of all endpoints, with unique service id(namespace name, service name, port) as key type endpointSliceInfoMap map[string][]endpointSliceInfo +func checkRPFilter1(ifname string) bool { + rpFilterValue, err := utils.GetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, ifname) + if err != nil { + klog.Errorf("failed to get rp_filter value for %s: %s", ifname, err.Error()) + return false + } + return strings.TrimSpace(rpFilterValue) == "1" +} + // Run periodically sync ipvs configuration to reflect desired state of services and endpoints func (nsc *NetworkServicesController) Run(healthChan chan<- *healthcheck.ControllerHeartbeat, stopCh <-chan struct{}, wg *sync.WaitGroup) { @@ -286,16 +295,22 @@ func (nsc *NetworkServicesController) Run(healthChan chan<- *healthcheck.Control // https://github.com/kubernetes/kubernetes/pull/70530/files setSysCtlAndCheckError(utils.IPv4ConfAllArpAnnounce, arpAnnounceUseBestLocalAddress) - // Ensure rp_filter=2 for DSR capability, see: + // Ensure rp_filter=2 (or leave 0 untouched) for DSR capability, see: // * https://access.redhat.com/solutions/53031 // * https://github.com/cloudnativelabs/kube-router/pull/1651#issuecomment-2072851683 + // Only override rp_filter if it is set to 1, as enabling it from 0 to 2 can cause issues + // with some network configurations which use reverse routing if nsc.krNode.IsIPv4Capable() { - sysctlErr := utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, "all", 2) - if sysctlErr != nil { - if sysctlErr.IsFatal() { - klog.Fatal(sysctlErr.Error()) - } else { - klog.Error(sysctlErr.Error()) + for _, ifname := range []string{"all", "kube-bridge", "kube-dummy-if", nsc.krNode.GetNodeInterfaceName()} { + if checkRPFilter1(ifname) { + sysctlErr := utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, ifname, 2) + if sysctlErr != nil { + if sysctlErr.IsFatal() { + klog.Fatal(sysctlErr.Error()) + } else { + klog.Error(sysctlErr.Error()) + } + } } } } diff --git a/pkg/utils/sysctl.go b/pkg/utils/sysctl.go index f44a5c9b5..33fb6b0ae 100644 --- a/pkg/utils/sysctl.go +++ b/pkg/utils/sysctl.go @@ -29,13 +29,18 @@ type SysctlError struct { additionalInfo string err error option string + hasValue bool value int fatal bool } // Error return the error as string func (e *SysctlError) Error() string { - return fmt.Sprintf("Sysctl %s=%d : %s (%s)", e.option, e.value, e.err, e.additionalInfo) + value := "" + if e.hasValue { + value = fmt.Sprintf("=%d", e.value) + } + return fmt.Sprintf("Sysctl %s%s : %s (%s)", e.option, value, e.err, e.additionalInfo) } // IsFatal was the error fatal and reason to exit kube-router @@ -48,6 +53,39 @@ func (e *SysctlError) Unwrap() error { return e.err } +func sysctlStat(path string, hasValue bool, value int) (string, *SysctlError) { + sysctlPath := fmt.Sprintf("/proc/sys/%s", path) + if _, err := os.Stat(sysctlPath); err != nil { + if os.IsNotExist(err) { + return sysctlPath, &SysctlError{ + "option not found, Does your kernel version support this feature?", + err, path, hasValue, value, false} + } + return sysctlPath, &SysctlError{"path existed, but could not be stat'd", err, path, hasValue, value, true} + } + return sysctlPath, nil +} + +// GetSysctlSingleTemplate gets a sysctl value by first formatting the PathTemplate parameter with the substitute string +// and then getting the sysctl value and converting it into a string +func GetSysctlSingleTemplate(pathTemplate string, substitute string) (string, *SysctlError) { + actualPath := fmt.Sprintf(pathTemplate, substitute) + return GetSysctl(actualPath) +} + +// GetSysctl gets a sysctl value +func GetSysctl(path string) (string, *SysctlError) { + sysctlPath, err := sysctlStat(path, false, 0) + if err != nil { + return "", err + } + buf, readErr := os.ReadFile(sysctlPath) + if readErr != nil { + return "", &SysctlError{"path could not be read", err, path, false, 0, true} + } + return string(buf), nil +} + // SetSysctlSingleTemplate sets a sysctl value by first formatting the PathTemplate parameter with the substitute string // and then setting the sysctl to the value parameter func SetSysctlSingleTemplate(pathTemplate string, substitute string, value int) *SysctlError { @@ -57,18 +95,13 @@ func SetSysctlSingleTemplate(pathTemplate string, substitute string, value int) // SetSysctl sets a sysctl value func SetSysctl(path string, value int) *SysctlError { - sysctlPath := fmt.Sprintf("/proc/sys/%s", path) - if _, err := os.Stat(sysctlPath); err != nil { - if os.IsNotExist(err) { - return &SysctlError{ - "option not found, Does your kernel version support this feature?", - err, path, value, false} - } - return &SysctlError{"path existed, but could not be stat'd", err, path, value, true} - } - err := os.WriteFile(sysctlPath, []byte(strconv.Itoa(value)), 0640) + sysctlPath, err := sysctlStat(path, true, value) if err != nil { - return &SysctlError{"path could not be set", err, path, value, true} + return err + } + writeErr := os.WriteFile(sysctlPath, []byte(strconv.Itoa(value)), 0640) + if writeErr != nil { + return &SysctlError{"path could not be set", err, path, true, value, true} } return nil }