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

[v1.14] bpf: Fix identity determination in bpf_overlay.c #29606

Merged
merged 7 commits into from
Dec 11, 2023

Conversation

ysksuzuki
Copy link
Member

@ysksuzuki ysksuzuki commented Dec 4, 2023

This PR is the manual backport of #29155 for v1.14.

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: #29153

Also, this PR backports the following PRs to reduce the divergence from the main to make it easier to backport later changes.

@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Dec 4, 2023
@julianwiedmann
Copy link
Member

/test-backport-1.14

@ysksuzuki
Copy link
Member Author

ysksuzuki commented Dec 5, 2023

Need #29553 to run BPF complexity tests

@ysksuzuki
Copy link
Member Author

ysksuzuki commented Dec 5, 2023

There is a verifier complexity issue on kernel 6.1 with my branch. e3367b9 fixed it. (It doesn't occur with v1.14 branch with the same config)

level=fatal msg="Failed to compile XDP program" error="loading eBPF collection into the kernel: program tail_handle_snat_fwd_ipv6: load program: argument list too long: BPF program is too large. Processed 1000001 insn (1143 line(s) omitted)" subsys=datapath-loader

# Config
cilium install --wait \
    --chart-directory=/home/yusuke/go/src/github.com/cilium/cilium/install/kubernetes/cilium \
    --helm-set=debug.enabled=true \
    --helm-set=debug.verbose=envoy \
    --helm-set=hubble.eventBufferCapacity=65535 \
    --helm-set=bpf.monitorAggregation=none \
    --helm-set=cluster.name=default \
    --helm-set=authentication.mutual.spire.enabled=false \
    --nodes-without-cilium=kind-worker3 \
    --helm-set-string=kubeProxyReplacement=true \
    --helm-set=image.repository=localhost:5000/cilium/cilium-dev \
    --helm-set=image.useDigest=false \
    --helm-set=image.tag=local \
    --helm-set=image.pullPolicy=Never \
    --helm-set=operator.image.repository=localhost:5000/cilium/operator \
    --helm-set=operator.image.suffix= \
    --helm-set=operator.image.tag=local \
    --helm-set=operator.image.useDigest=false \
    --helm-set=operator.image.pullPolicy=Never \
    --helm-set-string=routingMode=native \
    --helm-set-string=tunnelProtocol=geneve \
    --helm-set-string=autoDirectNodeRoutes=true \
    --helm-set-string=ipv4NativeRoutingCIDR=10.244.0.0/16 \
    --helm-set-string=ipv6NativeRoutingCIDR=fd00:10:244::/56 \
    --helm-set=devices='eth0' \
    --helm-set-string=loadBalancer.mode=dsr \
    --helm-set-string=loadBalancer.dsrDispatch=geneve \
    --helm-set-string=endpointRoutes.enabled=false \
    --helm-set=ipv6.enabled=true \
    --helm-set=bpf.masquerade=true \
    --helm-set=egressGateway.enabled=true \
    --helm-set=loadBalancer.acceleration=testing-only 

@ysksuzuki
Copy link
Member Author

/ci-verifier

@ysksuzuki
Copy link
Member Author

The BPF complexity test is green. It seems that the issue only occurs with kernel 6.1
https://github.com/cilium/cilium/actions/runs/7101950048/job/19331222570

@julianwiedmann
Copy link
Member

/test-backport-1.14

Copy link
Member

@julianwiedmann julianwiedmann 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, and bringing the additional CT changes doesn't feel too invasive. Thank you!

@ysksuzuki ysksuzuki marked this pull request as ready for review December 7, 2023 06:02
@ysksuzuki ysksuzuki requested a review from a team as a code owner December 7, 2023 06:02
@ti-mo
Copy link
Contributor

ti-mo commented Dec 7, 2023

@ysksuzuki Looks like the commit messages are missing upstream commit references, see https://docs.cilium.io/en/latest/contributing/release/backports/#codecell6 for an example. If you're not using the backporting scripts/tools, you have to add them in by yourself. 🙂

@ti-mo ti-mo changed the title [v1.14] [manual backport] bpf: Fix identity determination in bpf_overlay.c [v1.14] bpf: Fix identity determination in bpf_overlay.c Dec 7, 2023
@ysksuzuki ysksuzuki marked this pull request as draft December 7, 2023 23:04
julianwiedmann and others added 7 commits December 11, 2023 10:09
[ upstream commit 124d6f6 ]

CT lookups are typically done in reply direction first, and potentially
in forward direction afterwards. tuple->flags is selected accordingly,
and then swapped inbetween the two lookups.

But with SCOPE_FORWARD we perform just one lookup in the forward direction.
Currently the relevant code still expects a "reverse" tuple, and then swaps
it into forward layout. Here we can clean things up a bit by having the
caller build a "forward" tuple, and feeding that into the CT lookup code.

Start by selecting the actual tuple type (TUPLE_F_*) from the start (and
while at it also de-dup the existing code for this). We'll deal with
swapping the addrs/ports in a subsequent patch.

Suggested-by: Maxim Mikityanskiy <maxim@isovalent.com>
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
[ upstream commit 2495587 ]

Complete the refactor of the SCOPE_FORWARD path, by doing the needed
addr/port swap in the callers.

Suggested-by: Maxim Mikityanskiy <maxim@isovalent.com>
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
[ upstream commit d5ee4e7 ]

We're still using all variants of the tuple-swap helpers in some parts of
the code base. But let's at least re-use the intermediate helpers to
compose the bigger ones.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
[ upstream commit 5c39551 ]

For a CT lookup, the calling paths currently need to determine a
`ct_action` parameter. This is typically ACTION_CREATE (for TCP/UDP/SCTP),
or ACTION_UNSPEC for most types of ICMP traffic.

But ever since fd5ea2a ("bpf: Don't reset TCP timer on final ACK"),
__ct_lookup() will only apply ACTION_CREATE handling to TCP packets with
the SYN flag set. Otherwise it has the same effect as ACTION_UNSPEC.

Simplify the logic by ignoring the passed-in value, and manually selecting
ACTION_CREATE for applicable traffic in the CT lookup helpers (same as we
already do for ACTION_CLOSE). A subsequent patch will remove all the unused
code that passed the ct_action around.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
[ upstream commit 07c05fe ]

__ct_lookup*() now selects this value internally, and the caller's
parameter is ignored. Remove all the calling logic that determined the
ct_action and plumbed it into the CT lookup.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
[ upstream commit c00d313 ]

The DSR Ingress path was initially implemented through a tail-call for
program size reasons. But with the recent improvements to the CT handling,
we have sufficient program space to run the DSR ingress handling straight
from nodeport_lb*().

This reduces duplicated code (eg. extracting the CT tuple and DSR info
twice), avoids additional error handling, and simplifies the overall code
flow.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
[ upstream commit 895630b ]

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>
@ysksuzuki
Copy link
Member Author

/test-backport-1.14

@ysksuzuki ysksuzuki marked this pull request as ready for review December 11, 2023 04:38
@ysksuzuki
Copy link
Member Author

@ti-mo I have added upstream commit references. PTAL. Thanks!

@aanm aanm merged commit 06a9f14 into cilium:v1.14 Dec 11, 2023
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants