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

Introducing safels and safetree commands to the consul-template. #1132

Closed
wants to merge 2 commits into from

Conversation

vaLski
Copy link
Contributor

@vaLski vaLski commented Aug 15, 2018

This pull request is based on rough idea dropped by Aaron Hurt.

safels and safetree behave exactly like the native ls and tree with one exception. They will refuse to render template, if KV prefix query return blank/empty data.

This is especially useful for rendering mission critical files that do not tolerate ls/tree KV queries to return blank data.

safels and safetree work in stale mode just as their ancestors but we get extra safety on top.

safels and safetree commands were born as an attempt to mitigate issues described here:

#1131
hashicorp/consul#3975
hashicorp/consul-replicate#82

safels and safetree behave exactly like the native ls and tree with one exception. They will *refuse* to render template, if KV prefix query return blank/empty data.

This is especially usefull for rendering mission critical files that do not tolerate ls/tree KV queries to return blank data.

safels and safetree work in stale mode just as their ancestors but we get extra safety on top.

safels and safetree commands were born as an attempt to mitigate issues described here:

  hashicorp#1131
  hashicorp/consul#3975
  hashicorp/consul-replicate#82
…aming convention (key, keyOrDefault etc.) renaming safels to safeLs and safetree to safeTree
@pierresouchay
Copy link

Might not be needed since root cause is fIxed by hashicorp/consul#4554

@eikenb
Copy link
Contributor

eikenb commented Jun 18, 2019

Hey @vaLski, thanks for the PR.

In light of hashicorp/consul#4554 do you still think this is a good addition?

@mitaka, @dkanchev. You both 👍'd this... what do you think?

Thanks.

@vaLski
Copy link
Contributor Author

vaLski commented Jun 19, 2019

@eikenb Thank you for getting back to me.

Indeed the root cause of the issue I reported was fixed.

However if you think that the proposed changes are good enough and might help people preventing a disaster feel free to merge them or ask for further improvements.

While evaluating, kindly note the following:

  • If an operator is rendering mission critical data (like authorized_keys, iptables firewalls etc.), he might consider using safels/safetree to ensure that he will not end up with an empty file. Due to bug in consul like the one described or accidental / operator error that will consul kv delete -recurse /authkeysprefix/
  • Besides [BUGFIX] Avoid returning empty data on startup of a non-leader server consul#4554 I can recall another discussion I had around (alas can't find it) where operator reported that consul was returning 404 for KV lookups, instead of 5XX during an outage. In his case I can recall that the following DC could not fetch its ACLs from the main dc due to network error or something, consul started answering with 404 for kv lookups, consul-template decided that those KVs were erased and result was case similar to mine - data deleted in following dc KV store. I think that was fixed too in consul master but I can't find the exact discussion / PR etc.
  • While digging for that discussion mentioned above I found yet one more case similar to those two described KV API returning 404 for existing keys with ACLs enabled consul#2637. So historically there is a trend those things happen and I assume that they will continue to via one way or another.
  • We personally use custom build of consul-template with the proposed changes since that outage and leverage those operators where data is considered of high importance.
  • For data that should be rendered and is of absolute critical importance, it is no longer pulled from the KV store but rather hardcoded into the template.
  • We also changed consul-replicate settings to never use stale data.

Feel free to comment.

@eikenb
Copy link
Contributor

eikenb commented Aug 21, 2019

I think I'll accept this as new template functions don't really complicate the code base much as they are pretty well self contained. I have been asking other new function writer to provide function tests... but I'll let that slide here as this is implemented more as a tweak to an existing function vs. a brand new one. But if you feel like it.... feel free. :)

@eikenb eikenb closed this in 2734e51 Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants