Adds from comments

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

deploy/manifests/00-crds.yaml

@@ -1825,7 +1825,9 @@ spec:
         metadata:
           type: object
         spec:
-          description: CertificateSpec defines the desired state of Certificate
+          description: CertificateSpec defines the desired state of Certificate. A
+            valid Certificate requires at least one of a CommonName, DNSName, or URISAN
+            to be valid.
           properties:
             commonName:
               description: CommonName is a common name to be used on the Certificate.

pkg/apis/certmanager/v1alpha2/types_certificate.go

@@ -68,11 +68,10 @@ const (
 	PKCS8 KeyEncoding = "pkcs8"
 )
 
-// CertificateSpec defines the desired state of Certificate
+// CertificateSpec defines the desired state of Certificate.
+// A valid Certificate requires at least one of a CommonName, DNSName, or
+// URISAN to be valid.
 type CertificateSpec struct {
-	// A valid Certificate requires at least one of a CommonName, DNSName, or
-	// URISAN to be valid.
-
 	// CommonName is a common name to be used on the Certificate.
 	// The CommonName should have a length of 64 characters or fewer to avoid
 	// generating invalid CSRs.

pkg/controller/certificaterequests/acme/acme.go

@@ -96,12 +96,11 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer
 	// If the CommonName is also not present in the DNS names of the CSR then hard fail.
 	if len(csr.Subject.CommonName) > 0 && !util.Contains(csr.DNSNames, csr.Subject.CommonName) {
 		err = fmt.Errorf("%q does not exist in %s", csr.Subject.CommonName, csr.DNSNames)
-		message := fmt.Sprintf("Requested certificate contains CommonName not present in the requested DNS Subject Alternative Names: %s",
-			err)
+		message := "The CSR PEM requests a commonName that is not present in the list of dnsNames. If a commonName is set, ACME requires that the value is also present in the list of dnsNames"
 
-		a.reporter.Failed(cr, err, "OrderBuildingError", message)
+		a.reporter.Failed(cr, err, "InvalidOrder", message)
 
-		log.Error(err, message)
+		log.V(4).Info(fmt.Sprintf("%s: %s", message, err))
 
 		return nil, nil
 	}

pkg/controller/certificaterequests/acme/acme_test.go

@@ -50,15 +50,15 @@ var (
 	fixedClock      = fakeclock.NewFakeClock(fixedClockStart)
 )
 
-func generateCSR(t *testing.T, secretKey crypto.Signer, extraDNSNames ...string) []byte {
+func generateCSR(t *testing.T, secretKey crypto.Signer, commonName string, dnsNames ...string) []byte {
 	// The CommonName of the certificate request must also be present in the DNS
 	// Names.
 	template := x509.CertificateRequest{
 		Subject: pkix.Name{
-			CommonName: "example.com",
+			CommonName: commonName,
 		},
 		SignatureAlgorithm: x509.SHA256WithRSA,
-		DNSNames:           append(extraDNSNames, "foo.com"),
+		DNSNames:           dnsNames,
 	}
 
 	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, secretKey)
@@ -83,8 +83,8 @@ func TestSign(t *testing.T) {
 		t.FailNow()
 	}
 
-	csrPEM := generateCSR(t, sk, "example.com")
-	csrPEMExampleNotPresent := generateCSR(t, sk)
+	csrPEM := generateCSR(t, sk, "example.com", "example.com", "foo.com")
+	csrPEMExampleNotPresent := generateCSR(t, sk, "example.com", "foo.com")
 
 	baseCR := gen.CertificateRequest("test-cr",
 		gen.SetCertificateRequestCSR(csrPEM),
@@ -160,7 +160,7 @@ func TestSign(t *testing.T) {
 			builder: &testpkg.Builder{
 				CertManagerObjects: []runtime.Object{baseCR.DeepCopy(), baseIssuer.DeepCopy()},
 				ExpectedEvents: []string{
-					`Warning OrderBuildingError Requested certificate contains CommonName not present in the requested DNS Subject Alternative Names: "example.com" does not exist in [foo.com]: "example.com" does not exist in [foo.com]`,
+					`Warning InvalidOrder The CSR PEM requests a commonName that is not present in the list of dnsNames. If a commonName is set, ACME requires that the value is also present in the list of dnsNames: "example.com" does not exist in [foo.com]`,
 				},
 				ExpectedActions: []testpkg.Action{
 					testpkg.NewAction(coretesting.NewUpdateAction(
@@ -172,7 +172,7 @@ func TestSign(t *testing.T) {
 								Type:               cmapi.CertificateRequestConditionReady,
 								Status:             cmmeta.ConditionFalse,
 								Reason:             cmapi.CertificateRequestReasonFailed,
-								Message:            `Requested certificate contains CommonName not present in the requested DNS Subject Alternative Names: "example.com" does not exist in [foo.com]: "example.com" does not exist in [foo.com]`,
+								Message:            `The CSR PEM requests a commonName that is not present in the list of dnsNames. If a commonName is set, ACME requires that the value is also present in the list of dnsNames: "example.com" does not exist in [foo.com]`,
 								LastTransitionTime: &metaFixedClockStart,
 							}),
 							gen.SetCertificateRequestFailureTime(metaFixedClockStart),

pkg/util/pki/csr.go

@@ -48,25 +48,42 @@ func IPAddressesForCertificate(crt *v1alpha2.Certificate) []net.IP {
 }
 
 func URIsForCertificate(crt *v1alpha2.Certificate) ([]*url.URL, error) {
-	var uris []*url.URL
+	uris, err := URLsFromStrings(crt.Spec.URISANs)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse URIs: %s", err)
+	}
+
+	return uris, nil
+}
+
+func DNSNamesForCertificate(crt *v1alpha2.Certificate) ([]string, error) {
+	_, err := URLsFromStrings(crt.Spec.DNSNames)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse DNSNames: %s", err)
+	}
+
+	return crt.Spec.DNSNames, nil
+}
+
+func URLsFromStrings(urlStrs []string) ([]*url.URL, error) {
+	var urls []*url.URL
 	var errs []string
 
-	for _, uriStr := range crt.Spec.URISANs {
-		uri, err := url.Parse(uriStr)
+	for _, urlStr := range urlStrs {
+		url, err := url.Parse(urlStr)
 		if err != nil {
 			errs = append(errs, err.Error())
 			continue
 		}
 
-		uris = append(uris, uri)
+		urls = append(urls, url)
 	}
 
 	if len(errs) > 0 {
-		return nil, fmt.Errorf("failed to parse URIs: %s",
-			strings.Join(errs, ", "))
+		return nil, errors.New(strings.Join(errs, ", "))
 	}
 
-	return uris, nil
+	return urls, nil
 }
 
 func IPAddressesToString(ipAddresses []net.IP) []string {
@@ -148,9 +165,14 @@ func buildUsages(usages []v1alpha2.KeyUsage, isCA bool) (ku x509.KeyUsage, eku [
 // to the x509.CreateCertificateRequest function.
 func GenerateCSR(crt *v1alpha2.Certificate) (*x509.CertificateRequest, error) {
 	commonName := crt.Spec.CommonName
-	dnsNames := crt.Spec.DNSNames
 	iPAddresses := IPAddressesForCertificate(crt)
 	organization := OrganizationForCertificate(crt)
+
+	dnsNames, err := DNSNamesForCertificate(crt)
+	if err != nil {
+		return nil, err
+	}
+
 	uriNames, err := URIsForCertificate(crt)
 	if err != nil {
 		return nil, err

test/e2e/suite/conformance/certificates/suite.go

@@ -294,6 +294,31 @@ func (s *Suite) Define() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		It("should issue a certificate that defines a distinct DNS Name and another distinct Common Name", func() {
+			s.checkFeatures(CommonNameFeature)
+
+			testCertificate := &cmapi.Certificate{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "testcert",
+					Namespace: f.Namespace.Name,
+				},
+				Spec: cmapi.CertificateSpec{
+					SecretName: "testcert-tls",
+					CommonName: s.newDomain(),
+					IssuerRef:  issuerRef,
+					DNSNames:   []string{s.newDomain()},
+				},
+			}
+
+			By("Creating a Certificate")
+			err := f.CRClient.Create(ctx, testCertificate)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Waiting for the Certificate to be issued...")
+			err = f.Helper().WaitCertificateIssuedValid(f.Namespace.Name, "testcert", time.Minute*5)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
 		It("should issue a certificate that defines a DNS Name and sets a duration", func() {
 			s.checkFeatures(DurationFeature)
 

test/e2e/suite/conformance/certificates/venafi/venafi.go

@@ -38,10 +38,6 @@ var _ = framework.ConformanceDescribe("Certificates", func() {
 		// key or using the same private key multiple times.
 		certificates.ECDSAFeature,
 		certificates.ReusePrivateKeyFeature,
-
-		// Venafi does not support signing a certificate with only URISANs and an
-		// empty common name.
-		certificates.URISANsFeature,
 	)
 
 	provisioner := new(venafiProvisioner)