From fb32f46b013d5c33a0d65eef173be5538954fbe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20W=C3=B6hrl?= Date: Tue, 3 Oct 2023 13:25:42 +0200 Subject: [PATCH] fix: recover from previously failed attempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Lukas Wöhrl --- .../certificaterequest_controller.go | 16 +- .../certificaterequest_controller_test.go | 147 ++++++++++++++---- 2 files changed, 130 insertions(+), 33 deletions(-) diff --git a/pkg/controllers/certificaterequest_controller.go b/pkg/controllers/certificaterequest_controller.go index e9459e75..5d942464 100644 --- a/pkg/controllers/certificaterequest_controller.go +++ b/pkg/controllers/certificaterequest_controller.go @@ -156,8 +156,11 @@ func (r *CertificateRequestReconciler) Reconcile(ctx context.Context, req ctrl.R provisioner, ok := aws.GetProvisioner(issuerName) if !ok { err := fmt.Errorf("provisioner for %s not found", issuerName) - log.Error(err, "failed to retrieve provisioner") - _ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonFailed, "failed to retrieve provisioner") + + errorMsg := fmt.Sprintf("failed to retrieve provisioner (name: %s, namespace: %s)", issuerName.Name, issuerName.Namespace) + + log.Error(err, errorMsg) + _ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonFailed, errorMsg) return ctrl.Result{}, err } @@ -169,6 +172,15 @@ func (r *CertificateRequestReconciler) Reconcile(ctx context.Context, req ctrl.R cr.Status.Certificate = pem cr.Status.CA = ca + //has previous failed attempts + if cmutil.CertificateRequestHasCondition(cr, cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestReasonFailed, + Status: cmmeta.ConditionFalse, + }) { + //remove failed condition + cmutil.SetCertificateRequestCondition(cr, cmapi.CertificateRequestReasonFailed, cmmeta.ConditionTrue, cmapi.CertificateRequestReasonFailed, "certificate issued (after previous failed attempt)") + } + return ctrl.Result{}, r.setStatus(ctx, cr, cmmeta.ConditionTrue, cmapi.CertificateRequestReasonIssued, "certificate issued") } diff --git a/pkg/controllers/certificaterequest_controller_test.go b/pkg/controllers/certificaterequest_controller_test.go index 30a842df..e20258fe 100644 --- a/pkg/controllers/certificaterequest_controller_test.go +++ b/pkg/controllers/certificaterequest_controller_test.go @@ -1,14 +1,16 @@ /* - Copyright 2021. - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - http://www.apache.org/licenses/LICENSE-2.0 - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. +Copyright 2021. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. */ package controllers @@ -63,15 +65,17 @@ func TestProvisonerOperation(t *testing.T) { func TestCertificateRequestReconcile(t *testing.T) { type testCase struct { - name types.NamespacedName - objects []client.Object - expectedResult ctrl.Result - expectedError bool - expectedReadyConditionStatus cmmeta.ConditionStatus - expectedReadyConditionReason string - expectedCertificate []byte - expectedCACertificate []byte - mockProvisioner createMockProvisioner + name types.NamespacedName + objects []client.Object + expectedResult ctrl.Result + expectedError bool + expectedReadyConditionStatus cmmeta.ConditionStatus + expectedReadyConditionReason string + expectedFailedConditionStatus cmmeta.ConditionStatus + expectedFailedConditionReason string + expectedCertificate []byte + expectedCACertificate []byte + mockProvisioner createMockProvisioner } tests := map[string]testCase{ "success-issuer": { @@ -135,14 +139,15 @@ func TestCertificateRequestReconcile(t *testing.T) { }, }, "success-cluster-issuer": { - name: types.NamespacedName{Name: "cr1"}, + name: types.NamespacedName{Namespace: "ns1", Name: "cr1"}, objects: []client.Object{ cmgen.CertificateRequest( "cr1", + cmgen.SetCertificateRequestNamespace("ns1"), cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: "clusterissuer1", Group: issuerapi.GroupVersion.Group, - Kind: "ClusterIssuer", + Kind: "AWSPCAClusterIssuer", }), cmgen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionReady, @@ -190,6 +195,69 @@ func TestCertificateRequestReconcile(t *testing.T) { awspca.StoreProvisioner(types.NamespacedName{Name: "clusterissuer1"}, &fakeProvisioner{caCert: []byte("cacert"), cert: []byte("cert")}) }, }, + "success-previous-failure": { + name: types.NamespacedName{Namespace: "ns1", Name: "cr1"}, + objects: []client.Object{ + cmgen.CertificateRequest( + "cr1", + cmgen.SetCertificateRequestNamespace("ns1"), + cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ + Name: "clusterissuer1", + Group: issuerapi.GroupVersion.Group, + Kind: "AWSPCAClusterIssuer", + }), + cmgen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + }), + cmgen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestReasonFailed, + Status: cmmeta.ConditionFalse, + }), + ), + &issuerapi.AWSPCAClusterIssuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "clusterissuer1", + }, + Spec: issuerapi.AWSPCAIssuerSpec{ + SecretRef: issuerapi.AWSCredentialsSecretReference{ + SecretReference: v1.SecretReference{ + Name: "clusterissuer1-credentials", + }, + }, + Region: "us-east-1", + Arn: "arn:aws:acm-pca:us-east-1:account:certificate-authority/12345678-1234-1234-1234-123456789012", + }, + Status: issuerapi.AWSPCAIssuerStatus{ + Conditions: []metav1.Condition{ + { + Type: issuerapi.ConditionTypeReady, + Status: metav1.ConditionTrue, + }, + }, + }, + }, + &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "clusterissuer1-credentials", + }, + Data: map[string][]byte{ + "AWS_ACCESS_KEY_ID": []byte("ZXhhbXBsZQ=="), + "AWS_SECRET_ACCESS_KEY": []byte("ZXhhbXBsZQ=="), + }, + }, + }, + expectedReadyConditionStatus: cmmeta.ConditionTrue, + expectedReadyConditionReason: cmapi.CertificateRequestReasonIssued, + expectedFailedConditionStatus: cmmeta.ConditionTrue, + expectedFailedConditionReason: cmapi.CertificateRequestReasonFailed, + expectedError: false, + expectedCertificate: []byte("cert"), + expectedCACertificate: []byte("cacert"), + mockProvisioner: func() { + awspca.StoreProvisioner(types.NamespacedName{Name: "clusterissuer1"}, &fakeProvisioner{caCert: []byte("cacert"), cert: []byte("cert")}) + }, + }, "failure-issuer-not-ready": { name: types.NamespacedName{Namespace: "ns1", Name: "cr1"}, objects: []client.Object{ @@ -404,16 +472,19 @@ func TestCertificateRequestReconcile(t *testing.T) { var cr cmapi.CertificateRequest err = fakeClient.Get(ctx, tc.name, &cr) require.NoError(t, client.IgnoreNotFound(err), "unexpected error from fake client") - if err == nil { - if tc.expectedReadyConditionStatus != "" { - assertCertificateRequestHasReadyCondition(t, tc.expectedReadyConditionStatus, tc.expectedReadyConditionReason, &cr) - } - if tc.expectedCertificate != nil { - assert.Equal(t, tc.expectedCertificate, cr.Status.Certificate) - } - if tc.expectedCACertificate != nil { - assert.Equal(t, tc.expectedCACertificate, cr.Status.CA) - } + if tc.expectedReadyConditionStatus != "" { + assertCertificateRequestHasReadyCondition(t, tc.expectedReadyConditionStatus, tc.expectedReadyConditionReason, &cr) + } + + if tc.expectedFailedConditionStatus != "" { + assertCertificateRequestHasFailedCondition(t, tc.expectedFailedConditionStatus, tc.expectedFailedConditionReason, &cr) + } + + if tc.expectedCertificate != nil { + assert.Equal(t, tc.expectedCertificate, cr.Status.Certificate) + } + if tc.expectedCACertificate != nil { + assert.Equal(t, tc.expectedCACertificate, cr.Status.CA) } }) } @@ -432,3 +503,17 @@ func assertCertificateRequestHasReadyCondition(t *testing.T, status cmmeta.Condi assert.Contains(t, validReasons, reason, "unexpected condition reason") assert.Equal(t, reason, condition.Reason, "unexpected condition reason") } + +func assertCertificateRequestHasFailedCondition(t *testing.T, status cmmeta.ConditionStatus, reason string, cr *cmapi.CertificateRequest) { + condition := cmutil.GetCertificateRequestCondition(cr, cmapi.CertificateRequestReasonFailed) + if !assert.NotNil(t, condition, "Failed condition not found") { + return + } + assert.Equal(t, status, condition.Status, "unexpected condition status") + validReasons := sets.NewString( + cmapi.CertificateRequestReasonFailed, + cmapi.CertificateRequestReasonIssued, + ) + assert.Contains(t, validReasons, reason, "unexpected condition reason") + assert.Equal(t, reason, condition.Reason, "unexpected condition reason") +}