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

Add TLS docs #813

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

andreasgerstmayr
Copy link
Collaborator

Added a document with an overview of our server TLS settings for each pod.

@rubenvp8510 is this doc accurate?

Also, a few questions:

  • Should the gateway use the certificate and CA of spec.template.distributor.tls if configured?
    • if the gateway is enabled and spec.template.distributor.tls is configured, will it break gateway -> distributor connections?
  • Currently there is no way to configure TLS for the gateway on non-OpenShift clusters?
  • Should the TLS config be more flexible? allow choosing between

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
docs/tls.md Outdated
Comment on lines 41 to 42
Azure | no | - |
GCP | no | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean TLS cannot be used for those storages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tempo doesn't expose any TLS settings for Azure or GCP: https://grafana.com/docs/tempo/latest/configuration/#storage

When using https:// for the endpoint it will use TLS, but it'll use the certificate bundle of the container. I'll clarify that.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.35%. Comparing base (83b6a17) to head (8f0d5a3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #813   +/-   ##
=======================================
  Coverage   75.35%   75.35%           
=======================================
  Files          89       89           
  Lines        6383     6383           
=======================================
  Hits         4810     4810           
  Misses       1343     1343           
  Partials      230      230           
Flag Coverage Δ
unittests 75.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rubenvp8510
Copy link
Collaborator

Added a document with an overview of our server TLS settings for each pod.

@rubenvp8510 is this doc accurate?

Also, a few questions:

* Should the gateway use the certificate and CA of `spec.template.distributor.tls` if configured?
  
  * if the gateway is enabled and `spec.template.distributor.tls` is configured, will it break gateway -> distributor connections?

I remember we prevent this when we generate the cofniguration, if the gateway is enabled TLS configuration is ignored., actually I think it will generate a bad configuration. https://github.com/grafana/tempo-operator/blob/main/internal/manifests/config/tempo-config.yaml#L12 I'll test today afternoon. I think if that is the case we need to add some validations to the webhook.

* Currently there is no way to configure TLS for the gateway on non-OpenShift clusters?

It seems is not :/

* Should the TLS config be more flexible? allow choosing between
  
  * `internal` (self-signed by the operator)
  * `serving-ca` ([generated by OpenShift](https://docs.openshift.com/container-platform/4.14/security/certificates/service-serving-certificate.html))?
  * `custom` (ConfigMap and Secret)

I think yes, in the case of self-signed vs custom, you can control this using the feature flags, wondering if we need to control this better on the CRD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants