Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Add proposal for ssl specification in Grafana external block #1594

Conversation

aboulay-numspot
Copy link
Contributor

Aim of the merge request

This MR aims to create a document to propose an evolution on the Grafana CR to permits to give certificate to the Grafana external block.

Breaking change

There is no breaking change because it is just a proposal document. Nothing is implemented yet.

This should be discussed during a maintainer meeting.

@theSuess
Copy link
Member

theSuess commented Jul 1, 2024

Thanks for this! IMHO this approach is better than #1590.

Regarding the proposal - do you see any use case where the certificates & keys live in different secrets?

I'd prefer to keep this as simple as possible and think that the use case can be covered by providing a single kubernetes.io/tls secret reference with the keys tls.crt,tls.key and ca.crt

@aboulay-numspot
Copy link
Contributor Author

Thanks for this! IMHO this approach is better than #1590.

Regarding the proposal - do you see any use case where the certificates & keys live in different secrets?

I'd prefer to keep this as simple as possible and think that the use case can be covered by providing a single kubernetes.io/tls secret reference with the keys tls.crt,tls.key and ca.crt

I agree with it. To be honest, I want to use the same mechanism that we use for Grafana credentials because I want to avoid implementing multiple mechanisms to retrieve secrets (and use an existing mechanism). This is why the key name is present multiple times.

If you think there could be a better model for this proposal, please let me know. :)

@aboulay-numspot
Copy link
Contributor Author

Thanks for this! IMHO this approach is better than #1590.

Regarding the proposal - do you see any use case where the certificates & keys live in different secrets?

I'd prefer to keep this as simple as possible and think that the use case can be covered by providing a single kubernetes.io/tls secret reference with the keys tls.crt,tls.key and ca.crt

@theSuess After thinking on the subject, I don't think enforcing the secret type to tls is a good idea. The current behavior I was aiming for is like something you can found in the FluxCD's HelmRepository CR : https://fluxcd.io/flux/components/source/helmrepositories/#cert-secret-reference

Here, you get two behaviors:

  • tls.crt and tls.key, to specify the client certificate and private key used for TLS client authentication. These must be used in conjunction, i.e. specifying one without the other will lead to an error.
  • ca.crt, to specify the CA certificate used to verify the server, which is required if the server is using a self-signed certificate.

To ensure these behaviors "The Secret should be of type Opaque or kubernetes.io/tls." because you cannot create a tls type secret if you don't have tls.crt and tls.key declared at the creation (this leads to an error).

This behavior is the same with Grafana Alloy's Loki block where the behavior is something similar: https://grafana.com/docs/alloy/latest/reference/components/loki.write/

This means that the proposed block will evolve into something like this:

<...>
    tls:
      certSecretRef: <name of the secret which contains the certificate>
      insecureSkipVerify: false
<...>

@aboulay-numspot
Copy link
Contributor Author

Proposal updated to present the validated format from #1594 (comment)

@aboulay-numspot aboulay-numspot force-pushed the docs/add-ssl-specification-to-grafana-external branch from c8a7a62 to e4d86c7 Compare July 8, 2024 09:51
@aboulay-numspot aboulay-numspot force-pushed the docs/add-ssl-specification-to-grafana-external branch from e4d86c7 to 4dcc3d3 Compare July 15, 2024 08:18
@aboulay-numspot aboulay-numspot force-pushed the docs/add-ssl-specification-to-grafana-external branch from 4dcc3d3 to 54954da Compare July 15, 2024 15:02
@theSuess theSuess enabled auto-merge July 16, 2024 09:03
@theSuess theSuess added this pull request to the merge queue Jul 16, 2024
Merged via the queue into grafana:master with commit 5c4960f Jul 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants