#3761 Provide metrics to monitor certificates expiration

[steffen-karlsson]

Type of change

• Enhancement / new feature: [Enhancement] Provide metrics to monitor certificates expiration #3761
• Rewrite/optimise metrics logic

Description

Implement metric emitter for certificates expiration and dashboard to monitor expiration for certificates.

Checklist

• Write tests
• Make sure all tests pass
• Update documentation
• Check RBAC rights for Kubernetes / OpenShift roles
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
• Reference relevant issue(s) and close them after merging
• Update CHANGELOG.md
• Supply screenshots for visual changes, such as Grafana dashboards (added in comment)

[maciej-tatarski]

Tested wit grafana 7.3.7 and 10.3.4. Visualised as a table, which allows to show multiple expire date based on cluster name:

For multiple kafka clusters:

On cert renewal, value of the metric is also changing:

[scholzj]

Sorry, I was at KubeCon so I got to the Pr only now. I left some comments.

[steffen-karlsson]

Sorry, I was at KubeCon so I got to the Pr only now. I left some comments.

Sure, no worries at all :) Hope you had a good time! Hopefully will join next year - was at Kafka Summit this year.

[ppatierno]

Struggling to understand what the latest graph represents.
The text says "On cert renewal, value of the metric is also changing:". What did exactly happen? A certificate was expiring and renewed to expire after 5 mins?

[maciej-tatarski]

@ppatierno Cert was renewed after 5 mins and it was just to show that it is reflected in the metric. This graph is not included in any dashboard.

[fvaleri]

Hi, this is definitely useful. Thanks.

I tried changes on Grafana 7.3.7, including having multiple Kafka clusters, and deleting a cluster.

---

Issue: Expiry time set to epoch start

To reproduce the issue, create a Kafka cluster with metrics, wait for the new metric to appear in operator dashboard, then delete the whole cluster, finally recreate the cluster with the same name.

Certificate content:

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            5f:25:69:2e:67:74:21:35:4e:e8:16:7a:1b:ad:43:01:7f:29:ba:7f
        Signature Algorithm: sha512WithRSAEncryption
        Issuer: O=io.strimzi, CN=cluster-ca v0
        Validity
            Not Before: Mar 25 17:51:58 2024 GMT
            Not After : Mar 25 17:51:58 2025 GMT
        Subject: O=io.strimzi, CN=my-cluster-kafka

Raw metric:

# HELP strimzi_certificate_expiration_ms Time in milliseconds when the certificate expires
# TYPE strimzi_certificate_expiration_ms gauge
strimzi_certificate_expiration_ms{cluster_name="my-cluster",kind="Kafka",namespace="test",selector="",} 0.0

Grafana dashboard:

[steffen-karlsson]

Hello @fvaleri

Thanks for your comments will access them tomorrow.
Regarding your bug, @maciej-tatarski and I are aware of it, we have a solution for now, very ugly - thus not committed it, but will fix, when its decided by @scholzj, @ppatierno and yourselves probably, where KafkaEventHandler fits the best as its depending.

In short essence the problem is that MetricsHolder and its subsets are not singleton, so the new Map (reconciliationsTimerMap) is cleared pr instance, which is not shared across the controllers.
So StrimziPodSetController which resets it has one instance of MetricsHolder and KafkaReconciler who uses and populates it, has another instance. Temporary proven as a fix to make reconciliationsTimerMap static, but definitely not a final solution.

[scholzj]

The screenshot lists only a Cluster CA. Is the Clients CA planned for a follow-up PR?

[scholzj]

when its decided by @scholzj, @ppatierno and yourselves probably, where KafkaEventHandler fits the best as its depending.

@steffen-karlsson I replied to the main open points. If you want to have a Zoom call or something to go through it together, feel free to ping me on Slack. Sometimes it's easier than through the GitHub messages.

[scholzj]

@steffen-karlsson ... I see 122 commits including stuff from other people/PRs, why? Can you fix this? Otherwise I see an history which doesn't make sense for this PR in github, or?

Well, we will squash it when merging it. So not sure it matters that much. But that is likely one of the things confusing the DCO signoff.

[ppatierno]

Well, we will squash it when merging it. So not sure it matters that much. But that is likely one of the things confusing the DCO signoff.

Well couldn't be better to have them squashed now and providing a meaningful single commit message instead of using the squash and merge from GitHub which will produce a very long commit message including even all the others commit messages not related to this PR.
And yes, I agree that was the problem of the DCO imho.

[steffen-karlsson]

Well, we will squash it when merging it. So not sure it matters that much. But that is likely one of the things confusing the DCO signoff.

Well couldn't be better to have them squashed now and providing a meaningful single commit message instead of using the squash and merge from GitHub which will produce a very long commit message including even all the others commit messages not related to this PR.
And yes, I agree that was the problem of the DCO imho.

I don't know what happened, the commits are actually all mine minus a few, had to alter the last half amount of mine as they were not including the Sign off message, therefore the high amount.

While altering the messages, it added the other ones, don't know why, have tried to fix it, but cannot seems to do it, branch is fully up to date with main branch and number of files changes makes sense in terms of the PR.

If you @ppatierno or @scholzj also does not have any idea, the only solution I see is to create a new branch and diff all changes and create a new PR.

I used the suggestions by the DCO action.

[scholzj]

I don't know what happened ...

I guess Git happened ... I think that all of us had this kind of issue in the past.

If you @ppatierno or @scholzj also does not have any idea, the only solution I see is to create a new branch and diff all changes and create a new PR.

I think creating a new PR from another branch might be an option. I think that would be completely fine. Alternatively, I think if you simply squash all of the commits and fix the commit message, it might work as well (but make sure to have a backup copy before you do that :-o)

[ppatierno]

I am fine with whatever is the best solution for you to pick from ... 1. new PR or 2. squashing and leaving one single commit message which makes sense for the overall PR

[steffen-karlsson]

Squashed it all into one commit @ppatierno, DCO is green and tests will be soon :)

Only missing last comment from @scholzj regarding KafkaAssemblyOperator.removeMetrics test, other comments are fixed and handled.

[scholzj]

Sorry, two more things ... one is my fault and one isn't ...

[scholzj]

LGTM. Thanks.

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[fvaleri]

Not sure if it is me, but on Grafana 7.3.7 the dashboard now looks like this. Data is correct, but "type" is hidden, and "prometheus" data source is shown.

[maciej-tatarski]

Not sure if it is me, but on Grafana 7.3.7 the dashboard now looks like this. Data is correct, but "type" is hidden, and "prometheus" data source is shown.

You are right, I removed aggregation and excluded some labels by transformation. I reverted it now as we don't really know which labels people will have so it is better to just aggregate by labels that are always there. @fvaleri please check now, thanks!

[fvaleri]

LGTM. Thanks @maciej-tatarski and @steffen-karlsson.

[ppatierno]

LGTM. Thanks for the PR!
I was waiting for the commits storm to finish :-)
I didn't run it but trust Fede and Jakub for this, but I had another pass on the code, and it makes sense to me.