Cert manager v1.3 - Approval Condition #30
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
terinjokes
reviewed
Apr 22, 2021
| @@ -50,6 +55,55 @@ func (r *CertificateRequestController) Reconcile(req reconcile.Request) (reconci | |||
| return reconcile.Result{}, nil | |||
| } | |||
|
|
|||
| // Ignore CertificateRequest if it is already Ready | |||
| if cmutil.CertificateRequestHasCondition(cr, certmanager.CertificateRequestCondition{ | |||
There was a problem hiding this comment.
I have two questions:
- Since this code is now checking the Ready condition (and
cr.Status.FailureTime), do I still need to check forcr.Status.Certificate? I'm still drinking my morning coffee, but it seems to me thatReady==Truecould only be possible ifcr.Status.Certificatewas set. - Does it make sense to move some or all of these checks into
cmutil? Making one call into thecmutilpackage seems better than chaining multiple together checks here.
There was a problem hiding this comment.
-
We shouldn't have to, but we do anyway to make sure we cover any cases where another signer has behaved improperly. This makes the code as safe as possible.
-
Yes, we could think about moving these things to a chain in cert-manager utils. This would definitely help out with code duplication across projects. We would loose the specific logging for each case however.
There was a problem hiding this comment.
The utility function could return an error with that detailed information, if it's found to be useful to logging. In the meantime, this is fine.
Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
terinjokes
force-pushed
the
cert-manager-v1.3
branch
from
e32c04b
to
ccfef49
Compare
Adds an options package to contain an options struct along with reasonable defaults.
…ests Added support for cert-manager's approved certificates feature, this controller will now wait for a certificate request to be approved before communicating with the Cloudflare API. Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com> [terinjokes@gmail.com: reworked to use options struct and pflag] Signed-off-by: Terin Stock <terinjokes@gmail.com>
terinjokes
force-pushed
the
cert-manager-v1.3
branch
from
ccfef49
to
fd3015b
Compare
cbroglie
approved these changes
May 4, 2021
muff1nman
approved these changes
May 6, 2021
|
@JoshVanL I've released v0.6.0. Should I open a PR to update the documentation site? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In cert-manager v1.3, we introduced the approval mechanism. This PR adds an optional check, which is enabled by default, to wait for the presence of this Approval condition before signing.
This PR also adds RBAC which enables the internal cert-manager approver to approve all Origin CertificateRequests.
Once this is merged and released we can move this issuer to the list of approval honoured issuers here.
/assign @terinjokes