Remove platformstack dependency

[jujugrrr]

The main goal is to avoid to have to install all the requirements from platformstack.

[martinb3]

For lines like this one:

es_health = Elkstack.get_platformstack_elkstack_monitoring(node, 'plugins', 'elasticsearch_health')

Should we be doing something like include Elkstack on Chef recipes, so we can just reference this everywhere by its short name get_platformstack_elkstack_monitoring? Or perhaps we could make a method that maps all of the older attributes to their new namespaces, just once, so you don't have to write backwards-compat method calls in so many places?

[jujugrrr]

Should we be doing something like include Elkstack on Chef recipes, so we can just reference this everywhere by its short name get_platformstack_elkstack_monitoring?

We could, I like it dirty as I think it should be temporary. I'll update the PR

Or perhaps we could make a method that maps all of the older attributes to their new namespaces, just once, so you don't have to write backwards-compat method calls in so many places?

I wanted to avoid "magic" where we copy attributes to other, which is why I've copied your approach. Ideally all of that should disappear wuickly

[martinb3]

We could, I like it dirty as I think it should be temporary. I'll update the PR :+1: Let's do it the way you described, and then we'll do the version bump and rip it out again :)

[jujugrrr]

Are we happy to merge it ? I'll write some "migration action" before to release it

[martinb3]

Are we happy to merge it ? I'll write some "migration action" before to release it

I think my only remaining concern was that we're turning on a firewall and specifically choosing iptables in the acl recipe. That feels like it's forcing a choice on downstream. Would it be better to omit the firewall definition and just expect people to have declared a firewall before including that recipe?

[jujugrrr]

I think my only remaining concern was that we're turning on a firewall and specifically choosing iptables in the acl recipe.

Before It was setting attributes which were only working for Iptables -> rackspace_iptables.

Before we had a flag called elkstack/config/enabled set to true by default, which was setting attributes but doing nothing until platformstack was in the runlist, so it's was a pretty confusing flag.

Now if the flag is enabled it will make sure you have iptables firewall with the appropriate rules. If you don't want that when using elkstack, set the flag to false and you can decide how to implement it.

That feels like it's forcing a choice on downstream. Would it be better to omit the firewall definition and just expect people to have declared a firewall before including that recipe?

If I've declared a firewall service Iptables before to include elkstack on a Debian, and the rules in elkstack are not specifying anything it will take the default to apply rules which is UFW. Therefore it will conflict. https://github.com/opscode-cookbooks/firewall/blob/master/libraries/z_provider_mapping.rb#L21-L22.

I'd say there is a new behavior because now even without platformstack in your runlist you will end up with firewall rules (which seems sane as the flag is set to true by default). So I'd add a note in the migration plan saying you should set the flag to false if you don't want this.

[jujugrrr]

All of that is highlighting how difficult it is to contribute to ELKstack without breaking something. It's required though so let's make it a major version.

[martinb3]

I believe the previous behavior was to not run the acl recipe by default:

# if iptables toggle is set, include host based firewall rules
iptables_enabled = node.deep_fetch('elkstack', 'config', 'iptables')
if !iptables_enabled.nil? && iptables_enabled
  include_recipe 'elkstack::acl'
end

I also think the more 'chef' way to do this would be to let people include the recipe or not, vs. an attribute as a flag.

[martinb3]

Ah, nevermind -- default['elkstack']['config']['iptables'] = true. It was on by default :( I think it should be off by default. Or maybe even to omit them entirely. It just doesn't feel like a generic thing that everyone will want (some folks won't even want the dependency).

[jujugrrr]

Ah, nevermind -- default['elkstack']['config']['iptables'] = true. It was on by default :(

So what do we do with this PR :wink:

[martinb3]

@jujugrrr Would you be okay with turning off the ACLs by default, once you do the major bump and remove all the backwards compatible stuff? I'd be good with merging this, if we could plan to do that later on the breaking-change version.