weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Issuing controller fails issuances for denied/invalid CRs

Browse Source 
This is not necessarily a breaking change as this appears to have been the current behaviour in most cases due to the race condition that this commit fixes

Signed-off-by: irbekrm <irbekrm@gmail.com>
This commit is contained in:
irbekrm 2023-03-24 15:37:57 +00:00
parent 0c071f8d2f
commit f5ea958317
2 changed files with 309 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
pkg/controller/certificates/issuing/issuing_controller.go
View File

@ -279,25 +279,31 @@ 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
// 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
}

286
pkg/controller/certificates/issuing/issuing_controller_test.go
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 {