Skip to content

Add update permission to certificaterequests/finalizers to the cert-manager-controller-certificates clusterrole.#2325

Merged
jetstack-bot merged 1 commit into
cert-manager:masterfrom
CoaxVex:fix-2305
Nov 15, 2019
Merged

Add update permission to certificaterequests/finalizers to the cert-manager-controller-certificates clusterrole.#2325
jetstack-bot merged 1 commit into
cert-manager:masterfrom
CoaxVex:fix-2305

Conversation

@CoaxVex

@CoaxVex CoaxVex commented Nov 5, 2019 •
edited by munnerz
Loading

Copy link
Copy Markdown
Contributor

fixes: #2305

Signed-off-by: Nils Cant nils.cant@vargen.io

Fix issue causing certificates not to be issued when running with `OwnerReferencesPermissionEnforcement` admission controller enabled

@CoaxVex
Add update permission to certificaterequests/finalizers to the
c51c179 
cert-manager-controller-certificates clusterrole.

Fixes: cert-manager#2305

Signed-off-by: Nils Cant <nils.cant@vargen.io>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/deploy Indicates a PR modifies deployment configuration labels Nov 5, 2019
@jetstack-bot jetstack-bot requested a review from munnerz November 5, 2019 14:44
@jetstack-bot jetstack-bot added 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-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 5, 2019
@jetstack-bot

jetstack-bot commented Nov 5, 2019

Copy link
Copy Markdown
Contributor

Hi @CoaxVex. Thanks for your PR.

I'm waiting for a jetstack or cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@jetstack-bot jetstack-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 5, 2019
@munnerz

munnerz commented Nov 6, 2019

Copy link
Copy Markdown
Member

Hey - could you describe why this is needed? We don't set any finalizers on ChallengeRequest resources 🤔

@CoaxVex

CoaxVex commented Nov 6, 2019

Copy link
Copy Markdown
Contributor Author

I got this error message (See #2305)

cert-manager/controller/certificaterequests-issuer-acme/sign "msg"="Failed create new order resource openshift-ingress/apps-<redacted>-1251014904-1491890962" "error"="orders.acme.cert-manager.io \"apps-<redacted>-1251014904-1491890962\" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: RBAC: clusterrole.rbac.authorization.k8s.io \"cert-manager-leaderelection\" not found, \u003cnil\u003e"

When an order is created, it sets an ownerReference to the certificaterequest, and I assume this is what is going wrong. Since nobody else experiences this except me, I'm guessing it's specific to OpenShift 4?
The OwnerReference function comes from k8s.io/apimachinery, so I'm not sure why this sets the blockOwnerDeletion to true on my installation...

I have just reproduced it on another cluster in another namespace. I also deleted the cert-manager-leaderelection clusterrolebinding:

E1106 12:04:09.780407       1 acme.go:128] cert-manager/controller/certificaterequests-issuer-acme/sign "msg"="Failed create new order resource nils-test/test-4007258333-2099780622" "error"="orders.acme.cert-manager.io \"test-4007258333-2099780622\" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: , \u003cnil\u003e" "resource_kind"="CertificateRequest" "resource_name"="test-4007258333" "resource_namespace"="nils-test" 
E1106 12:04:09.780508       1 sync.go:123] cert-manager/controller/certificaterequests-issuer-acme "msg"="error issuing certificate request" "error"="orders.acme.cert-manager.io \"test-4007258333-2099780622\" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: , \u003cnil\u003e" "related_resource_kind"="Issuer" "related_resource_name"="letsencrypt-staging" "related_resource_namespace"="nils-test" "resource_kind"="CertificateRequest" "resource_name"="test-4007258333" "resource_namespace"="nils-test" 
E1106 12:04:09.780613       1 controller.go:131] cert-manager/controller/certificaterequests-issuer-acme "msg"="re-queuing item  due to error processing" "error"="orders.acme.cert-manager.io \"test-4007258333-2099780622\" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: , \u003cnil\u003e" "key"="nils-test/test-4007258333"

@CoaxVex

CoaxVex commented Nov 6, 2019

Copy link
Copy Markdown
Contributor Author

This may be relevant:

If an object’s ownerReferences field is set by a controller (such as Deployment or ReplicaSet), blockOwnerDeletion is set automatically and you do not need to manually modify this field

https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/

Still doesn't explain why nobody else is experiencing this though.

@munnerz

munnerz commented Nov 15, 2019

Copy link
Copy Markdown
Member

/lgtm
/approve
/milestone v0.12
/ok-to-test

@jetstack-bot jetstack-bot added this to the v0.12 milestone Nov 15, 2019
@jetstack-bot jetstack-bot added ok-to-test lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 15, 2019
@jetstack-bot

jetstack-bot commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CoaxVex, munnerz

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

@jetstack-bot jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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. labels Nov 15, 2019
@jetstack-bot jetstack-bot merged commit 8ac1ba8 into cert-manager:master Nov 15, 2019
@munnerz munnerz added the kind/bug Categorizes issue or PR as related to a bug. label Nov 15, 2019
@rassie rassie mentioned this pull request Mar 20, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@munnerz munnerz Awaiting requested review from munnerz

Assignees

@munnerz munnerz

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/deploy Indicates a PR modifies deployment configuration dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. ok-to-test release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

v0.12

Development

Successfully merging this pull request may close these issues.

blockOwnerDeletion forbidden when creating order on OpenShift 4.2.1

3 participants

@CoaxVex @jetstack-bot @munnerz