Inject certs for metrics endpoint when cert-manager is enabled #478

ardaguclu · 2025-03-28T08:12:53Z

What type of PR is this?

/kind feature

What this PR does / why we need it

ServiceMonitor resource is configured to serve TLS with insecureSkipVerify: true . However, this is not recommended for production use cases. This PR injects the appropriate certificates to the ServiceMonitor to serve from the trusted certificates within the cluster, when cert-manager (or another external certificate management tool) is enabled.

This PR configures kustomize and helm charts according to this.

There is no functional change, when internal cert management is enabled (which is the default setting).

This PR is inspired by kubernetes-sigs/kueue#4385

Does this PR introduce a user-facing change?

Injecting certificates to ServiceMonitor in order to serve securely, when cert-manager is enabled

k8s-ci-robot · 2025-03-28T08:12:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ardaguclu
Once this PR has been reviewed and has the lgtm label, please assign ahg-g for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

netlify · 2025-03-28T08:13:09Z

✅ Deploy Preview for kubernetes-sigs-lws canceled.

Name	Link
🔨 Latest commit	`21ae63b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-lws/deploys/67f7a3e1a60b3b0008109cd0

yankay · 2025-04-01T05:46:34Z

I'm not sure, whether additional documentation needs to be supplemented later, like kubernetes-sigs/kueue#4598

ardaguclu · 2025-04-02T05:12:31Z

I'm not sure, whether additional documentation needs to be supplemented later, like kubernetes-sigs/kueue#4598

I think, adding documentation in a followup PR is reasonable and useful.

/cc @kerthcet

kannon92 · 2025-04-02T19:34:34Z

api/config/v1alpha1/configuration_types.go

@@ -108,6 +108,10 @@ type InternalCertManagement struct {
 	// Enable controls whether to enable internal cert management or not.
 	// Defaults to true. If you want to use a third-party management, e.g. cert-manager,
 	// set it to false. See the user guide for more information.
+	// This secret is mounted to the lws controller manager pod. The mount


What secret do you mean here?

Thanks for dropping your comment. I think, mentioning this behavior in this flag is not relevant. So that I've removed this comment entirely.

atiratree

Btw, I have opened a similar PR for the jobset controller: kubernetes-sigs/jobset#863

config/default/kustomization.yaml

charts/lws/templates/certmanager/certificatemetrics.yaml

config/default/kustomization.yaml

cmd/main.go

atiratree · 2025-04-03T09:17:38Z

config/components/prometheus/monitor_tls_patch.yaml

+        key: ca.crt
+    cert:
+      secret:
+        name: metrics-server-cert


IMO it is a bit weird to let the prometheus client use the same cert that we use in our server. I have left this up to the user to specify which certificate their prometheus instance is using https://github.com/kubernetes-sigs/jobset/pull/863/files#diff-d1323215bef44ba29d323d647a042eed78e05ce849be1e3f8c067a89fd735359R23-R26

We are hardcoding nearly all fields and in my opinion we can hardcode it too.

IMO it is not entirely correct to let the prometheus use this as a client cert for auth. As mentioned in kubernetes-sigs/jobset#863 (comment) there are many things that have to be configured before running cert manager + prometheus. So I think the best way would be to let user decide if they want to use the server certificate for this. They can easily opt in to it.

I have tried to document this in:

https://github.com/atiratree/kubernetes-sigs-jobset/blob/enable-tls-metrics/config/components/prometheus/kustomization.yaml

https://github.com/atiratree/kubernetes-sigs-jobset/blob/enable-tls-metrics/config/components/prometheus/monitor_tls_patch.yaml

there are many things that have to be configured before running cert manager + prometheus.

We only require to uncomment some configurations in kustomize.

If user relies on cert-manager, we can use the same certificates both in client and server because source of truth is the same (i.e. cert-manager) within this cluster.

If user relies on internal certificate management, skipping tls verification in servicemonitor would simply work. If we force users to find and inject a certificate in servicemonitor, users can't inject any arbitrary certificate. This certificate must comply with the one that is managed internally in LWS. I think, this dramatically hurts the user experience. Because I don't think, as a user I can successfully configure this cluster on my own.

I understand the usability perspective, but technically that certificate is wrong. In mutual TLS the domain name would be off.

I am okay, by adding it as a default, but IMO it should be configurable and we should write a disclaimer there.

atiratree · 2025-04-03T09:26:19Z

config/default/kustomization.yaml

@@ -23,6 +23,7 @@ resources:
 - ../webhook
 # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'. 'WEBHOOK' components are required.
 - ../internalcert
+#- ../certmanager
 # [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.


btw I think it would be good to allow passing certs to metrics even without a cert manager: https://github.com/kubernetes-sigs/jobset/pull/863/files#diff-c0dbb993e3ea0a70d85d76af178d499d9fe0ffa7fc9ad44d58d1acc7875e71d0R91

atiratree · 2025-04-03T09:27:24Z

cc @kannon92, the questions can apply also to kubernetes-sigs/kueue#4385

ardaguclu · 2025-04-03T10:50:38Z

/hold
to apply the suggested changes from @atiratree

ardaguclu · 2025-04-04T05:34:22Z

/hold cancel

ardaguclu · 2025-04-07T07:53:39Z

I've tested this PR on a live cluster and it worked without any issue. Certificates are injected correctly and Prometheus successfully scraped the metrics in a secure way.
@kerthcet would you mind taking a look at this PR, when you have a chance?.

charts/lws/templates/certmanager/certificatemetrics.yaml

atiratree · 2025-04-07T22:46:14Z

charts/lws/templates/certmanager/certificatemetrics.yaml

+spec:
+  dnsNames:
+    - lws-controller-manager-metrics-service.{{ .Release.Namespace }}.svc
+    - lws-controller-manager-metrics-service.{{ .Release.Namespace }}.svc.cluster.local


It would be better if the service name referenced a helm variable.

I think, users prefer simply applying these configurations to work with whatever set as default. Probably there is no functional need to configure the service name. I'll defer the decision to approvers.

I am not saying it has to be configurable. I just mean to share a variable so we do not have to harcdode the name in multiple places.

atiratree · 2025-04-07T22:49:53Z

charts/lws/templates/certmanager/certificatemetrics.yaml

@@ -0,0 +1,18 @@
+{{- if .Values.enableCertManager }}


shouldn't this depend both on the cert manager and prometheus?

If cert-manager is enabled, metrics will be configured to run with TLS. That is independent from whether prometheus is enabled or not. If prometheus is not enabled, that means nobody scrapes metrics, but metrics server still runs in secure.

Right, it makes sense to secure the metrics even without prometheus scraping. I will update my PR as well.

charts/lws/templates/certmanager/certificatemetrics.yaml

atiratree · 2025-04-07T22:53:58Z

charts/lws/templates/manager/deployment.yaml

+        {{- if .Values.enableCertManager }}
+        - name: metrics-cert
+          secret:
+            secretName: lws-metrics-server-cert


it would be better if we could reference a common variable here and in the certificate

atiratree · 2025-04-07T22:55:54Z

charts/lws/templates/manager/deployment.yaml

@@ -88,3 +93,16 @@ spec:
          secret:
            defaultMode: 420
            secretName: lws-webhook-server-cert
+        {{- if .Values.enableCertManager }}


Should we mount the secret when prometheus is disabled?

not relevant anymore: https://github.com/kubernetes-sigs/lws/pull/478/files#r2032112051

charts/lws/values.yaml

charts/lws/templates/prometheus/monitor.yaml

atiratree · 2025-04-07T23:05:01Z

charts/lws/values.yaml

@@ -49,3 +49,7 @@ resources:
 nodeSelector: {}
 tolerations: []
 affinity: {}
+metrics:


lws-prometheus-k8s rolebinding monitoring namespace should be configurable IMO

atiratree · 2025-04-07T23:07:57Z

config/certmanager/certificate-metrics.yaml

+apiVersion: cert-manager.io/v1
+kind: Certificate
+metadata:
+  name: metrics-cert  # this name should match the one appeared in kustomizeconfig.yaml


helm review applies here as well

I'll check any missing piece in helm configurations. But I tested kustomize configurations extensively and it works.

I meant mostly this https://github.com/kubernetes-sigs/lws/pull/478/files#r2032107950

atiratree · 2025-04-07T23:08:35Z

config/certmanager/kustomization.yaml

@@ -1,5 +1,6 @@
 resources:
 - certificate.yaml
+- certificate-metrics.yaml


does it make sense to enable this by default?

By default cert-manager is disabled. If we enable this by default, I think we would get Certificate resource not found error?.

Ok makes sense, let's secure metrics regardless of prometheus.

atiratree · 2025-04-10T10:02:18Z

config/components/prometheus/monitor_tls_patch.yaml

+        key: ca.crt
+    cert:
+      secret:
+        name: metrics-server-cert


I understand the usability perspective, but technically that certificate is wrong. In mutual TLS the domain name would be off.

I am okay, by adding it as a default, but IMO it should be configurable and we should write a disclaimer there.

atiratree · 2025-04-10T10:12:40Z

charts/lws/templates/certmanager/certificatemetrics.yaml

@@ -0,0 +1,18 @@
+{{- if .Values.enableCertManager }}


Right, it makes sense to secure the metrics even without prometheus scraping. I will update my PR as well.

charts/lws/templates/certmanager/certificatemetrics.yaml

atiratree · 2025-04-10T10:19:43Z

charts/lws/templates/certmanager/certificatemetrics.yaml

+spec:
+  dnsNames:
+    - lws-controller-manager-metrics-service.{{ .Release.Namespace }}.svc
+    - lws-controller-manager-metrics-service.{{ .Release.Namespace }}.svc.cluster.local


I am not saying it has to be configurable. I just mean to share a variable so we do not have to harcdode the name in multiple places.

atiratree · 2025-04-10T10:21:18Z

charts/lws/templates/manager/deployment.yaml

@@ -88,3 +93,16 @@ spec:
          secret:
            defaultMode: 420
            secretName: lws-webhook-server-cert
+        {{- if .Values.enableCertManager }}


not relevant anymore: https://github.com/kubernetes-sigs/lws/pull/478/files#r2032112051

atiratree · 2025-04-10T10:22:30Z

charts/lws/templates/prometheus/monitor.yaml

@@ -15,7 +15,23 @@ spec:
      port: https
      scheme: https
      tlsConfig:
-        insecureSkipVerify: true
+        {{- if .Values.enableCertManager }}
+        serverName: {{ include "lws.fullname" . }}-controller-manager-metrics-service.{{ .Release.Namespace }}.svc


we could use a variable here

atiratree · 2025-04-10T10:26:42Z

config/certmanager/certificate-metrics.yaml

+apiVersion: cert-manager.io/v1
+kind: Certificate
+metadata:
+  name: metrics-cert  # this name should match the one appeared in kustomizeconfig.yaml


I meant mostly this https://github.com/kubernetes-sigs/lws/pull/478/files#r2032107950

charts/lws/templates/prometheus/monitor.yaml

atiratree · 2025-04-10T10:36:00Z

config/certmanager/kustomization.yaml

@@ -1,5 +1,6 @@
 resources:
 - certificate.yaml
+- certificate-metrics.yaml


Ok makes sense, let's secure metrics regardless of prometheus.

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 28, 2025

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 28, 2025

k8s-ci-robot requested review from Edwinhr716 and yankay March 28, 2025 08:13

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 28, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2025

ardaguclu force-pushed the secure-serving-metrics branch from c5ded7e to c26d738 Compare April 2, 2025 05:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2025

ardaguclu force-pushed the secure-serving-metrics branch from c26d738 to 2fcf1fa Compare April 2, 2025 05:11

kannon92 reviewed Apr 2, 2025

View reviewed changes

ardaguclu force-pushed the secure-serving-metrics branch from 2fcf1fa to aa1a9c4 Compare April 3, 2025 06:34

atiratree reviewed Apr 3, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2025

ardaguclu force-pushed the secure-serving-metrics branch from aa1a9c4 to ec507c1 Compare April 4, 2025 05:27

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2025

ardaguclu force-pushed the secure-serving-metrics branch from ec507c1 to 34deba8 Compare April 7, 2025 07:51

atiratree reviewed Apr 7, 2025

View reviewed changes

ardaguclu force-pushed the secure-serving-metrics branch 2 times, most recently from 86f5385 to 5e67291 Compare April 8, 2025 10:42

Inject certs for metrics endpoint when cert-manager is enabled

db31af0

ardaguclu force-pushed the secure-serving-metrics branch from 5e67291 to db31af0 Compare April 8, 2025 10:50

atiratree reviewed Apr 10, 2025

View reviewed changes

Define commonName for Certificates to comply with cert-manager

21ae63b

ardaguclu mentioned this pull request Apr 11, 2025

Release 0.7 Roadmap #497

Open

Inject certs for metrics endpoint when cert-manager is enabled #478

Are you sure you want to change the base?

Inject certs for metrics endpoint when cert-manager is enabled #478

Conversation

ardaguclu commented Mar 28, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 28, 2025

netlify bot commented Mar 28, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-lws canceled.

yankay commented Apr 1, 2025

ardaguclu commented Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree left a comment

Choose a reason for hiding this comment

atiratree Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Apr 3, 2025

ardaguclu commented Apr 3, 2025

ardaguclu commented Apr 4, 2025

ardaguclu commented Apr 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardaguclu commented Mar 28, 2025 •

edited

Loading

netlify bot commented Mar 28, 2025 •

edited

Loading

ardaguclu commented Apr 2, 2025 •

edited

Loading

atiratree Apr 3, 2025 •

edited

Loading