Skip to content

bpf: remove cilium_host ip addr scope link#21738

Closed
liuxu623 wants to merge 1 commit intocilium:masterfrom
liuxu623:master
Closed

bpf: remove cilium_host ip addr scope link#21738
liuxu623 wants to merge 1 commit intocilium:masterfrom
liuxu623:master

Conversation

@liuxu623
Copy link
Copy Markdown
Contributor

@liuxu623 liuxu623 commented Oct 14, 2022

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
Copy Markdown
Contributor Author

liuxu623 commented Oct 17, 2022

@borkmann Hi, could you take a look?

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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

pchaigno commented Jan 23, 2023

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