Skip to content

bpf: remove cilium_host ip addr scope link #21738

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

bpf: remove cilium_host ip addr scope link #21738

wants to merge 1 commit into from

Conversation

liuxu623
Copy link
Contributor

make iptables masquerade choose cilium_host ip addr when access service external ip (backend is a pod in another node) in a pod

Fixes: #21737

@liuxu623 liuxu623 requested a review from a team as a code owner October 14, 2022 11:21
@liuxu623 liuxu623 requested a review from borkmann October 14, 2022 11:21
@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 14, 2022
@liuxu623
bpf: remove cilium_host ip addr scope link
c440d82 
make iptables masquerade choose cilium_host ip addr
when access service external ip (backend is a pod in another node) in a pod

Signed-off-by: liuxu <liuxu623@gmail.com>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 14, 2022
@liuxu623
Copy link
Contributor Author

@borkmann Hi, could you take a look?

pchaigno
pchaigno requested changes Oct 25, 2022
View reviewed changes
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.

Could you explain in the commit description why you are making this change? Is it fixing a bug and, if so, what is the bug?

@pchaigno pchaigno added the area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Oct 25, 2022
@liuxu623
Copy link
Contributor Author

liuxu623 commented Oct 25, 2022 •
edited
Loading

Could you explain in the commit description why you are making this change? Is it fixing a bug and, if so, what is the bug?

The bug is when access service external ip / node port in a pod, and backend is a pod in another node, reply package will not through cilium_vxlan, it maybe drop by network device like router, you can see #21737 more details.

@github-actions

This comment was marked as off-topic.

Sign in to view
@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 Nov 25, 2022
@pchaigno pchaigno removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 25, 2022
@aanm aanm added the kind/community-contribution This was a contribution made by a community member. label Dec 1, 2022
@sathieu
Copy link
Contributor

sathieu commented Dec 2, 2022

I confirm the bug and fix.

We use metallb in ARP mode, and access from a pod in the cluster to a loadbalancer IP only works from the node elected as leader for the given IP. Once the scope of cilium_host IP is set to global, the issue is gone.

@github-actions
Copy link

github-actions bot commented Jan 2, 2023

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 2, 2023
@sathieu
Copy link
Contributor

sathieu commented Jan 3, 2023

This PR is not stale.

The "link" scope was added 4 years ago by 2d099f4 (#1614).

@tgraf @aanm @aalemayhu Any input on this PR?

@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 Jan 4, 2023
@aanm aanm requested a review from pchaigno January 10, 2023 10:53
pchaigno
pchaigno requested changes Jan 17, 2023
View reviewed changes
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.

My above review is still valid.

@pchaigno
Copy link
Member

I hit this same bug in a different context, so I resubmitted this as #23241, with a proper commit description.

@pchaigno pchaigno closed this Jan 23, 2023
@pchaigno pchaigno mentioned this pull request Jan 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno requested changes

@borkmann borkmann Awaiting requested review from borkmann borkmann was automatically assigned from cilium/loader

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. kind/community-contribution This was a contribution made by a community member.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failed access service external ip in pod
4 participants
@liuxu623 @sathieu @pchaigno @aanm