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

Fix IPv6 cilium_host address assignment #28633

Merged
merged 1 commit into from Oct 17, 2023

Conversation

CallMeFoxie
Copy link
Contributor

@CallMeFoxie CallMeFoxie commented Oct 17, 2023

Fixes #28327

This patch stops adding /64 route to the cilium_net iface, which brought all the traffic to local node. When using something like /128 per node and having pods within the same /64 as the node meant that no cross-node traffic from pods worked.

Don't bind a /64 address to cilium_host to avoid misrouting cross-node traffic

@CallMeFoxie CallMeFoxie requested a review from a team as a code owner October 17, 2023 06:37
@CallMeFoxie CallMeFoxie requested a review from rgo3 October 17, 2023 06:37
@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 Oct 17, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 17, 2023
@CallMeFoxie
Copy link
Contributor Author

This time against main and with better description I hope. I need somebody who uses tunelling to test this thoguh, we use eBGP with /120 per node and pod subnet being within the same /64.

@rgo3
Copy link
Contributor

rgo3 commented Oct 17, 2023

Nit: Could you change the commit message to something like:

This patch fixes the subnet assigned to the cilium_host device from /64 back to
/128, a bug introduced in #ecd4fc0adf9840e1a08786610a0e705cb3d73fb9 when
converting the init.sh script to Go code. The conversion never meant to change
the behavior in init.sh and the bug caused connectivity issues in certain cilium
configurations.

Fixes: #28327

Signed-off-by: Ashley Reese <ashley@victorianfox.com>

That way we reference my commit that introduced the bug and explain that this happened when moving away from init.sh.

@rgo3 rgo3 added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Oct 17, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.3 Oct 17, 2023
@rgo3
Copy link
Contributor

rgo3 commented Oct 17, 2023

Is it not auto-linking the bug commit because of the # in front of the SHA? I guess you only need that for issues 🤦 Sorry for being so nit picky here, but the whole idea of the proposed commit message was to be able to click through to the bad commit.

@rgo3 rgo3 changed the title Fix IPv6 route steering traffic Fix IPv6 cilium_host address assignment Oct 17, 2023
Fixes cilium#28327

This patch fixes the subnet assigned to the cilium_host device from /64 back to
/128, a bug introduced in ecd4fc0 when
converting the init.sh script to Go code. The conversion never meant to change
the behavior in init.sh and the bug caused connectivity issues in certain cilium
configurations.

Signed-off-by: Ashley Reese <ashley@victorianfox.com>
@CallMeFoxie
Copy link
Contributor Author

better :)

@rgo3
Copy link
Contributor

rgo3 commented Oct 17, 2023

Thank you! 🙏

@rgo3
Copy link
Contributor

rgo3 commented Oct 17, 2023

/test

@ti-mo ti-mo added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Oct 17, 2023
@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 Oct 17, 2023
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks!

@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 Oct 17, 2023
@joestringer joestringer merged commit 9f808d4 into cilium:main Oct 17, 2023
61 of 62 checks passed
@jrajahalme jrajahalme added this to Needs backport from main in 1.14.4 Oct 18, 2023
@jrajahalme jrajahalme removed this from Needs backport from main in 1.14.3 Oct 18, 2023
@tklauser tklauser added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 24, 2023
@tklauser tklauser mentioned this pull request Oct 24, 2023
14 tasks
@tklauser tklauser added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Oct 25, 2023
@thorn3r thorn3r moved this from Needs backport from main to Backport done to v1.14 in 1.14.4 Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.14.4
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

IPv6 not working between nodes because of extra route
5 participants