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

microsatellites with tls, multiple ports, and image update #12

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

Conversation

justin-chizer
Copy link

I made some changes to help align this chart to how we use microsatellites while following your docs.
Currently we have to use Kustomize along with Helm to deploy the chart and these changes would make that unnecessary. Please let me know if there is anything else I need to change.

@justin-chizer justin-chizer changed the title changes to lightstep helm chart microsatellites with tls, multiple ports, and image update May 5, 2022
Increment chart version
Run helm-docs to update README
Copy link
Collaborator

@AjohnsonLS AjohnsonLS left a comment

Choose a reason for hiding this comment

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

This looks great, and thank you for your additions!

@AjohnsonLS
Copy link
Collaborator

This is currently failing to lint. I believe something in the deployments file introduced either some white space or one of the new values is being improperly indented. I'm investigating this currently, but welcome if you see anything obvious in that regard.

@justin-chizer
Copy link
Author

This is currently failing to lint. I believe something in the deployments file introduced either some white space or one of the new values is being improperly indented. I'm investigating this currently, but welcome if you see anything obvious in that regard.

I think I fixed the lint issue. Just waiting on the GHA to run.

@flamein
Copy link

flamein commented Apr 7, 2023

This approach is at odds with the way kubernets stores certificate secrets though, we're looking for a way on how we could concat the ca.crt and and tls.crt as cert-manager makes them into the expected bundle.pem

@flamein
Copy link

flamein commented Apr 12, 2023

to clarify my previous comment:

What I’m saying is that the proposed solution is going against the standard way of handling certificates in kubernetes, namely, store them in a kubernets.io/tls type secret, which looks like this:

Name:         some-name
Namespace:    some-namespace
Labels:       <none>
Annotations:  cert-manager.io/alt-names: some.some-namespace.svc.cluster.local,some-name.some-namespace.svc.cluster.local
              cert-manager.io/certificate-name: some-name
              cert-manager.io/common-name:
              cert-manager.io/ip-sans:
              cert-manager.io/issuer-group: cas-issuer.jetstack.io
              cert-manager.io/issuer-kind: GoogleCASClusterIssuer
              cert-manager.io/issuer-name: some-issuer
              cert-manager.io/uri-sans:

Type:  kubernetes.io/tls

Data
====
tls.key:  1679 bytes
ca.crt:   2110 bytes
tls.crt:  1899 bytes

In this case it has some more annotations because of our use of cert-manager, but the tls type secret is a kubernetes standard.

Kubernetes then allows to mount this secret on a directory, and expose the keys as files in that directory, keeping them updated as the secret is updated, by for instance cert-manager.
This refresh does not happen when you have to rename the files by use of a subpath mount in order to have them picked up by the lightstep microsatellite.

Of course doing it like I propose would require changes on the microsatellite side, like:

  • Monitoring of changed certs, and some reload/refresh/restart of the satellite nodes
  • Allowing full path to the certificate to be provided

but in the end it would mean a more automated deployment that wouldn't require a manual intervention to change the certificate secret and restart the deployment to pick it up.

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.

3 participants