fix(hairpin): set hairpin_mode for veth iface #1582
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It used to be that the kubelet handled setting hairpin mode for us: kubernetes/kubernetes#13628 Then this functionality moved to the dockershim: kubernetes/kubernetes#62212 Then the functionality was removed entirely: kubernetes/kubernetes@83265c9171f Unfortunately, it was lost that we ever depended on this in order for our hairpin implementation to work, if we ever knew it at all. Additionally, I suspect that containerd and cri-o implementations never worked correctly with hairpinning. Without this, the NAT rules that we implement for hairpinning don't work correctly. Because hairpin_mode isn't implemented on the virtual interface of the container on the host, the packet bubbles up to the kube-bridge. At some point in the traffic flow, the route back to the pod gets resolved to the mac address inside the container, at that point, the packet's source mac and destination mac don't match the kube-bridge interface and the packet is black-holed. This can also be fixed by putting the kube-bridge interface into promiscuous mode so that it accepts all mac addresses, but I think that going back to the original functionality of enabling hairpin_mode on the veth interface of the container is likely the lesser of two evils here as putting the kube-bridge interface into promiscuous mode will likely have unintentional consequences.
szuecs
reviewed
Dec 7, 2023
| // ugly workaround, refactoring of pkg/Proxy is required | ||
| pid, err = hpc.nsc.ln.getContainerPidWithCRI(hpc.nsc.dsr.runtimeEndpoint, containerID) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to prepare endpoint %s to do DSR due to: %v", endpointIP, err) |
There was a problem hiding this comment.
you should use fmt.Errorf("...: %w", ..., err) instead of %v for error type propagation that you can later check with errors.Is or convert with errors.As
There was a problem hiding this comment.
Yeah, probably at some point we're going to have to convert the entire code base. For now I've just been continuing on the %v wagon to stay consistent within the code base. So far we haven't really needed the functionality to look at error types or wrapped errors, so it has worked, but we should probably upgrade anyway.
aauren
added a commit
to aauren/kube-router
that referenced
this pull request
Dec 8, 2023
This is needed because cloudnativelabs#1582 which was recently merged relies upon finding the correct veth interface via /proc/<pid> which isn't available unless kube-router is in the same process namespace. hostPID and hostIPC was always required for DSR functionality, but now hostPID is needed for hairpin to be available.
aauren
added a commit
that referenced
this pull request
Dec 8, 2023
This is needed because #1582 which was recently merged relies upon finding the correct veth interface via /proc/<pid> which isn't available unless kube-router is in the same process namespace. hostPID and hostIPC was always required for DSR functionality, but now hostPID is needed for hairpin to be available.
|
Progress towards: #1596 |
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It used to be that the kubelet handled setting hairpin mode for us: kubernetes/kubernetes#13628
Then this functionality moved to the dockershim:
kubernetes/kubernetes#62212
Then the functionality was removed entirely:
kubernetes/kubernetes@83265c9171f
Unfortunately, it was lost that we ever depended on this in order for our hairpin implementation to work, if we ever knew it at all. Additionally, I suspect that containerd and cri-o implementations never worked correctly with hairpinning.
Without this, the NAT rules that we implement for hairpinning don't work correctly. Because hairpin_mode isn't implemented on the virtual interface of the container on the host, the packet bubbles up to the kube-bridge. At some point in the traffic flow, the route back to the pod gets resolved to the mac address inside the container, at that point, the packet's source mac and destination mac don't match the kube-bridge interface and the packet is black-holed.
This can also be fixed by putting the kube-bridge interface into promiscuous mode so that it accepts all mac addresses, but I think that going back to the original functionality of enabling hairpin_mode on the veth interface of the container is likely the lesser of two evils here as putting the kube-bridge interface into promiscuous mode will likely have unintentional consequences.
With this fix, kube-router now passes the Kubernetes end-to-end conformance test for hairpinning traffic.