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.

Author: dsseng

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())
+					}
+				}
 			}
 		}
 	}

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
 }