smallstep / helm-charts

Helm packages for Kubernetes
Apache License 2.0
45 stars 69 forks source link

Extra Containers #170

Closed kfox1111 closed 5 months ago

kfox1111 commented 6 months ago

You can add extraInitContainers but can't currently add extraContainers.

kfox1111 commented 6 months ago

being able to set shareProcessNamespace: true on the pod would be useful as well.

maraino commented 6 months ago

Hi @kfox1111, are you talking about step-certificates? What's the use case for adding extra containers?

kfox1111 commented 6 months ago

Current idea is a sidecar container for refreshing X5C ca certificates.

maraino commented 6 months ago

How do you plan to do that?

If your provisioners are in the ca.json, this is mounted as readOnly from a configMap, so you will need to change the configMap and then reload step-ca, kill -HUP will work for that. But as you already need permissions to change the configMap why not run this in a different pod and rollout restart the step-certificates deployment?

If your provisioners are stored in the database, authority.enableAdmin: true, you can use step ca provisioner update to do that, which also can be executed from a pod.

In any case, if you're willing to provide a PR for this, we will take a look.

kfox1111 commented 6 months ago

hmm.... maybe a flag to override the configfile location too then (https://github.com/smallstep/helm-charts/blob/master/step-certificates/templates/ca.yaml#L73)? An init container can copy the config file to a temp writable emptydir, do whatever initial config needed, and then a sidecar container can update it as needed. the extraVolumes/extraVolumeMounts is already there.

I've been playing around with the rollout restart idea in the time between when I filed the issue and now already. Ran into some snags:

  1. rollout restart doesn't seem to work for me properly on the statefulset. Haven't figured out why yet though.
  2. It gets complicated around upgrade time. If you helm upgrade the step-certificates then it looses the right config. The deployment is a bit tricky to get to notice the update and do another configmap update then the rollout restart.

Using the admin stuff would work, but does expose additional services out via the rest api, so ideally would be nice not to use unless there wasn't an alternative? Better security to use the config file I think.

One more option would be is if the X5C root could be read from a separate file, rather then in the ca.json. But still would want a sidecar to update that file and send the signal?

I'd be happy to write a PR, but unfortunately a quick reading of the CLA, its likely going to be a problem with the lawyers I think. :/ Similar CLA's have been blocked by them I think.

maraino commented 6 months ago

You could also do kubectl exec step-certificates-0 -- kil -HUP 1, but that doesn't guarantee that the ca.json in the configMap is actually refreshed.

rollout restart doesn't seem to work for me properly on the statefulset. Haven't figured out why yet though.

Have you tried this:

kubectl rollout restart statefulsets/step-certificates

It gets complicated around upgrade time. If you helm upgrade the step-certificates then it looses the right config. The deployment is a bit tricky to get to notice the update and do another configmap update then the rollout restart.

From those words, I get you have initialized the CA using the deprecated but still default bootstrap script. And yes, that doesn't make it easy to upgrade the deployment, although I think it doesn't recreate the configuration.

The documented method creates a values.yaml that contains the full configuration and makes the upgrade way easier. You can see it in the README.md

step ca init --helm > values.yaml
echo "password" | base64 > password.txt
helm install -f values.yaml \
     --set inject.secrets.ca_password=$(cat password.txt) \
     --set inject.secrets.provisioner_password=$(cat password.txt) \
     --set service.targetPort=9000 \
     step-certificates smallstep/step-certificates

You can adapt the values.yaml to your needs and update the configuration when desired.

You should be able to keep your current PKI, either updating the values.yaml manually, you can run the command to get an example, or just creating a new one using the root you already have:

step ca init --help --root root_ca.crt --key root_ca.key > values.yaml

This will create a new intermediate, but it will keep the same root.

You also can play with existingSecrets, but the step ca init --helm should be the way to go.

Regarding the CLI, it's very standard, and you are not obligated to do anything. You and, in this case, your employer are basically agreeing that we can do whatever we want with that contribution. These CLAs protect us against your patents, but that PR won't have anything you can actually patent.

kfox1111 commented 6 months ago

You could also do kubectl exec step-certificates-0 -- kil -HUP 1, but that doesn't guarantee that the ca.json in the configMap is actually refreshed.

Yeah. Deleting the pod would be more reliable. But updating the config in the pod even more so.

rollout restart doesn't seem to work for me properly on the statefulset. Haven't figured out why yet though.

Have you tried this:

kubectl rollout restart statefulsets/step-certificates

Yes. That. Didn't restart though. It may be because I tried a few options to make rollout smooth. Blocking in an init container until the X5C root is specified. or letting it start without the init container but X5C blank causes it to crashloop, but not get replaced on rollout. delete pod on the pod though though, which is weird.

It gets complicated around upgrade time. If you helm upgrade the step-certificates then it looses the right config. The deployment is a bit tricky to get to notice the update and do another configmap update then the rollout restart.

From those words, I get you have initialized the CA using the deprecated but still default bootstrap script. And yes, that doesn't make it easy to upgrade the deployment, although I think it doesn't recreate the configuration.

The documented method creates a values.yaml that contains the full configuration and makes the upgrade way easier. You can see it in the README.md

step ca init --helm > values.yaml
echo "password" | base64 > password.txt
helm install -f values.yaml \
     --set inject.secrets.ca_password=$(cat password.txt) \
     --set inject.secrets.provisioner_password=$(cat password.txt) \
     --set service.targetPort=9000 \
     step-certificates smallstep/step-certificates

I had done it with that procedure.

You can adapt the values.yaml to your needs and update the configuration when desired.

Was trying to get to minimal tweaking of values.yaml but instead deploying it then tweaking the X5C at runtime with the right value. Having to modify in a dynamically refreshed X5C root at upgrade time is difficult.

You should be able to keep your current PKI, either updating the values.yaml manually, you can run the command to get an example, or just creating a new one using the root you already have:

step ca init --help --root root_ca.crt --key root_ca.key > values.yaml

This will create a new intermediate, but it will keep the same root.

You also can play with existingSecrets, but the step ca init --helm should be the way to go.

there doesn't seem to be an easy way to decouple the management of the X5C Roots from the rest of the ca settings, so its difficult to update without modifying parts of a values file and doing a helm upgrade.

Regarding the CLI, it's very standard, and you are not obligated to do anything. You and, in this case, your employer are basically agreeing that we can do whatever we want with that contribution. These CLAs protect us against your patents, but that PR won't have anything you can actually patent.

Yeah, the CLA patent stuff has been a problem in the past. Its hard to get the lawyers to "risk the patent portfolio" of a national lab over a helm chart, even when there really shouldn't ever be any patents involved. So, the CLA is kind of limiting to some contributors. Sorry. :/

maraino commented 5 months ago

Hi @kfox1111, I've created a PR allowing you to add extra containers, share the process namespace and volumes, and more. Take a look at #173, and feel free to comment.

maraino commented 5 months ago

I've created a new release of the step-certificates chart.