-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add a convenience wrapper for restorecon execs #205
Conversation
df2070e
to
51c5d62
Compare
} | ||
|
||
exec {"selinux::exec_restorecon ${path}": | ||
path => '/sbin:/usr/sbin', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everytime i see path set on exec in forge modules i start thinking what is right: setting it here or leave it up to the user to set global defaults?
but this will work on all our supported platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be fine. If there's a need to fix it, that can be done without breaking things.
I like specifying paths explicitly so that the exec is not vulnerable to the user's environment being somehow compromised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, all resources inside receive the notify.
just wanted to know if the explicit notify was inteded. :) this is fine with me.
exec {"selinux::exec_restorecon ${path}": | ||
path => '/sbin:/usr/sbin', | ||
command => sprintf('%s %s', $command, shellquote($path)), | ||
refreshonly => $refreshonly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's refershonly ... how does it receive notifys?
Is it tought a user defines explicit notifies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you notify the define, I think all resources inside the define receive a notify too. That will work in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yeah, this is not integrated with the fcontext define yet. That's trickier to do, so for now it's just a convenience wrapper for the user's own manifests.
We can later extend the fcontext define to include an optional automatic restorecon exec
@@ -0,0 +1,45 @@ | |||
require 'spec_helper' | |||
|
|||
describe 'selinux::exec_restorecon' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3 tests. :) 👍
} | ||
|
||
exec {"selinux::exec_restorecon ${path}": | ||
path => '/sbin:/usr/sbin', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, all resources inside receive the notify.
just wanted to know if the explicit notify was inteded. :) this is fine with me.
manifests/exec_restorecon.pp
Outdated
Boolean $refreshonly = true, | ||
Boolean $recurse = true, | ||
Optional[String] $unless = undef, | ||
Optional[String] $onlyif = undef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you reformat this to allign the =?
The vim plugin junegunn/vim-easy-align can easily do that for you.
This wraps restorecon execution so that it will execute after all other SELinux changes have been applied
51c5d62
to
ce95b59
Compare
Given that it's a pretty useful convenience tool in many cases (we use a similar custom define ourselves), I'll just deem this reviewed well enough. |
Add a convenience wrapper for restorecon execs
Add a convenience wrapper for restorecon execs
This wraps a restorecon exec so that it will execute after all other SELinux changes have been applied
Not yet very thorougly tested (or integrated with the fcontext define) but it should be useful enough standalone. Any comments?