From 78b1b8d69dee55e7e19cbf6d9643d4377272af4e Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 14 Mar 2018 01:04:22 -0700 Subject: [PATCH] issuer/acme/dns: refactor provider construction Previously, each provider's package-level 'New' function was being called. That made mocking it out for a different function that records data or returns different output quite difficult. This PR introduces an additional layer of abstraction in the form of effectively a vtable struct for the dns providers. It's defaulted to the same package-level constructors as before, but unit tests in the dns package can easily override it. A new test for the previously-introduced route53 trimming behavior is also added. --- pkg/issuer/acme/dns/dns.go | 43 ++++++++++--- pkg/issuer/acme/dns/dns_test.go | 111 ++++++++++++++++++++++++++++++-- 2 files changed, 141 insertions(+), 13 deletions(-) diff --git a/pkg/issuer/acme/dns/dns.go b/pkg/issuer/acme/dns/dns.go index 22ec1f71e..10a1ab573 100644 --- a/pkg/issuer/acme/dns/dns.go +++ b/pkg/issuer/acme/dns/dns.go @@ -28,11 +28,25 @@ type solver interface { Timeout() (timeout, interval time.Duration) } +// dnsProviderConstructors defines how each provider may be constructed. +// It is useful for mocking out a given provider since an alternate set of +// constructors may be set. +type dnsProviderConstructors struct { + cloudDNS func(project string, serviceAccount []byte) (*clouddns.DNSProvider, error) + cloudFlare func(email, apikey string) (*cloudflare.DNSProvider, error) + route53 func(accessKey, secretKey, hostedZoneID, region string) (*route53.DNSProvider, error) + azureDNS func(clientID, clientSecret, subscriptionID, tenentID, resourceGroupName, hostedZoneName string) (*azuredns.DNSProvider, error) +} + +// Solver is a solver for the acme dns01 challenge. +// Given a Certificate object, it determines the correct DNS provider based on +// the certificate, and configures it based on the referenced issuer. type Solver struct { - issuer v1alpha1.GenericIssuer - client kubernetes.Interface - secretLister corev1listers.SecretLister - resourceNamespace string + issuer v1alpha1.GenericIssuer + client kubernetes.Interface + secretLister corev1listers.SecretLister + dnsProviderConstructors dnsProviderConstructors + resourceNamespace string } func (s *Solver) Present(ctx context.Context, crt *v1alpha1.Certificate, domain, token, key string) error { @@ -124,7 +138,7 @@ func (s *Solver) solverFor(crt *v1alpha1.Certificate, domain string) (solver, er } saBytes := saSecret.Data[providerConfig.CloudDNS.ServiceAccount.Key] - impl, err = clouddns.NewDNSProviderServiceAccountBytes(providerConfig.CloudDNS.Project, saBytes) + impl, err = s.dnsProviderConstructors.cloudDNS(providerConfig.CloudDNS.Project, saBytes) if err != nil { return nil, fmt.Errorf("error instantiating google clouddns challenge solver: %s", err.Error()) } @@ -137,7 +151,7 @@ func (s *Solver) solverFor(crt *v1alpha1.Certificate, domain string) (solver, er email := providerConfig.Cloudflare.Email apiKey := string(apiKeySecret.Data[providerConfig.Cloudflare.APIKey.Key]) - impl, err = cloudflare.NewDNSProviderCredentials(email, apiKey) + impl, err = s.dnsProviderConstructors.cloudFlare(email, apiKey) if err != nil { return nil, fmt.Errorf("error instantiating cloudflare challenge solver: %s", err.Error()) } @@ -152,7 +166,7 @@ func (s *Solver) solverFor(crt *v1alpha1.Certificate, domain string) (solver, er return nil, fmt.Errorf("error getting route53 secret access key: key '%s' not found in secret", providerConfig.Route53.SecretAccessKey.Key) } - impl, err = route53.NewDNSProviderAccessKey( + impl, err = s.dnsProviderConstructors.route53( strings.TrimSpace(providerConfig.Route53.AccessKeyID), strings.TrimSpace(string(secretAccessKeyBytes)), providerConfig.Route53.HostedZoneID, @@ -172,7 +186,7 @@ func (s *Solver) solverFor(crt *v1alpha1.Certificate, domain string) (solver, er return nil, fmt.Errorf("error getting azure dns client secret: key '%s' not found in secret", providerConfig.AzureDNS.ClientSecret.Key) } - impl, err = azuredns.NewDNSProviderCredentials( + impl, err = s.dnsProviderConstructors.azureDNS( providerConfig.AzureDNS.ClientID, string(clientSecretBytes), providerConfig.AzureDNS.SubscriptionID, @@ -188,5 +202,16 @@ func (s *Solver) solverFor(crt *v1alpha1.Certificate, domain string) (solver, er } func NewSolver(issuer v1alpha1.GenericIssuer, client kubernetes.Interface, secretLister corev1listers.SecretLister, resourceNamespace string) *Solver { - return &Solver{issuer, client, secretLister, resourceNamespace} + return &Solver{ + issuer, + client, + secretLister, + dnsProviderConstructors{ + clouddns.NewDNSProviderServiceAccountBytes, + cloudflare.NewDNSProviderCredentials, + route53.NewDNSProviderAccessKey, + azuredns.NewDNSProviderCredentials, + }, + resourceNamespace, + } } diff --git a/pkg/issuer/acme/dns/dns_test.go b/pkg/issuer/acme/dns/dns_test.go index f8bc7702f..94acd9832 100644 --- a/pkg/issuer/acme/dns/dns_test.go +++ b/pkg/issuer/acme/dns/dns_test.go @@ -1,6 +1,7 @@ package dns import ( + "errors" "reflect" "testing" @@ -11,7 +12,10 @@ import ( kubefake "k8s.io/client-go/kubernetes/fake" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" + "github.com/jetstack/cert-manager/pkg/issuer/acme/dns/azuredns" + "github.com/jetstack/cert-manager/pkg/issuer/acme/dns/clouddns" "github.com/jetstack/cert-manager/pkg/issuer/acme/dns/cloudflare" + "github.com/jetstack/cert-manager/pkg/issuer/acme/dns/route53" ) type fixture struct { @@ -29,6 +33,50 @@ type fixture struct { // certificate used in the test Certificate *v1alpha1.Certificate + + DNSProviders *fakeDNSProviders +} + +type fakeDNSProviderCall struct { + name string + args []interface{} +} + +type fakeDNSProviders struct { + constructors dnsProviderConstructors + calls []fakeDNSProviderCall +} + +func (f *fakeDNSProviders) call(name string, args ...interface{}) { + f.calls = append(f.calls, fakeDNSProviderCall{name: name, args: args}) +} + +func newFakeDNSProviders() *fakeDNSProviders { + f := &fakeDNSProviders{ + calls: []fakeDNSProviderCall{}, + } + f.constructors = dnsProviderConstructors{ + cloudDNS: func(project string, serviceAccount []byte) (*clouddns.DNSProvider, error) { + f.call("clouddns", project, serviceAccount) + return nil, nil + }, + cloudFlare: func(email, apikey string) (*cloudflare.DNSProvider, error) { + f.call("cloudflare", email, apikey) + if email == "" || apikey == "" { + return nil, errors.New("invalid email or apikey") + } + return nil, nil + }, + route53: func(accessKey, secretKey, hostedZoneID, region string) (*route53.DNSProvider, error) { + f.call("route53", accessKey, secretKey, hostedZoneID, region) + return nil, nil + }, + azureDNS: func(clientID, clientSecret, subscriptionID, tenentID, resourceGroupName, hostedZoneName string) (*azuredns.DNSProvider, error) { + f.call("azuredns", clientID, clientSecret, subscriptionID, tenentID, resourceGroupName, hostedZoneName) + return nil, nil + }, + } + return f } func (f *fixture) solver() *Solver { @@ -41,11 +89,16 @@ func (f *fixture) solver() *Solver { stopCh := make(chan struct{}) defer close(stopCh) sharedInformerFactory.Start(stopCh) + dnsProvider := f.DNSProviders + if dnsProvider == nil { + dnsProvider = newFakeDNSProviders() + } return &Solver{ - issuer: f.Issuer, - client: kubeClient, - secretLister: secretsLister, - resourceNamespace: f.ResourceNamespace, + issuer: f.Issuer, + client: kubeClient, + secretLister: secretsLister, + dnsProviderConstructors: dnsProvider.constructors, + resourceNamespace: f.ResourceNamespace, } } @@ -276,3 +329,53 @@ func TestSolverFor(t *testing.T) { t.Run(name, testFn(test)) } } + +func TestRoute53TrimCreds(t *testing.T) { + f := &fixture{ + Issuer: newIssuer("test", "default", []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "fake-route53", + Route53: &v1alpha1.ACMEIssuerDNS01ProviderRoute53{ + AccessKeyID: " test_with_spaces ", + Region: "us-west-2", + SecretAccessKey: v1alpha1.SecretKeySelector{ + LocalObjectReference: v1alpha1.LocalObjectReference{ + Name: "route53", + }, + Key: "secret", + }, + }, + }, + }), + SecretLister: []*corev1.Secret{newSecret("route53", "default", map[string][]byte{ + "secret": []byte("AKIENDINNEWLINE \n"), + })}, + ResourceNamespace: "default", + Certificate: newCertificate("test", "default", "example.com", nil, []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"example.com"}, + DNS01: &v1alpha1.ACMECertificateDNS01Config{ + Provider: "fake-route53", + }, + }, + }), + DNSProviders: newFakeDNSProviders(), + } + + s := f.solver() + _, err := s.solverFor(f.Certificate, "example.com") + if err != nil { + t.Fatalf("expected solverFor to not error, but got: %s", err) + } + + expectedR53Call := []fakeDNSProviderCall{ + { + name: "route53", + args: []interface{}{"test_with_spaces", "AKIENDINNEWLINE", "", "us-west-2"}, + }, + } + + if !reflect.DeepEqual(expectedR53Call, f.DNSProviders.calls) { + t.Fatalf("expected %+v == %+v", expectedR53Call, f.DNSProviders.calls) + } +}