Add functions to ignore certain issuers and/or CertificateRequests, Kubernetes CSRs #37
Conversation
…ubernetes CSRs Signed-off-by: Tim Ramlot <[email protected]>
jetstack-bot
added
do-not-merge/work-in-progress
Signed-off-by: Tim Ramlot <[email protected]>
jetstack-bot
removed
the
do-not-merge/work-in-progress
controllers/signer/interface.go
Outdated
| // IgnoreIssuer is an optional function that can prevent the issuer controllers from | ||
| // reconciling an issuer resource. By default, the controllers will reconcile all | ||
| // issuer resources that match the owned types. | ||
| type IgnoreIssuer func( | ||
| ctx context.Context, | ||
| issuerObject v1alpha1.Issuer, | ||
| ) (bool, error) | ||
|
|
||
| // IgnoreCertificateRequest is an optional function that can prevent the CertificateRequest | ||
| // and Kubernetes CSR controllers from reconciling a CertificateRequest resource. By default, | ||
| // the controllers will reconcile all CertificateRequest resources that match the issuerRef type. | ||
| type IgnoreCertificateRequest func( | ||
| ctx context.Context, | ||
| cr CertificateRequestObject, | ||
| issuerGvk schema.GroupVersionKind, | ||
| issuerName types.NamespacedName, | ||
| ) (bool, error) |
There was a problem hiding this comment.
suggestion: My understanding of these functions is that someone implementing an issuer using issuer-lib may want to define them for their issuer.
If that's correct, would it be useful in the comments for these functions to describe when and how they're called? At the moment I can understand that these functions can prevent reconciliation but I don't know how or when they do!
There was a problem hiding this comment.
I tried to improve the comments PTAL.
| return result, nil, fmt.Errorf("failed to check if issuer should be ignored: %v", err) // retry | ||
| } | ||
| if ignore { | ||
| logger.V(1).Info("Ignoring issuer") |
There was a problem hiding this comment.
suggestion: I think in Reconcile we log "Starting reconcile loop" with the issuer name at V(2) but here we're logging at V(1).
That means that if the user chose to log at level V(1) they'll see a lot of "Ignoring issuer" messages but they won't see which issuer is being ignored.
Would it make sense to make these log levels match? If I log at V(1) and see tonnes of "ignoring issuer" log lines with no further context I'm not sure that my experience is being improved!
Alternatively, if this log level has to be V(1) then might it be worthwhile adding the namespace + name to the "ignoring issuer" log line?
There was a problem hiding this comment.
controller-runtime already adds the resource name & namespace as fields in its log messages, that is why the log messages above also use V(1)
There was a problem hiding this comment.
Starting reconcile loop should not additionally log those fields, that seems like a bug.
There was a problem hiding this comment.
e.g. this is what the Starting reconcile loop message looks like now:
{
"level": "Level(-2)",
"ts": "2023-08-01T14:33:12Z",
"logger": "SI.Reconcile",
"msg": "Starting reconcile loop",
"controller": "certificaterequest",
"controllerGroup": "cert-manager.io",
"controllerKind": "CertificateRequest",
"CertificateRequest": {
"name": "test-cert-6krjb",
"namespace": "wvqzpspdtvfsshq"
},
"namespace": "wvqzpspdtvfsshq",
"name": "test-cert-6krjb",
"reconcileID": "a32cdd0b-7968-41a3-b643-41ebf6e03470"
}There was a problem hiding this comment.
for some reason, other log messages also contain both a CertificateRequest and namespace and name field:
{
"level": "debug",
"ts": "2023-08-01T14:32:51Z",
"logger": "SI.Reconcile",
"msg": "Issuer not found. Waiting for it to be created",
"controller": "certificaterequest",
"controllerGroup": "cert-manager.io",
"controllerKind": "CertificateRequest",
"CertificateRequest": {
"name": "test-cert-6krjb",
"namespace": "wvqzpspdtvfsshq"
},
"namespace": "wvqzpspdtvfsshq",
"name": "test-cert-6krjb",
"reconcileID": "28baff62-8b42-4a67-a89a-d25a302f7eec"
}There was a problem hiding this comment.
So, I think there is enough information. But in the future we should revise the logging (possibly improve upstream controller-runtime) & remove the duplicate fields.
Signed-off-by: Tim Ramlot <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon, SgtCoDFish 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 |
This new options allows implementers to ignore certain issuers (eg. only reconcile all issuers that live in a specific namespace or only reconcile issuers that have a name starting with a certain prefix).
Also, we can ignore the certificate requests that refer to these issuers.
This is useful when you are partitioning issuers (eg. one instance reconciles all issuers with a certain name prefix, while another issuer instance reconciles the other issuers).