Skip to content

feat: optionally serve metrics over TLS#2549

Open
npapapietro wants to merge 3 commits intografana:masterfrom
npapapietro:sevmon-tls
Open

feat: optionally serve metrics over TLS#2549
npapapietro wants to merge 3 commits intografana:masterfrom
npapapietro:sevmon-tls

Conversation

@npapapietro
Copy link
Copy Markdown
Contributor

In the scenario that the operator is running in tls mode, this will allow the metrics scraping to work in https mode.

Signed-off-by: Nathan Papapietro <npapapietro95@gmail.com>
Copy link
Copy Markdown
Collaborator

@weisdd weisdd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npapapietro Thanks for the PR. Could you, please, elaborate on this change more? - From what I see, we expose metrics endpoint only via HTTP, so I'm not sure why you would want to have tlsConfig section inside the ServiceMonitor.

@npapapietro
Copy link
Copy Markdown
Contributor Author

On an environment that has TLS enforcing (with cillium or something of your choice) you can mount TLS like this

volumeMounts:
  - mountPath: /tmp/k8s-metrics-server/serving-certs
    name: cert-secret-volume

and the underlying controller metrics server you are using should pick up certs and move to HTTPS. I think this is automatic based on the source code here.

Here is the specific line that calls out this directory.

@weisdd
Copy link
Copy Markdown
Collaborator

weisdd commented Mar 10, 2026

@npapapietro Have you tried it in a lab? - I think TLS will not be served unless SecureServing flag is set to true (link), having certs in the default location will not be enough.

As for the TLS enforcement, it sounds a bit odd to me. - Normally, such enforcement exists when there's automated mTLS in place, which requires no changes to the end workloads (= transparent). Haven't heard of cases where each individual workload would be expected to serve TLS on its own. 🤔

@npapapietro
Copy link
Copy Markdown
Contributor Author

@weisdd I completely missed that flag, I added an cli option and values to helm to allow it.

@weisdd
Copy link
Copy Markdown
Collaborator

weisdd commented Mar 12, 2026

@npapapietro two things:

  1. Could you open an issue describing your use case in more details and link to this PR? Aside from other details, it has to include an explanation on why TLS setup is not transparent to the end workloads like in more or less any mTLS setup.
    We'll then discuss it internally to see if it makes sense to make that change. At the moment, I'm not sure if it's a valid use case.

  2. (assuming the feature request gets accepted) In my view, it's insufficient to add only that flag here and leave the rest to the controller-runtime defaults. - We'll also need to offer path customizations that would be in-sync with the service monitor. Also, the repository has a collection of examples that gets rendered into documentation (examples folder), we'll need to add something there as well.

@theSuess
Copy link
Copy Markdown
Collaborator

@npapapietro are you still interested in this? If so, we'd really appreciate a more detailed explanation of your use case as @weisdd asked - thanks!

@npapapietro
Copy link
Copy Markdown
Contributor Author

Yes I am, I'll fill out an issue today.

Copy link
Copy Markdown
Collaborator

@weisdd weisdd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, as now the feature request is accepted, we can proceed to the implementation.

At the moment, at least the following things are missing:

  • customization options for cert path (directory) and cert / key filenames;
  • dynamic certificate reloading (certs tend to expire).

To simplify the development and to align with operator-sdk & controller-runtime, my suggestion for you would be to check the docs on how to generate the default operator skaffolding (it's literally one command) and then port their implementation to grafana-operator. You'll see dynamic certificate reloading logic there as well.

Please, re-use their flag names and the corresponding defaults where makes sense. It doesn't have to be 1:1 implementation, but should still be very close to their code.

@weisdd weisdd added the feature this PR introduces a new feature label Apr 3, 2026
@weisdd weisdd changed the title Adding tls options for service monitor feat: optionally serve metrics over TLS Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature this PR introduces a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants