Fix bug with `toServices` policy where service backend churn left stale CIDR identities #25687

christarazi · 2023-05-25T18:00:22Z

k8s: Plumb old Endpoints object through handlers
k8s: Fix toServices rule translation cleanup
k8s: Add test for RuleTranslator.Translate()
k8s: Remove unnecessary AllocatePrefixes from RuleTranslator

Fixes: #20477

Main commit "k8s: Fix toServices rule translation cleanup" will be pasted below for ease of review:

Previously, toServices-based rules did not properly cleanup CIDR
identities. When service backends were removed or changed, the deletion
logic acted on the new object rather than on the old object, thus
the entries that were supposed to be deleted were simply added back in
generateToCidrFromEndpoint().

Fix this by passing the deletion logic the old endpoint object state
and performing a diff between the old and new states.

christarazi · 2023-05-25T18:19:05Z

/test

christarazi · 2023-05-25T20:47:16Z

cc @squeed @joestringer for opening #20477

operator/watchers/k8s_service_sync.go

jrajahalme · 2023-06-01T07:51:06Z

#25559 has merged, so this needs a rebase?

operator/watchers/k8s_service_sync.go

christarazi · 2023-06-07T01:38:24Z

/test

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

Click to show.

Test Name

K8sDatapathConfig Transparent encryption DirectRouting Check connectivity with transparent encryption and direct routing with bpf_host

Failure Output

FAIL: Connectivity test between nodes failed

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

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

Then please upload the Jenkins artifacts to that issue.

christarazi · 2023-06-07T06:42:09Z

/mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19

👍 created #25964

pkg/k8s/service_cache.go

christarazi · 2023-06-07T21:43:44Z

/test

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

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: connectivity-check pods are not ready after timeout

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

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

Then please upload the Jenkins artifacts to that issue.

giorio94

A couple of comments/questions about the OldEndpoints propagation, while the rest of the changes looks good to me.

pkg/k8s/watchers/watcher.go

giorio94 · 2023-06-08T13:35:20Z

pkg/k8s/service_cache.go

+			ID:           esID.ServiceID,
+			Service:      svc,
+			Endpoints:    endpoints,
+			OldEndpoints: oldEPs,


Should OldEndpoints be set also in the other cases in which an UpdateService event is emitted? IMO it is mainly necessary in the DeleteEndpoints method below.

Given #25687 (comment), can you elaborate why it's necessary?

I'm referring in particular to the event generated here. Essentially, that happens when one endpointslice gets deleted, triggering an update event with the remaining endpoints for that service. Since OldEndpoints is not propagated, the corresponding PrefixesToRelease rules will not be eventually generated by the K8sTranslator.

There's also another aspect that I didn't realize earlier, OldEndpoints will contain the previous list of endpoints from a given endpointslice, while Endpoints the ones merged from all endpointslices. It seems to me that it should not create problems though.

In the end, I've set the OldEndpoints for all UpdateService events. Please take another look. Thanks for double checking!

🤔 I see that OldEndpoints and Endpoints are always the same for updates from remote service backends. Shouldn't they be different? (I'm not 100% sure about how this feature should play with global services)

toServices (currently) only supports backends that are outside of the cluster (world), and not pod-based backends.

The same logic is also used in the external workloads case, and IIUC in that case the backends processed through that function might be outside of the cluster. Still that is most likely an extremely rare condition.

@giorio94 Could you file a followup issue so that we don't forget this?

christarazi · 2023-06-08T18:08:11Z

/test

This exposes the old endpoints object so that subsequent commits can make use of it to perform proper diff logic for rule translation (`pkg/k8s/rule_translate.go`). This commit should have no functional impact as the code merely references the new endpoint object and does not touch the newly exposed old object. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Previously, toServices-based rules did not properly cleanup CIDR identities. When service backends were removed or changed, the deletion logic acted on the new object rather than on the old object, thus the entries that were supposed to be deleted were simply added back in generateToCidrFromEndpoint(). Fix this by passing the deletion logic the old endpoint object state and performing a diff between the old and new states. Signed-off-by: Chris Tarazi <chris@isovalent.com>

The previous commit fixed toServices diffing logic. This commit adds a test that was used to validate the fix. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This variable is no longer necessary because it doesn't actually prevent ipcache interaction as of commit 4b87ccc ("pkg/k8s/watcher: fix deadlock with service event handler & CES watcher."). Remove it as provides no functional impact. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2023-06-09T22:40:13Z

/test

giorio94

/lgtm

christarazi · 2023-06-12T16:56:57Z

Thanks for the very thorough review @giorio94!

maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 25, 2023

christarazi changed the title ~~pr/christarazi/to services fixups~~ k8s: Fix toServices rule translation cleanup May 25, 2023

christarazi changed the title ~~k8s: Fix toServices rule translation cleanup~~ Fix bug with toServices policy where backend churn left stale CIDR identities in the ipcache May 25, 2023

christarazi changed the title ~~Fix bug with toServices policy where backend churn left stale CIDR identities in the ipcache~~ Fix bug with toServices policy where service backend churn left stale CIDR identities in the ipcache May 25, 2023

christarazi force-pushed the pr/christarazi/to-services-fixups branch 2 times, most recently from 6da6251 to f9adfdf Compare May 25, 2023 18:11

christarazi changed the title ~~Fix bug with toServices policy where service backend churn left stale CIDR identities in the ipcache~~ Fix bug with toServices policy where service backend churn left stale CIDR identities May 25, 2023

christarazi force-pushed the pr/christarazi/to-services-fixups branch from f9adfdf to f301050 Compare May 25, 2023 18:18

christarazi marked this pull request as ready for review May 25, 2023 20:46

christarazi requested review from a team as code owners May 25, 2023 20:46

christarazi requested review from youngnick, giorio94 and joamaki May 25, 2023 20:46

giorio94 reviewed May 26, 2023

View reviewed changes

operator/watchers/k8s_service_sync.go Outdated Show resolved Hide resolved

joamaki requested changes Jun 1, 2023

View reviewed changes

operator/watchers/k8s_service_sync.go Outdated Show resolved Hide resolved

christarazi force-pushed the pr/christarazi/to-services-fixups branch 2 times, most recently from 77d3561 to 647f901 Compare June 7, 2023 01:34

giorio94 reviewed Jun 7, 2023

View reviewed changes

pkg/k8s/service_cache.go Outdated Show resolved Hide resolved

christarazi force-pushed the pr/christarazi/to-services-fixups branch from 647f901 to 5d37c69 Compare June 7, 2023 21:26

christarazi requested review from joamaki and giorio94 June 7, 2023 22:57

joamaki approved these changes Jun 8, 2023

View reviewed changes

giorio94 reviewed Jun 8, 2023

View reviewed changes

christarazi force-pushed the pr/christarazi/to-services-fixups branch from 5d37c69 to ae516b4 Compare June 8, 2023 18:07

christarazi added 4 commits June 9, 2023 15:31

k8s: Add test for RuleTranslator.Translate()

4afae13

The previous commit fixed toServices diffing logic. This commit adds a test that was used to validate the fix. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/to-services-fixups branch from ae516b4 to 776d773 Compare June 9, 2023 22:32

christarazi mentioned this pull request Jun 9, 2023

Remove unnecessary revert parameter from NewK8sTranslator() #26094

Closed

christarazi requested a review from giorio94 June 9, 2023 22:40

giorio94 approved these changes Jun 12, 2023

View reviewed changes

youngnick approved these changes Jun 12, 2023

View reviewed changes

christarazi mentioned this pull request Jun 12, 2023

k8s / policy: allow all services for toServices when using highscale ipcache #26127

Merged

christarazi merged commit 2920010 into main Jun 12, 2023
63 of 64 checks passed

christarazi deleted the pr/christarazi/to-services-fixups branch June 12, 2023 17:26

giorio94 mentioned this pull request Jun 13, 2023

Service cache: populate OldEndpoints when merging information from remote clusters #26176

Closed

giorio94 mentioned this pull request Jul 4, 2023

Computed and propagated the value of OldEndpoints field when merging … #26474

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug with `toServices` policy where service backend churn left stale CIDR identities #25687

Fix bug with `toServices` policy where service backend churn left stale CIDR identities #25687

christarazi commented May 25, 2023 •

edited

christarazi commented May 25, 2023

christarazi commented May 25, 2023

jrajahalme commented Jun 1, 2023

christarazi commented Jun 7, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

christarazi commented Jun 7, 2023 •

edited by maintainer-s-little-helper bot

christarazi commented Jun 7, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

giorio94 left a comment

giorio94 Jun 8, 2023

christarazi Jun 8, 2023 •

edited

giorio94 Jun 8, 2023

giorio94 Jun 8, 2023

christarazi Jun 8, 2023 •

edited

giorio94 Jun 9, 2023

christarazi Jun 9, 2023 •

edited

giorio94 Jun 12, 2023

christarazi Jun 12, 2023

giorio94 Jun 13, 2023

christarazi commented Jun 8, 2023

christarazi commented Jun 9, 2023

giorio94 left a comment

christarazi commented Jun 12, 2023

Fix bug with toServices policy where service backend churn left stale CIDR identities #25687

Fix bug with toServices policy where service backend churn left stale CIDR identities #25687

Conversation

christarazi commented May 25, 2023 • edited

christarazi commented May 25, 2023

christarazi commented May 25, 2023

jrajahalme commented Jun 1, 2023

christarazi commented Jun 7, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

christarazi commented Jun 7, 2023 • edited by maintainer-s-little-helper bot

christarazi commented Jun 7, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

giorio94 left a comment

Choose a reason for hiding this comment

giorio94 Jun 8, 2023

Choose a reason for hiding this comment

christarazi Jun 8, 2023 • edited

Choose a reason for hiding this comment

giorio94 Jun 8, 2023

Choose a reason for hiding this comment

giorio94 Jun 8, 2023

Choose a reason for hiding this comment

christarazi Jun 8, 2023 • edited

Choose a reason for hiding this comment

giorio94 Jun 9, 2023

Choose a reason for hiding this comment

christarazi Jun 9, 2023 • edited

Choose a reason for hiding this comment

giorio94 Jun 12, 2023

Choose a reason for hiding this comment

christarazi Jun 12, 2023

Choose a reason for hiding this comment

giorio94 Jun 13, 2023

Choose a reason for hiding this comment

christarazi commented Jun 8, 2023

christarazi commented Jun 9, 2023

giorio94 left a comment

Choose a reason for hiding this comment

christarazi commented Jun 12, 2023

Fix bug with `toServices` policy where service backend churn left stale CIDR identities #25687

Fix bug with `toServices` policy where service backend churn left stale CIDR identities #25687

christarazi commented May 25, 2023 •

edited

christarazi commented Jun 7, 2023 •

edited by maintainer-s-little-helper bot

christarazi commented Jun 7, 2023 •

edited by maintainer-s-little-helper bot

christarazi commented Jun 7, 2023 •

edited by maintainer-s-little-helper bot

christarazi Jun 8, 2023 •

edited

christarazi Jun 8, 2023 •

edited

christarazi Jun 9, 2023 •

edited