Skip to content

Fix source identity determination for DSR with Geneve-dispatch #29155

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
Merged
merged 1 commit into from
Nov 28, 2023
Merged

Fix source identity determination for DSR with Geneve-dispatch #29155

merged 1 commit into from
Nov 28, 2023

Conversation

chez-shanpu
Copy link
Contributor

@chez-shanpu chez-shanpu commented Nov 14, 2023 •
edited by julianwiedmann
Loading

When DSR with Geneve is enabled, Cilium identity is not determined by the client's IP address and requests from outside cluster are dropped even though they are permitted by CiliumNetworkPolicy using fromCIDR.

This PR inputs identity that is from the client IP address.

Fixes: #29153

Fix source identity determination for DSR with Geneve-dispatch, by looking it up from the ipcache.

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!

@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 Nov 14, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 14, 2023
@chez-shanpu chez-shanpu mentioned this pull request Nov 14, 2023
Closed
2 tasks
@chez-shanpu chez-shanpu force-pushed the fix-dsr-geneve-identity branch from 759caac to 237a541 Compare November 14, 2023 05:38
YutaroHayakawa
YutaroHayakawa reviewed Nov 15, 2023
View reviewed changes
@chez-shanpu chez-shanpu force-pushed the fix-dsr-geneve-identity branch from 237a541 to 58c9255 Compare November 15, 2023 03:06
@chez-shanpu chez-shanpu changed the title [WIP] Fix identity determination in bpf_overlay.c Fix identity determination in bpf_overlay.c Nov 15, 2023
@chez-shanpu chez-shanpu marked this pull request as ready for review November 15, 2023 03:06
@chez-shanpu chez-shanpu requested a review from a team as a code owner November 15, 2023 03:06
@chez-shanpu chez-shanpu force-pushed the fix-dsr-geneve-identity branch from 58c9255 to e1ae396 Compare November 15, 2023 03:10
ysksuzuki
ysksuzuki requested changes Nov 15, 2023
View reviewed changes
@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. area/loadbalancing Impacts load-balancing and Kubernetes service implementations needs-backport/1.14 labels Nov 15, 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 Nov 15, 2023
@chez-shanpu chez-shanpu force-pushed the fix-dsr-geneve-identity branch from e1ae396 to 7e64984 Compare November 17, 2023 06:42
@chez-shanpu chez-shanpu force-pushed the fix-dsr-geneve-identity branch 2 times, most recently from 6023481 to 8e4c0a2 Compare November 17, 2023 07:52
julianwiedmann
julianwiedmann reviewed Nov 17, 2023
View reviewed changes
@julianwiedmann julianwiedmann self-requested a review November 20, 2023 16:14
julianwiedmann
julianwiedmann approved these changes Nov 20, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good from my side, ty!

(please add a release note!)

@julianwiedmann julianwiedmann added the dont-merge/waiting-for-review Requires further review before merging. label Nov 20, 2023
@julianwiedmann
Copy link
Member

/test

@chez-shanpu
Copy link
Contributor Author

@julianwiedmann

Good from my side, ty!

(please add a release note!)

I added a release note to this PR description.
Could you check it?

@chez-shanpu
Copy link
Contributor Author

It seems that the connection to the k8s api-server is lost when authentication.mutual.spire.enabled is true.

@ysksuzuki
Copy link
Member

ysksuzuki commented Nov 27, 2023 •
edited
Loading

This Conformance Ginkgo failure is also relevant?
https://github.com/cilium/cilium/actions/runs/7001542415/job/19044349776

K8sDatapathServicesTest Checks N/S loadbalancing 
  Tests with direct routing and DSR
  /home/runner/work/cilium/cilium/test/ginkgo-ext/scopes.go:515

    2023-11-27T07:25:18.645895266Z level=error msg=k8sError error="github.com/cilium/cilium/pkg/k8s/resource/resource.go:725: Failed to watch *v1.Node: failed to list *v1.Node: Get \"https://172.18.0.2:6443/api/v1/nodes?fieldSelector=metadata.name%3Dkind-worker&resourceVersion=2762\": http2: client connection lost" subsys=k8s
    2023-11-27T07:25:18.646114114Z level=error msg=k8sError error="github.com/cilium/cilium/pkg/k8s/resource/resource.go:725: Failed to watch *v2.CiliumNode: failed to list *v2.CiliumNode: Get \"https://172.18.0.2:6443/apis/cilium.io/v2/ciliumnodes?fieldSelector=metadata.name%3Dkind-worker&resourceVersion=2731\": http2: client connection lost" subsys=k8s
    2023-11-27T07:25:18.646137357Z level=error msg=k8sError error="github.com/cilium/cilium/pkg/k8s/resource/resource.go:725: Failed to watch *v2.CiliumNetworkPolicy: failed to list *v2.CiliumNetworkPolicy: Get \"https://172.18.0.2:6443/apis/cilium.io/v2/ciliumnetworkpolicies?resourceVersion=2726\": http2: client connection lost" subsys=k8s
    2023-11-27T07:25:18.645379138Z level=error msg=k8sError error="github.com/cilium/cilium/pkg/k8s/resource/resource.go:725: Failed to watch *v2alpha1.CiliumCIDRGroup: failed to list *v2alpha1.CiliumCIDRGroup: Get \"https://172.18.0.2:6443/apis/cilium.io/v2alpha1/ciliumcidrgroups?resourceVersion=2720\": http2: client connection lost" subsys=k8s

ysksuzuki
ysksuzuki requested changes Nov 27, 2023
View reviewed changes
Copy link
Member

@ysksuzuki ysksuzuki left a comment

Choose a reason for hiding this comment

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

dsr should be *dsr here. I guess it causes an unexpected error and the packet drop (Invalid packet)

https://github.com/cilium/cilium/pull/29155/files#diff-304969c111195cf4253b336fc67c9891f17b91d49738a0cc334333ac11e84b9eR1365

https://github.com/cilium/cilium/pull/29155/files#diff-304969c111195cf4253b336fc67c9891f17b91d49738a0cc334333ac11e84b9eR2852

On CP node, cilium monitor shows drop logs.

xx drop (Invalid packet) flow 0x3bc1dc4f to endpoint 0, ifindex 1430, file bpf_host.c:837, , identity remote-node->unknown: 192.168.16.3:41918 -> 192.168.16.5:6443 tcp ACK
xx drop (Invalid packet) flow 0x36882803 to endpoint 0, ifindex 1430, file bpf_host.c:837, , identity remote-node->unknown: 192.168.16.4:50738 -> 192.168.16.5:6443 tcp ACK

@chez-shanpu
bpf: Fix identity determination in bpf_overlay.c
ed83d70 
When DSR with Geneve is enabled, Cilium identity is not determined by
the client's IP address and requests from outside cluster are dropped even
though they are permitted by CiliumNetworkPolicy using `fromCIDR`.

This commit inputs identity that is from the client IP address.

Fixes: cilium#29153

Signed-off-by: Tomoki Sugiura <tomoki-sugiura@cybozu.co.jp>
@chez-shanpu chez-shanpu force-pushed the fix-dsr-geneve-identity branch from c54a535 to ed83d70 Compare November 28, 2023 00:33
@ysksuzuki
Copy link
Member

/test

ysksuzuki
ysksuzuki approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@ysksuzuki ysksuzuki left a comment

Choose a reason for hiding this comment

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

All tests are green

@julianwiedmann julianwiedmann removed the dont-merge/waiting-for-review Requires further review before merging. label Nov 28, 2023
@julianwiedmann julianwiedmann added this pull request to the merge queue Nov 28, 2023
@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 Nov 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 28, 2023
@julianwiedmann julianwiedmann added this pull request to the merge queue Nov 28, 2023
Merged via the queue into cilium:main with commit 895630b Nov 28, 2023
@chez-shanpu chez-shanpu deleted the fix-dsr-geneve-identity branch November 29, 2023 00:06
@joamaki joamaki mentioned this pull request Nov 29, 2023
Merged
14 tasks
@joamaki joamaki added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. needs-backport/1.14 and removed needs-backport/1.14 backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Nov 29, 2023
@julianwiedmann julianwiedmann added backport/author The backport will be carried out by the author of the PR. backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 labels Nov 30, 2023
@julianwiedmann julianwiedmann mentioned this pull request Dec 7, 2023
Merged
@ysksuzuki ysksuzuki 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 Dec 11, 2023
@julianwiedmann
Copy link
Member

FYI - I just stumbled over

cilium/bpf/lib/nodeport.h

Line 2315 in fa00376

ct_state_new.src_sec_id = WORLD_IPV4_ID;
. That's probably just copy & pasted (I wouldn't know who uses it), but we are obviously not putting the right data there at the moment.

For the TC program it would be easy to fix (we already resolved the src-sec-identity). XDP and DSR-Geneve would need a bit more work. But maybe it's easiest to just remove that code 😁 .

@chez-shanpu
Copy link
Contributor Author

@julianwiedmann

But maybe it's easiest to just remove that code 😁 .

Is it because the src_sec_id in CT is not used?

@squeed squeed mentioned this pull request Jul 25, 2024
Closed
3 tasks
@julianwiedmann julianwiedmann mentioned this pull request Jan 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YutaroHayakawa YutaroHayakawa YutaroHayakawa left review comments

@ysksuzuki ysksuzuki ysksuzuki approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/loadbalancing Impacts load-balancing and Kubernetes service implementations backport/author The backport will be carried out by the author of the PR. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. 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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cilium Identity cannot be specified correctly when Geneve DSR is enabled
5 participants
@chez-shanpu @julianwiedmann @YutaroHayakawa @ysksuzuki @joamaki