feat(metrics): refactoring certificate metrics to collector by hjoshi123 · Pull Request #7856 · cert-manager/cert-manager

hjoshi123 · 2025-07-13T23:21:29Z

Migrated certificate metrics to the new collector format instead of using a controller.

Pull Request Motivation

Kind

/kind feature

Release Note

updated `certificate` metrics to the collector approach.

cert-manager-prow · 2025-07-13T23:21:38Z

Hi @hjoshi123. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

erikgb · 2025-07-14T09:06:06Z

/ok-to-test

erikgb

Thanks @hjoshi123! This generally looks good to me. I have added a couple of comments/questions. Hoping that @ThatsMrTalbot can review this when he is back.

erikgb · 2025-07-17T17:50:27Z

internal/apis/config/controller/v1alpha1/defaults.go

 	AllControllers = []string{
 		issuerscontroller.ControllerName,
 		clusterissuerscontroller.ControllerName,
-		certificatesmetricscontroller.ControllerName,


Could this potentially represent a breaking change? Are we somehow able to deprecate this obsolete controller instead?

you mean like keep both approaches and use a feature flag kinda thing?

No, I was thinking if we could keep the controller setting as a no-op. To avoid breaking users who might have configured it?

Ah.. so basically call nothing in ProcessItem func? That kinda makes sense and I think we should also add like a TODO to probably remove it in a future release right?

Yes, that was my thinking. Removing it would only be semi-breaking, so we could do it. But then we need to inform clearly about this in the release notes.

Yup that makes sense and in a future minor release we can remove the controller.

pkg/metrics/metrics_test.go

erikgb

This looks really great, @hjoshi123! Thanks for working on this! I've added a couple of nit suggestions.

pkg/metrics/certificates_test.go

test/integration/framework/helpers.go

test/integration/certificates/metrics_controller_test.go

Signed-off-by: hjoshi123 <mail@hjoshi.me>

erikgb

/lgtm

Hoping that @ThatsMrTalbot can do a level 2 review. 😉

ThatsMrTalbot · 2025-07-24T13:17:42Z

pkg/controller/certificates/metrics/certificates.go

This is fine for now - but we should follow up with a PR that instead of creating a no-op controller allows is to maintain a list of deprecated controller names that are valid as flags/config but don't actually do anything.

ThatsMrTalbot · 2025-07-24T13:21:23Z

This looks great - I would raise a follow up issue around removing the no-op controller in favour of updating how we parse config to maintain a list of "deprecated" controllers instead, but happy to merge as is

/lgtm
/approve

cert-manager-prow · 2025-07-24T13:21:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ThatsMrTalbot

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ThatsMrTalbot]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [cert-manager](https://cert-manager.io) ([source](https://github.com/cert-manager/cert-manager)) | minor | `v1.18.2` -> `v1.19.0` | --- ### Release Notes <details> <summary>cert-manager/cert-manager (cert-manager)</summary> ### [`v1.19.0`](https://github.com/cert-manager/cert-manager/releases/tag/v1.19.0) [Compare Source](cert-manager/cert-manager@v1.18.2...v1.19.0) cert-manager is the easiest way to automatically manage certificates in Kubernetes and OpenShift clusters. This release focuses on expanding platform compatibility, improving deployment flexibility, enhancing observability, and addressing key reliability issues. > 📖 Read the full release notes at cert-manager.io: <https://cert-manager.io/docs/releases/release-notes/release-notes-1.19> Changes since `v1.18.0`: #### Feature - Add IPv6 rules to the default network policy ([#7726](cert-manager/cert-manager#7726), [@jcpunk](https://github.com/jcpunk)) - Add `global.nodeSelector` to helm chart to allow for a single `nodeSelector` to be set across all services. ([#7818](cert-manager/cert-manager#7818), [@StingRayZA](https://github.com/StingRayZA)) - Add a feature gate to default to Ingress `pathType` `Exact` in ACME HTTP01 Ingress challenge solvers. ([#7795](cert-manager/cert-manager#7795), [@sspreitzer](https://github.com/sspreitzer)) - Add generated `applyconfigurations` allowing clients to make type-safe server-side apply requests for cert-manager resources. ([#7866](cert-manager/cert-manager#7866), [@erikgb](https://github.com/erikgb)) - Added API defaults to issuer references group (cert-manager.io) and kind (Issuer). ([#7414](cert-manager/cert-manager#7414), [@erikgb](https://github.com/erikgb)) - Added `certmanager_certificate_challenge_status` Prometheus metric. ([#7736](cert-manager/cert-manager#7736), [@hjoshi123](https://github.com/hjoshi123)) - Added `protocol` field for `rfc2136` DNS01 provider ([#7881](cert-manager/cert-manager#7881), [@hjoshi123](https://github.com/hjoshi123)) - Added experimental field `hostUsers` flag to all pods. Not set by default. ([#7973](cert-manager/cert-manager#7973), [@hjoshi123](https://github.com/hjoshi123)) - Support configurable resource requests and limits for ACME HTTP01 solver pods through ClusterIssuer and Issuer specifications, allowing granular resource management that overrides global `--acme-http01-solver-resource-*` settings. ([#7972](cert-manager/cert-manager#7972), [@lunarwhite](https://github.com/lunarwhite)) - The `CAInjectorMerging` feature has been promoted to BETA and is now enabled by default ([#8017](cert-manager/cert-manager#8017), [@ThatsMrTalbot](https://github.com/ThatsMrTalbot)) - The controller, webhook and ca-injector now log their version and git commit on startup for easier debugging and support. ([#8072](cert-manager/cert-manager#8072), [@prasad89](https://github.com/prasad89)) - Updated `certificate` metrics to the collector approach. ([#7856](cert-manager/cert-manager#7856), [@hjoshi123](https://github.com/hjoshi123)) #### Bug or Regression - ACME: Increased challenge authorization timeout to 2 minutes to fix `error waiting for authorization` ([#7796](cert-manager/cert-manager#7796), [@hjoshi123](https://github.com/hjoshi123)) - BUGFIX: permitted URI domains were incorrectly used to set the excluded URI domains in the CSR's name constraints ([#7816](cert-manager/cert-manager#7816), [@kinolaev](https://github.com/kinolaev)) - Enforced ACME HTTP-01 solver validation to properly reject configurations when multiple ingress options (`class`, `ingressClassName`, `name`) are specified simultaneously ([#8021](cert-manager/cert-manager#8021), [@lunarwhite](https://github.com/lunarwhite)) - Increase maximum sizes of PEM certificates and chains which can be parsed in cert-manager, to handle leaf certificates with large numbers of DNS names or other identities ([#7961](cert-manager/cert-manager#7961), [@SgtCoDFish](https://github.com/SgtCoDFish)) - Reverted adding the `global.rbac.disableHTTPChallengesRole` Helm option. ([#7836](cert-manager/cert-manager#7836), [@inteon](https://github.com/inteon)) - This change removes the `path` label of core ACME client metrics and will require users to update their monitoring dashboards and alerting rules if using those metrics. ([#8109](cert-manager/cert-manager#8109), [@mladen-rusev-cyberark](https://github.com/mladen-rusev-cyberark)) - Use the latest version of `ingress-nginx` in E2E tests to ensure compatibility ([#7792](cert-manager/cert-manager#7792), [@wallrj](https://github.com/wallrj)) #### Other (Cleanup or Flake) - Helm: Fix naming template of `tokenrequest` RoleBinding resource to improve consistency ([#7761](cert-manager/cert-manager#7761), [@lunarwhite](https://github.com/lunarwhite)) - Improve error messages when certificates, CRLs or private keys fail admission due to malformed or missing PEM data ([#7928](cert-manager/cert-manager#7928), [@SgtCoDFish](https://github.com/SgtCoDFish)) - Major upgrade of Akamai SDK. NOTE: The new version has not been fully tested end-to-end due to the lack of cloud infrastructure. ([#8003](cert-manager/cert-manager#8003), [@hjoshi123](https://github.com/hjoshi123)) - Update kind images to include the Kubernetes 1.33 node image ([#7786](cert-manager/cert-manager#7786), [@wallrj](https://github.com/wallrj)) - Use `maps.Copy` for cleaner map handling ([#8092](cert-manager/cert-manager#8092), [@quantpoet](https://github.com/quantpoet)) - Vault: Migrate Vault E2E add-on tests from deprecated `vault-client-go` to the new `vault/api` client. ([#8059](cert-manager/cert-manager#8059), [@armagankaratosun](https://github.com/armagankaratosun)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/1711 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>

wallrj-cyberark · 2025-10-15T15:59:35Z

@hjoshi123 We have released this. Please test and feedback: https://github.com/cert-manager/cert-manager/releases/tag/v1.19.1

cert-manager-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 13, 2025

hjoshi123 force-pushed the feat/certificate-metric-collector-migration branch from 11ff65d to 06f81bc Compare July 13, 2025 23:24

cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2025

hjoshi123 changed the title ~~added cert collector and moved unit test~~ feat(metrics): refactoring certificate metrics to collector Jul 13, 2025

cert-manager-prow bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 14, 2025

hjoshi123 force-pushed the feat/certificate-metric-collector-migration branch 2 times, most recently from 53c4f18 to 803b8cd Compare July 15, 2025 02:30

erikgb reviewed Jul 17, 2025

View reviewed changes

hjoshi123 force-pushed the feat/certificate-metric-collector-migration branch from 803b8cd to 72df9f2 Compare July 20, 2025 03:24

cert-manager-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 20, 2025

hjoshi123 force-pushed the feat/certificate-metric-collector-migration branch from 72df9f2 to eb5aaf2 Compare July 20, 2025 03:32

erikgb mentioned this pull request Jul 20, 2025

Add ability to monitor validity period for CAs in bundle cert-manager/trust-manager#588

Open

erikgb reviewed Jul 20, 2025

View reviewed changes

added cert collector and moved unit test

021a9a4

Signed-off-by: hjoshi123 <mail@hjoshi.me>

hjoshi123 force-pushed the feat/certificate-metric-collector-migration branch from eb5aaf2 to 021a9a4 Compare July 20, 2025 21:29

hjoshi123 requested a review from erikgb July 20, 2025 21:29

erikgb reviewed Jul 23, 2025

View reviewed changes

cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2025

erikgb requested a review from ThatsMrTalbot July 24, 2025 09:18

ThatsMrTalbot reviewed Jul 24, 2025

View reviewed changes

cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2025

cert-manager-prow bot merged commit 76ca203 into cert-manager:master Jul 24, 2025
6 checks passed

This was referenced Jul 24, 2025

REQUEST: New membership for hjoshi123 cert-manager/community#52

Closed

Remove no-op certificate metrics controller #7879

Open

erikgb mentioned this pull request Aug 30, 2025

Support prometheus metrics cert-manager/csi-lib#73

Merged

Conversation

hjoshi123 commented Jul 13, 2025

Pull Request Motivation

Kind

Release Note

Uh oh!

cert-manager-prow bot commented Jul 13, 2025

Uh oh!

erikgb commented Jul 14, 2025

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

Uh oh!

erikgb Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

hjoshi123 Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

erikgb Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

hjoshi123 Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

erikgb Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

hjoshi123 Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

Uh oh!

ThatsMrTalbot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

ThatsMrTalbot commented Jul 24, 2025

Uh oh!

cert-manager-prow bot commented Jul 24, 2025

Uh oh!

Uh oh!

wallrj-cyberark commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants