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 Backports 2023-08-03 #27238

Merged
merged 32 commits into from Aug 4, 2023
Merged

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Aug 3, 2023

@sayboras sayboras added kind/backports This PR provides functionality previously merged into master. backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. labels Aug 3, 2023
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

#27122

looks good, thank you!

Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

#27115 lgtm

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.

Thanks for backporting my behemoth of a PR!

I believe I found one issue below. Also, were there conflicts in any other commits?

daemon/cmd/status.go Outdated Show resolved Hide resolved
@sayboras sayboras force-pushed the pr/v1.14-backport-2023-08-03 branch from 07e26ce to 4895b12 Compare August 3, 2023 09:57
@sayboras
Copy link
Member Author

sayboras commented Aug 3, 2023

/test-backport-1.14

@sayboras sayboras marked this pull request as ready for review August 3, 2023 10:07
@sayboras sayboras requested review from a team as code owners August 3, 2023 10:07
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

API changes lgtm.

@sayboras sayboras requested a review from pchaigno August 3, 2023 10:09
bpf/lib/common.h Outdated Show resolved Hide resolved
@sayboras sayboras force-pushed the pr/v1.14-backport-2023-08-03 branch from 4895b12 to 0b32a7b Compare August 3, 2023 11:02
@sayboras
Copy link
Member Author

sayboras commented Aug 3, 2023

/test-backport-1.14

@sayboras
Copy link
Member Author

sayboras commented Aug 3, 2023

/ci-multicluster

Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

LGTM.

@sayboras sayboras force-pushed the pr/v1.14-backport-2023-08-03 branch from 0b32a7b to e2c1146 Compare August 3, 2023 13:30
pchaigno and others added 18 commits August 4, 2023 12:21
[ upstream commit 52b0430 ]

The fake node ID map currently only defines the minimum set of functions
needed for the interface. This commit extends it to simulate the actual
behavior of the BPF map with an in-memory Golang map.

This will allow us to use check that the map content matches our
expectations for various events of the node object lifecycle.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit fc8da72 ]

The node ID lifecycle is tied to the node lifecycle. This commit adds an
integration test to cover this lifecycle. It covers the creation and
deletion of nodes, as well as the update of a node's IP addresses. It
also checks that two nodes don't end up with the same node ID and that
each node has a single node ID for all its IP addresses.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 825b91d ]

This commit prevents a deadlock in startup when local redirect policies
are in use.

Involved in the deadlock is the node-local pod watcher and the
redirectpolicy manager. The pod watcher (synchronously) calls into the
manager, in oder to notify it of a pod event. The manager then tries to
acquire a reference to the pod store. Due to recent refactoring,
however, this leads to a wait cycle:

1. Acquiring the pod store reference blocks on initial synchronisation
   of the store, by waiting for the `podStoreSet` channel to be closed.
2. This channel is closed by the watcher once it receives a `Sync` event
   from the local pod resource. Before the resource emits a Sync event,
   however, it will 'replay' the consumer up to the current snapshot by
   emitting Upsert events.
3. In the Upsert call, the redirectpolicy manager is called (blocking on
   1.), and we have a classical deadlock.

To fix this, instead of going through the k8s watcher to acquire a
reference to the local pod store, inject the resource directly into the
redirect policy manager. This will also block on the store being
synchronised, but not by way of the `podStoreSet` channel (closed by the
k8s watcher): it instead relies on the Resource[T]-internal tracking.

Fixes: 3db8b14 (k8s: Use LocalPodResource in pod watcher)

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 6ba173d ]

Signed-off-by: Renat Tuktarov <yandzeek@gmail.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit fded1ab ]

We want to block any configurations that enable ingress controllers when using delegated IPAM.
Cilium allocates its own IP address for sending and differentiating ingress traffic, which is
not possible with delegated IPAM. This change will provide a clearer error message for this.

Signed-off-by: Ricky Ho <horicky78@gmail.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 685439c ]

We recently added a call to "sed" in Documentation's Makefile, as part
of the generation process for the Helm reference. We use the "-i" option
to edit the file in-place; but this option is not portable, and the
command fails when running BSD versions of sed, for example on MacOS, as
opposed to GNU sed.

We have two options to fix this: using $(SED), which is set to gsed when
available, and using an additional temporary file. Let's use both to
have the Makefile as portable as possible.

Fixes: 2e9b20f ("docs: Ignore Helm value names for spellcheck")
Reported-by: Liz Rice <liz@lizrice.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 4c21556 ]

Signed-off-by: Liz Rice <liz@lizrice.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 018616a ]

Signed-off-by: Liz Rice <liz@lizrice.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 6f19a80 ]

Adds docs for changing IPAM mode and cluster pool CIDR settings.

Signed-off-by: darox <maderdario@gmail.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit f50c51a ]

Co-authored-by: ZSC <zacharysarah@users.noreply.github.com>
Signed-off-by: Dario Mader <maderdario@gmail.com>
[ upstream commit 953b26f ]

Co-authored-by: Joe Stringer <joestringernz@gmail.com>
Signed-off-by: Dario Mader <maderdario@gmail.com>
[ upstream commit 4eb694a ]

When kvstoremesh is enabled, local agents connect to the local
clustermesh-apiserver instance, rather than the remote one. Yet,
when the TLS key/certificate pair of a remote cluster is manually
specified, the helm chart currently configures it both to connect
to the remote clustermesh-apiserver instance (which is correct)
and to the local one (which prevents the connection from being
established correctly as it is signed with a different CA).

Let's fix this making sure that we always use the local TLS
key/certificate pair when connecting to the local
clustermesh-apiserver instance.

Fixes: bd53860 ("kvstoremesh: add helm configuration")
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit eb80fb1 ]

In 7a9447d we reworked a few workflows
to now be triggered by Ariane, however we missed some changes to make
sure that they checkout actions and code from the correct contexts.

In particular:

- Gateway API / Ingress / Integration tests were checking out
  environment variables from the default branch instead of the
  appropriate context ref (all in all not a big deal and still safe, but
  could be annoying to troubleshoot later down the road).
- Runtime tests were checking out environment variables from the PR
  branch instead of the appropriate context ref (this was a potential
  security issue), and then incorrectly pulling the default branch for
  executing tests instead of the appropriate PR branch context (so we
  were not testing what we expected).

Fixes: 7a9447d

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit b471f4f ]

No idea why but the steps were not aligned properly here.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 96f3fd7 ]

Workflows running on PRs and based on `pull_request_target` and
`workflow_dispatch` are executed in a privileged context (e.g. access to
repository secrets), hence we take extra care not to execute anything
coming from the PR directly in the context of the workflow steps, but
instead always in a sandboxed or controlled environment (e.g. a managed
Kubernetes cluster or LVH VMs).

This commit standardizes and adds some context around which checkouts
are trusted and which are not, and where to be start being careful with
what the workflow steps are doing.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 8f4c98b ]

The use of a /9 here as an example instead of the /8 was confusing
various users. Reuse the /8 instead.

Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 5d4cd6f ]

This is to make sure if multiple matching conditions are satisfied, the
header condition should be prioritized.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit fbbca84 ]

This is as part of regular update to the latest version from upstream.

Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras force-pushed the pr/v1.14-backport-2023-08-03 branch from 6731e1a to 560e887 Compare August 4, 2023 02:21
@sayboras
Copy link
Member Author

sayboras commented Aug 4, 2023

/test-backport-1.14

@sayboras
Copy link
Member Author

sayboras commented Aug 4, 2023

Due to a huge number of commits in this backport, build-commit GHA timed out after 1.5h. Hence, I manually verify with the below commands:

$ time git rebase --exec "make build -j $(nproc)" e391c76420
...
Successfully rebased and updated refs/heads/pr/v1.14-backport-2023-08-03.
git rebase --exec "make build -j $(nproc)" e391c76420  2836.36s user 532.20s system 1471% cpu 3:48.88 total

$ time git rebase --exec "make -C bpf build_all -j $(nproc)" e391c76420
...
Successfully rebased and updated refs/heads/pr/v1.14-backport-2023-08-03.
git rebase --exec "make -C bpf build_all -j $(nproc)" e391c76420  6199.31s user 190.38s system 278% cpu 38:17.06 total

@sayboras
Copy link
Member Author

sayboras commented Aug 4, 2023

Most of the reviews are in, CI is green, marking this ready to merge.

@sayboras sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 4, 2023
@dylandreimerink dylandreimerink merged commit 2044860 into v1.14 Aug 4, 2023
196 checks passed
@dylandreimerink dylandreimerink deleted the pr/v1.14-backport-2023-08-03 branch August 4, 2023 11:25
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 4, 2023
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