Shopify / krane

A command-line tool that helps you ship changes to a Kubernetes namespace and understand the result
MIT License
1.38k stars 114 forks source link

Piping render to deploy command is potentially dangerous #914

Open kimmobrunfeldt opened 1 year ago

kimmobrunfeldt commented 1 year ago

This isn't a bug in code but a note from the documentation. I'm opening this up for knowledge sharing purposes. We've used krane for deployments for a few years now. Along the way we've had a few incidents including downtime where rendering an empty output (due to error during rendering), caused deploy command to prune all resources.

I spotted two references from the documentation where piping is mentioned. First under Using templates:

If you want dynamic templates, you may render ERB with krane render and then pipe that result to krane deploy -f -.

Second under Deploy walkthrough:

You can test this out for yourself by running the following command:

krane render -f test/fixtures/hello-cloud --current-sha 1 | krane deploy my-namespace my-k8s-cluster -f -

As soon as you run this, you'll start seeing some output being streamed to STDERR.

The latter reference says "you can test this" but nevertheless it ended up being the command used for our production deployments.

Bash piping starts both commands at the same time (ref), so krane deploy doesn't have a way to know that krane render exited with non-zero exit code. We use -o pipefail, but it doesn't help to prevent this scenario. It only affects on the final exit code of the pipeline operation (ref). --no-prune would prevent this but it's not recommended with reasoning I agree with:

Not recommended, as it allows your namespace to accumulate cruft that is not reflected in your deploy directory.

Would you be up for a documentation PR that removes the piping recommendations and adds a warning about it? We changed the render-deploy one liner to two separate commands: 1) render ... > output.yml 2) deploy -f output.yml. This in combination with set -e makes the deployments commands safe.

There could be alternative solutions. For example deploy command requiring e.g. a single empty line written in stdin to prune resources, and render command would be changed to reflect this. I'm not that familiar with krane so take the suggestion with a grain of salt.

timothysmith0609 commented 9 months ago

👋🏻

What krane version are you running? As of https://github.com/Shopify/krane/pull/866/files#diff-96a88df3c7000093b2e97be25854993ba6b78c37a225960c157583b4418140daR258, an empty resource set should trigger a FatalDeploymentError. Interested to hear if that's not the case