enhanced default configuration

[sebastianschauenburg]

I noticed /etc/ being chmod'ed when the $keys_file is located at "/etc/ntp.keys". The original configuration causes permissions in /etc/ to be recursively changed. I've mitigated this by checking if there are at least 2 directories above the $keys_file (the $keys_file should at least be in /etc/ or /usr/local/ if it's custom compiled). Furthermore changed the default permissions from 0755 to 0640, since the execute bit automatically set by puppet on directories. Usually the service group of this daemon is named 'ntp', as such changed the group. Is this a welcome contribution right now?

[hunner]

For the recursive permissions thing, #212 seems to fix that.

I'm not sure why the module is even managing the parent directory, but not the keys_file itself. Original commit 0235486

[hunner]

(Closed by accident)

What if we just don't manage the parent directory? I can't figure out why it was originally done that way.

[sebastianschauenburg]

The parent directory should be managed, but it indeed should be managed separately (example with an optional $config_dir variable) if you want to do it nicely (IMHO). Then you can include sane defaults.

It's also possible to not include the parent directory at all, but then (if ntp has its own configuration directory) an administrator needs to 'notice' that the configuration directory is not managed. I personally would assume it would be managed with sane defaults.

[hunner]

Yeah, to directly manage $config_dir would be a better option. I mean, this bug should have blown up AIX even, as it's keys file is /etc/ntp.keys by default!

Would you like to update this PR to make it more "confdir" centric instead of "keys file" centric?

[sebastianschauenburg]

Agreed, I'll update it ASAP

[tphoney]

Thanks for the effort, This needs rebased.

[sebastianschauenburg]

@tphoney thanks for the pointer 👍
@hunner please check if my modifications are correct (I also had to modify the spec file)

[hunner]

Indented too far

[sebastianschauenburg]

Agreed

[tphoney]

@hunner ping, did @Swatx response satisfy you ?

[DavidS]

This can still be easily merged, @hunner?

[DavidS]

Cherry-picking this commit on top of master is still quite easy, if someone want's to resurrect this change.

Then it needs a entry for $config_dir in the README and actual unit-tests, instead of removing the existing ones.

Meanwhile, I added a check to avoid managing '/' or '/etc' in #292 to fix the immediate problem.

[igalic]

👍 do it so, number 1.