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

Guard against owning /etc #254

Merged
merged 1 commit into from
Dec 11, 2015
Merged

Guard against owning /etc #254

merged 1 commit into from
Dec 11, 2015

Conversation

bdclark
Copy link
Contributor

@bdclark bdclark commented Dec 11, 2015

The consul_config resource ensures the directory exists for the config file and is owned by the service user. Unfortunately since the default path for the config file is /etc/config.json, this cookbook is changing ownership of /etc to the Consul user, which is causing problems on some of my nodes.

This PR simply guards the directory resource from managing the directory if it is /etc. This may not be the best approach, but it was the simplest way to solve the problem. Please let me know if there's a better way and I'll submit another PR. Thanks!

@codecov-io
Copy link

Current coverage is 55.62%

Merging #254 into master will decrease coverage by -16.94% as of 62aa280

@@            master    #254   diff @@
======================================
  Files            6       6       
  Stmts          328     329     +1
  Branches         0       0       
  Methods          0       0       
======================================
- Hit            238     183    -55
  Partial          0       0       
- Missed          90     146    +56

Review entire Coverage Diff as of 62aa280

Powered by Codecov. Updated on successful CI builds.

@Ginja
Copy link
Contributor

Ginja commented Dec 11, 2015

@johnbellone, any reason to not change the default values for node['consul']['config']['path'] & node['consul']['config']['data_dir'] to:

node.default['consul']['config']['path'] = '/etc/consul/consul.json'
node.default['consul']['config']['data_dir'] = '/etc/consul/data'

?

Although it would still be good to guard against common locations.

@johnbellone
Copy link
Contributor

I don't have any complaints here.

johnbellone added a commit that referenced this pull request Dec 11, 2015
Guard against owning /etc
@johnbellone johnbellone merged commit f316bee into sous-chefs:master Dec 11, 2015
@johnbellone
Copy link
Contributor

@Ginja do you want to put a separate PR in?

@bdclark
Copy link
Contributor Author

bdclark commented Dec 12, 2015

@Ginja would that be changing the defaults for node.default['consul']['config']['data_dir'] or node.default['consul']['config']['config_dir']? Not sure if data_dir is relevant to the issue, but maybe I'm mistaken.

@Ginja
Copy link
Contributor

Ginja commented Dec 12, 2015

I'd say both as I like to keep everything Consul in one location, but if anyone has any objections to that the default location for node.default['consul']['config']['data_dir'] (i.e. /var/lib/consul) is fine.

@bdclark
Copy link
Contributor Author

bdclark commented Dec 12, 2015

Before taking the easy way out and guarding /etc I was going to suggest...

node.default['consul']['config']['path'] = '/etc/consul/consul.json'
node.default['consul']['config']['config_dir'] = '/etc/consul/conf.d'

Leaving all things configuration in /etc/consul/<foo> and leaving data stuff in /var/lib/consul, however at the end of the day I'm with you and am fine with either.

@Ginja Ginja mentioned this pull request Jan 4, 2016
@johnbellone
Copy link
Contributor

Now that I am reading this again I am a little concerned - the /etc/consul/data directory shouldn't be the default.

@bdclark bdclark deleted the not_etc branch January 19, 2016 22:58
@lock
Copy link

lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants