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

Piping render to deploy command is potentially dangerous #914

Open
kimmobrunfeldt opened this issue Jan 16, 2023 · 1 comment
Open

Piping render to deploy command is potentially dangerous #914

kimmobrunfeldt opened this issue Jan 16, 2023 · 1 comment

Comments

@kimmobrunfeldt
Copy link

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
Copy link
Contributor

👋🏻

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

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

No branches or pull requests

2 participants