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

🐛Remove invalid ClusterRoleBinding from CertManager #2931

Conversation

wfernandes
Copy link
Contributor

@wfernandes wfernandes commented Apr 17, 2020

Removes the cert-manager-leaderelection ClusterRoleBinding from
cert-manager v0.11.0 manifest

What this PR does / why we need it:
To get velero backup working we needed to get rid of dangling ClusterRoleBinding cert-manager-leaderelection from cert-manager v0.11.0. This PR stores a local copy of the cert-manager v0.11.0 release manifest which has been edited with the CRB removed and bindata regenerated.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2928

Release Notes

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 17, 2020
@wfernandes
Copy link
Contributor Author

Not sure where to keep the cert-manager yaml. It could also go into the top level hack directory. But feedback appreciated.

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@wfernandes thanks!
overall LGTM to me. Only one minor nit to check if it is possible to skip the copy step

Makefile Outdated Show resolved Hide resolved
@voor
Copy link
Member

voor commented Apr 20, 2020

This PR will be a great first step to then adding PSP for cert-manager, once this is done you'd just need to re-generate this yaml with the PSP stuff turned on.

@voor
Copy link
Member

voor commented Apr 20, 2020

Related Issue (this would make doing this issue for cert-manager much easier): #2934

@wfernandes wfernandes force-pushed the remove-cert-manager-clusterrolebinding branch from d8e31fc to 6d9c7e5 Compare April 20, 2020 15:40
@wfernandes
Copy link
Contributor Author

@fabriziopandini I moved the cert-manager manifest under cmd/clusterctl/config/assets. Let me know if that works. Thanks.

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@wfernandes thanks! the PR is ok for me!
/approve

I leave to @vincepri final world on the PR/the asset location

@fabriziopandini
Copy link
Member

/assign @vincepri

@wfernandes, one additional question. what is the plan for existing clusters? to provide instruction to the users about how to manually delete the invalid role binding or are we planning to make clusterctl to delete it automatically?

@wfernandes
Copy link
Contributor Author

That's a good point. We could add some documentation about it. I can make a note of deleting the ClusterRoleBinding in the Release Notes.
I'm leaning more towards the documentation solution for now because we have #2635 that could revamp how we manage the cert-manager.

@wfernandes
Copy link
Contributor Author

Also...clusterctl delete doesn't remove the cert-manager components so it doesn't matter I guess 🤷‍♂️

Removes the cert-manager-leaderelection ClusterRoleBinding from
cert-manager v0.11.0 manifest
@wfernandes wfernandes force-pushed the remove-cert-manager-clusterrolebinding branch from 6d9c7e5 to 65b5a88 Compare April 20, 2020 19:51
@ncdc
Copy link
Contributor

ncdc commented Apr 20, 2020

+1 to relnotes/docs for removing the bogus binding from existing clusters.

@fabriziopandini
Copy link
Member

Also...clusterctl delete doesn't remove the cert-manager components so it doesn't matter I guess

This should be tracked by an issue

@wfernandes
Copy link
Contributor Author

@ncdc I added my release notes in the description of my PR. Let me know if I should make any changes to that.

@detiber
Copy link
Member

detiber commented Apr 21, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2020
@ncdc
Copy link
Contributor

ncdc commented Apr 21, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, ncdc, wfernandes

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3a4c1fb into kubernetes-sigs:master Apr 21, 2020
@wfernandes wfernandes deleted the remove-cert-manager-clusterrolebinding branch April 21, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clusterctl: filter out cert-manager-leaderelection ClusterRoleBinding
7 participants