Skip to content

bpf,nodeport: Fix broken nodeport_nat_egress_ipv4_hook #39418

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

Merged
merged 2 commits into from
May 12, 2025

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented May 8, 2025

Since 3c4693f, the nodeport_nat_egress_ipv4_hook is broken because it passes the undefined variable in the non-tunnel mode. This PR fixes it and adds an extra validation for the input values for preventing the future bug. If we cannot agree with the prevention, I can drop the second commit.

Fixes: #39074

bpf,nodeport: Fix broken nodeport nat egress hook

Currently, the nodeport_nat_egress_ipv4_hook in the
tail_nodeport_nat_egress_ipv4 is broken in non-tunnel mode because info
variable is not declared. Revert the changes made in the 3c4693f to make
it work again.

Fixes: #39074

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@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 May 8, 2025
@YutaroHayakawa YutaroHayakawa added the release-note/misc This PR makes changes that have no direct user impact. label May 8, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 8, 2025
@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/fix-nodeport_nat_egress_hook branch from a939356 to 9801fcf Compare May 8, 2025 04:43
nodeport_nat_egress_ipv4_hook and nodeport_rev_dnat_ipv4_hook is defined
as a macro and we substitute it to the constant value. This kills the
compile-time check for the input value entirely. The input value is not
type-checked or we can even use the undeclared variable like the bug in
the previous commit. To avoid such a mistake. Evaluate the input value
properly. The following is an example for detecting the error in the
previous commit.

```
./lib/nodeport.h:2488:48: error: use of undeclared identifier 'info'
 2488 |         ret = nodeport_nat_egress_ipv4_hook(ctx, ip4, info->sec_identity, &tuple, l4_off, &ext_err);
```

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/fix-nodeport_nat_egress_hook branch from 9801fcf to 3e34931 Compare May 8, 2025 04:46
@YutaroHayakawa
Copy link
Member Author

/test

@YutaroHayakawa YutaroHayakawa changed the title bpf,nodeport: Fix broken nodeport nat egress hook bpf,nodeport: Fix broken nodeport_nat_egress_ipv4_hook May 8, 2025
@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review May 8, 2025 04:56
@YutaroHayakawa YutaroHayakawa requested a review from a team as a code owner May 8, 2025 04:56
@YutaroHayakawa YutaroHayakawa requested a review from aditighag May 8, 2025 04:56
@aditighag
Copy link
Member

Seems reasonable to me, but let's wait for Paul to take a look in case the revert would expose some other issue. /cc @pchaigno

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Nice catch :)

@pchaigno pchaigno added this pull request to the merge queue May 12, 2025
Merged via the queue into main with commit 4687fbd May 12, 2025
300 of 303 checks passed
@pchaigno pchaigno deleted the yutaro/fix-nodeport_nat_egress_hook branch May 12, 2025 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants