From 2ed2a720afe1812e809c25484f806d47e865ae42 Mon Sep 17 00:00:00 2001
From: Terin Stock <terinjokes@gmail.com>
Date: Fri, 1 Nov 2024 13:22:41 +0100
Subject: [PATCH] fix(certificaterequest): ignore empty issuer group

A CertificateRequest's reference to an Issuer can have an empty Group,
which we were considering as equivalent to matching, then later
rejecting the CertificateRequest when it was for an unknown issuer.
While setting the group is optional in the schema, our documentation has
always included the group in the issuer reference. The rejection was
racing with other controllers, including the built-in controllers, and
preventing them from issuing certificates.

This changeset updates CertificateRequestController to ignore
CertificateRequests where the issuer group is unset, preventing the
race with controllers for other issuers. We'll continue to reject
CertificateRequests for unknown kinds if the group matches our group.

Bug: #149
Fixes: d8c1b77 ("feat: add ClusterOriginIssuer")
---
 pkgs/controllers/certificaterequest.go      |  2 +-
 pkgs/controllers/certificaterequest_test.go | 51 +++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/pkgs/controllers/certificaterequest.go b/pkgs/controllers/certificaterequest.go
index c7d5c3f..ddd0783 100644
--- a/pkgs/controllers/certificaterequest.go
+++ b/pkgs/controllers/certificaterequest.go
@@ -44,7 +44,7 @@ type CertificateRequestController struct {
 func (r *CertificateRequestController) Reconcile(ctx context.Context, cr *certmanager.CertificateRequest) (reconcile.Result, error) {
 	log := r.Log.WithValues("namespace", cr.Namespace, "certificaterequest", cr.Name)
 
-	if cr.Spec.IssuerRef.Group != "" && cr.Spec.IssuerRef.Group != v1.GroupVersion.Group {
+	if cr.Spec.IssuerRef.Group != v1.GroupVersion.Group {
 		log.V(4).Info("resource does not specify an issuerRef group name that we are responsible for", "group", cr.Spec.IssuerRef.Group)
 
 		return reconcile.Result{}, nil
diff --git a/pkgs/controllers/certificaterequest_test.go b/pkgs/controllers/certificaterequest_test.go
index 90cdb38..3104ddc 100644
--- a/pkgs/controllers/certificaterequest_test.go
+++ b/pkgs/controllers/certificaterequest_test.go
@@ -302,6 +302,57 @@ func TestCertificateRequestReconcile(t *testing.T) {
 				Name:      "foobar",
 			},
 		},
+		{
+			name: "ignores CertificateRequests with empty Issuer group reference",
+			objects: []runtime.Object{
+				cmgen.CertificateRequest("foobar",
+					cmgen.SetCertificateRequestNamespace("default"),
+					cmgen.SetCertificateRequestDuration(&metav1.Duration{Duration: 7 * 24 * time.Hour}),
+					cmgen.SetCertificateRequestCSR(golden.Get(t, "csr.golden")),
+					cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{
+						Name: "foobar",
+						Kind: "StepIssuer", // 👋 hello friends!
+					}),
+				),
+				&v1.ClusterOriginIssuer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "foobar",
+					},
+					Spec: v1.OriginIssuerSpec{
+						RequestType: v1.RequestTypeOriginECC,
+						Auth: v1.OriginIssuerAuthentication{
+							TokenRef: &v1.SecretKeySelector{
+								Name: "token-issuer",
+								Key:  "token",
+							},
+						},
+					},
+					Status: v1.OriginIssuerStatus{
+						Conditions: []v1.OriginIssuerCondition{
+							{
+								Type:   v1.ConditionReady,
+								Status: v1.ConditionTrue,
+							},
+						},
+					},
+				},
+				&corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "token-issuer",
+						Namespace: "super-secret",
+					},
+					Data: map[string][]byte{
+						"token": []byte("api-token"),
+					},
+				},
+			},
+			recorder: RecorderMust(t, "testdata/working"),
+			expected: cmapi.CertificateRequestStatus{},
+			namespaceName: types.NamespacedName{
+				Namespace: "default",
+				Name:      "foobar",
+			},
+		},
 		{
 			name: "OriginIssuer without authentication",
 			objects: []runtime.Object{