Skip to content

bug(controller): fix missing ignore issuers logic #350

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −2
Open

bug(controller): fix missing ignore issuers logic #350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions controllers/combined_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ func (r *CombinedController) SetupWithManager(ctx context.Context, mgr ctrl.Mana

Client: cl,
Sign: r.Sign,
IgnoreIssuer: r.IgnoreIssuer,
IgnoreCertificateRequest: r.IgnoreCertificateRequest,
EventRecorder: r.EventRecorder,
Clock: r.Clock,
Expand All @@ -164,6 +165,7 @@ func (r *CombinedController) SetupWithManager(ctx context.Context, mgr ctrl.Mana

Client: cl,
Sign: r.Sign,
IgnoreIssuer: r.IgnoreIssuer,
IgnoreCertificateRequest: r.IgnoreCertificateRequest,
EventRecorder: r.EventRecorder,
Clock: r.Clock,
Expand Down
11 changes: 11 additions & 0 deletions controllers/issuer_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,17 @@ func (r *IssuerReconciler) reconcileStatusPatch(
return result, nil, fmt.Errorf("unexpected get error: %v", err) // requeue with backoff
}

if r.IgnoreIssuer != nil {
ignore, err := r.IgnoreIssuer(ctx, issuer)
if err != nil {
return result, nil, fmt.Errorf("failed to check if issuer should be ignored: %v", err) // requeue with backoff
}
if ignore {
logger.V(1).Info("IgnoreIssuer() returned true. Ignoring.")
return result, nil, nil // done
}
}

readyCondition := conditions.GetIssuerStatusCondition(issuer.GetConditions(), v1alpha1.IssuerConditionTypeReady)

// Ignore Issuer if it is already permanently Failed
Expand Down
27 changes: 25 additions & 2 deletions controllers/request_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ type RequestController struct {
// and Kubernetes CSR controllers from reconciling a Request resource.
signer.IgnoreCertificateRequest

// IgnoreIssuer is an optional function that can prevent the Request
// and Kubernetes CSR controllers from reconciling an issuer resource.
signer.IgnoreIssuer

// EventRecorder is used for creating Kubernetes events on resources.
EventRecorder record.EventRecorder

Expand Down Expand Up @@ -236,16 +240,35 @@ func (r *RequestController) reconcileStatusPatch(

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 {
Copy link
Member

@inteon inteon Nov 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ThatsMrTalbot ThatsMrTalbot Nov 12, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@inteon inteon Nov 20, 2025

Choose a reason for hiding this comment

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

@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."

Copy link
Contributor Author

@hjoshi123 hjoshi123 Nov 21, 2025

Choose a reason for hiding this comment

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

@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?

statusPatch.SetWaitingForIssuerExist(err)
}

return result, statusPatch, nil // apply patch, done
} else if err != nil {
logger.V(1).Error(err, "Unexpected error while getting Issuer")
statusPatch.SetUnexpectedError(err)

if r.IgnoreIssuer == nil {
statusPatch.SetUnexpectedError(err)
}

return result, nil, fmt.Errorf("unexpected get error: %v", err) // requeue with backoff
}

if r.IgnoreIssuer != nil {
ignore, err := r.IgnoreIssuer(ctx, issuerObject)

if err != nil {
logger.V(1).Error(err, "Unexpected error while checking if Request should be ignored")
return result, nil, fmt.Errorf("failed to check if Request should be ignored: %v", err) // requeue with backoff
}

if ignore {
logger.V(1).Info("Ignoring Request")
return result, nil, nil // done
}
}

readyCondition := conditions.GetIssuerStatusCondition(
issuerObject.GetConditions(),
v1alpha1.IssuerConditionTypeReady,
Expand Down