fix(NSC): only set rp_filter to 2 if it is 1

[dsseng]

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.

Reference: #1651 (comment) and subsequent

[dsseng]

Also this doesn't work in all cases as I see, since this won't disable rp_filter on these in case all is set to 1 or 2:

kube-router/pkg/controllers/proxy/linux_networking.go

Lines 769 to 787 in 93498fb

	// disable rp_filter on all interface
	sysctlErr := utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, "kube-tunnel-if", 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on kube-tunnel-if in the endpoint container: %s",
	sysctlErr.Error())
	}
	sysctlErr = utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, assumedContainerIfaceName, 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on eth0 in the endpoint container: %s", sysctlErr.Error())
	}
	sysctlErr = utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, "all", 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on `all` in the endpoint container: %s", sysctlErr.Error())
	}

[aauren]

There's one linting error that needs correcting, and I left a request to add one more interface that I missed in our previous conversation, and I had one question.

I also deployed it to my test harness and while I haven't tested all of the boundary conditions for rp_filter values, I was able to confirm that it runs similar to how it ran before.

But other than that, this change looks really good! Thanks for putting this together for us!

[aauren]

Also this doesn't work in all cases as I see, since this won't disable rp_filter on these in case all is set to 1 or 2:

kube-router/pkg/controllers/proxy/linux_networking.go

Lines 769 to 787 in 93498fb

	// disable rp_filter on all interface
	sysctlErr := utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, "kube-tunnel-if", 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on kube-tunnel-if in the endpoint container: %s",
	sysctlErr.Error())
	}
	sysctlErr = utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, assumedContainerIfaceName, 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on eth0 in the endpoint container: %s", sysctlErr.Error())
	}
	sysctlErr = utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, "all", 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on `all` in the endpoint container: %s", sysctlErr.Error())
	}

I'm not totally sure if I understand this comment correctly, doesn't the last case set rp_filter to 0 on the all interface and 0 is more permissive than 1 or 2?

Also, just to be clear, this logic is setting that value for the interfaces found inside the container's network namespace in case that helps clarify anything for you.

[dsseng]

Also this doesn't work in all cases as I see, since this won't disable rp_filter on these in case all is set to 1 or 2:

kube-router/pkg/controllers/proxy/linux_networking.go

Lines 769 to 787 in 93498fb

	// disable rp_filter on all interface
	sysctlErr := utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, "kube-tunnel-if", 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on kube-tunnel-if in the endpoint container: %s",
	sysctlErr.Error())
	}
	sysctlErr = utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, assumedContainerIfaceName, 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on eth0 in the endpoint container: %s", sysctlErr.Error())
	}
	sysctlErr = utils.SetSysctlSingleTemplate(utils.IPv4ConfRPFilterTemplate, "all", 0)
	if sysctlErr != nil {
	attemptNamespaceResetAfterError(hostNetworkNamespaceHandle)
	return fmt.Errorf("failed to disable rp_filter on `all` in the endpoint container: %s", sysctlErr.Error())
	}

I'm not totally sure if I understand this comment correctly, doesn't the last case set rp_filter to 0 on the all interface and 0 is more permissive than 1 or 2?

Also, just to be clear, this logic is setting that value for the interfaces found inside the container's network namespace in case that helps clarify anything for you.

Okay, if it's inside the container that's not to be considered. Thanks for clarifying

[aauren]

Thanks for the update! 2 more comments, but I think we're just about at the end here.

[aauren]

LGTM!

Thanks so much for your contribution @dsseng!