Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpf: nodeport: wire up ext_err in revSNAT path #25406

Merged
merged 1 commit into from May 12, 2023

Conversation

julianwiedmann
Copy link
Member

We've recently added error handling for the snat_*_rev_nat() calls from nodeport context. Now also wire up the reporting for any returned ext_err.

We've recently added error handling for the snat_*_rev_nat() calls from
nodeport context. Now also wire up the reporting for any returned ext_err.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels May 12, 2023
@julianwiedmann julianwiedmann requested a review from a team as a code owner May 12, 2023 07:20
@julianwiedmann
Copy link
Member Author

Note that I don't expect this to get any actual use today. We really shouldn't need to create CT entries from the revSNAT path :). But it's good future-proofing.

@julianwiedmann
Copy link
Member Author

/test

Copy link
Contributor

@gentoo-root gentoo-root left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really shouldn't need to create CT entries from the revSNAT path

That makes me think: maybe we should add an error counter and increase it if we happen to call ct_create from revSNAT?

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 12, 2023
@julianwiedmann
Copy link
Member Author

We really shouldn't need to create CT entries from the revSNAT path

That makes me think: maybe we should add an error counter and increase it if we happen to call ct_create from revSNAT?

Yeah definitely good to have another look once your PR(s) are in. I'm not sure what this ct_create() would even do for us ... it's in the wrong direction, so would it still help to prevent GC for the revSNAT entry? (and why is there even a revSNAT entry, but no corresponding CT entry?!)

So at best we might even be able to just rip out the ct_create() for the revSNAT path. Which would be a nice reduction in instruction count I believe.

@julianwiedmann julianwiedmann merged commit cf3bca3 into cilium:main May 12, 2023
59 checks passed
@julianwiedmann julianwiedmann deleted the 1.14-bpf-revnat-ext-err branch May 12, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants