Add basic resource validation at start of sync loops

pkg/apis/certmanager/validation/certificate.go

@@ -0,0 +1,123 @@
+package validation
+
+import (
+	"fmt"
+
+	"github.com/golang/glog"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+)
+
+// Validation functions for cert-manager v1alpha1 Certificate types
+
+func ValidateCertificate(crt *v1alpha1.Certificate) field.ErrorList {
+	allErrs := ValidateCertificateSpec(&crt.Spec, field.NewPath("spec"))
+	return allErrs
+}
+
+func ValidateCertificateSpec(crt *v1alpha1.CertificateSpec, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if crt.SecretName == "" {
+		el = append(el, field.Required(fldPath.Child("secretName"), "must be specified"))
+	}
+	issuerRefPath := fldPath.Child("issuerRef")
+	if crt.IssuerRef.Name == "" {
+		el = append(el, field.Required(issuerRefPath.Child("name"), "must be specified"))
+	}
+	switch crt.IssuerRef.Kind {
+	case "":
+		// For now we disable this check in order to support older versions where
+		// defaulting doesn't occur
+		glog.Infof("Certificate does not set issuerRef.kind - " +
+			"in future versions of cert-manager, this will be a hard failure.")
+		// el = append(el, field.Required(issuerRefPath.Child("kind"), "must be specified"))
+	case "Issuer", "ClusterIssuer":
+	default:
+		el = append(el, field.Invalid(issuerRefPath.Child("kind"), crt.IssuerRef.Kind, "must be one of Issuer or ClusterIssuer"))
+	}
+	if len(crt.CommonName) == 0 && len(crt.DNSNames) == 0 {
+		el = append(el, field.Required(fldPath.Child("dnsNames"), "at least one dnsName is required if commonName is not set"))
+	}
+	if crt.ACME != nil {
+		el = append(el, validateACMEConfigForAllDNSNames(crt, fldPath)...)
+		el = append(el, ValidateACMECertificateConfig(crt.ACME, fldPath.Child("acme"))...)
+	}
+	return el
+}
+
+// validateACMEConfigForAllDNSNames will ensure that if the provided Certificate
+// specifies any ACME configuration, all domains listed on the Certificate have
+// a configuration entry.
+func validateACMEConfigForAllDNSNames(a *v1alpha1.CertificateSpec, fldPath *field.Path) field.ErrorList {
+	if a.ACME == nil {
+		return nil
+	}
+	el := field.ErrorList{}
+	acmeFldPath := fldPath.Child("acme")
+	errFn := func(s string) string {
+		return fmt.Sprintf("no ACME solver configuration specified for domain %q", s)
+	}
+	if a.CommonName != "" {
+		cfg := a.ACME.ConfigForDomain(a.CommonName)
+		if cfg == nil || len(cfg.Domains) == 0 {
+			el = append(el, field.Required(acmeFldPath.Child("config"), errFn(a.CommonName)))
+		}
+	}
+	for _, d := range a.DNSNames {
+		cfg := a.ACME.ConfigForDomain(d)
+		if cfg == nil || len(cfg.Domains) == 0 {
+			el = append(el, field.Required(acmeFldPath.Child("config"), errFn(d)))
+		}
+	}
+	return el
+}
+
+func ValidateACMECertificateConfig(a *v1alpha1.ACMECertificateConfig, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	for i, cfg := range a.Config {
+		el = append(el, ValidateACMECertificateDomainConfig(&cfg, fldPath.Child("config").Index(i))...)
+	}
+	return el
+}
+
+func ValidateACMECertificateDomainConfig(a *v1alpha1.ACMECertificateDomainConfig, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if len(a.Domains) == 0 {
+		el = append(el, field.Required(fldPath.Child("domains"), "at least one domain must be specified"))
+	}
+	numTypes := 0
+	if a.DNS01 != nil {
+		numTypes++
+		el = append(el, ValidateACMECertificateDNS01Config(a.DNS01, fldPath.Child("dns01"))...)
+	}
+	if a.HTTP01 != nil {
+		if numTypes > 0 {
+			el = append(el, field.Forbidden(fldPath.Child("http01"), "may not specify more than one solver type"))
+		} else {
+			numTypes++
+			el = append(el, ValidateACMECertificateHTTP01Config(a.HTTP01, fldPath.Child("http01"))...)
+		}
+	}
+	if numTypes == 0 {
+		el = append(el, field.Required(fldPath, "at least one solver must be configured"))
+	}
+	return el
+}
+
+func ValidateACMECertificateDNS01Config(a *v1alpha1.ACMECertificateDNS01Config, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if a.Provider == "" {
+		el = append(el, field.Required(fldPath.Child("provider"), "provider name must be set"))
+	}
+	return el
+}
+
+func ValidateACMECertificateHTTP01Config(a *v1alpha1.ACMECertificateHTTP01Config, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if a.Ingress != "" && a.IngressClass != nil {
+		el = append(el, field.Forbidden(fldPath, "only one of 'ingress' and 'ingressClass' should be specified"))
+	}
+	// TODO: ensure 'ingress' is a valid resource name (i.e. DNS name)
+	return el
+}

pkg/apis/certmanager/validation/clusterissuer.go

@@ -0,0 +1,14 @@
+package validation
+
+import (
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+)
+
+// Validation functions for cert-manager v1alpha1 ClusterIssuer types
+
+func ValidateClusterIssuer(iss *v1alpha1.ClusterIssuer) field.ErrorList {
+	allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec"))
+	return allErrs
+}

pkg/apis/certmanager/validation/issuer.go

@@ -0,0 +1,198 @@
+package validation
+
+import (
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+)
+
+// Validation functions for cert-manager v1alpha1 Issuer types
+
+func ValidateIssuer(iss *v1alpha1.Issuer) field.ErrorList {
+	allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec"))
+	return allErrs
+}
+
+func ValidateIssuerSpec(iss *v1alpha1.IssuerSpec, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	el = ValidateIssuerConfig(&iss.IssuerConfig, fldPath)
+	return el
+}
+
+func ValidateIssuerConfig(iss *v1alpha1.IssuerConfig, fldPath *field.Path) field.ErrorList {
+	numConfigs := 0
+	el := field.ErrorList{}
+	if iss.ACME != nil {
+		if numConfigs > 0 {
+			el = append(el, field.Forbidden(fldPath.Child("acme"), "may not specify more than one issuer type"))
+		} else {
+			numConfigs++
+			el = append(el, ValidateACMEIssuerConfig(iss.ACME, fldPath.Child("acme"))...)
+		}
+	}
+	if iss.CA != nil {
+		if numConfigs > 0 {
+			el = append(el, field.Forbidden(fldPath.Child("ca"), "may not specify more than one issuer type"))
+		} else {
+			numConfigs++
+			el = append(el, ValidateCAIssuerConfig(iss.CA, fldPath.Child("ca"))...)
+		}
+	}
+	if iss.SelfSigned != nil {
+		if numConfigs > 0 {
+			el = append(el, field.Forbidden(fldPath.Child("selfSigned"), "may not specify more than one issuer type"))
+		} else {
+			numConfigs++
+			el = append(el, ValidateSelfSignedIssuerConfig(iss.SelfSigned, fldPath.Child("selfSigned"))...)
+		}
+	}
+	if iss.Vault != nil {
+		if numConfigs > 0 {
+			el = append(el, field.Forbidden(fldPath.Child("vault"), "may not specify more than one issuer type"))
+		} else {
+			numConfigs++
+			el = append(el, ValidateVaultIssuerConfig(iss.Vault, fldPath.Child("vault"))...)
+		}
+	}
+	if numConfigs == 0 {
+		el = append(el, field.Required(fldPath, "at least one issuer must be configured"))
+	}
+	return el
+}
+
+func ValidateACMEIssuerConfig(iss *v1alpha1.ACMEIssuer, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if len(iss.Email) == 0 {
+		el = append(el, field.Required(fldPath.Child("email"), "email address is a required field"))
+	}
+	if len(iss.PrivateKey.Name) == 0 {
+		el = append(el, field.Required(fldPath.Child("privateKey", "name"), "private key secret name is a required field"))
+	}
+	if len(iss.Server) == 0 {
+		el = append(el, field.Required(fldPath.Child("server"), "acme server URL is a required field"))
+	}
+	if iss.HTTP01 != nil {
+		el = append(el, ValidateACMEIssuerHTTP01Config(iss.HTTP01, fldPath.Child("http01"))...)
+	}
+	if iss.DNS01 != nil {
+		el = append(el, ValidateACMEIssuerDNS01Config(iss.DNS01, fldPath.Child("dns01"))...)
+	}
+	return el
+}
+
+func ValidateCAIssuerConfig(iss *v1alpha1.CAIssuer, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if len(iss.SecretName) == 0 {
+		el = append(el, field.Required(fldPath.Child("secretName"), ""))
+	}
+	return el
+}
+
+func ValidateSelfSignedIssuerConfig(iss *v1alpha1.SelfSignedIssuer, fldPath *field.Path) field.ErrorList {
+	return nil
+}
+
+func ValidateVaultIssuerConfig(iss *v1alpha1.VaultIssuer, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if len(iss.Server) == 0 {
+		el = append(el, field.Required(fldPath.Child("server"), ""))
+	}
+	if len(iss.Path) == 0 {
+		el = append(el, field.Required(fldPath.Child("path"), ""))
+	}
+	return el
+	// TODO: add validation for Vault authentication types
+}
+
+func ValidateACMEIssuerHTTP01Config(iss *v1alpha1.ACMEIssuerHTTP01Config, fldPath *field.Path) field.ErrorList {
+	return nil
+}
+
+func ValidateACMEIssuerDNS01Config(iss *v1alpha1.ACMEIssuerDNS01Config, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	providersFldPath := fldPath.Child("providers")
+	for i, p := range iss.Providers {
+		fldPath := providersFldPath.Index(i)
+		if len(p.Name) == 0 {
+			el = append(el, field.Required(fldPath.Child("name"), "name must be specified"))
+		}
+		numProviders := 0
+		if p.Akamai != nil {
+			numProviders++
+			el = append(el, ValidateSecretKeySelector(&p.Akamai.AccessToken, fldPath.Child("akamai", "accessToken"))...)
+			el = append(el, ValidateSecretKeySelector(&p.Akamai.ClientSecret, fldPath.Child("akamai", "clientSecret"))...)
+			el = append(el, ValidateSecretKeySelector(&p.Akamai.ClientToken, fldPath.Child("akamai", "clientToken"))...)
+			if len(p.Akamai.ServiceConsumerDomain) == 0 {
+				el = append(el, field.Required(fldPath.Child("akamai", "serviceConsumerDomain"), ""))
+			}
+		}
+		if p.AzureDNS != nil {
+			if numProviders > 0 {
+				el = append(el, field.Forbidden(fldPath.Child("azuredns"), "may not specify more than one provider type"))
+			} else {
+				numProviders++
+				el = append(el, ValidateSecretKeySelector(&p.AzureDNS.ClientSecret, fldPath.Child("azuredns", "clientSecretSecretRef"))...)
+				if len(p.AzureDNS.ClientID) == 0 {
+					el = append(el, field.Required(fldPath.Child("azuredns", "clientID"), ""))
+				}
+				if len(p.AzureDNS.SubscriptionID) == 0 {
+					el = append(el, field.Required(fldPath.Child("azuredns", "subscriptionID"), ""))
+				}
+				if len(p.AzureDNS.TenantID) == 0 {
+					el = append(el, field.Required(fldPath.Child("azuredns", "tenantID"), ""))
+				}
+				if len(p.AzureDNS.ResourceGroupName) == 0 {
+					el = append(el, field.Required(fldPath.Child("azuredns", "resourceGroupName"), ""))
+				}
+			}
+		}
+		if p.CloudDNS != nil {
+			if numProviders > 0 {
+				el = append(el, field.Forbidden(fldPath.Child("clouddns"), "may not specify more than one provider type"))
+			} else {
+				numProviders++
+				el = append(el, ValidateSecretKeySelector(&p.CloudDNS.ServiceAccount, fldPath.Child("clouddns", "serviceAccountSecretRef"))...)
+				if len(p.CloudDNS.Project) == 0 {
+					el = append(el, field.Required(fldPath.Child("clouddns", "project"), ""))
+				}
+			}
+		}
+		if p.Cloudflare != nil {
+			if numProviders > 0 {
+				el = append(el, field.Forbidden(fldPath.Child("cloudflare"), "may not specify more than one provider type"))
+			} else {
+				numProviders++
+				el = append(el, ValidateSecretKeySelector(&p.Cloudflare.APIKey, fldPath.Child("cloudflare", "apiKeySecretRef"))...)
+				if len(p.Cloudflare.Email) == 0 {
+					el = append(el, field.Required(fldPath.Child("cloudflare", "email"), ""))
+				}
+			}
+		}
+		if p.Route53 != nil {
+			if numProviders > 0 {
+				el = append(el, field.Forbidden(fldPath.Child("route53"), "may not specify more than one provider type"))
+			} else {
+				numProviders++
+				// region is the only required field for route53 as ambient credentials can be used instead
+				if len(p.Route53.Region) == 0 {
+					el = append(el, field.Required(fldPath.Child("route53", "region"), ""))
+				}
+			}
+		}
+		if numProviders == 0 {
+			el = append(el, field.Required(fldPath, "at least one provider must be configured"))
+		}
+	}
+	return el
+}
+
+func ValidateSecretKeySelector(sks *v1alpha1.SecretKeySelector, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if sks.Name == "" {
+		el = append(el, field.Required(fldPath.Child("name"), "secret name is required"))
+	}
+	if sks.Key == "" {
+		el = append(el, field.Required(fldPath.Child("key"), "secret key is required"))
+	}
+	return el
+}

pkg/controller/certificates/sync.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/x509"
 	"fmt"
+	"reflect"
 	"strings"
 	"time"
 
@@ -15,6 +16,7 @@ import (
 
 	"github.com/golang/glog"
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+	"github.com/jetstack/cert-manager/pkg/apis/certmanager/validation"
 	"github.com/jetstack/cert-manager/pkg/issuer"
 	"github.com/jetstack/cert-manager/pkg/util"
 	"github.com/jetstack/cert-manager/pkg/util/errors"
@@ -29,6 +31,7 @@ const (
 	errorIssuerNotReady    = "IssuerNotReady"
 	errorIssuerInit        = "IssuerInitError"
 	errorSavingCertificate = "SaveCertError"
+	errorConfig            = "ConfigError"
 
 	reasonIssuingCertificate  = "IssueCert"
 	reasonRenewingCertificate = "RenewCert"
@@ -46,13 +49,36 @@ const (
 )
 
 func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err error) {
+	crtCopy := crt.DeepCopy()
+	defer func() {
+		if _, saveErr := c.updateCertificateStatus(crt, crtCopy); saveErr != nil {
+			err = utilerrors.NewAggregate([]error{saveErr, err})
+		}
+	}()
+
+	el := validation.ValidateCertificate(crtCopy)
+	if len(el) > 0 {
+		msg := fmt.Sprintf("Resource validation failed: %v", el.ToAggregate())
+		crtCopy.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorConfig, msg, false)
+		return
+	} else {
+		for i, c := range crtCopy.Status.Conditions {
+			if c.Type == v1alpha1.CertificateConditionReady {
+				if c.Reason == errorConfig && c.Status == v1alpha1.ConditionFalse {
+					crtCopy.Status.Conditions = append(crtCopy.Status.Conditions[:i], crtCopy.Status.Conditions[i+1:]...)
+					break
+				}
+			}
+		}
+	}
+
 	// step zero: check if the referenced issuer exists and is ready
-	issuerObj, err := c.getGenericIssuer(crt)
+	issuerObj, err := c.getGenericIssuer(crtCopy)
 
 	if err != nil {
 		s := fmt.Sprintf("Issuer %s does not exist", err.Error())
 		glog.Info(s)
-		c.recorder.Event(crt, api.EventTypeWarning, errorIssuerNotFound, s)
+		c.recorder.Event(crtCopy, api.EventTypeWarning, errorIssuerNotFound, s)
 		return err
 	}
 
@@ -63,7 +89,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e
 	if !issuerReady {
 		s := fmt.Sprintf("Issuer %s not ready", issuerObj.GetObjectMeta().Name)
 		glog.Info(s)
-		c.recorder.Event(crt, api.EventTypeWarning, errorIssuerNotReady, s)
+		c.recorder.Event(crtCopy, api.EventTypeWarning, errorIssuerNotReady, s)
 		return fmt.Errorf(s)
 	}
 
@@ -71,12 +97,12 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e
 	if err != nil {
 		s := "Error initializing issuer: " + err.Error()
 		glog.Info(s)
-		c.recorder.Event(crt, api.EventTypeWarning, errorIssuerInit, s)
+		c.recorder.Event(crtCopy, api.EventTypeWarning, errorIssuerInit, s)
 		return err
 	}
 
-	expectedCN := pki.CommonNameForCertificate(crt)
-	expectedDNSNames := pki.DNSNamesForCertificate(crt)
+	expectedCN := pki.CommonNameForCertificate(crtCopy)
+	expectedDNSNames := pki.DNSNamesForCertificate(crtCopy)
 	if expectedCN == "" || len(expectedDNSNames) == 0 {
 		// TODO: Set certificate invalid condition on certificate resource
 		// TODO: remove this check in favour of resource validation
@@ -84,8 +110,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e
 	}
 
 	// grab existing certificate and validate private key
-	cert, err := kube.SecretTLSCert(c.secretLister, crt.Namespace, crt.Spec.SecretName)
-
+	cert, err := kube.SecretTLSCert(c.secretLister, crtCopy.Namespace, crtCopy.Spec.SecretName)
 	// if an error is returned, and that error is something other than
 	// IsNotFound or invalid data, then we should return the error.
 	if err != nil && !k8sErrors.IsNotFound(err) && !errors.IsInvalidData(err) {
@@ -95,9 +120,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e
 	// as there is an existing certificate, or we may create one below, we will
 	// run scheduleRenewal to schedule a renewal if required at the end of
 	// execution.
-	defer c.scheduleRenewal(crt)
-
-	crtCopy := crt.DeepCopy()
+	defer c.scheduleRenewal(crtCopy)
 
 	// if the certificate was not found, or the certificate data is invalid, we
 	// should issue a new certificate.
@@ -105,12 +128,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e
 	// listed in the certificate spec, we should re-issue the certificate.
 	if k8sErrors.IsNotFound(err) || errors.IsInvalidData(err) ||
 		expectedCN != cert.Subject.CommonName || !util.EqualUnsorted(cert.DNSNames, expectedDNSNames) {
-		err := c.issue(ctx, i, crtCopy)
-		updateErr := c.updateCertificateStatus(crtCopy)
-		if err != nil || updateErr != nil {
-			return utilerrors.NewAggregate([]error{err, updateErr})
-		}
-		return nil
+		return c.issue(ctx, i, crtCopy)
 	}
 
 	// calculate the amount of time until expiry
@@ -120,11 +138,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e
 	renewIn := durationUntilExpiry - renewBefore
 	// if we should being attempting to renew now, then trigger a renewal
 	if renewIn <= 0 {
-		err := c.renew(ctx, i, crtCopy)
-		updateErr := c.updateCertificateStatus(crtCopy)
-		if err != nil || updateErr != nil {
-			return utilerrors.NewAggregate([]error{err, updateErr})
-		}
+		return c.renew(ctx, i, crtCopy)
 	}
 
 	return nil
@@ -306,10 +320,12 @@ func (c *Controller) renew(ctx context.Context, issuer issuer.Interface, crt *v1
 	return nil
 }
 
-func (c *Controller) updateCertificateStatus(crt *v1alpha1.Certificate) error {
+func (c *Controller) updateCertificateStatus(old, new *v1alpha1.Certificate) (*v1alpha1.Certificate, error) {
+	if reflect.DeepEqual(old.Status, new.Status) {
+		return nil, nil
+	}
 	// TODO: replace Update call with UpdateStatus. This requires a custom API
 	// server with the /status subresource enabled and/or subresource support
 	// for CRDs (https://github.com/kubernetes/kubernetes/issues/38113)
-	_, err := c.cmClient.CertmanagerV1alpha1().Certificates(crt.Namespace).Update(crt)
-	return err
+	return c.cmClient.CertmanagerV1alpha1().Certificates(new.Namespace).Update(new)
 }

pkg/controller/clusterissuers/sync.go

@@ -2,6 +2,7 @@ package clusterissuers
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 
 	"github.com/golang/glog"
@@ -9,37 +10,46 @@ import (
 	"k8s.io/apimachinery/pkg/util/errors"
 
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+	"github.com/jetstack/cert-manager/pkg/apis/certmanager/validation"
 )
 
 const (
 	errorInitIssuer = "ErrInitIssuer"
+	errorConfig     = "ConfigError"
 
 	messageErrorInitIssuer = "Error initializing issuer: "
 )
 
 func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.ClusterIssuer) (err error) {
 	issuerCopy := iss.DeepCopy()
-	i, err := c.issuerFactory.IssuerFor(issuerCopy)
+	defer func() {
+		if _, saveErr := c.updateIssuerStatus(iss, issuerCopy); saveErr != nil {
+			err = errors.NewAggregate([]error{saveErr, err})
+		}
+	}()
 
+	el := validation.ValidateClusterIssuer(issuerCopy)
+	if len(el) > 0 {
+		msg := fmt.Sprintf("Resource validation failed: %v", el.ToAggregate())
+		issuerCopy.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorConfig, msg)
+		return
+	} else {
+		for i, c := range issuerCopy.Status.Conditions {
+			if c.Type == v1alpha1.IssuerConditionReady {
+				if c.Reason == errorConfig && c.Status == v1alpha1.ConditionFalse {
+					issuerCopy.Status.Conditions = append(issuerCopy.Status.Conditions[:i], issuerCopy.Status.Conditions[i+1:]...)
+					break
+				}
+			}
+		}
+	}
+
+	i, err := c.issuerFactory.IssuerFor(issuerCopy)
 	if err != nil {
 		return err
 	}
 
 	err = i.Setup(ctx)
-	defer func() {
-		// TODO: replace this with more efficient comparison?
-		if reflect.DeepEqual(issuerCopy.Status, iss.Status) {
-			return
-		}
-		if saveErr := c.updateIssuerStatus(issuerCopy); saveErr != nil {
-			errs := []error{saveErr}
-			if err != nil {
-				errs = append(errs, err)
-			}
-			err = errors.NewAggregate(errs)
-		}
-	}()
-
 	if err != nil {
 		s := messageErrorInitIssuer + err.Error()
 		glog.Info(s)
@@ -50,10 +60,12 @@ func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.ClusterIssuer) (err
 	return nil
 }
 
-func (c *Controller) updateIssuerStatus(iss *v1alpha1.ClusterIssuer) error {
+func (c *Controller) updateIssuerStatus(old, new *v1alpha1.ClusterIssuer) (*v1alpha1.ClusterIssuer, error) {
+	if reflect.DeepEqual(old.Status, new.Status) {
+		return nil, nil
+	}
 	// TODO: replace Update call with UpdateStatus. This requires a custom API
 	// server with the /status subresource enabled and/or subresource support
 	// for CRDs (https://github.com/kubernetes/kubernetes/issues/38113)
-	_, err := c.cmClient.CertmanagerV1alpha1().ClusterIssuers().Update(iss)
-	return err
+	return c.cmClient.CertmanagerV1alpha1().ClusterIssuers().Update(new)
 }

pkg/controller/clusterissuers/sync_test.go

@@ -51,7 +51,7 @@ func TestUpdateIssuerStatus(t *testing.T) {
 
 	issuerCopy := issuer.DeepCopy()
 	issuerCopy.Status = newStatus
-	err = c.updateIssuerStatus(issuerCopy)
+	_, err = c.updateIssuerStatus(issuer, issuerCopy)
 	assertErrIsNil(t, fatalf, err)
 
 	actions := cmClient.Actions()

pkg/controller/issuers/sync.go

@@ -2,6 +2,7 @@ package issuers
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 
 	"github.com/golang/glog"
@@ -9,16 +10,40 @@ import (
 	"k8s.io/apimachinery/pkg/util/errors"
 
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+	"github.com/jetstack/cert-manager/pkg/apis/certmanager/validation"
 )
 
 const (
 	errorInitIssuer = "ErrInitIssuer"
+	errorConfig     = "ConfigError"
 
 	messageErrorInitIssuer = "Error initializing issuer: "
 )
 
 func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.Issuer) (err error) {
 	issuerCopy := iss.DeepCopy()
+	defer func() {
+		if _, saveErr := c.updateIssuerStatus(iss, issuerCopy); saveErr != nil {
+			err = errors.NewAggregate([]error{saveErr, err})
+		}
+	}()
+
+	el := validation.ValidateIssuer(issuerCopy)
+	if len(el) > 0 {
+		msg := fmt.Sprintf("Resource validation failed: %v", el.ToAggregate())
+		issuerCopy.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorConfig, msg)
+		return
+	} else {
+		for i, c := range issuerCopy.Status.Conditions {
+			if c.Type == v1alpha1.IssuerConditionReady {
+				if c.Reason == errorConfig && c.Status == v1alpha1.ConditionFalse {
+					issuerCopy.Status.Conditions = append(issuerCopy.Status.Conditions[:i], issuerCopy.Status.Conditions[i+1:]...)
+					break
+				}
+			}
+		}
+	}
+
 	i, err := c.issuerFactory.IssuerFor(issuerCopy)
 
 	if err != nil {
@@ -26,20 +51,6 @@ func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.Issuer) (err error)
 	}
 
 	err = i.Setup(ctx)
-	defer func() {
-		// TODO: replace this with more efficient comparison?
-		if reflect.DeepEqual(issuerCopy.Status, iss.Status) {
-			return
-		}
-		if saveErr := c.updateIssuerStatus(issuerCopy); saveErr != nil {
-			errs := []error{saveErr}
-			if err != nil {
-				errs = append(errs, err)
-			}
-			err = errors.NewAggregate(errs)
-		}
-	}()
-
 	if err != nil {
 		s := messageErrorInitIssuer + err.Error()
 		glog.Info(s)
@@ -50,10 +61,12 @@ func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.Issuer) (err error)
 	return nil
 }
 
-func (c *Controller) updateIssuerStatus(iss *v1alpha1.Issuer) error {
+func (c *Controller) updateIssuerStatus(old, new *v1alpha1.Issuer) (*v1alpha1.Issuer, error) {
+	if reflect.DeepEqual(old.Status, new.Status) {
+		return nil, nil
+	}
 	// TODO: replace Update call with UpdateStatus. This requires a custom API
 	// server with the /status subresource enabled and/or subresource support
 	// for CRDs (https://github.com/kubernetes/kubernetes/issues/38113)
-	_, err := c.cmClient.CertmanagerV1alpha1().Issuers(iss.Namespace).Update(iss)
-	return err
+	return c.cmClient.CertmanagerV1alpha1().Issuers(new.Namespace).Update(new)
 }

pkg/controller/issuers/sync_test.go

@@ -51,7 +51,7 @@ func TestUpdateIssuerStatus(t *testing.T) {
 
 	issuerCopy := issuer.DeepCopy()
 	issuerCopy.Status = newStatus
-	err = c.updateIssuerStatus(issuerCopy)
+	_, err = c.updateIssuerStatus(issuer, issuerCopy)
 	assertErrIsNil(t, fatalf, err)
 
 	actions := cmClient.Actions()