search

cert-manager / approver-policy

approver-policy is a cert-manager approver that allows users to define policies that restrict what certificates can be requested.
https://cert-manager.io/docs/policy/approval/approver-policy/
Apache License 2.0
67 stars 23 forks source link

fix: apply cert-manager default issuer kind/group when matching policies #523

Closed erikgb closed 6 days ago

erikgb commented 3 weeks ago

This PR makes approver-policy adhere to the cert-manager controller defaults for issuer kind/group.

When referencing issuers in cert-manager Certificate and CertificateRequest, the issuerRef kind and group are optional and defaulted in the cert-manager controller (a.k.a. controller defaults). This becomes problematic in approver-policy if you want to enforce a policy addressing cert-manager issuers. Example (irrelevant details omitted):

Policy:

apiVersion: policy.cert-manager.io/v1alpha1
kind: CertificateRequestPolicy
metadata:
  name: allow-all-cert-manager-issuers
spec:
  selector:
    issuerRef:
      group: cert-manager.io
      kind: Issuer

Certificate:

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: my-cert
spec:
  issuerRef:
    name: my-issuer

This kind of simple certificate is common in operators using kubebuilder/operator-sdk for development. The reason probably being to keep the scaffolding in these CLI tools as simple as possible,

Expected behavior: The policy matches certificate requests because cert-manager defaults match policy issuerRef group and kind.

Actual behavior: Policy doesn't select the requests ending up in never-approved requests. 😒

Possible workaround: Use a mutating webhook to materialize the cert-manager controller defaults into cert-manager resources. Tutorial: https://cert-manager.io/docs/tutorials/certificate-defaults/. I am personally not a big fan of this workaround, as every webhook call increases the operational risk of outages in your cluster.

This issue was also discussed in this long thread on Slack, even if it turned out to not be the root cause of the issue that started the thread.

erikgb commented 3 weeks ago

/test pull-cert-manager-approver-policy-test

erikgb commented 2 weeks ago

I haven't done any E2E testing, but I trust that you have.

@wallrj I haven't been able to test this end-to-end. I would love to write an e2e-test for this, but I don't want to duplicate a lot of code. Could you please take a look at https://github.com/cert-manager/approver-policy/pull/525?

I am putting this PR back into draft mode while I work on adding an e2e-test.

erikgb commented 1 week ago

I haven't done any E2E testing, but I trust that you have.

@wallrj I haven't been able to test this end-to-end. I would love to write an e2e-test for this, but I don't want to duplicate a lot of code. Could you please take a look at #525?

I am putting this PR back into draft mode while I work on adding an e2e-test.

@wallrj After our common understanding in today's stand-up, I added an integration test (where envTest is used) instead of an additional e2e-test running on kind. Should be ready for another round of review now. PTAL!

cert-manager-prow[bot] commented 6 days ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/cert-manager/approver-policy/blob/main/OWNERS)~~ [wallrj] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
inteon commented 6 days ago

/unhold LGTM, as discussed we can release this in v0.17.0 for added visibility.

erikgb commented 6 days ago

Thanks for reviewing, @wallrj!

/unhold