Improve filtered certs error reporting

[Hoega]

When creating a bundle from a source (configmap, secret, or inline) with exclusively expired certificates, the bundle will fail and indicate that the source contains no PEM data. There is no indication that certs are expired and it can be tricky to troubleshoot.

This is because 'ValidateAndSanitizePEMBundleWithOptions' parses sources individually, therefore if a source has only expired cert(s), the cert(s) will be skipped and the sanitize function will find an empty output thus returning an error.

For example, if I have:

• 1 valid cert in configmap source
• 1 valid cert in secret source
• 1 expired cert in inline source

The bundle will fail with this error message:
Warning SourceBuildError 1s (x12 over 12s) bundles Failed to build bundle sources: invalid PEM data in source: bundle contains no PEM certificates

Also, the bundle is pending:

NAME         TARGET   SYNCED   REASON   AGE
my-org.com                              5m15s

Whereas If I add 1 valid cert to the inline source the bundle would be OK. This is because the sanitize function requires at least one valid cert by source.

I propose to parse and sanitize sources globally and/or at least to improve the error reporting to highlight a cert is expired.

[arsenalzp]

Can we accept this as the new feature? I think I can work on it once the decision is made.

[erikgb]

Can we accept this as the new feature? I think I can work on it once the decision is made.

Please work on it! I think we all agree that the error handling could be improved here. Could you start by describing the proposed solution in more detail? I agree this would become a lesser problem if a bundle is sanitized after reading all sources. But what if the resulting bundle would contain exclusively expired certificates?

[arsenalzp]

Can we accept this as the new feature? I think I can work on it once the decision is made.

Please work on it! I think we all agree that the error handling could be improved here. Could you start by describing the proposed solution in more detail? I agree this would become a lesser problem if a bundle is sanitized after reading all sources. But what if the resulting bundle would contain exclusively expired certificates?

Hello,
Thank you for letting me work again!
I guess it would be optionally possible either allow or disallow to include expired certificates into the resulting bundle. But I don't know any reason why users want to have expired certificates in bundles, except legacy requirements, etc.

I would look something like
allowExpiredCerts: true
with default false.
Then we can suppress expired certificate warnings and include it in the bundle, or just write ones and exclude expired certificates from the bundle.

[erikgb]

Ah, you haven't seen #273? There is now an opt-in to filter out expired certificates in trust-manager. I understand this issue mostly as a usability improvement to this recently added feature. It could make sense to include an expired certificate in a bundle to get better error messages.

[arsenalzp]

Ah, you haven't seen #273? There is now an opt-in to filter out expired certificates in trust-manager. I understand this issue mostly as a usability improvement. It could make sense to include an expired certificate in a bundle to get better error messages.

Oh, I didn't see that because pull request wasn't linked to this particular issue.

[erikgb]

It seems most reasonable to validate to validate at least one certificate per bundle, and not per bundle source.

/priority important-soon

[cert-manager-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
/lifecycle stale

[erikgb]

I think the issue was fixed by #375. Feel free to reopen if that is not the case.

/close

[cert-manager-prow]

@erikgb: Closing this issue.

In response to this:

I think the issue was fixed by #375. Feel free to reopen if that is not the case.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.