fix: don't error if source selector selects no sources #432
Conversation
cert-manager-prow
bot
added
dco-signoff: yes
|
/cc @SgtCoDFish |
erikgb
force-pushed
the
fix-non-matching-source-selector
branch
2 times, most recently
from
59c12c5 to
3ec505e
Compare
cert-manager-prow
bot
added
size/S
|
@SgtCoDFish I think this PR is ready for another review now! |
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
erikgb
force-pushed
the
fix-non-matching-source-selector
branch
from
3ec505e to
e7049f0
Compare
cert-manager-prow
bot
added
size/M
There was a problem hiding this comment.
I'm not sure if I missed something but wasn't there a new check to ensure the bundle wasn't empty, so we still required a bundle to resolve to at least one cert?
Is that missing here? Was it added elsewhere? I might be missing it but it looks like this would permit an empty target?
The check is already there: https://github.com/cert-manager/trust-manager/blob/main/pkg/bundle/source.go#L90-L92, and it is now not changed in this PR. 😉 Tested by an existing test, but also with one of the additional tests introduced in this PR (selects nothing). |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
cert-manager-prow
bot
added
the
approved
Fixes #426?
We have a test that asserts an error when the bundle is empty. I doubt if this is correct, and to fix #426 properly, I think this behavior must go. We could eventually assert that a Bundle is invalid if it doesn't contain at least one source spec. But that is better done with the webhook (or API CEL expression) IMO.UPDATE: After discussing this PR in today's stand-up, we decided to keep the existing (and tested) behavior where a target bundle needs to contain at least one certificate, at least for now. This means that this PR only fixes a minor bug: before this PR it would error if any source selector selects no sources. Now this is ok, but the resulting bundle must contain at least one certificate.