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.

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

Fix compilation error when enabling Wireguard and XDP #25734

Merged
merged 2 commits into from Jun 22, 2023
Merged

Fix compilation error when enabling Wireguard and XDP #25734

merged 2 commits into from Jun 22, 2023

Conversation

ysksuzuki
Copy link
Member

@ysksuzuki ysksuzuki commented May 28, 2023 •
edited

This PR fixes the compilation failure of bpf_xdp.o when enabling Wireguard with XDP by adding a guard for Wireguard and XDP. Also, it shows a warning message to alert users about certain traffic(between an intermediate node and a backend node)
not being encrypted.

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #25354

@ysksuzuki ysksuzuki requested review from a team as code owners May 28, 2023 10:00
@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 28, 2023
@ysksuzuki ysksuzuki marked this pull request as draft May 28, 2023 10:01
@ysksuzuki
Copy link
Member Author

It seems that we need to exclude cilium_wg0 from the target devices when XDP acceleration is enabled

level=fatal msg="Failed to compile XDP program" error="program cil_xdp_entry: attaching XDP program to interface cilium_wg0: operation not supported" subsys=datapath-loader

@ysksuzuki ysksuzuki force-pushed the fix-wireguard-xdp branch 5 times, most recently from f26de3e to f606c6a Compare May 29, 2023 13:44
@ysksuzuki
Copy link
Member Author

/test

@ysksuzuki ysksuzuki marked this pull request as ready for review May 30, 2023 02:49
@ysksuzuki ysksuzuki requested a review from a team as a code owner May 30, 2023 02:49
@julianwiedmann julianwiedmann requested review from a team and brb and removed request for a team May 30, 2023 14:53
@julianwiedmann julianwiedmann added the area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. label May 30, 2023
gentoo-root
gentoo-root requested changes Jun 5, 2023
View reviewed changes
daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved
@brb brb mentioned this pull request Jun 6, 2023
Open
5 tasks
@brb brb marked this pull request as draft June 12, 2023 07:25
@joestringer joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 14, 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 Jun 14, 2023
@joestringer joestringer added the kind/bug This is a bug in the Cilium logic. label Jun 14, 2023
@ysksuzuki
Copy link
Member Author

/test

@ysksuzuki
Copy link
Member Author

ConformanceKindEnvoyDaemonSet seems to be stabilized by #26260

zacharysarah
zacharysarah approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@ysksuzuki Good work. Some nits for clarity, otherwise LGTM. Approving to unblock with the understanding that changes are required prior to merge.

Documentation/security/network/encryption-wireguard.rst Outdated
Comment on lines 286 to 290
N/S Loadbalancer traffic won't be encrypted when an intermediate node redirects
a request to another node where a selected backend is running,
with the following configuration:

- LoadBalancer & NodePort XDP Acceleration
- Direct Server Return (DSR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Edits for clarity, use present tense, use clear, plain language

Suggested change
N/S Loadbalancer traffic won't be encrypted when an intermediate node redirects
a request to another node where a selected backend is running,
with the following configuration:
- LoadBalancer & NodePort XDP Acceleration
- Direct Server Return (DSR)
N/S load balancer traffic isn't encrypted when an intermediate node redirects
a request to a different node with the following backend configuration:
- LoadBalancer & NodePort XDP Acceleration
- Direct Server Return (DSR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your comment! I have updated the doc accordingly.

@ysksuzuki
Copy link
Member Author

K8sUpstreamNetConformance is unstable. It has been reported here
https://cilium.slack.com/archives/C7PE7V806/p1686843215310069

@ysksuzuki
Copy link
Member Author

/test

@ysksuzuki
Copy link
Member Author

/ci-multicluster

1 similar comment
@ysksuzuki
Copy link
Member Author

/ci-multicluster

@ysksuzuki ysksuzuki marked this pull request as ready for review June 16, 2023 05:18
@ysksuzuki
Copy link
Member Author

I don't see any jenkins jobs. Do we no longer run the Jenkins jobs?

@brb
Copy link
Member

brb commented Jun 16, 2023

I don't see any jenkins jobs. Do we no longer run the Jenkins jobs?

Noup, they got moved to the new ci-ginkgo GHA 😎

brb
brb approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

@brb brb added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Jun 16, 2023
@ysksuzuki
Copy link
Member Author

Noup, they got moved to the new ci-ginkgo GHA 😎

Great! It's a tremendous boost to development productivity, isn't it?

gentoo-root
gentoo-root approved these changes Jun 16, 2023
View reviewed changes
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.

Looks good to me, as far as you reached an agreement with @brb.

gandro
gandro approved these changes Jun 19, 2023
View reviewed changes
daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved
@ysksuzuki
datapath: Fix compilation error when enabling Wireguard with XDP
53281b6 
This commit fixes the compilation failure of bpf_xdp.o
when enabling Wireguard with XDP by adding a guard for
Wireguard and XDP.

Also, it shows a warning message to alert users about
certain traffic(between an intermediate node and a backend node)
not being encrypted.

Fixes: cilium#25354

Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>
@ysksuzuki
docs: add limitation for Wireguard node-to-node encryption
edb7e8f 
Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>
@ysksuzuki
Copy link
Member Author

/test

@ysksuzuki
Copy link
Member Author

Hit #25816

@ysksuzuki
Copy link
Member Author

/ci-multicluster

@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 Jun 22, 2023
@joestringer joestringer merged commit 3fc1e87 into cilium:main Jun 22, 2023
62 checks passed
@joestringer joestringer mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@brb brb brb approved these changes

@gentoo-root gentoo-root gentoo-root approved these changes

@zacharysarah zacharysarah zacharysarah approved these changes

@rgo3 rgo3 rgo3 approved these changes

Assignees
No one assigned
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compilation failure of bpf_xdp.o when enabling Wireguard with XDP
8 participants
@ysksuzuki @brb @gandro @gentoo-root @zacharysarah @rgo3 @joestringer @julianwiedmann