Enables disabling enabled cert-manager-controller controllers #3791
Conversation
|
Is this the same approach that kube-controller-manager takes for disabling controllers? I thought we could prefix the name of a controller with a |
cmd/controller/app/controller.go
Outdated
| @@ -81,7 +81,7 @@ func Run(opts *options.ControllerOptions, stopCh <-chan struct{}) { | |||
| log := log.WithValues("controller", n) | |||
|
|
|||
| // only run a controller if it's been enabled | |||
| if !util.Contains(opts.EnabledControllers, n) { | |||
| if !util.Contains(opts.EnabledControllers, n) || util.Contains(opts.DisabledControllers, n) { | |||
There was a problem hiding this comment.
It would be good to have this specifically be unit tested with a few combinations of enabled and disabled controllers. Could be a new func in util which is called from here:
func IsDisabled(n string, enabledControllers []string, disabledControllers []string) boolI don't think it's required, though.
| @@ -222,6 +225,8 @@ func (s *ControllerOptions) AddFlags(fs *pflag.FlagSet) { | |||
|
|
|||
| fs.StringSliceVar(&s.EnabledControllers, "controllers", defaultEnabledControllers, ""+ | |||
| "The set of controllers to enable.") | |||
| fs.StringSliceVar(&s.DisabledControllers, "disabled-controllers", defaultDisabledControllers, ""+ | |||
| "The set of controllers to disable. Supersedes --controllers.") | |||
There was a problem hiding this comment.
nit: I'm not sure if "supersedes" is the right word here:
supersede: take the place of (a person or thing previously in authority or use); supplant.
I first read the comment as meaning:
"The set of controllers to disable. Replaces --controllers."
But it doesn't really replace --controllers, since EnabledControllers is still used. Maybe:
"The set of controllers to disable. Takes priority over --controllers."
Thanks, I'll add the same behaviour encoded here https://github.com/kubernetes/controller-manager/blob/385c02a3964be0e326cc5803e410aa1028939249/options/generic.go#L97 |
There was a problem hiding this comment.
Looks good to me!
I have one request to remove the remaining bit from --disabled-controllers flag and two optional suggestions that could be ignored, otherwise happy to lgt/approve this.
I have tried deploying this with various options and seemed working well.
(The disabled controllers seem to take preference as if I pass both -foo and foo it ends up being disabled.)
There was a problem hiding this comment.
Looks good to me!
I've tried to deploy with the latest change passing the flag.
Output showing enabled controllers:
cert-manager/controller "msg"="enabled controllers: map[CertificateIssuing:{} CertificateKeyManager:{} CertificateMetrics:{} CertificateReadiness:{} CertificateRequestManager:{} CertificateTrigger:{} certificaterequests-issuer-acme:{} certificaterequests-issuer-ca:{} certificaterequests-issuer-selfsigned:{} certificaterequests-issuer-vault:{} certificaterequests-issuer-venafi:{} challenges:{} clusterissuers:{} ingress-shim:{} issuers:{} orders:{}]"
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irbekrm, JoshVanL 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 |
|
Feel free to unhold when ready to merge 😄 /retest |
ad69d39 to
e810a11
Compare
|
@irbekrm Should be good to look at again |
|
🥳 |
|
/hold cancel |
the certificaterequests-approver Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
same behaviour as kube-controller-manager Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
log enabled controllers on start Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
e810a11 to
8f5b034
Compare
|
/retest |
|
/lgtm |
/kind feature
/assign @jakexks @irbekrm