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-04-11 #24821

Merged
merged 14 commits into from
Apr 12, 2023
Merged

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Apr 11, 2023

Once this PR is merged, you can update the PR labels via:

$ for pr in 24521 24742 24612 24666 24769 24792 24797 24773; do contrib/backporting/set-labels.py $pr done 1.13; done

@pchaigno pchaigno requested review from a team as code owners April 11, 2023 21:46
@pchaigno pchaigno 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 Apr 11, 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.

Thanks!

@pchaigno pchaigno added the release-blocker/1.13 This issue will prevent the release of the next version of Cilium. label Apr 12, 2023
sayboras and others added 11 commits April 12, 2023 11:24
[ upstream commit 8602272 ]

This commit is to skip HTTPRouteListenerHostnameMatching test in Gateway
API conformance jobs.

Relates: cilium#24217
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit bca13ae ]

The commit adds a unit test to delete a service with non-active
backends. This is to catch any regressions (such as backend leaks) [1]
in the logic that gracefully terminates such backends.

[1] cilium#23858.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit ce4bf37 ]

Introduces new BGP state model in API, BGP Peers and BGP Families.
BGP Peers express session information and BGP Families expresses number
of AFI/SAFI routes exchanged.

Signed-off-by: harsimran pabla <hpabla@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit beebff0 ]

Exposes BGP Peering state from gobgp using API ListPeers. This change
also introduces state.go SHIM layer in bgp agent to which underlying bgp
implementation translates various BGP parameters.

Signed-off-by: harsimran pabla <hpabla@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit 4b9c234 ]

Introduces new bgp cli 'cilium bgp peers', which returns status of bgp peers.

Signed-off-by: harsimran pabla <hpabla@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit c2c4b74 ]

This reverts commit bc2ed14.

Currently, in the helm chart, if the cert-manager approach is selected
to generate the hubble and clustermesh certificates but no issuer is
specified, a new issuer is created for each of them, along with a secret
containing the CA information. Still, this approach is currently broken,
since the CA secret which is created does not match the format expected
by cert-manager. At the same time, this might also hide misconfigurations
(e.g., if there is a typo in the issuer configuration) and possibly lead
to different CAs for different components. Hence, let's just stick to
the approach documented in the user guide and make it mandatory to specify
the issuer when cert-manager is used. It is a task of the users (as
unrelated from cilium) to create the appropriate issuer in advance,
according to their own preference.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit 62f72cd ]

This reverts commit 082fa15.

Currently, in the helm chart, if the cert-manager approach is selected
to generate the hubble and clustermesh certificates but no issuer is
specified, a new issuer is created for each of them, along with a secret
containing the CA information. Still, this approach is currently broken,
since the CA secret which is created does not match the format expected
by cert-manager. At the same time, this might also hide misconfigurations
(e.g., if there is a typo in the issuer configuration) and possibly lead
to different CAs for different components. Hence, let's just stick to
the approach documented in the user guide and make it mandatory to specify
the issuer when cert-manager is used. It is a task of the users (as
unrelated from cilium) to create the appropriate issuer in advance,
according to their own preference.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit 92407a8 ]

Today we always compile a .asm files for endpoints, even though we
rarely use them. They take a lot of space in the sysdumps and increase
the overall compile time.

This commit changes it to only compile those files if debugging mode is
enabled.

Reported-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit 157f5c7 ]

When walking an IPv6 packet's extension headers to find a DSR extension,
only advance the `nh` variable _after_ calculating the current extension
header's length. For AUTH headers we otherwise fail to determine the
correct header length.

We recently fixed the same issue in the generic IPv6 code with
commit e76d074 ("bpf: fix ipv6 extension header parsing error"), but
missed that the nodeport code has its own IPv6 extension header parsing
logic.

Fixes: 5fbf127 ("datapath: Support multiple IPv6 extensions with DSR")
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit a269948 ]

After walking the IPv6 extension headers, ipv6_hdrlen() returns the L4
proto type in `nexthdr` parameter. If we pass in a pointer to the IPv6
header's nexthdr field, then the actual packet content is changed and
subsequent processing of the packet is broken (because we treat the first
IPv6 extension header as an ICMPv6 header).

So even if we don't care about the L4 proto type (because we already know
that it's ICMPv6), we still need to provide some stack space to store the
`nexthdr`.

This only becomes relevant when ENABLE_ICMP_RULE is set, which is
currently controlled by a hidden agent flag.

Fixes: d49311c ("policy: Add bpf ICMP policy support with the "ENABLE_ICMP_POLICY" flag")
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit e802c29 ]

These wildcard variables will be used by a later commit in the IPsec
logic.

Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit ddd491b ]

UpsertIPsecEndpoint is currently unable to replace stale XFRM states. We
use XfrmStateAdd, which fails with EEXIST if a state with the same key
(IPs, SPI, and mark) already exists. We can't use XfrmStateUpdate because
it fails with ESRCH is no state with the specified key exist.

Note we don't have the same issue for XFRM policies because
XfrmPolicyUpdate doesn't return ESRCH if no such policy already exists.
No idea why the two APIs are not consistent.

We therefore need to implement a proper 'update or insert' logic
for XFRM states ourselves.

To that end, we first check if the state we want to add already exists.
If it doesn't, we attempt to add it. If it fails with EEXIST, we know
that some other state is conflicting. In that case, we attempt to remove
any conflicting XFRM states that are found and then attempt to add the
new state again. To find conflicting XFRM states, we use the same logic
as the kernel does (cf. __xfrm_state_lookup).

Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit 7d44f37 ]

This commit adds a catch-all XFRM policy for outgoing traffic that has the
encryption bit. The goal here is to catch any traffic that may passthrough
our encryption while we are replacing XFRM policies & states. Those
operations cannot always be performed atomically so we may have brief
moments where there is no XFRM policy to encrypt a subset of traffic. This
policy ensures we drop such traffic and don't let it flow in plain text.

We do need to match on the mark because there is also traffic flowing
through XFRM that we don't want to encrypt (e.g., hostns traffic).

Signed-off-by: Paul Chaignon <paul@cilium.io>
[ upstream commit 688dc9a ]

We recently changed our XFRM states and policies (IPs and marks). We
however failed to remove the stale XFRM states and policies and it turns
out that they conflict (e.g., the kernel ends up picking the stale
policies for encryption instead of the new one).

This commit therefore cleans up those stale XFRM states and policies. We
can identify them based on mark values and masks (we switched from
0xFF00 to 0XFFFFFF00).

The new XFRM states and policies are added as we receive the information
on remote nodes. By removing the stale states and policies before the
new ones are installed for all nodes, we could cause plain-text traffic
on egress and packet drops on ingress.

To ensure we never let plain-text traffic out, we will clean up the
stale config only once the catch-all default-drop policy is installed.
In that way, if there is a brief moment where, for a connection
nodeA -> nodeB, we don't have a policy, traffic will be dropped instead
of sent in plain-text.

For each connection nodeA -> nodeB, those packet drops on egress and
ingress of nodeA will happen between the time we replace the BPF
datapath and the time we've installed the new XFRM state and policy
corresponding to nodeB. Waiting longer to remove the stale states and
policies doesn't impact the drops as they will keep happening until the
new states and policies are installed. This is all happening on agent
startup, as soon as we have the necessary information from k8s.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/v1.13-backport-2023-04-11 branch from 201d08b to f53be49 Compare April 12, 2023 09:24
@pchaigno
Copy link
Member Author

/test-backport-1.13

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks and LGTM ✔️

@harsimran-pabla
Copy link
Contributor

Thank you, LGTM

Copy link
Member

@aditighag aditighag 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 for my backports. Thanks!

@pchaigno
Copy link
Member Author

I ran some manual checks on my two PRs to ensure they work as expected. Tests are passing and reviews are in. Marking ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 12, 2023
@gandro gandro merged commit ba40ed2 into cilium:v1.13 Apr 12, 2023
37 checks passed
@pchaigno pchaigno deleted the pr/v1.13-backport-2023-04-11 branch April 12, 2023 16:21
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. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.13 This issue will prevent the release of the next version of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants