MODULES-10548: make files readonly #906
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #906 +/- ##
===========================================
- Coverage 73.07% 49.73% -23.34%
===========================================
Files 5 5
Lines 260 382 +122
===========================================
Hits 190 190
- Misses 70 192 +122
Continue to review full report at Codecov.
|
|
Thanks, LGTM. |
Files created by the apt module are mode writable by the owner. Because those files are managed by Puppet, they should really not be writable by anyone, even root. While root can bypass those warnings, having files readonly does provide an immediate and reliable indication that a file should not be edited on site, on top of the usual top of file warnings. This also fixes a problem with sources.list.d being non-executable, which Puppet seems to ignore, but seems better to keep consistent.
|
👍 LGTM, as there are no changes in Ruby code, the codecov should not have any impact in this PR |
raoulbhatia
added a commit
to raoulbhatia/puppet-unattended_upgrades
that referenced
this pull request
Mar 1, 2020
anarcat
added a commit
to anarcat/puppetlabs-apt
that referenced
this pull request
Mar 11, 2020
MODULES-10583 makes a good point: "why are you messing with my file permissions"? In my case, the entire reason I made the following change (in PR puppetlabs#906): ab2e06b MODULES-10548: make files readonly ... is exactly *because* Puppet was changing the file modes from under me. I was migrating from our own in-house APT module to the forge one, and our module did *not* intervene in those file modes: it left the file resources alone. Which means we could have a directive like this: File { owner => root, group => root, mode => '444', ensure => file, } ... which made all files readonly by default. So when I migrated to the Puppetlabs APT module, modes were changed to be writable, which I did not want. As I reasoned in MODULES-10548, having files readonly provides an excellent indicator that a file is managed by Puppet, even if some module does not add a warning header - either because it forgot or because it's impossible. But I also understand if people do not like that policy. I think the proper way of doing this is not specifying a mode at all, and let local site-specific policies apply. I specifically proppose this as an alternative to puppetlabs#921 because I believe adding more parameters to the resources will needlessly complicate the script, when we have a native, Puppet-DSL supported way of changing those modes according to the right scope and context. In a similar way, we might want to reconsider user and group ownership of the files, but that can be done in a later time. This reverts commit 316fd8f. Signed-off-by: Antoine Beaupré <anarcat@debian.org>
anarcat
added a commit
to anarcat/puppetlabs-apt
that referenced
this pull request
Mar 11, 2020
MODULES-10583 makes a good point: "why are you messing with my file permissions"? In my case, the entire reason I made the following change (in PR puppetlabs#906): ab2e06b MODULES-10548: make files readonly ... is exactly *because* Puppet was changing the file modes from under me. I was migrating from our own in-house APT module to the forge one, and our module did *not* intervene in those file modes: it left the file resources alone. Which means we could have a directive like this: File { owner => root, group => root, mode => '444', ensure => file, } ... which made all files readonly by default. So when I migrated to the Puppetlabs APT module, modes were changed to be writable, which I did not want. As I reasoned in MODULES-10548, having files readonly provides an excellent indicator that a file is managed by Puppet, even if some module does not add a warning header - either because it forgot or because it's impossible. But I also understand if people do not like that policy. I think the proper way of doing this is not specifying a mode at all, and let local site-specific policies apply. I specifically proppose this as an alternative to puppetlabs#921 because I believe adding more parameters to the resources will needlessly complicate the script, when we have a native, Puppet-DSL supported way of changing those modes according to the right scope and context. In a similar way, we might want to reconsider user and group ownership of the files, but that can be done in a later time. This reverts commit 316fd8f. Signed-off-by: Antoine Beaupré <anarcat@debian.org>
anarcat
added a commit
to anarcat/puppetlabs-apt
that referenced
this pull request
Mar 11, 2020
MODULES-10583 makes a good point: "why are you messing with my file permissions"? In my case, the entire reason I made the following change (in PR puppetlabs#906): ab2e06b MODULES-10548: make files readonly ... is exactly *because* Puppet was changing the file modes from under me. I was migrating from our own in-house APT module to the forge one, and our module did *not* intervene in those file modes: it left the file resources alone. Which means we could have a directive like this: File { owner => root, group => root, mode => '444', ensure => file, } ... which made all files readonly by default. So when I migrated to the Puppetlabs APT module, modes were changed to be writable, which I did not want. As I reasoned in MODULES-10548, having files readonly provides an excellent indicator that a file is managed by Puppet, even if some module does not add a warning header - either because it forgot or because it's impossible. But I also understand if people do not like that policy. I think the proper way of doing this is not specifying a mode at all, and let local site-specific policies apply. I specifically proppose this as an alternative to puppetlabs#921 because I believe adding more parameters to the resources will needlessly complicate the script, when we have a native, Puppet-DSL supported way of changing those modes according to the right scope and context. In a similar way, we might want to reconsider user and group ownership of the files, but that can be done in a later time. This reverts commit 316fd8f. Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Files created by the apt module are mode writable by the
owner. Because those files are managed by Puppet, they should really
not be writable by anyone, even root. While root can bypass those
warnings, having files readonly does provide an immediate and reliable
indication that a file should not be edited on site, on top of the
usual top of file warnings.
This also fixes a problem with sources.list.d being non-executable,
which Puppet seems to ignore, but seems better to keep consistent.