Skip to content

Commit

Permalink
fix(cfapi): requeue after DB error
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@terinjokes
terinjokes committed May 17, 2024
1 parent 9cbc0d5 commit 241b0f9
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 58 deletions.
4 changes: 2 additions & 2 deletions internal/cfapi/cfapi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
}
Expand Down
36 changes: 16 additions & 20 deletions internal/cfapi/cfapi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"testing"
"time"

"github.com/google/go-cmp/cmp"
"gotest.tools/v3/assert"
)

func TestSignResponse_Unmarshal(t *testing.T) {
Expand Down Expand Up @@ -59,24 +59,20 @@ 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)
})
}
}

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) {
Expand Down Expand Up @@ -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{},
},
}

Expand All @@ -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)
}
})
}
Expand Down
15 changes: 0 additions & 15 deletions internal/cfapi/testing/cfapi.go
View file

This file was deleted.

13 changes: 13 additions & 0 deletions pkgs/controllers/certificaterequest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {
Expand Down Expand Up @@ -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))
Expand Down
109 changes: 88 additions & 21 deletions pkgs/controllers/certificaterequest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -97,17 +96,17 @@ 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"},
Expiration: time.Time{},
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)
Expand All @@ -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 {
Expand All @@ -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)
}

0 comments on commit 241b0f9

Please sign in to comment.