Skip to content

fix: Make Cluster Mesh work with OwnerReferencesPermissionEnforcement#43912

Merged
giorio94 merged 2 commits intocilium:mainfrom
fgiloux:pr/fgiloux/OwnerReferencesPermissionEnforcement
Jan 22, 2026
Merged

fix: Make Cluster Mesh work with OwnerReferencesPermissionEnforcement#43912
giorio94 merged 2 commits intocilium:mainfrom
fgiloux:pr/fgiloux/OwnerReferencesPermissionEnforcement

Conversation

@fgiloux
Copy link
Copy Markdown
Contributor

@fgiloux fgiloux commented Jan 21, 2026

Some clusters have the admission plugin OwnerReferencesPermissionEnforcement activated. This plugin protects access to metadata.ownerReferences[x].blockOwnerDeletion, only users with the update permission to the finalizers subresource of the referenced owner can change it.
This adds such permissions to the cilium-operator clusterRole, as the operator sets EnpointSlices owner references.

Add permissions to the cilium-operator so that it can create EndpointSlices when the admission plugin OwnerReferencesPermissionEnforcement is activated

@fgiloux fgiloux requested a review from a team as a code owner January 21, 2026 19:10
@fgiloux fgiloux requested a review from gandro January 21, 2026 19:10
@maintainer-s-little-helper 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 Jan 21, 2026
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 21, 2026
@fgiloux
Copy link
Copy Markdown
Contributor Author

fgiloux commented Jan 21, 2026 •
edited
Loading

Tested locally the additional permisssions with:

# Tweaked contrib/scripts/kind.sh
    kind: ClusterConfiguration
    apiServer:
        extraArgs:
          enable-admission-plugins: NodeRestriction,OwnerReferencesPermissionEnforcement  

$ make kind-clustermesh
# actually installed 1.18.2 instead of the current code and made the clusterrole change manually as the cilium operator was having panics (also without any configuration change)
$ cilium install --chart-directory install/kubernetes/cilium --set cluster.name=kind-clustermesh1 --set cluster.id=1 --set clustermesh.enableEndpointSliceSynchronization=true --context kind-clustermesh1
$ cilium install --chart-directory install/kubernetes/cilium --set cluster.name=kind-clustermesh2 --set cluster.id=2 --set clustermesh.enableEndpointSliceSynchronization=true --context kind-clustermesh2

$ kubectl --context=kind-clustermesh1 get secret -n kube-system cilium-ca -o yaml > /tmp/cilium-ca.yaml
# remove cluster specifics
$ kubectl --context kind-clustermesh2 apply -f /tmp/cilium-ca.yaml
$ cilium clustermesh enable --context kind-clustermesh1 --service-type NodePort
$ cilium clustermesh enable --context kind-clustermesh2 --service-type NodePort
$ cilium clustermesh status --context kind-clustermesh1 --wait
$ cilium clustermesh status --context kind-clustermesh2 --wait
$ cilium clustermesh connect --context kind-clustermesh1 --destination-context kind-clustermesh2

$ kubectl --context kind-clustermesh1 apply -f examples/kubernetes/clustermesh/cluster1.yaml
$ kubectl --context kind-clustermesh1 apply -f examples/kubernetes/clustermesh/global-service-example.yaml

$ kubectl --context kind-clustermesh2 apply -f examples/kubernetes/clustermesh/cluster2.yaml
$ kubectl --context kind-clustermesh2 apply -f examples/kubernetes/clustermesh/global-service-example.yaml
$ kubectl get endpointslices --context kind-clustermesh1
NAME                                          ADDRESSTYPE   PORTS   ENDPOINTS              AGE
kubernetes                                    IPv4          6443    172.19.0.2             6m28s
rebel-base-b9gnx                              IPv4          80      10.1.0.207,10.1.1.77   3m4s
rebel-base-headless-kind-clustermesh2-6vgst   IPv4          80      10.2.0.53,10.2.1.230   2m18s
rebel-base-headless-tw7xb                     IPv4          80      10.1.0.207,10.1.1.77   3m4s

@MrFreezeex MrFreezeex added area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch needs-backport/1.19 This PR / issue needs backporting to the v1.19 branch and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 21, 2026
@MrFreezeex
Copy link
Copy Markdown
Member

MrFreezeex commented Jan 21, 2026

Hi 👋, TIL about this and thanks for tackling this! Here is the similar fixes on the EndpointSlice controller back in 2020 😅: kubernetes/kubernetes#89741. Apparently we would only need the update verbs if the linked PR is still correct today. There's also a nice explanation in the comment there which I would suggest to (at least partially) steal:

				// The controller needs to be able to set a service's finalizers to be able to create an EndpointSlice
				// resource that is owned by the service and sets blockOwnerDeletion=true in its ownerRef.

@fgiloux
Copy link
Copy Markdown
Contributor Author

fgiloux commented Jan 22, 2026

Apparently we would only need the update verbs

Yes, you are correct. I just checked the system:controller:endpointslice-controller and the system:controller:endpointslicemirroring-controller clusterroles and they only have the update verb.

@fgiloux
fix: Cluster Mesh work with OwnerReferencesPermissionEnforcement
88bca72 
Some clusters have the admission plugin OwnerReferencesPermissionEnforcement
activated. This plugin protects access to metadata.ownerReferences[x].blockOwnerDeletion,
only users with the update permission to the finalizers subresource of the referenced
owner can change it.
This adds such permissions to the cilium-operator clusterRole, as the operator sets
EnpointSlices owner references.

Signed-off-by: Frederic Giloux <frederic.giloux@isovalent.com>
@fgiloux fgiloux force-pushed the pr/fgiloux/OwnerReferencesPermissionEnforcement branch from 7902ee3 to 88bca72 Compare January 22, 2026 07:34
@giorio94 giorio94 added affects/v1.16 This issue affects v1.16 branch affects/v1.17 This issue affects v1.17 branch labels Jan 22, 2026
giorio94
giorio94 approved these changes Jan 22, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@giorio94 giorio94 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @fgiloux for taking care of this!

Would you be up for updating the definition of our test clusters (Cluster Mesh workflows use .github/kind-config.yaml.tmpl) to enable the extra admission plugins as you did in your testing? That would allow to immediately spot possible problems like this in the future.

@giorio94
Copy link
Copy Markdown
Member

giorio94 commented Jan 22, 2026

/test

@fgiloux
chore: Add OwnerReferencesPermissionEnforcement to kind in CI
20f493c 
The admission plugin OwnerReferencesPermissionEnforcement adds
constraints when owner references are set. This plugin is, for
instance, activated by default on OpenShift. This will help with
catching permission issues in CI.

Signed-off-by: Frederic Giloux <frederic.giloux@isovalent.com>
@fgiloux fgiloux requested review from a team as code owners January 22, 2026 08:36
@fgiloux fgiloux requested a review from brlbil January 22, 2026 08:36
@fgiloux
Copy link
Copy Markdown
Contributor Author

fgiloux commented Jan 22, 2026

Would you be up for updating the definition of our test clusters (Cluster Mesh workflows use .github/kind-config.yaml.tmpl) to enable the extra admission plugins as you did in your testing? That would allow to immediately spot possible problems like this in the future.

I have added a commit for it. I wish the way to set up kind (and possibly other things) would be shared between local dev and CI. It would make local validation easier.

@giorio94
Copy link
Copy Markdown
Member

giorio94 commented Jan 22, 2026

/test

@giorio94
Copy link
Copy Markdown
Member

giorio94 commented Jan 22, 2026

Multiple workflows failed due to #43901. Rerunning.

@giorio94 giorio94 enabled auto-merge January 22, 2026 10:45
@giorio94 giorio94 added this pull request to the merge queue Jan 22, 2026
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 22, 2026
Merged via the queue into cilium:main with commit f682f08 Jan 22, 2026
81 of 83 checks passed
@fgiloux fgiloux mentioned this pull request Jan 22, 2026
Merged
@julianwiedmann
Copy link
Copy Markdown
Member

julianwiedmann commented Jan 23, 2026

This was reverted, let's not do backports for now.

@julianwiedmann julianwiedmann removed affects/v1.16 This issue affects v1.16 branch affects/v1.17 This issue affects v1.17 branch needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch needs-backport/1.19 This PR / issue needs backporting to the v1.19 branch labels Jan 23, 2026
@julianwiedmann
Copy link
Copy Markdown
Member

julianwiedmann commented Jan 23, 2026

This was reverted, let's not do backports for now.

Ah it was only a partial revert. So let's bring it back, but require a manual backport.

@julianwiedmann julianwiedmann added backport/author The backport will be carried out by the author of the PR. affects/v1.16 This issue affects v1.16 branch affects/v1.17 This issue affects v1.17 branch needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch needs-backport/1.19 This PR / issue needs backporting to the v1.19 branch labels Jan 23, 2026
@giorio94 giorio94 mentioned this pull request Jan 23, 2026
Merged
@giorio94
Copy link
Copy Markdown
Member

giorio94 commented Jan 23, 2026

This was reverted, let's not do backports for now.

Ah it was only a partial revert. So let's bring it back, but require a manual backport.

Makes sense, I'll take care of the backports once #43949 lands.

@giorio94 giorio94 mentioned this pull request Jan 27, 2026
Merged
2 tasks
@giorio94 giorio94 added backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. and removed needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch labels Jan 27, 2026
@github-actions github-actions bot added backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. and removed backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. labels Jan 28, 2026
@giorio94 giorio94 mentioned this pull request Feb 2, 2026
Merged
2 tasks
@giorio94 giorio94 added backport-pending/1.19 The backport for Cilium 1.19.x for this PR is in progress. and removed needs-backport/1.19 This PR / issue needs backporting to the v1.19 branch labels Feb 2, 2026
@github-actions github-actions bot added backport-done/1.19 The backport for Cilium 1.19.x for this PR is done. and removed backport-pending/1.19 The backport for Cilium 1.19.x for this PR is in progress. labels Feb 2, 2026
@cilium-release-bot cilium-release-bot bot moved this to Released in cilium v1.19.0 Feb 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gandro gandro gandro approved these changes

@brlbil brlbil brlbil approved these changes

@giorio94 giorio94 giorio94 approved these changes

Assignees

No one assigned

Labels

affects/v1.16 This issue affects v1.16 branch affects/v1.17 This issue affects v1.17 branch area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience backport/author The backport will be carried out by the author of the PR. backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. backport-done/1.19 The backport for Cilium 1.19.x for this PR is done. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

No open projects
cilium v1.19.0
Status: Released

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fgiloux @MrFreezeex @giorio94 @julianwiedmann @gandro @brlbil @msune