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.13 Backports 2023-11-29 #29475

Merged
merged 5 commits into from
Dec 1, 2023
Merged

v1.13 Backports 2023-11-29 #29475

merged 5 commits into from
Dec 1, 2023

Conversation

@joamaki joamaki added kind/backports This PR provides functionality previously merged into master. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. labels Nov 29, 2023
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

My commit looks good, thanks!

bpf/lib/nodeport.h Outdated Show resolved Hide resolved
@joamaki joamaki marked this pull request as ready for review November 29, 2023 14:37
@joamaki joamaki requested review from a team as code owners November 29, 2023 14:37
julianwiedmann and others added 5 commits November 30, 2023 12:05
[ upstream commit 2393707 ]

With eff26cf ("datapath: Fix double SNAT") the outbound SNAT path now
sets ctx_snat_done_set() after checking whether a packet requires SNAT.
This was meant to avoid double-NATing when a packet passes through multiple
network interfaces with a to-netdev program.

But looking at the SNAT code in detail, some of its conditions only apply
on specific interfaces (see nodeport_has_nat_conflict_ipv4(), which
checks for `NATIVE_DEV_IFINDEX == DIRECT_ROUTING_DEV_IFINDEX`). So if a
packet passes through multiple interfaces which all have `to-netdev`
attached, the first `to-netdev` program might set SNAT_DONE even when some
subsequent program (attached to DIRECT_ROUTING_DEV_IFINDEX) would still
want to apply SNAT to the packet.

Therefore we should apply the SNAT checks on *each* interface, until we
have actually SNATed the packet. Only then set the SNAT_DONE marker.

Note that this also fixes an EgressGW bug in nodeport_snat_fwd_ipv4(),
where we would redirect the packet even if snat_v4_nat() reported an error.

Fixes: eff26cf ("datapath: Fix double SNAT")
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
[ upstream commit f7a58af ]

The SPI parsing logic is fairly complex so let's move it to its own
function and write a unit test for that.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
[ upstream commit ba3fa89 ]

ipSecJoinState is never called without ipSecNewState and vice versa. So
let's just merge both to have all XFRM state initialization in the same
place.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
[ upstream commit a587b88 ]

Currently, we only wait for the images from the current PR/main to be
available in the clustermesh upgrade tests. Yet, it can happen that the
ones from the stable branch are not available (either because they are
being built, or something went wrong), leading to confusing failures
(as the installation eventually fails due to ImagePullBackOff errors).

To prevent this, let's add an explicit step to additionally wait for
downgrade images to be available before proceeding with the installation
step, so that also the error message is clear.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
[ upstream commit a6e22ba ]

To detect that the key rotation began or that it successfully ended, we
rely on the number of keys in use reported by
`cilium-dbg encrypt status`. When either of those steps times out, it
would be good to have information on what the number of keys was.

This commit adds that debug information to the test.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
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 PRs look good. Thank you!

@joamaki
Copy link
Contributor Author

joamaki commented Nov 30, 2023

/test-backport-1.13

Job 'Cilium-PR-K8s-1.21-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) 

Failure Output

FAIL: Request from k8s1 to service tftp://[fd04::12]:30805/hello failed

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.21-kernel-4.19/279/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

@joamaki
Copy link
Contributor Author

joamaki commented Dec 1, 2023

/test-1.16-4.19

@joamaki
Copy link
Contributor Author

joamaki commented Dec 1, 2023

/test-1.21-4.19

@aanm aanm merged commit 2d61813 into v1.13 Dec 1, 2023
136 of 138 checks passed
@aanm aanm deleted the pr/v1.13-backport-2023-11-29 branch December 1, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master.
Projects
No open projects
Status: Released
Development

Successfully merging this pull request may close these issues.

None yet

5 participants