Skip to content

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Dec 14, 2022

When forwarding external service requests to a remote backend in
hostNetwork, the NodePort code in tail_nodeport_nat_egress_ipv4() doesn't
redirect into the tunnel (as it's effectively a host-to-host connection).

Therefore it uses IPV4_DIRECT_ROUTING as src address. So if we're not
populating IPV4_DIRECT_ROUTING, such forwarded requests end up being SNATed
with 0.0.0.0.

Fixes: #22557
Fixes: 5283ec9 ("datapath: Introduce DirectRoutingDeviceRequired helper")
Signed-off-by: Julian Wiedmann jwi@isovalent.com

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 14, 2022
@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Dec 14, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 14, 2022
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann force-pushed the nodeport-remote-hostns-tunnel branch from 0b6e77f to 7f165a7 Compare December 15, 2022 08:20
@julianwiedmann
Copy link
Member Author

/test-runtime

@julianwiedmann
Copy link
Member Author

/test-runtime

@julianwiedmann julianwiedmann force-pushed the nodeport-remote-hostns-tunnel branch from c0010a5 to 6cccf16 Compare December 15, 2022 11:50
@julianwiedmann
Copy link
Member Author

/test-runtime

@github-actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 15, 2023
@github-actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Jan 29, 2023
@michaelasp
Copy link
Contributor

Hey @julianwiedmann, is this change still valid? This issue has been affecting us for a while now, so just wondering if we can reopen this. Thanks!

When forwarding external service requests to a remote backend in
hostNetwork, the NodePort code in tail_nodeport_nat_egress_ipv4() doesn't
redirect into the tunnel (as it's effectively a host-to-host connection).

Therefore it uses IPV4_DIRECT_ROUTING as src address. So if we're not
populating IPV4_DIRECT_ROUTING, such forwarded requests end up being SNATed
with 0.0.0.0.

Fixes: cilium#22557
Fixes: 5283ec9 ("datapath: Introduce DirectRoutingDeviceRequired helper")
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann force-pushed the nodeport-remote-hostns-tunnel branch from 66c288b to 76837ea Compare March 20, 2023 15:20
@julianwiedmann
Copy link
Member Author

Hey @julianwiedmann, is this change still valid? This issue has been affecting us for a while now, so just wondering if we can reopen this. Thanks!

Heya @michaelasp - yeah sorry, lost track of this :/. IIRC the change was hitting troubles in the agent tests (as it was now expecting the direct-routing device to be defined in a lot more tests). Discussed with @joamaki what would need to change there, but never got to it... want to give it a shot?

I actually just hit the same problem in a different PR (your timing is spot-on 😄), so hoping we can get this fixed 👍 .

@julianwiedmann julianwiedmann force-pushed the nodeport-remote-hostns-tunnel branch from 6cccf16 to 66c288b Compare March 20, 2023 15:24
@julianwiedmann
Copy link
Member Author

/test-runtime

@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Mar 21, 2023
@shen-cloud
Copy link

Hi @julianwiedmann, do you have any updates on this commit? Looks like the unit test is failing.

@julianwiedmann
Copy link
Member Author

Hi @julianwiedmann, do you have any updates on this commit? Looks like the unit test is failing.

Yes, that's the fallout in agent tests I mentioned in my previous comment.

@github-actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label May 19, 2023
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Jun 2, 2023
@qmonnet
Copy link
Member

qmonnet commented Jun 9, 2023

Removing backport labels from closed PR, please re-add if you reopen this.

@michaelasp
Copy link
Contributor

Hey @julianwiedmann, with the pr being closed, I assume the issue is still present? I remember that due to test issues it was sidelined. I'd be more than happy to take on the work of trying to fix this if necessary :)

@julianwiedmann
Copy link
Member Author

Hey @julianwiedmann, with the pr being closed, I assume the issue is still present? I remember that due to test issues it was sidelined. I'd be more than happy to take on the work of trying to fix this if necessary :)

That would be great @michaelasp ! I honestly don't know at what point I would find the cycles to get this wrapped up :/.

@michaelasp
Copy link
Contributor

Sounds good! I'll try and get a PR out in the next couple weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't connect to remote NodePort backend in hostNetwork (with tunnel enabled)
4 participants