agent: Handle correctly state when CEP is present in multiple CESs #24838

alan-kut · 2023-04-12T12:06:50Z

There are condition possible in which CEP changes CES. This leads to CEP being present in multiple CESs for some time. In such cases the standard logic may not work as it always expect to have a single CEP representation.

This commit changes to logic to handle multiple CEPs properly.

Fix incorrect network policy ebpf setup that may lead to incorrect packets denies when CEP is present in multiple CES

pchaigno

agent: Fix a when CEP is present in multiple CESs, e.g. when it's transferred between CESs.

What's the user-visible impact of this bug?

alan-kut · 2023-04-12T13:17:01Z

agent: Fix a when CEP is present in multiple CESs, e.g. when it's transferred between CESs.

What's the user-visible impact of this bug?

CEP missing from the ipcache.

Current state:
There is a CEP being moved from CES1 to CES2 and agent received the following events in the following order:

Add CES2 with CEP -> endpointUpdated(nil, CEP from CES2)
Add CES1 with CEP -> endpointUpdated(nil, CEP from CES1)
Update CES1 remove CEP -> endpointDeleted(CEP from CES1)

The end state is that CEP is missing from the ipcache.
In the middle CEP is not updated properly as all the calls assume there is just one CEP and it's not updated.

pkg/k8s/watchers/cilium_endpoint_slice_subscriber.go

pkg/k8s/watchers/cilium_endpoint_slice_subscriber_test.go

skmatti

lgtm

pkg/k8s/watchers/cilium_endpoint_slice_subscriber.go

tommyp1ckles · 2023-04-13T17:42:31Z

@alan-kut Thanks for the contribution! Is there any issues open for this bug?

pkg/k8s/watchers/cilium_endpoint_slice_subscriber.go

pchaigno

The release note in the PR description needs to describe the user-visible impact (e.g., pods can't start, packets are dropped due to policy denies, etc.) such that it is useful to users when displayed in our release notes.

alan-kut · 2023-04-17T09:24:40Z

I updated the code with the tommyp1ckles suggestions.

I updated the release notes - the user visible impact is that some traffic may be incorrectly denied or allowed because eBPF maps will not be set properly because ipcache inside agent is misconfigured on some cases when CEP changes CES.

I'm not aware of any issue created for this problem.

tommyp1ckles · 2023-04-19T04:23:27Z

@alan-kut The changes look good to me. I am curious, what are the circumstances that a CEP can exist in two CES?

alan-kut · 2023-04-19T07:14:19Z

One example is when CEP is moved to a different CES (especially easy to trigger with identity batching when identity changes): add CEP to 2nd CES and then remove CEP from the 1st CES.

alan-kut · 2023-04-20T13:18:25Z

So it's not theoretical bug - I can reproduce it - but in general - it's valid, temporarily state that CEP exists in multiple CESs. Even if we remove all current occurrences it may happen in the future, as intended or due to bug.

I think long term we need proper reconciliation logic but for now this should work.

Are we good to merge?

There are condition possible in which CEP changes CES. This leads to CEP being present in multiple CESs for some time. In such cases the standard logic may not work as it always expect to have a single CEP representation. This commit changes to logic to handle multiple CEPs properly. Signed-off-by: Alan Kutniewski <kutniewski@google.com>

My concern was addressed. I'll let others more familiar with that logic review the changes.

joestringer · 2023-04-26T15:37:53Z

We had some discussion during the community meeting today (notes), for now we will apply the regular policy for this PR. We'll follow up on clarifying the backporting policy for beta features going forward.

joestringer · 2023-04-26T15:38:42Z

Marking for backport to v1.12, v1.13 unless anyone has concerns about the risk for backporting these changes to older branches.

ferozsalam · 2023-05-05T15:12:59Z

Hi @alan-kut – did you assess the applicability of this issue against v1.11 in addition to v1.12? We would want to backport it to that version too unless you have reason to believe it doesn't apply.

alan-kut · 2023-05-09T08:56:13Z

Hi @ferozsalam - it applies and the backport is not complex. I backported it manually in my internal repo.

julianwiedmann · 2023-05-09T10:29:30Z

Backports for v1.12 and v1.13 are done.

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 Apr 12, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 12, 2023

alan-kut force-pushed the cep-change-ces branch from a71b662 to dd36ea4 Compare April 12, 2023 12:25

pchaigno reviewed Apr 12, 2023

View reviewed changes

pchaigno added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 12, 2023

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 Apr 12, 2023

dlapcevic approved these changes Apr 12, 2023

View reviewed changes

skmatti approved these changes Apr 12, 2023

View reviewed changes

alan-kut marked this pull request as ready for review April 13, 2023 06:57

alan-kut requested a review from a team as a code owner April 13, 2023 06:57

alan-kut requested a review from tommyp1ckles April 13, 2023 06:57

alan-kut force-pushed the cep-change-ces branch from dd36ea4 to b13e1c9 Compare April 13, 2023 08:08

skmatti reviewed Apr 13, 2023

View reviewed changes

pkg/k8s/watchers/cilium_endpoint_slice_subscriber.go Outdated Show resolved Hide resolved

tommyp1ckles reviewed Apr 14, 2023

View reviewed changes

pkg/k8s/watchers/cilium_endpoint_slice_subscriber.go Show resolved Hide resolved

pkg/k8s/watchers/cilium_endpoint_slice_subscriber.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/cilium_endpoint_slice_subscriber.go Outdated Show resolved Hide resolved

pchaigno previously requested changes Apr 17, 2023

View reviewed changes

alan-kut force-pushed the cep-change-ces branch from b13e1c9 to 67049e6 Compare April 17, 2023 09:22

alan-kut requested review from tommyp1ckles and pchaigno April 18, 2023 07:58

alan-kut force-pushed the cep-change-ces branch from 67049e6 to 28969d9 Compare April 18, 2023 08:00

alan-kut force-pushed the cep-change-ces branch from 28969d9 to 1b05a80 Compare April 20, 2023 13:28

pchaigno removed their request for review April 20, 2023 17:42

joestringer added needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 26, 2023

sayboras mentioned this pull request Apr 28, 2023

v1.13 backports 2023-04-28 #25184

Merged

3 tasks

sayboras added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 28, 2023

sayboras mentioned this pull request Apr 28, 2023

v1.12 backports 2023-04-28 #25188

Merged

1 task

sayboras added backport-pending/1.12 and removed needs-backport/1.12 labels Apr 28, 2023

ferozsalam added needs-backport/1.11 affects/v1.11 This issue affects v1.11 branch labels May 9, 2023

julianwiedmann added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.12 backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels May 9, 2023

jrajahalme mentioned this pull request May 11, 2023

Pr/v1.11 backport 2023 05 11 #25382

Merged

5 tasks

jrajahalme added backport-pending/1.11 backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed needs-backport/1.11 labels May 11, 2023

This was referenced May 12, 2023

Prepare for release v1.11.17 #25428

Closed

Prepare for release v1.11.17 #25445

Closed

Prepare for release v1.11.17 #25446

Closed

Prepare for release v1.11.17 #25447

Merged

This was referenced May 16, 2023

Prepare for release v1.12.10 #25490

Merged

Prepare for release v1.13.3 #25514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agent: Handle correctly state when CEP is present in multiple CESs #24838

agent: Handle correctly state when CEP is present in multiple CESs #24838

alan-kut commented Apr 12, 2023 •

edited

pchaigno left a comment

alan-kut commented Apr 12, 2023

skmatti left a comment

tommyp1ckles commented Apr 13, 2023

pchaigno left a comment

alan-kut commented Apr 17, 2023

tommyp1ckles commented Apr 19, 2023

alan-kut commented Apr 19, 2023

alan-kut commented Apr 20, 2023

joestringer commented Apr 26, 2023

joestringer commented Apr 26, 2023

ferozsalam commented May 5, 2023

alan-kut commented May 9, 2023

julianwiedmann commented May 9, 2023

agent: Handle correctly state when CEP is present in multiple CESs #24838

agent: Handle correctly state when CEP is present in multiple CESs #24838

Conversation

alan-kut commented Apr 12, 2023 • edited

pchaigno left a comment

Choose a reason for hiding this comment

alan-kut commented Apr 12, 2023

skmatti left a comment

Choose a reason for hiding this comment

tommyp1ckles commented Apr 13, 2023

pchaigno left a comment

Choose a reason for hiding this comment

alan-kut commented Apr 17, 2023

tommyp1ckles commented Apr 19, 2023

alan-kut commented Apr 19, 2023

alan-kut commented Apr 20, 2023

joestringer commented Apr 26, 2023

joestringer commented Apr 26, 2023

ferozsalam commented May 5, 2023

alan-kut commented May 9, 2023

julianwiedmann commented May 9, 2023

alan-kut commented Apr 12, 2023 •

edited