Merge pull request #5887 from irbekrm/retry_denied_crs

Fixes a race condition in retrying denied/invalid issuances
This commit is contained in:
jetstack-bot 2023-03-27 10:47:23 +01:00 committed by GitHub
commit ac134ec14e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 362 additions and 67 deletions

View File

@ -11,7 +11,7 @@ approvers:
- "@munnerz"
editor: "@joshvanl"
creation-date: 2019-07-08
last-updated: 2021-03-24
last-updated: 2023-03-24
status: implementable
---
@ -52,6 +52,10 @@ status: implementable
* [Version Skew Strategy](#version-skew-strategy)
<!-- /toc -->
:warning: Parts of this design are out of date with regards to the current implementation.
See also https://cert-manager.io/docs/concepts/certificaterequest/.
## Summary
Currently, certificates issued via cert-manager rely on the `Certificate`
@ -223,11 +227,9 @@ implementation of the approver. For example, the name of the resource that
approves this request, the violations which caused the request to be denied, or
the team to who manually approved the request.
When a CertificateRequest has been Denied, it is the responsibility of the
referenced issuer to then add a Ready condition with the status of "False",
along with a relevant Reason and Message.
A CertificateRequest that is Denied is considered to be in a final, failed
state. If it was created for an issuance of a Certificate, the associated
issuance will be failed.
##### RBAC
Approved and Denied conditions are set by requesting against the `/status`
@ -397,8 +399,13 @@ minimal as possible in that the single goal of them is to enable its owning
`CertificateRequest` has been observed, the general flow is as follows:
- Check the group belongs to the owning `Issuer`, exit if not.
- Check if `CertificateRequest` is in a failed state, exit if true. TODO: more
tightly define what a 'failed state' exactly is.
- Check if `CertificateRequest` is in a terminal failed state.
A controller may choose to add additional conditions to a failed `CertificateRequest`, but must not attempt to issue a certificate.
Currently terminal failed states are:
- `Ready` condition with a `Failed` reason // usually set by the issuer
- `InvalidRequest` condition with `True` status // usually set by the issuer
- `Denied` condition with `True` status // usually set by approver
- Check the `Issuer` type is of the same type, exit if not.
- Verify the Spec of the `CertificateRequest`.
- If a certificate exits then update the status if needed and exit.
@ -427,50 +434,21 @@ this resource.
#### Issuing Controller
Since external issuers have been built before the addition of Approved and
Denied conditions, the issuing controller needs to be permissive. An external
issuer may not honour an Approved condition and will sign and set a
CertificateRequest as being Ready, before the request has been approved. The
issuing controller must mark issuance as being successful in this case. In
practice, this means that the issuing controller is never concerned with
Approved conditions.
Issuing controller considers all Denied CertificateRequests to be in a final failed state.
The issuance will be failed and will be repeatedly retried with an exponential backoff ../20220118.certificate-issuance-exponential-backoff.md.
If the cause of the denial was a misconfigured Certificate spec, the issuance will be retried immediately once the spec is corrected.
If the cause of the denial was misconfigured policy resources, a user who has fixed the resources and wants to retry immediately can do so using [cmctl renew](https://cert-manager.io/docs/reference/cmctl/#renew)
External issuers that do not honour Denied conditions will sign
CertificateRequests, even if they have a Denied condition set. In this case, the
issuing controller will successfully complete the issuance of the Certificate.
External Issuers and internal issuers that honour the Denied condition will
never sign CertificateRequests with the Denied condition set, and thus never set
Ready condition. In this case, the issuing controller will consider this
CertificateRequest as failed, and will set the condition `Issuing=False`
as well as setting the status field `lastFailureTime`. Note that the issuing
controller is not responsible for setting the `Ready=False` condition on
the CertificateRequests; that's the issuer's responsibility.
- The Certificate is clearly reported as Failed to users who may miss the
Denied request from a cursory view.
- The Spec may genuinely be violating the policy, and so can be changed by the
user. This will cause an immediate reissue.
- The policy may be misconfigured, and as such, the Certificate will be retired
later with no user intervention.
- The [manual renew
command](https://cert-manager.io/docs/usage/kubectl-plugin/#renew) relies on
the Issuing condition. In the case of policy being misconfigured, the user
is able to immediately retry the request using the CLI plugin.
The issuing controller does not check Approved condition. It is the issuer's
responsibility not to issue certificates for CertificateRequests that have not
been approved.
### Failure
The `CertificateRequest` resource has a `FailureTime` field in its Status. If
the `CertificateRequest` fails for any reason then this field is set to the
current time. This field can then be used by a higher order controller, such as
the `Certificate` controller, to take further action and facilitate a backoff.
The `Certificate` controller will retry all failed `CertificateRequest` resources
by creating a new request with an identical Spec, only when the `FailureTime`
field is a least 1 hour in the past. The old failed `CertificateRequest` will be
deleted and the new `CertificateRequest` resource will be created with the same
name.
A `CertificateRequest` is considered in a final failed state if:
- it has a Ready condition with Failed reason
- it has a Denied condition with True status
- it has InvalidRequest reason with True status
### Internal API Resource Behaviour

View File

@ -63,6 +63,12 @@ func (c *Controller) Sync(ctx context.Context, cr *cmapi.CertificateRequest) (er
return nil
}
// If CertificateRequest is invalid, do not process it
if apiutil.CertificateRequestHasInvalidRequest(cr) {
dbg.Info("certificate request is invalid and will not be further processed")
return nil
}
// If CertificateRequest has not been approved, exit early.
if !apiutil.CertificateRequestIsApproved(cr) {
dbg.Info("certificate request has not been approved")

View File

@ -372,6 +372,21 @@ func TestSync(t *testing.T) {
ExpectedActions: []testpkg.Action{},
},
},
"should return nil (no action) if certificate request invalidrequest is set to true": {
certificateRequest: gen.CertificateRequestFrom(baseCRNotApproved,
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionInvalidRequest,
Status: cmmeta.ConditionTrue,
Reason: "InvalidRequest",
Message: "Certificate request is invalid",
LastTransitionTime: &nowMetaTime,
}),
),
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{baseIssuer, baseCR},
ExpectedActions: []testpkg.Action{},
},
},
"should return nil (no action) if certificate request is ready and reason Issued": {
certificateRequest: gen.CertificateRequestFrom(baseCR,
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{

View File

@ -279,25 +279,37 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
return nil
}
// Some issuers won't honor the "Denied=True" condition, and we don't want
// to break these issuers. To avoid breaking these issuers, we skip bubbling
// up the "Denied=True" condition from the certificate request object to the
// certificate object when the issuer ignores the "Denied" state.
//
// To know whether or not an issuer ignores the "Denied" state, we pay
// attention to the "Ready" condition on the certificate request. If a
// certificate request is "Denied=True" and that the issuer still proceeds
// to adding the "Ready" condition (to either true or false), then we
// consider that this issuer has ignored the "Denied" state.
// Now check if CertificateRequest is in any of the final states so that
// this issuance can be completed as either succeeded or failed. Failed
// issuance will be retried with a delay (the logic for that lives in
// certificates-trigger controller). Final states are: Denied condition
// with status True => fail issuance InvalidRequest condition with
// status True => fail issuance Ready conidtion with reason Failed =>
// fail issuance Ready condition with reason Issued => finalize issuance
// as succeeded.
// In case of a non-compliant issuer, a CertificateRequest can have both
// Denied status True (set by an approver) and Ready condition with
// reason Issued (set by the issuer). In this case, we prioritize the
// Denied condition and fail the issuance. This is done for consistency
// and also to avoid race conditions between the non-compliant issuer
// and this control loop.
// If the certificate request was denied, set the last failure time to
// now, bump the issuance attempts and set the Issuing status condition
// to False.
if apiutil.CertificateRequestIsDenied(req) {
return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied))
}
// If the certificate request is invalid, set the last failure time to
// now, bump the issuance attempts and set the Issuing status condition
// to False.
if apiutil.CertificateRequestHasInvalidRequest(req) {
return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionInvalidRequest))
}
if crReadyCond == nil {
if apiutil.CertificateRequestIsDenied(req) {
return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied))
}
if apiutil.CertificateRequestHasInvalidRequest(req) {
return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionInvalidRequest))
}
log.V(logf.DebugLevel).Info("CertificateRequest does not have Ready condition, waiting...")
return nil
}

View File

@ -1010,7 +1010,7 @@ func TestIssuingController(t *testing.T) {
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest, but has been denied, report denial and set last failed time and issuance attempts": {
"if certificate is in Issuing state, one CertificateRequest without Ready condition, but with Denied condition, report denial and set last failed time and issuance attempts": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{
@ -1062,6 +1062,290 @@ func TestIssuingController(t *testing.T) {
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest with a pending Ready condition and a Denied condition, report denial and set last failed time and issuance attempts": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{
gen.CertificateFrom(issuingCert),
gen.CertificateRequestFrom(exampleBundle.CertificateRequest,
gen.AddCertificateRequestAnnotations(map[string]string{
cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionDenied,
Status: cmmeta.ConditionTrue,
Reason: "DeniedReason",
Message: "The certificate request has been denied",
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionReady,
Status: cmmeta.ConditionFalse,
Reason: "Pending",
Message: "The certificate request is pending",
}),
)},
KubeObjects: []runtime.Object{
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: nextPrivateKeySecretName,
Namespace: exampleBundle.Certificate.Namespace,
},
Data: map[string][]byte{
corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes,
},
},
},
ExpectedActions: []testpkg.Action{
testpkg.NewAction(coretesting.NewUpdateSubresourceAction(
cmapi.SchemeGroupVersion.WithResource("certificates"),
"status",
exampleBundle.Certificate.Namespace,
gen.CertificateFrom(exampleBundle.Certificate,
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{
Type: cmapi.CertificateConditionIssuing,
Status: cmmeta.ConditionFalse,
Reason: "DeniedReason",
Message: "The certificate request has failed to complete and will be retried: The certificate request has been denied",
LastTransitionTime: &metaFixedClockStart,
ObservedGeneration: 3,
}),
gen.SetCertificateLastFailureTime(metaFixedClockStart),
gen.SetCertificateIssuanceAttempts(pointer.Int(1)),
),
)),
},
ExpectedEvents: []string{
"Warning DeniedReason The certificate request has failed to complete and will be retried: The certificate request has been denied",
},
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest that has been issued, but also has a Denied condition, report denial and set last failed time and issuance attempts": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{
gen.CertificateFrom(issuingCert),
gen.CertificateRequestFrom(exampleBundle.CertificateRequest,
gen.AddCertificateRequestAnnotations(map[string]string{
cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionDenied,
Status: cmmeta.ConditionTrue,
Reason: "DeniedReason",
Message: "The certificate request has been denied",
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionReady,
Status: cmmeta.ConditionTrue,
Reason: "Issued",
Message: "The certificate request has been issued",
}),
)},
KubeObjects: []runtime.Object{
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: nextPrivateKeySecretName,
Namespace: exampleBundle.Certificate.Namespace,
},
Data: map[string][]byte{
corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes,
},
},
},
ExpectedActions: []testpkg.Action{
testpkg.NewAction(coretesting.NewUpdateSubresourceAction(
cmapi.SchemeGroupVersion.WithResource("certificates"),
"status",
exampleBundle.Certificate.Namespace,
gen.CertificateFrom(exampleBundle.Certificate,
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{
Type: cmapi.CertificateConditionIssuing,
Status: cmmeta.ConditionFalse,
Reason: "DeniedReason",
Message: "The certificate request has failed to complete and will be retried: The certificate request has been denied",
LastTransitionTime: &metaFixedClockStart,
ObservedGeneration: 3,
}),
gen.SetCertificateLastFailureTime(metaFixedClockStart),
gen.SetCertificateIssuanceAttempts(pointer.Int(1)),
),
)),
},
ExpectedEvents: []string{
"Warning DeniedReason The certificate request has failed to complete and will be retried: The certificate request has been denied",
},
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest that has no ready condition and has been marked as invalid, report error and set last failed time and issuance attempts": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{
gen.CertificateFrom(issuingCert),
gen.CertificateRequestFrom(exampleBundle.CertificateRequest,
gen.AddCertificateRequestAnnotations(map[string]string{
cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionInvalidRequest,
Status: cmmeta.ConditionTrue,
Reason: "InvalidRequest",
Message: "The certificate request is invalid",
}),
)},
KubeObjects: []runtime.Object{
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: nextPrivateKeySecretName,
Namespace: exampleBundle.Certificate.Namespace,
},
Data: map[string][]byte{
corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes,
},
},
},
ExpectedActions: []testpkg.Action{
testpkg.NewAction(coretesting.NewUpdateSubresourceAction(
cmapi.SchemeGroupVersion.WithResource("certificates"),
"status",
exampleBundle.Certificate.Namespace,
gen.CertificateFrom(exampleBundle.Certificate,
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{
Type: cmapi.CertificateConditionIssuing,
Status: cmmeta.ConditionFalse,
Reason: "InvalidRequest",
Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid",
LastTransitionTime: &metaFixedClockStart,
ObservedGeneration: 3,
}),
gen.SetCertificateLastFailureTime(metaFixedClockStart),
gen.SetCertificateIssuanceAttempts(pointer.Int(1)),
),
)),
},
ExpectedEvents: []string{
"Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid",
},
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest that has a pending ready condition and has been marked as invalid, report error and set last failed time and issuance attempts": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{
gen.CertificateFrom(issuingCert),
gen.CertificateRequestFrom(exampleBundle.CertificateRequest,
gen.AddCertificateRequestAnnotations(map[string]string{
cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionInvalidRequest,
Status: cmmeta.ConditionTrue,
Reason: "InvalidRequest",
Message: "The certificate request is invalid",
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionReady,
Status: cmmeta.ConditionFalse,
Reason: "Pending",
Message: "The certificate request is being issued",
}),
)},
KubeObjects: []runtime.Object{
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: nextPrivateKeySecretName,
Namespace: exampleBundle.Certificate.Namespace,
},
Data: map[string][]byte{
corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes,
},
},
},
ExpectedActions: []testpkg.Action{
testpkg.NewAction(coretesting.NewUpdateSubresourceAction(
cmapi.SchemeGroupVersion.WithResource("certificates"),
"status",
exampleBundle.Certificate.Namespace,
gen.CertificateFrom(exampleBundle.Certificate,
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{
Type: cmapi.CertificateConditionIssuing,
Status: cmmeta.ConditionFalse,
Reason: "InvalidRequest",
Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid",
LastTransitionTime: &metaFixedClockStart,
ObservedGeneration: 3,
}),
gen.SetCertificateLastFailureTime(metaFixedClockStart),
gen.SetCertificateIssuanceAttempts(pointer.Int(1)),
),
)),
},
ExpectedEvents: []string{
"Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid",
},
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest that has been issued, but has also been marked as invalid, report error and set last failed time and issuance attempts": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{
gen.CertificateFrom(issuingCert),
gen.CertificateRequestFrom(exampleBundle.CertificateRequest,
gen.AddCertificateRequestAnnotations(map[string]string{
cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionInvalidRequest,
Status: cmmeta.ConditionTrue,
Reason: "InvalidRequest",
Message: "The certificate request is invalid",
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionReady,
Status: cmmeta.ConditionTrue,
Reason: "Issued",
Message: "The certificate request has been issued",
}),
)},
KubeObjects: []runtime.Object{
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: nextPrivateKeySecretName,
Namespace: exampleBundle.Certificate.Namespace,
},
Data: map[string][]byte{
corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes,
},
},
},
ExpectedActions: []testpkg.Action{
testpkg.NewAction(coretesting.NewUpdateSubresourceAction(
cmapi.SchemeGroupVersion.WithResource("certificates"),
"status",
exampleBundle.Certificate.Namespace,
gen.CertificateFrom(exampleBundle.Certificate,
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{
Type: cmapi.CertificateConditionIssuing,
Status: cmmeta.ConditionFalse,
Reason: "InvalidRequest",
Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid",
LastTransitionTime: &metaFixedClockStart,
ObservedGeneration: 3,
}),
gen.SetCertificateLastFailureTime(metaFixedClockStart),
gen.SetCertificateIssuanceAttempts(pointer.Int(1)),
),
)),
},
ExpectedEvents: []string{
"Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid",
},
},
expectedErr: false,
},
}
for name, test := range tests {