From 241b0f9ff22aa5c3b12bd6f5775daa9f1b9454e4 Mon Sep 17 00:00:00 2001 From: Terin Stock Date: Thu, 16 May 2024 23:59:35 +0200 Subject: [PATCH] fix(cfapi): requeue after DB error The Origin CA API occasionally returns error code 1100 signaling a failure to write the certificate to the database. This reconciler intepreted this error as a permament failure, despite no local issues. This changeset detects this error code and requeues the CertificateRequest with controller-runtime's default backoff behavior. Fixes: #115 Fixes: #116 --- internal/cfapi/cfapi.go | 4 +- internal/cfapi/cfapi_test.go | 36 +++---- internal/cfapi/testing/cfapi.go | 15 --- pkgs/controllers/certificaterequest.go | 13 +++ pkgs/controllers/certificaterequest_test.go | 109 ++++++++++++++++---- 5 files changed, 119 insertions(+), 58 deletions(-) delete mode 100644 internal/cfapi/testing/cfapi.go diff --git a/internal/cfapi/cfapi.go b/internal/cfapi/cfapi.go index 0fd8e2a..a9bdc39 100644 --- a/internal/cfapi/cfapi.go +++ b/internal/cfapi/cfapi.go @@ -85,7 +85,7 @@ type APIError struct { RayID string `json:"-"` } -func (a *APIError) Error() string { +func (a APIError) Error() string { return fmt.Sprintf("Cloudflare API Error code=%d message=%s ray_id=%s", a.Code, a.Message, a.RayID) } @@ -117,7 +117,7 @@ func (c *Client) Sign(ctx context.Context, req *SignRequest) (*SignResponse, err } if !api.Success { - err := &api.Errors[0] + err := api.Errors[0] err.RayID = rayID return nil, err } diff --git a/internal/cfapi/cfapi_test.go b/internal/cfapi/cfapi_test.go index 6d2ac03..2ced810 100644 --- a/internal/cfapi/cfapi_test.go +++ b/internal/cfapi/cfapi_test.go @@ -9,7 +9,7 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" + "gotest.tools/v3/assert" ) func TestSignResponse_Unmarshal(t *testing.T) { @@ -59,13 +59,8 @@ func TestSignResponse_Unmarshal(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var resp SignResponse - if err := json.Unmarshal(tt.payload, &resp); err != nil { - t.Fatalf("unable to unmarsahl: %s", err) - } - - if diff := cmp.Diff(resp, expected); diff != "" { - t.Fatalf("diff: (-want +got)\n%s", diff) - } + assert.NilError(t, json.Unmarshal(tt.payload, &resp)) + assert.DeepEqual(t, resp, expected) }) } } @@ -73,10 +68,11 @@ func TestSignResponse_Unmarshal(t *testing.T) { func TestSign(t *testing.T) { expectedTime := time.Date(2020, time.December, 25, 6, 27, 0, 0, time.UTC) tests := []struct { - name string - handler http.Handler - response *SignResponse - error string + name string + handler http.Handler + response *SignResponse + error string + errorType error }{ {name: "API success", handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -118,8 +114,9 @@ func TestSign(t *testing.T) { "result": {} }`) }), - response: nil, - error: "Cloudflare API Error code=9001 message=Over Nine Thousand! ray_id=0123456789abcdef-ABC", + response: nil, + error: "Cloudflare API Error code=9001 message=Over Nine Thousand! ray_id=0123456789abcdef-ABC", + errorType: APIError{}, }, } @@ -140,14 +137,13 @@ func TestSign(t *testing.T) { CSR: "Lorem ipsum dolor sit amet, consectetur adipiscing elit.", }) - if diff := cmp.Diff(resp, tt.response); diff != "" { - t.Fatalf("diff: (-want +got)\n%s", diff) - } + assert.DeepEqual(t, resp, tt.response) if tt.error != "" { - if diff := cmp.Diff(err.Error(), tt.error); diff != "" { - t.Fatalf("diff: (-want +got)\n%s", diff) - } + assert.Error(t, err, tt.error) + assert.ErrorType(t, err, tt.errorType) + } else { + assert.NilError(t, err) } }) } diff --git a/internal/cfapi/testing/cfapi.go b/internal/cfapi/testing/cfapi.go deleted file mode 100644 index 1420b93..0000000 --- a/internal/cfapi/testing/cfapi.go +++ /dev/null @@ -1,15 +0,0 @@ -package testingcfapi - -import ( - "context" - - "github.com/cloudflare/origin-ca-issuer/internal/cfapi" -) - -type FakeClient struct { - Response *cfapi.SignResponse -} - -func (f *FakeClient) Sign(context.Context, *cfapi.SignRequest) (*cfapi.SignResponse, error) { - return f.Response, nil -} diff --git a/pkgs/controllers/certificaterequest.go b/pkgs/controllers/certificaterequest.go index 07d408c..2f97ad5 100644 --- a/pkgs/controllers/certificaterequest.go +++ b/pkgs/controllers/certificaterequest.go @@ -2,11 +2,13 @@ package controllers import ( "context" + "errors" "fmt" cmutil "github.com/cert-manager/cert-manager/pkg/api/util" certmanager "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + "github.com/cloudflare/origin-ca-issuer/internal/cfapi" v1 "github.com/cloudflare/origin-ca-issuer/pkgs/apis/v1" "github.com/cloudflare/origin-ca-issuer/pkgs/provisioners" "github.com/go-logr/logr" @@ -17,6 +19,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +const originDBWriteErrorCode = 1100 + // CertificateRequestController implements a controller that reconciles CertificateRequests // that references this controller. type CertificateRequestController struct { @@ -135,6 +139,15 @@ func (r *CertificateRequestController) Reconcile(ctx context.Context, cr *certma } pem, err := p.Sign(ctx, cr) + + var apiError cfapi.APIError + if errors.As(err, &apiError) { + if apiError.Code == originDBWriteErrorCode { + log.Error(err, "requeue-ing after API error") + return reconcile.Result{}, err + } + } + if err != nil { log.Error(err, "failed to sign certificate request") _ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, certmanager.CertificateRequestReasonFailed, fmt.Sprintf("Failed to sign certificate request: %v", err)) diff --git a/pkgs/controllers/certificaterequest_test.go b/pkgs/controllers/certificaterequest_test.go index 36d2168..e4f7274 100644 --- a/pkgs/controllers/certificaterequest_test.go +++ b/pkgs/controllers/certificaterequest_test.go @@ -11,10 +11,9 @@ import ( cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" cmgen "github.com/cert-manager/cert-manager/test/unit/gen" "github.com/cloudflare/origin-ca-issuer/internal/cfapi" - fakeapi "github.com/cloudflare/origin-ca-issuer/internal/cfapi/testing" v1 "github.com/cloudflare/origin-ca-issuer/pkgs/apis/v1" "github.com/cloudflare/origin-ca-issuer/pkgs/provisioners" - "github.com/google/go-cmp/cmp" + "gotest.tools/v3/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -97,8 +96,8 @@ func TestCertificateRequestReconcile(t *testing.T) { Namespace: "default", }, Provisioner: (func() *provisioners.Provisioner { - c := &fakeapi.FakeClient{ - Response: &cfapi.SignResponse{ + c := SignerFunc(func(ctx context.Context, sr *cfapi.SignRequest) (*cfapi.SignResponse, error) { + return &cfapi.SignResponse{ Id: "1", Certificate: "bogus", Hostnames: []string{"example.com"}, @@ -106,8 +105,8 @@ func TestCertificateRequestReconcile(t *testing.T) { Type: "colemak", Validity: 0, CSR: "foobar", - }, - } + }, nil + }) p, err := provisioners.New(c, v1.RequestTypeOriginRSA, logf.Log) if err != nil { t.Fatalf("error creating provisioner: %s", err) @@ -134,6 +133,78 @@ func TestCertificateRequestReconcile(t *testing.T) { Name: "foobar", }, }, + { + name: "requeue after API error", + objects: []runtime.Object{ + cmgen.CertificateRequest("foobar", + cmgen.SetCertificateRequestNamespace("default"), + cmgen.SetCertificateRequestDuration(&metav1.Duration{Duration: 7 * 24 * time.Hour}), + cmgen.SetCertificateRequestCSR((func() []byte { + csr, _, err := cmgen.CSR(x509.ECDSA) + if err != nil { + t.Fatalf("creating CSR: %s", err) + } + + return csr + })()), + cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ + Name: "foobar", + Kind: "OriginIssuer", + Group: "cert-manager.k8s.cloudflare.com", + }), + ), + &v1.OriginIssuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foobar", + Namespace: "default", + }, + Spec: v1.OriginIssuerSpec{ + Auth: v1.OriginIssuerAuthentication{ + ServiceKeyRef: v1.SecretKeySelector{ + Name: "service-key-issuer", + Key: "key", + }, + }, + }, + Status: v1.OriginIssuerStatus{ + Conditions: []v1.OriginIssuerCondition{ + { + Type: v1.ConditionReady, + Status: v1.ConditionTrue, + }, + }, + }, + }, + }, + collection: provisioners.CollectionWith([]provisioners.CollectionItem{ + { + NamespacedName: types.NamespacedName{ + Name: "foobar", + Namespace: "default", + }, + Provisioner: (func() *provisioners.Provisioner { + c := SignerFunc(func(ctx context.Context, sr *cfapi.SignRequest) (*cfapi.SignResponse, error) { + return nil, cfapi.APIError{ + Code: 1100, + Message: "Failed to write certificate to Database", + RayID: "7d3eb086eedab98e", + } + }) + p, err := provisioners.New(c, v1.RequestTypeOriginRSA, logf.Log) + if err != nil { + t.Fatalf("error creating provisioner: %s", err) + } + + return p + }()), + }, + }), + namespaceName: types.NamespacedName{ + Namespace: "default", + Name: "foobar", + }, + error: "unable to sign request: Cloudflare API Error code=1100 message=Failed to write certificate to Database ray_id=7d3eb086eedab98e", + }, } for _, tt := range tests { @@ -156,24 +227,20 @@ func TestCertificateRequestReconcile(t *testing.T) { }) if err != nil { - if diff := cmp.Diff(err.Error(), tt.error); diff != "" { - t.Fatalf("diff: (-wanted +got)\n%s", diff) - } + assert.Error(t, err, tt.error) + } else { + assert.NilError(t, err) } got := &cmapi.CertificateRequest{} - if err := client.Get(context.TODO(), tt.namespaceName, got); err != nil { - t.Fatalf("expected to retrieve issuer from client: %s", err) - } - if diff := cmp.Diff(got.Status, tt.expected); diff != "" { - t.Fatalf("diff: (-want +got)\n%s", diff) - } - - if tt.error == "" { - if _, ok := controller.Collection.Load(tt.namespaceName); !ok { - t.Fatal("was unable to find provisioner") - } - } + assert.NilError(t, client.Get(context.TODO(), tt.namespaceName, got)) + assert.DeepEqual(t, got.Status, tt.expected) }) } } + +type SignerFunc func(context.Context, *cfapi.SignRequest) (*cfapi.SignResponse, error) + +func (f SignerFunc) Sign(ctx context.Context, req *cfapi.SignRequest) (*cfapi.SignResponse, error) { + return f(ctx, req) +}