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

Update charm tracing libs + add support for exporting traces via HTTPS #465

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

Conversation

shayancanonical
Copy link
Contributor

@shayancanonical shayancanonical commented Jun 20, 2024

Issue

  1. We are using outdated charm tracing libs
  2. We do not have support for exporting traces via HTTPS

Solution

  1. Update the charm libs
  2. Add support for exporting traces via HTTPS
  • Use the cert handler observability charm lib for retrieving tracing server CA cert
  • Cert handler uses tls charm lib v3, so we add tls v3 charm lib to our repo. Upgrade from v2 to v3 for charm tls usage upcoming

Logs when relation with certificates operator does not exist while exporting traces

nit-mysql-0: 20:36:58 INFO juju.worker.uniter.operation ran "tracing-certificates-relation-departed" hook (via hook dispatching script: dispatch)
unit-mysql-0: 20:36:58 ERROR unit.mysql/0.juju-log tracing-certificates:4: Exception while exporting Span batch.
Traceback (most recent call last):
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/opentelemetry/sdk/trace/export/__init__.py", line 368, in _export_batch
    self.span_exporter.export(self.spans_list[:idx])  # type: ignore
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py", line 145, in export
    resp = self._export(serialized_data)
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py", line 114, in _export
    return self._session.post(
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/requests/sessions.py", line 637, in post
    return self.request("POST", url, data=data, json=json, **kwargs)
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/requests/sessions.py", line 589, in request
    resp = self.send(prep, **send_kwargs)
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/requests/sessions.py", line 703, in send
    r = adapter.send(request, **kwargs)
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/requests/adapters.py", line 458, in send
    self.cert_verify(conn, request.url, verify, cert)
  File "/var/lib/juju/agents/unit-mysql-0/charm/venv/requests/adapters.py", line 261, in cert_verify
    raise OSError(
OSError: Could not find a suitable TLS CA certificate bundle, invalid path: /var/snap/charmed-mysql/common/var/run/tracing-ca.crt
unit-mysql-0: 20:36:59 INFO juju.worker.uniter.operation ran "tracing-certificates-relation-broken" hook (via hook dispatching script: dispatch)

Demo

image
Notice that:

  1. Grafana is using HTTPS. Every trace is sent to COS with HTTPS
  2. We have additional info for every trace in the search results table! We do not have to scroll and click through a list of traces to find relevant traces. Furthermore, one can use the Span Name search to filter all traces for the exact type of trace they are interested in!

Steps to reproduce

Largely the same as https://charmhub.io/mysql/docs/h-enable-tracing
Notable differences include:

  1. We need to deploy self-signed-certificates in the COS model and run jhack imatrix fill
  2. We need to expose self-signed-certificates:certificates from the COS model
  3. We need to consume the self-signed-certificates:certificates relation from the COS model in the machine model where MySQL is deployed
  4. We also need to relate juju relate mysql:tracing-certificates self-signed-certificates:certificates

else:
logger.info(
"Creating CSR for %s with DNS %s and IPs %s",
self.cert_subject,

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (certificate)
as clear text.

tempo_ca_cert_path.parent.mkdir(parents=True, exist_ok=True)
if self.cert_handler.ca_cert:
tempo_ca_cert_path.write_text(self.cert_handler.ca_cert)

Check failure

Code scanning / CodeQL

Clear-text storage of sensitive information High

This expression stores
sensitive data (certificate)
as clear text.
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

LGTM in general:

  1. please check security finding fix/dismiss
  2. are we silently migrating from tls lib v2 to v3? How safe is that?
  3. please update PR description with example how to use it properly? should tempo will always be related with both tracing and tracing-certificates. Just curios, why TLS certs + CA are not returned inside tracing if related to TLS on COS side? The separate relation is a way for human mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we silently migrating to tls_certs v3 ? Is the migration v2->v3 transparent? juju refresh mysql?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are not migrating to tls_certs v3 just yet. tls_certs v3 will be used by CertHandler only whereas the charm will use tls_certs v2 until a separate PR is created to migrate and test the upgrade from v2 to v3 for the charm

@shayancanonical
Copy link
Contributor Author

  1. Will follow up with the security finding (not sure how we can fix by avoiding writing the CA Cert on disk)
  2. We are not migrating from v2 to v3 -- we are just using v3 for CertHandler (to retrieve CA cert for tempo-k8s). I will followup shortly with a PR exploring what it would entail to migrate from v2 to v3 for mysql TLS
  3. Will update the PR description shortly after this comment. Following up with the COS team, they indicated the idea to retrieve the TLS CA cert for tempo-k8s from the tracing relation was rejected due to a breach in separation of concerns (where tempo-k8s would be responsible for copying the CA cert from the certificates operator databag and into the tracing relation databag)

@shayancanonical
Copy link
Contributor Author

Blocker alert: CertHandler uses tls_certificates charm lib v3 which is only supported for version >= 3. Thus, exporting traces to tempo-k8s with HTTPS will fail for juju 2.9.X

Copy link
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

Let's go on with TLS for as is for now while we pledge for proxying a single cert.
We do need to update COS documentation to include tracing though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
donotmerge PR still blocked from merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants