From b898a2f9b9802f9a27bc2ffc53c6be542edfa5c3 Mon Sep 17 00:00:00 2001 From: Terin Stock Date: Sat, 1 Jun 2024 22:34:06 +0200 Subject: [PATCH] remove provisioners.Collection Fixes: ##109 --- cmd/controller/main.go | 19 ++--- pkgs/controllers/certificaterequest.go | 46 +++++++++-- pkgs/controllers/certificaterequest_test.go | 87 +++++++++------------ pkgs/controllers/originissuer.go | 32 ++------ pkgs/controllers/originissuer_suite_test.go | 20 ++--- pkgs/controllers/originissuer_test.go | 15 +--- pkgs/provisioners/origin.go | 44 ----------- 7 files changed, 100 insertions(+), 163 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 0d0daf5..7029e3d 100644 --- 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 index ac88fe3..b6a8032 100644 --- 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 index 03fc39a..5753426 100644 --- 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 index 6c629df..c1dae32 100644 --- 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 index 5175a98..e0dfb52 100644 --- 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 index 265bf44..4651c85 100644 --- 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") - } - } }) } } diff --git a/pkgs/provisioners/origin.go b/pkgs/provisioners/origin.go index e433327..6a7c69e 100644 --- a/pkgs/provisioners/origin.go +++ b/pkgs/provisioners/origin.go @@ -7,14 +7,12 @@ import ( "context" "fmt" "math" - "sync" certmanager "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/cert-manager/cert-manager/pkg/util/pki" "github.com/cloudflare/origin-ca-issuer/internal/cfapi" v1 "github.com/cloudflare/origin-ca-issuer/pkgs/apis/v1" "github.com/go-logr/logr" - "k8s.io/apimachinery/pkg/types" ) const ( @@ -24,30 +22,6 @@ const ( var allowedValidty = []int{7, 30, 90, 365, 730, 1095, 5475} -// Collection stores cached Provisioners, stored by namespaced names of the -// issuer. -type Collection struct { - m sync.Map -} - -// A CollectionItem allows for the namespaced name and provisioner to -// be stored together. -type CollectionItem struct { - NamespacedName types.NamespacedName - Provisioner *Provisioner -} - -// CollectionWith returns a Collection storing the provided provisioners. -func CollectionWith(items []CollectionItem) *Collection { - c := &Collection{} - - for _, i := range items { - c.Store(i.NamespacedName, i.Provisioner) - } - - return c -} - // Provisioner allows for CertificateRequests to be signed using the stored // Cloudflare API client. type Provisioner struct { @@ -73,24 +47,6 @@ func New(client Signer, reqType v1.RequestType, log logr.Logger) (*Provisioner, return p, nil } -// Store adds a provisioner to the collection. -func (c *Collection) Store(namespacedName types.NamespacedName, provisioner *Provisioner) { - c.m.Store(namespacedName, provisioner) -} - -// Load returns the stored provisioner, or returns false if nothing is cached with -// the proved namespaced name. -func (c *Collection) Load(namespacedName types.NamespacedName) (*Provisioner, bool) { - v, ok := c.m.Load(namespacedName) - if !ok { - return nil, ok - } - - p, ok := v.(*Provisioner) - - return p, ok -} - // Sign uses the Cloduflare API to sign a CertificateRequest. The validity of the CertificateRequest is // normalized to the closests validity allowed by the Cloudflare API, which make be significantly different // than the validity provided.