feat: remove provisioners.Collection

The controller was creating an instance of a signing provisioner in the OriginIssuer reconciler, as an attempt to optimize against creating new API clients. As the certificates issued by the Origin API are valid for a minimium of 7 days this seems like a premature optimization. As the reconciler was not watching the service key secret for updates, it was also persisting stale API tokens. This changeset removes the concept of the provisioner collection. A new API client and provisioner will be created as needed in the OriginIssuer reconciler. This allows users to update their API token without needing to restart the controller. Fixes: ##109
cloudflare · Jun 1, 2024 · fd52ada · fd52ada
1 parent 364ab34
commit fd52ada
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 163 deletions.
diff --git a/cmd/controller/main.go b/cmd/controller/main.go
@@ -10,7 +10,6 @@ import (
 	"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/controllers"
-	"github.com/cloudflare/origin-ca-issuer/pkgs/provisioners"
 	"github.com/go-logr/zerologr"
 	"github.com/rs/zerolog"
 	"github.com/spf13/pflag"
@@ -76,8 +75,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	collection := provisioners.CollectionWith(nil)
-
 	httpClient := &http.Client{
 		Timeout: 30 * time.Second,
 	}
@@ -89,11 +86,11 @@ func main() {
 		ControllerManagedBy(mgr).
 		For(&v1.OriginIssuer{}).
 		Complete(reconcile.AsReconciler(mgr.GetClient(), &controllers.OriginIssuerController{
-			Client:     mgr.GetClient(),
-			Clock:      clock.RealClock{},
-			Factory:    f,
-			Log:        log.WithName("controllers").WithName("OriginIssuer"),
-			Collection: collection,
+			Client:  mgr.GetClient(),
+			Reader:  mgr.GetAPIReader(),
+			Clock:   clock.RealClock{},
+			Factory: f,
+			Log:     log.WithName("controllers").WithName("OriginIssuer"),
 		}))
 
 	if err != nil {
@@ -105,9 +102,9 @@ func main() {
 		ControllerManagedBy(mgr).
 		For(&certmanager.CertificateRequest{}).
 		Complete(reconcile.AsReconciler(mgr.GetClient(), &controllers.CertificateRequestController{
-			Client:     mgr.GetClient(),
-			Log:        log.WithName("controllers").WithName("CertificateRequest"),
-			Collection: collection,
+			Client: mgr.GetClient(),
+			Reader: mgr.GetAPIReader(),
+			Log:    log.WithName("controllers").WithName("CertificateRequest"),
 
 			Clock:                  clock.RealClock{},
 			CheckApprovedCondition: !o.DisableApprovedCheck,

diff --git a/pkgs/controllers/certificaterequest.go b/pkgs/controllers/certificaterequest.go
@@ -12,6 +12,8 @@ import (
 	v1 "github.com/cloudflare/origin-ca-issuer/pkgs/apis/v1"
 	"github.com/cloudflare/origin-ca-issuer/pkgs/provisioners"
 	"github.com/go-logr/logr"
+	core "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/clock"
@@ -25,8 +27,9 @@ const originDBWriteErrorCode = 1100
 // that references this controller.
 type CertificateRequestController struct {
 	client.Client
-	Log        logr.Logger
-	Collection *provisioners.Collection
+	Reader  client.Reader
+	Log     logr.Logger
+	Factory cfapi.Factory
 
 	Clock                  clock.Clock
 	CheckApprovedCondition bool
@@ -128,12 +131,43 @@ func (r *CertificateRequestController) Reconcile(ctx context.Context, cr *certma
 		return reconcile.Result{}, err
 	}
 
-	p, ok := r.Collection.Load(issNamespaceName)
+	secret := core.Secret{}
+	secretNamespaceName := types.NamespacedName{
+		Namespace: iss.Namespace,
+		Name:      iss.Spec.Auth.ServiceKeyRef.Name,
+	}
+	if err := r.Reader.Get(ctx, secretNamespaceName, &secret); err != nil {
+		log.Error(err, "failed to retieve OriginIssuer auth secret", "namespace", secretNamespaceName.Namespace, "name", secretNamespaceName.Name)
+		if apierrors.IsNotFound(err) {
+			_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
+		} else {
+			_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "Error", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
+		}
+
+		return reconcile.Result{}, err
+	}
+
+	serviceKey, ok := secret.Data[iss.Spec.Auth.ServiceKeyRef.Key]
 	if !ok {
-		err := fmt.Errorf("provisioner %s not found", issNamespaceName)
-		log.Error(err, "failed to load provisioner for OriginIssuer resource")
+		err := fmt.Errorf("secret %s does not contain key %q", secret.Name, iss.Spec.Auth.ServiceKeyRef.Key)
+		log.Error(err, "failed to retrieve OriginIssuer auth secret")
+		_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
+
+		return reconcile.Result{}, err
+	}
+
+	c, err := r.Factory.APIWith(serviceKey)
+	if err != nil {
+		log.Error(err, "failed to create API client")
+
+		return reconcile.Result{}, err
+	}
+
+	p, err := provisioners.New(c, iss.Spec.RequestType, log)
+	if err != nil {
+		log.Error(err, "failed to create provisioner")
 
-		_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, certmanager.CertificateRequestReasonPending, fmt.Sprintf("Failed to load provisioner for OriginIssuer resource %s", issNamespaceName))
+		_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "Error", "Failed initialize provisioner")
 
 		return reconcile.Result{}, err
 	}

diff --git a/pkgs/controllers/certificaterequest_test.go b/pkgs/controllers/certificaterequest_test.go
@@ -12,8 +12,8 @@ import (
 	cmgen "github.com/cert-manager/cert-manager/test/unit/gen"
 	"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"
 	"gotest.tools/v3/assert"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -41,7 +41,7 @@ func TestCertificateRequestReconcile(t *testing.T) {
 	tests := []struct {
 		name          string
 		objects       []runtime.Object
-		collection    *provisioners.Collection
+		signer        SignerFunc
 		expected      cmapi.CertificateRequestStatus
 		error         string
 		namespaceName types.NamespacedName
@@ -88,33 +88,26 @@ func TestCertificateRequestReconcile(t *testing.T) {
 						},
 					},
 				},
-			},
-			collection: provisioners.CollectionWith([]provisioners.CollectionItem{
-				{
-					NamespacedName: types.NamespacedName{
-						Name:      "foobar",
+				&corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "service-key-issuer",
 						Namespace: "default",
 					},
-					Provisioner: (func() *provisioners.Provisioner {
-						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)
-						}
-
-						return p
-					}()),
+					Data: map[string][]byte{
+						"key": []byte("djEuMC0weDAwQkFCMTBD"),
+					},
 				},
+			},
+			signer: 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
 			}),
 			expected: cmapi.CertificateRequestStatus{
 				Conditions: []cmapi.CertificateRequestCondition{
@@ -175,29 +168,22 @@ func TestCertificateRequestReconcile(t *testing.T) {
 						},
 					},
 				},
-			},
-			collection: provisioners.CollectionWith([]provisioners.CollectionItem{
-				{
-					NamespacedName: types.NamespacedName{
-						Name:      "foobar",
+				&corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "service-key-issuer",
 						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
-					}()),
+					Data: map[string][]byte{
+						"key": []byte("djEuMC0weDAwQkFCMTBD"),
+					},
 				},
+			},
+			signer: 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",
+				}
 			}),
 			namespaceName: types.NamespacedName{
 				Namespace: "default",
@@ -217,9 +203,12 @@ func TestCertificateRequestReconcile(t *testing.T) {
 				Build()
 
 			controller := &CertificateRequestController{
-				Client:     client,
-				Log:        logf.Log,
-				Collection: tt.collection,
+				Client: client,
+				Reader: client,
+				Log:    logf.Log,
+				Factory: cfapi.FactoryFunc(func(serviceKey []byte) (cfapi.Interface, error) {
+					return tt.signer, nil
+				}),
 			}
 
 			_, err := reconcile.AsReconciler(client, controller).Reconcile(context.Background(), reconcile.Request{

diff --git a/pkgs/controllers/originissuer.go b/pkgs/controllers/originissuer.go
@@ -6,7 +6,6 @@ import (
 
 	"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"
 	core "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -20,10 +19,10 @@ import (
 // to OriginIssuer resources.
 type OriginIssuerController struct {
 	client.Client
-	Log        logr.Logger
-	Clock      clock.Clock
-	Factory    cfapi.Factory
-	Collection *provisioners.Collection
+	Reader  client.Reader
+	Log     logr.Logger
+	Clock   clock.Clock
+	Factory cfapi.Factory
 }
 
 //go:generate controller-gen rbac:roleName=originissuer-control paths=./. output:rbac:artifacts:config=../../deploy/rbac
@@ -49,7 +48,7 @@ func (r *OriginIssuerController) Reconcile(ctx context.Context, iss *v1.OriginIs
 		Name:      iss.Spec.Auth.ServiceKeyRef.Name,
 	}
 
-	if err := r.Client.Get(ctx, secretNamespaceName, &secret); err != nil {
+	if err := r.Reader.Get(ctx, secretNamespaceName, &secret); err != nil {
 		log.Error(err, "failed to retieve OriginIssuer auth secret", "namespace", secretNamespaceName.Namespace, "name", secretNamespaceName.Name)
 
 		if apierrors.IsNotFound(err) {
@@ -61,7 +60,7 @@ func (r *OriginIssuerController) Reconcile(ctx context.Context, iss *v1.OriginIs
 		return reconcile.Result{}, err
 	}
 
-	serviceKey, ok := secret.Data[iss.Spec.Auth.ServiceKeyRef.Key]
+	_, ok := secret.Data[iss.Spec.Auth.ServiceKeyRef.Key]
 	if !ok {
 		err := fmt.Errorf("secret %s does not contain key %q", secret.Name, iss.Spec.Auth.ServiceKeyRef.Key)
 		log.Error(err, "failed to retrieve OriginIssuer auth secret")
@@ -70,25 +69,6 @@ func (r *OriginIssuerController) Reconcile(ctx context.Context, iss *v1.OriginIs
 		return reconcile.Result{}, err
 	}
 
-	c, err := r.Factory.APIWith(serviceKey)
-	if err != nil {
-		log.Error(err, "failed to create API client")
-
-		return reconcile.Result{}, err
-	}
-
-	p, err := provisioners.New(c, iss.Spec.RequestType, log)
-	if err != nil {
-		log.Error(err, "failed to create provisioner")
-
-		_ = r.setStatus(ctx, iss, v1.ConditionFalse, "Error", "Failed initialize provisioner")
-
-		return reconcile.Result{}, err
-	}
-
-	// TODO: GC these references once the OriginIssuer has been removed.
-	r.Collection.Store(types.NamespacedName{Name: iss.Name, Namespace: iss.Namespace}, p)
-
 	return reconcile.Result{}, r.setStatus(ctx, iss, v1.ConditionTrue, "Verified", "OriginIssuer verified and ready to sign certificates")
 }
 

diff --git a/pkgs/controllers/originissuer_suite_test.go b/pkgs/controllers/originissuer_suite_test.go
@@ -14,7 +14,6 @@ import (
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/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/zerologr"
 	"github.com/rs/zerolog"
 	corev1 "k8s.io/api/core/v1"
@@ -94,11 +93,11 @@ func TestOriginIssuerReconcileSuite(t *testing.T) {
 	})
 
 	controller := &OriginIssuerController{
-		Client:     c,
-		Clock:      clock.RealClock{},
-		Factory:    f,
-		Log:        logf.Log,
-		Collection: provisioners.CollectionWith(nil),
+		Client:  c,
+		Reader:  c,
+		Clock:   clock.RealClock{},
+		Factory: f,
+		Log:     logf.Log,
 	}
 
 	builder.ControllerManagedBy(mgr).
@@ -137,15 +136,6 @@ func TestOriginIssuerReconcileSuite(t *testing.T) {
 
 		return IssuerHasCondition(iss, v1.OriginIssuerCondition{Type: v1.ConditionReady, Status: v1.ConditionTrue})
 	}, 5*time.Second, 10*time.Millisecond, "OriginIssuer reconciler")
-
-	_, ok := controller.Collection.Load(types.NamespacedName{
-		Namespace: issuer.Namespace,
-		Name:      issuer.Name,
-	})
-
-	if !ok {
-		t.Fatal("was unable to find provisioner")
-	}
 }
 
 func StartTestManager(mgr manager.Manager, t *testing.T) (context.CancelFunc, chan error) {

diff --git a/pkgs/controllers/originissuer_test.go b/pkgs/controllers/originissuer_test.go
@@ -8,7 +8,6 @@ import (
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/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/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -174,16 +173,14 @@ func TestOriginIssuerReconcile(t *testing.T) {
 				WithStatusSubresource(&v1.OriginIssuer{}).
 				Build()
 
-			collection := provisioners.CollectionWith(nil)
-
 			controller := &OriginIssuerController{
 				Client: client,
+				Reader: client,
 				Factory: cfapi.FactoryFunc(func(serviceKey []byte) (cfapi.Interface, error) {
 					return nil, nil
 				}),
-				Clock:      clock,
-				Log:        logf.Log,
-				Collection: collection,
+				Clock: clock,
+				Log:   logf.Log,
 			}
 
 			_, err := reconcile.AsReconciler(client, controller).Reconcile(context.Background(), reconcile.Request{
@@ -203,12 +200,6 @@ func TestOriginIssuerReconcile(t *testing.T) {
 			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")
-				}
-			}
 		})
 	}
 }