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

Allow controllers to modify finalizers without modify permissions on parent #2264

Closed
rainest opened this issue Jul 12, 2021 · 8 comments
Closed
Assignees
Labels
kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@rainest
Copy link

rainest commented Jul 12, 2021

What do you want to happen?

Overview

I'd like the documentation examples and template controller code to grant permissions to finalizer subresources directly, rather than relying on permissions to the parent. This would allow controllers with read-only permissions to resources to utilize finalizers still. I believe this changeset should accomplish that, though am looking for input to see if there's something I've overlooked.

Not sure if it'd make sense to provide additional context as to why this adds permissions on top of the parent resource permissions--the additional finalizer sub-resource permissions are superfluous if you can already modify the parent, but documentation currently only has the single example.

Background

Currently, examples and template controller code grant several permissions to a resource and only grant the update permission on its finalizer sub-resources, e.g.

// +kubebuilder:rbac:groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=batch.tutorial.kubebuilder.io,resources=cronjobs/finalizers,verbs=update

The finalizer update permission was originally added in #1688 to address a separate issue.

If you wish to create a controller that only has read permissions to a resource, but can manipulate that resource's finalizers, you might try to remove permissions from the resource, e.g.

// +kubebuilder:rbac:groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=get;list;watch
// +kubebuilder:rbac:groups=batch.tutorial.kubebuilder.io,resources=cronjobs/finalizers,verbs=update

Afterwards, you'll find that your Reconcile() fails if it adds and removes finalizers. A call like obj.SetFinalizers(append(finalizers, myFinalizer)) will result in errors like:

time="2021-07-09T01:19:57Z" level=error msg="Reconciler error" error="resource "resourceName" is forbidden: User "system:serviceaccount:namespace:whatever" cannot update resource "resources" in API group "" in the namespace "default"" logger=controller-runtime.manager.controller.resource name=resourceName namespace=default reconciler group= reconciler kind=Resource

We encountered this when limiting most of our controllers to read-only permissions, e.g. with this set of permissions on a resource where we still used a finalizer.

My understanding of the underlying Kubernetes RBAC implementation is imperfect, but my intuition is that you need the additional permissions because finalizers are a list under the resource metadata (versus status, which is a single object and only requires update). While you'll be able to edit that list if you have modify permissions to the parent resource, you cannot modify finalizers with update alone if you lack modify permissions on the parent.

I was able to clear the forbidden permissions error by adding create. Based on the rest of the interface exposed by controller-runtime and the Operator SDK examples, I'm guessing the rest are necessary, though I haven't tested it fully (we actually removed finalizers from our controller for other reasons, so I didn't go further to confirm that the delete code needed list and delete).

Extra Labels

/kind documentation

@rainest rainest added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 12, 2021
@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jul 12, 2021
@camilamacedo86 camilamacedo86 added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 27, 2021
@camilamacedo86
Copy link
Member

Hi @estroz,

Since you are checking this one and it was discussed already in the triage meeting I am adding triage accepted regards its analyse and checks.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2021
@rainest
Copy link
Author

rainest commented Oct 25, 2021

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 25, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 22, 2022
@camilamacedo86
Copy link
Member

@rashmigottipati @varshaprasad96 @ryantking could someone get this one?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants