bug(controller): fix missing ignore issuers logic #350
Conversation
Signed-off-by: hjoshi123 <[email protected]>
cert-manager-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
bot
added
the
size/M
|
/cc @ThatsMrTalbot. Could you review it whenever you are free? |
|
IDK if I should review since I helped write this bit 😂 |
| if err := r.Client.Get(ctx, issuerName, kubeutil.ObjectForIssuer(issuerObject)); err != nil && apierrors.IsNotFound(err) { | ||
| logger.V(1).Info("Issuer not found. Waiting for it to be created") | ||
| statusPatch.SetWaitingForIssuerExist(err) | ||
| if r.IgnoreIssuer == nil { |
There was a problem hiding this comment.
In case r.IgnoreIssuer is nil, statusPatch.SetWaitingForIssuerExist will never be called, this is not right.
Originally, my thinking was that issuers could be ignored using IgnoreIssuer for the check command and IgnoreCertificateRequest for the sign command. WDYT cc @ThatsMrTalbot
There was a problem hiding this comment.
When I was writing an example to check the UX of issuer-lib for internal issuers IgnoreCertificateRequest was a pain.
For each CertificateRequest you have to check if its an Issuer or a ClusterIssuer, load the correct type, then check if its one we care about.
Having IgnoreIssuer apply to the signer controller makes this so much easier
There was a problem hiding this comment.
@hjoshi123 Sorry, I was unclear in my original message, I updated my comment: "In case r.IgnoreIssuer is nil, statusPatch.SetWaitingForIssuerExist will never be called, this is not right."
There was a problem hiding this comment.
@inteon sorry I got confused now 😅. Do you mean when r.IgnoreIssuer is not nil the statusPatch.SetWaitingForIssuerExist will never be called?
I didnt think of that.. thank you for pointing that out.. I feel it makes sense to call the SetWaitingForIssuerExist irrespective of the IgnoreIssuer right?
3 participants
This PR addresses an already existing features but there was missing logic that made the functionality moot. Ignore issuer's value was not being consumed anywhere leading to not actually working. This is required for migrating in-tree issuers as we would need those controllers to ignore anything that doesn't match the CM GenericIssuer.