Skip to content

Fix permissions for endpointslice controller#89741

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
marun:fix-endpointslice-permissions
Apr 1, 2020
Merged

Fix permissions for endpointslice controller#89741
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
marun:fix-endpointslice-permissions

Conversation

@marun
Copy link
Copy Markdown
Contributor

@marun marun commented Apr 1, 2020 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:

When the OwnerReferencesPermissionEnforcement validating admission plugin is enabled, the EndpointSlice controller needs permission to update 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.

The lack of this permission breaks the EndpointSlice controller on openshift, which enables OwnerReferencesPermissionEnforcement by default. The problem isn't apparent on kube due to OwnerReferencesPermissionEnforcement being disabled by default.

Fixed the EndpointSlice controller to run without error on a cluster with the OwnerReferencesPermissionEnforcement validating admission plugin enabled.

@marun
Fix permissions for endpointslice controller
76207fe 
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.
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 1, 2020
@marun
Copy link
Copy Markdown
Contributor Author

marun commented Apr 1, 2020

/assign @robscott @thockin

@k8s-ci-robot k8s-ci-robot requested review from lavalamp and ncdc April 1, 2020 17:40
@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 1, 2020
thockin
thockin reviewed Apr 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

I don't know much about how blockOwnerDeletion is implemented - is that synchronous to the original REST call?

This seems OK, so I will approve, but Rob needs to LGTM

/approve

@thockin thockin added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2020
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Apr 1, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@robscott
Copy link
Copy Markdown
Member

robscott commented Apr 1, 2020

@thockin I've been talking with @marun on Slack about this PR, and agree that it's a good fix. This was not noticed in k/k because we do not enable the OwnerReferencesPermissionEnforcement admission controller by default. When that is enabled it prevents the EndpointSlice controller from working with errors like this: Error creating EndpointSlice for Service foo: endpointslices.discovery.k8s.io "foo-ttsrn" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on. So hopefully not something that a lot of people will run into, but still very much worth fixing. Likely worth cherry-picking this into 1.18 and 1.17 as well.

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Apr 1, 2020
@marun
Copy link
Copy Markdown
Contributor Author

marun commented Apr 1, 2020 •
edited
Loading

I've updated the PR description to reflect the involvement of the OwnerReferencesPermissionEnforcement admission plugin and added a release note.

@robscott
Copy link
Copy Markdown
Member

robscott commented Apr 1, 2020

/sig network
/priority important-soon

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 1, 2020
@robscott
Copy link
Copy Markdown
Member

robscott commented Apr 1, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit facda9a into kubernetes:master Apr 1, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Apr 1, 2020
This was referenced Apr 3, 2020
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Apr 7, 2020
@k8s-ci-robot
Merge pull request #89804 from marun/automated-cherry-pick-of-#89741-…
75b1354 
…upstream-release-1.18

Automated cherry pick of #89741: Fix permissions for endpointslice controller
k8s-ci-robot added a commit that referenced this pull request Apr 8, 2020
@k8s-ci-robot
Merge pull request #89805 from marun/automated-cherry-pick-of-#89741-…
2f738d2 
…upstream-release-1.17

Automated cherry pick of #89741: Fix permissions for endpointslice controller
@MrFreezeex MrFreezeex mentioned this pull request Jan 21, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thockin thockin thockin left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

@ncdc ncdc Awaiting requested review from ncdc

Assignees

@robscott robscott

@thockin thockin

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/network Categorizes an issue or PR as relevant to SIG Network. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

v1.19

Development

Successfully merging this pull request may close these issues.

4 participants

@marun @k8s-ci-robot @robscott @thockin