Enable sloppy_tcp when DSR and Maglev is enabled #1888
Conversation
There was a problem hiding this comment.
Thanks @AnupamGhosh!
Overall looks really good! I added just a few small requests.
| sysctlErr := sloppyTCP.WriteVal(1) | ||
| if sysctlErr != nil { | ||
| klog.Errorf("Failed to enable IPVS sloppy TCP: %s", sysctlErr.Error()) | ||
| } else { |
There was a problem hiding this comment.
Can you simplify this just a bit by putting a return after the klog.Errorf above and removing the else statement?
| } else { | ||
| klog.Infof("IPVS sloppy TCP enabled") | ||
| } | ||
| } |
There was a problem hiding this comment.
We should handle disabling Sloppy TCP in the case that it is enabled and there are no DSR services with maglev hashing found.
Can you add an else if here to handle !enableSloppyTCP && sloppyTCP.CachedVal() == 1? (I think that one of the linters in this project may ask you to change the if ... else if to a switch statement when making this change)
There was a problem hiding this comment.
I was thinking about the scenario when an user had already set sloppy_tcp to 1 and if we disable sloppy_tcp then we are going against the user even though kube-router can still function with sloppy_tcp being set.
There was a problem hiding this comment.
I think that its a pretty obscure option, so we're probably safe here. And since it potentially makes services less secure, if we set it, then we should unset it if its not needed.
I see what you're saying, but I think that we can probably just explain this a bit by adding something to the documentation saying that we expect to control this option when the service controller is enabled.
There was a problem hiding this comment.
okay.
I have updated the code to disable sloppy_tcp if there is no service with DSR or Maglev enabled.
| @@ -241,6 +251,10 @@ func NewKRNode(node *apiv1.Node, linkQ LocalLinkQuerier, enableIPv4, enableIPv6 | |||
| }, | |||
| linkQ: linkQ, | |||
| NodeInterfaceName: nodeInterfaceName, | |||
| sloppyTCP: SysctlConfig{ | |||
| name: IPv4IPVSSloppyTCP, | |||
| value: 0, | |||
There was a problem hiding this comment.
At first I was sceptical about hard-coding this this value, but I think that it makes sense, especially when handling the reverse condition that I asked for above. Essentially assuming that this is 0 from a kube-router standpoint, means that if the user has set this value for whatever reason manually, we won't revert it above unless we specifically set it to 1. Which means that we have less of a chance of reverting manual changes to the system that the admin may have made.
Do you mind writing something like that in a comment above this setting so that we remember that we purposefully didn't look up the live setting with GetSysctl()?
Fixes #1860