Remove direct calls to external deps from Setup function

Allow the functionality to set up a new ACME client and to retrieve and decode ACME account's key to be stubbed in tests Signed-off-by: irbekrm <irbekrm@gmail.com>
Art4Tech · May 10, 2021 · d8367cb · d8367cb
1 parent 3434c78
commit d8367cb
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 6 deletions.
diff --git a/pkg/acme/accounts/client.go b/pkg/acme/accounts/client.go
@@ -32,7 +32,12 @@ import (
 	"github.com/jetstack/cert-manager/pkg/util"
 )
 
-// NewClient will return a new ACME client.
+// NewClientFunc is a function type for building a new ACME client.
+type NewClientFunc func(*http.Client, cmacme.ACMEIssuer, *rsa.PrivateKey) acmecl.Interface
+
+var _ NewClientFunc = NewClient
+
+// NewClient is an implementation of NewClientFunc that returns a real ACME client.
 func NewClient(client *http.Client, config cmacme.ACMEIssuer, privateKey *rsa.PrivateKey) acmecl.Interface {
 	return &acmeapi.Client{
 		Key:          privateKey,

diff --git a/pkg/issuer/acme/acme.go b/pkg/issuer/acme/acme.go
@@ -17,6 +17,8 @@ limitations under the License.
 package acme
 
 import (
+	"context"
+	"crypto"
 	"fmt"
 
 	core "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -29,6 +31,7 @@ import (
 	"github.com/jetstack/cert-manager/pkg/controller"
 	"github.com/jetstack/cert-manager/pkg/issuer"
 	"github.com/jetstack/cert-manager/pkg/metrics"
+	"github.com/jetstack/cert-manager/pkg/util/kube"
 )
 
 // Acme is an issuer for an ACME server. It can be used to register and obtain
@@ -37,10 +40,16 @@ import (
 type Acme struct {
 	issuer v1.GenericIssuer
 
-	secretsLister corelisters.SecretLister
 	secretsClient core.SecretsGetter
 	recorder      record.EventRecorder
 
+	// keyFromSecret returns a decoded account key from a Kubernetes secret.
+	// It can be stubbed in unit tests.
+	keyFromSecret keyFromSecretFunc
+
+	// clientBuilder builds a new ACME client.
+	clientBuilder accounts.NewClientFunc
+
 	// namespace of referenced resources when the given issuer is a ClusterIssuer
 	clusterResourceNamespace string
 	// used as a cache for ACME clients
@@ -60,7 +69,8 @@ func New(ctx *controller.Context, issuer v1.GenericIssuer) (issuer.Interface, er
 
 	a := &Acme{
 		issuer:                   issuer,
-		secretsLister:            secretsLister,
+		keyFromSecret:            newKeyFromSecret(secretsLister),
+		clientBuilder:            accounts.NewClient,
 		secretsClient:            ctx.Client.CoreV1(),
 		recorder:                 ctx.Recorder,
 		clusterResourceNamespace: ctx.IssuerOptions.ClusterResourceNamespace,
@@ -71,6 +81,17 @@ func New(ctx *controller.Context, issuer v1.GenericIssuer) (issuer.Interface, er
 	return a, nil
 }
 
+// keyFromSecretFunc accepts name, namespace and keyName for secret, verifies
+// and returns a private key stored at keyName.
+type keyFromSecretFunc func(ctx context.Context, namespace, name, keyName string) (crypto.Signer, error)
+
+// newKeyFromSecret returns an implementation of keyFromSecretFunc for a secrets lister.
+func newKeyFromSecret(secretLister corelisters.SecretLister) keyFromSecretFunc {
+	return func(ctx context.Context, namespace, name, keyName string) (crypto.Signer, error) {
+		return kube.SecretTLSKeyRef(ctx, secretLister, namespace, name, keyName)
+	}
+}
+
 // Register this Issuer with the issuer factory
 func init() {
 	issuer.RegisterIssuer(apiutil.IssuerACME, New)

diff --git a/pkg/issuer/acme/setup.go b/pkg/issuer/acme/setup.go
@@ -37,7 +37,6 @@ import (
 	cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1"
 	logf "github.com/jetstack/cert-manager/pkg/logs"
 	"github.com/jetstack/cert-manager/pkg/util/errors"
-	"github.com/jetstack/cert-manager/pkg/util/kube"
 	"github.com/jetstack/cert-manager/pkg/util/pki"
 )
 
@@ -84,7 +83,7 @@ func (a *Acme) Setup(ctx context.Context) error {
 	// if it contains invalid data, warn the user and return without error.
 	// if any other error occurs, return it and retry.
 	privateKeySelector := acme.PrivateKeySelector(a.issuer.GetSpec().ACME.PrivateKey)
-	pk, err := kube.SecretTLSKeyRef(ctx, a.secretsLister, ns, privateKeySelector.Name, privateKeySelector.Key)
+	pk, err := a.keyFromSecret(ctx, ns, privateKeySelector.Name, privateKeySelector.Key)
 	switch {
 	case !a.issuer.GetSpec().ACME.DisableAccountKeyGeneration && apierrors.IsNotFound(err):
 		log.V(logf.InfoLevel).Info("generating acme account private key")
@@ -122,7 +121,7 @@ func (a *Acme) Setup(ctx context.Context) error {
 	//  and remove them when the corresponding issuer is updated/deleted.
 	a.accountRegistry.RemoveClient(string(a.issuer.GetUID()))
 	httpClient := accounts.BuildHTTPClient(a.metrics, a.issuer.GetSpec().ACME.SkipTLSVerify)
-	cl := accounts.NewClient(httpClient, *a.issuer.GetSpec().ACME, rsaPk)
+	cl := a.clientBuilder(httpClient, *a.issuer.GetSpec().ACME, rsaPk)
 
 	// TODO: perform a complex check to determine whether we need to verify
 	// the existing registration with the ACME server.
@@ -322,6 +321,7 @@ func (a *Acme) registerAccount(ctx context.Context, cl client.Interface, eabAcco
 		ExternalAccountBinding: eabAccount,
 	}
 
+	// private key, server URL and HTTP options are stored in the ACME client (cl).
 	acc, err := cl.Register(ctx, acc, acmeapi.AcceptTOS)
 	// If the account already exists, fetch the Account object and return.
 	if err == acmeapi.ErrAccountAlreadyExists {