Skip to content

Commit

Permalink
feat(types): make ServiceKeyRef optional
Browse files Browse the repository at this point in the history 
Prepare to allow multiple authentication types by allowing ServiceKeyRef
to become nil, and requiring the controllers to properly handle no
authentication types being set.

Bug: #135
  • Loading branch information
@terinjokes
terinjokes committed Oct 1, 2024
1 parent c1d8920 commit e09e726
Show file tree
Hide file tree
Showing 9 changed files with 212 additions and 93 deletions.
2 changes: 1 addition & 1 deletion pkgs/apis/v1/types_originissuer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ type OriginIssuerStatus struct {
type OriginIssuerAuthentication struct {
// ServiceKeyRef authenticates with an API Service Key.
// +optional
ServiceKeyRef SecretKeySelector `json:"serviceKeyRef,omitempty"`
ServiceKeyRef *SecretKeySelector `json:"serviceKeyRef,omitempty"`
}

// SecretKeySelector contains a reference to a secret.
Expand Down
12 changes: 8 additions & 4 deletions pkgs/apis/v1/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

60 changes: 35 additions & 25 deletions pkgs/controllers/certificaterequest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ func (r *CertificateRequestController) Reconcile(ctx context.Context, cr *certma
}

var (
secretNamespaceName types.NamespacedName
issuerspec v1.OriginIssuerSpec
secretNamespace string
issuerspec v1.OriginIssuerSpec
)

switch cr.Spec.IssuerRef.Kind {
Expand All @@ -139,10 +139,7 @@ func (r *CertificateRequestController) Reconcile(ctx context.Context, cr *certma
return reconcile.Result{}, err
}

secretNamespaceName = types.NamespacedName{
Namespace: iss.Namespace,
Name: iss.Spec.Auth.ServiceKeyRef.Name,
}
secretNamespace = iss.Namespace
issuerspec = iss.Spec
case "ClusterOriginIssuer":
iss := v1.ClusterOriginIssuer{}
Expand All @@ -165,10 +162,7 @@ func (r *CertificateRequestController) Reconcile(ctx context.Context, cr *certma
return reconcile.Result{}, err
}

secretNamespaceName = types.NamespacedName{
Namespace: r.ClusterResourceNamespace,
Name: iss.Spec.Auth.ServiceKeyRef.Name,
}
secretNamespace = r.ClusterResourceNamespace
issuerspec = iss.Spec
default:
err := fmt.Errorf("unknown issuer kind: %s", cr.Spec.IssuerRef.Kind)
Expand All @@ -178,28 +172,44 @@ func (r *CertificateRequestController) Reconcile(ctx context.Context, cr *certma
return reconcile.Result{}, err
}

var secret core.Secret
if err := r.Reader.Get(ctx, secretNamespaceName, &secret); err != nil {
log.Error(err, "failed to retieve OriginIssuer auth secret", "namespace", secretNamespaceName.Namespace, "name", secretNamespaceName.Name)
if apierrors.IsNotFound(err) {
_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
} else {
_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "Error", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
var c *cfapi.Client
switch {
case issuerspec.Auth.ServiceKeyRef != nil:
var secret core.Secret

secretNamespaceName := types.NamespacedName{
Namespace: secretNamespace,
Name: issuerspec.Auth.ServiceKeyRef.Name,
}

return reconcile.Result{}, err
}
if err := r.Reader.Get(ctx, secretNamespaceName, &secret); err != nil {
log.Error(err, "failed to retieve OriginIssuer auth secret", "namespace", secretNamespaceName.Namespace, "name", secretNamespaceName.Name)
if apierrors.IsNotFound(err) {
_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
} else {
_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "Error", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
}

return reconcile.Result{}, err
}

serviceKey, ok := secret.Data[issuerspec.Auth.ServiceKeyRef.Key]
if !ok {
err := fmt.Errorf("secret %s does not contain key %q", secret.Name, issuerspec.Auth.ServiceKeyRef.Key)
log.Error(err, "failed to retrieve OriginIssuer auth secret")
_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
serviceKey, ok := secret.Data[issuerspec.Auth.ServiceKeyRef.Key]
if !ok {
err := fmt.Errorf("secret %s does not contain key %q", secret.Name, issuerspec.Auth.ServiceKeyRef.Key)
log.Error(err, "failed to retrieve OriginIssuer auth secret")
_ = r.setStatus(ctx, cr, cmmeta.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))

return reconcile.Result{}, err
}

c = r.Builder.Clone().WithServiceKey(serviceKey).Build()
default:
// This issuer should not be ready!
err := fmt.Errorf("issuer %s does not have an authentication method configured", cr.Spec.IssuerRef.Name)
log.Error(err, "failed to retrieve issuer auth secret")
return reconcile.Result{}, err
}

c := r.Builder.Clone().WithServiceKey(serviceKey).Build()
p, err := provisioners.New(c, issuerspec.RequestType, log)
if err != nil {
log.Error(err, "failed to create provisioner")
Expand Down
49 changes: 43 additions & 6 deletions pkgs/controllers/certificaterequest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func TestCertificateRequestReconcile(t *testing.T) {
namespaceName types.NamespacedName
}{
{
name: "working OriginIssuer",
name: "working OriginIssuer with serviceKeyRef",
objects: []runtime.Object{
cmgen.CertificateRequest("foobar",
cmgen.SetCertificateRequestNamespace("default"),
Expand All @@ -69,7 +69,7 @@ func TestCertificateRequestReconcile(t *testing.T) {
Spec: v1.OriginIssuerSpec{
RequestType: v1.RequestTypeOriginECC,
Auth: v1.OriginIssuerAuthentication{
ServiceKeyRef: v1.SecretKeySelector{
ServiceKeyRef: &v1.SecretKeySelector{
Name: "service-key-issuer",
Key: "key",
},
Expand Down Expand Up @@ -113,7 +113,7 @@ func TestCertificateRequestReconcile(t *testing.T) {
},
},
{
name: "working ClusterOriginIssuer",
name: "working ClusterOriginIssuer with serviceKeyRef",
objects: []runtime.Object{
cmgen.CertificateRequest("foobar",
cmgen.SetCertificateRequestNamespace("default"),
Expand All @@ -132,7 +132,7 @@ func TestCertificateRequestReconcile(t *testing.T) {
Spec: v1.OriginIssuerSpec{
RequestType: v1.RequestTypeOriginECC,
Auth: v1.OriginIssuerAuthentication{
ServiceKeyRef: v1.SecretKeySelector{
ServiceKeyRef: &v1.SecretKeySelector{
Name: "service-key-issuer",
Key: "key",
},
Expand Down Expand Up @@ -175,6 +175,41 @@ func TestCertificateRequestReconcile(t *testing.T) {
Name: "foobar",
},
},
{
name: "OriginIssuer without authentication",
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: "OriginIssuer",
Group: "cert-manager.k8s.cloudflare.com",
}),
),
&v1.OriginIssuer{
ObjectMeta: metav1.ObjectMeta{
Name: "foobar",
Namespace: "default",
},
Spec: v1.OriginIssuerSpec{},
Status: v1.OriginIssuerStatus{
Conditions: []v1.OriginIssuerCondition{
{
Type: v1.ConditionReady,
Status: v1.ConditionTrue,
},
},
},
},
},
namespaceName: types.NamespacedName{
Namespace: "default",
Name: "foobar",
},
error: "issuer foobar does not have an authentication method configured",
},
{
name: "requeue after API error",
objects: []runtime.Object{
Expand All @@ -196,7 +231,7 @@ func TestCertificateRequestReconcile(t *testing.T) {
Spec: v1.OriginIssuerSpec{
RequestType: v1.RequestTypeOriginECC,
Auth: v1.OriginIssuerAuthentication{
ServiceKeyRef: v1.SecretKeySelector{
ServiceKeyRef: &v1.SecretKeySelector{
Name: "service-key-issuer",
Key: "key",
},
Expand Down Expand Up @@ -239,7 +274,9 @@ func TestCertificateRequestReconcile(t *testing.T) {
WithStatusSubresource(&cmapi.CertificateRequest{}).
Build()

defer tt.recorder.Stop()
if tt.recorder != nil {
defer tt.recorder.Stop()
}

controller := &CertificateRequestController{
Client: client,
Expand Down
46 changes: 26 additions & 20 deletions pkgs/controllers/clusteroriginissuer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,31 +41,37 @@ func (r *ClusterOriginIssuerController) Reconcile(ctx context.Context, iss *v1.C
return reconcile.Result{}, err
}

secret := core.Secret{}
secretNamespaceName := types.NamespacedName{
Namespace: r.ClusterResourceNamespace,
Name: iss.Spec.Auth.ServiceKeyRef.Name,
}
switch {
case iss.Spec.Auth.ServiceKeyRef != nil:
secret := &core.Secret{}
secretNamespaceName := types.NamespacedName{
Namespace: r.ClusterResourceNamespace,
Name: iss.Spec.Auth.ServiceKeyRef.Name,
}

if err := r.Reader.Get(ctx, secretNamespaceName, &secret); err != nil {
log.Error(err, "failed to retieve ClusterOriginIssuer auth secret", "namespace", secretNamespaceName.Namespace, "name", secretNamespaceName.Name)
if err := r.Reader.Get(ctx, secretNamespaceName, secret); err != nil {
log.Error(err, "failed to retieve ClusterOriginIssuer auth secret", "namespace", secretNamespaceName.Namespace, "name", secretNamespaceName.Name)

if apierrors.IsNotFound(err) {
_ = r.setStatus(ctx, iss, v1.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
} else {
_ = r.setStatus(ctx, iss, v1.ConditionFalse, "Error", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
}
if apierrors.IsNotFound(err) {
_ = r.setStatus(ctx, iss, v1.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
} else {
_ = r.setStatus(ctx, iss, v1.ConditionFalse, "Error", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
}

return reconcile.Result{}, err
}
return reconcile.Result{}, err
}

_, ok := secret.Data[iss.Spec.Auth.ServiceKeyRef.Key]
if !ok {
err := fmt.Errorf("secret %s does not contain key %q", secret.Name, iss.Spec.Auth.ServiceKeyRef.Key)
log.Error(err, "failed to retrieve ClusterOriginIssuer auth secret")
_ = r.setStatus(ctx, iss, v1.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))
_, ok := secret.Data[iss.Spec.Auth.ServiceKeyRef.Key]
if !ok {
err := fmt.Errorf("secret %s does not contain key %q", secret.Name, iss.Spec.Auth.ServiceKeyRef.Key)
log.Error(err, "failed to retrieve ClusterOriginIssuer auth secret")
_ = r.setStatus(ctx, iss, v1.ConditionFalse, "NotFound", fmt.Sprintf("Failed to retrieve auth secret: %v", err))

return reconcile.Result{}, err
return reconcile.Result{}, err
}
default:
_ = r.setStatus(ctx, iss, v1.ConditionFalse, "MissingAuthentication", "No authentication methods were configured")
return reconcile.Result{}, nil
}

return reconcile.Result{}, r.setStatus(ctx, iss, v1.ConditionTrue, "Verified", "ClusterOriginIssuer verified and ready to sign certificates")
Expand Down
42 changes: 36 additions & 6 deletions pkgs/controllers/clusteroriginissuer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestClusterOriginIssuerReconcile(t *testing.T) {
namespaceName types.NamespacedName
}{
{
name: "working with secrets",
name: "working serviceKeyRef",
objects: []runtime.Object{
&v1.ClusterOriginIssuer{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -48,7 +48,7 @@ func TestClusterOriginIssuerReconcile(t *testing.T) {
Spec: v1.OriginIssuerSpec{
RequestType: v1.RequestTypeOriginRSA,
Auth: v1.OriginIssuerAuthentication{
ServiceKeyRef: v1.SecretKeySelector{
ServiceKeyRef: &v1.SecretKeySelector{
Name: "issuer-service-key",
Key: "key",
},
Expand Down Expand Up @@ -81,7 +81,7 @@ func TestClusterOriginIssuerReconcile(t *testing.T) {
},
},
{
name: "missing secret",
name: "missing serviceKeyRef",
objects: []runtime.Object{
&v1.ClusterOriginIssuer{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -90,7 +90,7 @@ func TestClusterOriginIssuerReconcile(t *testing.T) {
Spec: v1.OriginIssuerSpec{
RequestType: v1.RequestTypeOriginRSA,
Auth: v1.OriginIssuerAuthentication{
ServiceKeyRef: v1.SecretKeySelector{
ServiceKeyRef: &v1.SecretKeySelector{
Name: "issuer-service-key",
Key: "key",
},
Expand All @@ -115,7 +115,7 @@ func TestClusterOriginIssuerReconcile(t *testing.T) {
},
},
{
name: "secret missing key",
name: "serviceKeyRef missing key",
objects: []runtime.Object{
&v1.ClusterOriginIssuer{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -124,7 +124,7 @@ func TestClusterOriginIssuerReconcile(t *testing.T) {
Spec: v1.OriginIssuerSpec{
RequestType: v1.RequestTypeOriginRSA,
Auth: v1.OriginIssuerAuthentication{
ServiceKeyRef: v1.SecretKeySelector{
ServiceKeyRef: &v1.SecretKeySelector{
Name: "issuer-service-key",
Key: "key",
},
Expand Down Expand Up @@ -155,6 +155,36 @@ func TestClusterOriginIssuerReconcile(t *testing.T) {
Name: "foo",
},
},
{
name: "unset authentication",
objects: []runtime.Object{
&v1.ClusterOriginIssuer{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "default",
},
Spec: v1.OriginIssuerSpec{
RequestType: v1.RequestTypeOriginRSA,
},
},
},
expected: v1.OriginIssuerStatus{
Conditions: []v1.OriginIssuerCondition{
{
Type: v1.ConditionReady,
Status: v1.ConditionFalse,
LastTransitionTime: &now,
Reason: "MissingAuthentication",
Message: "No authentication methods were configured",
},
},
},
error: `secret issuer-service-key does not contain key "key"`,
namespaceName: types.NamespacedName{
Namespace: "default",
Name: "foo",
},
},
}

for _, tt := range tests {
Expand Down
Loading

0 comments on commit e09e726

Please sign in to comment.