'requires' dependency in file-fragments pattern was not honored
Jon Jaroker
the file-fragments pattern below. I am assembling the sudo file using
fragments, with a "validate check file" step that confirms if the
assembled file is valid. The '/etc/sudoers' file should only be
overwritten if the check file is valid.
I accidentally created an invalid sudoers file, which failed the
validation check. Puppet still copied this invalid file to /etc/
sudoers. The dependency Exec["Validate_Check_File"] ->
File["Make_Sudo_File_Live"] was not honored and I am trying to
understand the reason.
The class and puppet output are below. I am using puppet 2.6.9.
Thank you,
Jon
=========
class s_sudo (
$wheel_req_password = true
) inherits s_sudo::params {
### Assemble the sudo check file from fragments
file { "Sudo_Fragment_Directory":
path => "${s_sudo::params::sudo_fragment_directory}",
ensure => directory,
purge => true,
recurse => true,
}
file {"Sudo_Header":
path => "${s_sudo::params::sudo_header_file}",
content => template("s_sudo/00-sudobase.erb"),
notify => Exec["Assemble_Sudo_Fragments"];
}
exec { "Assemble_Sudo_Fragments":
command => "/bin/cat $
{s_sudo::params::sudo_fragment_directory}/* > $
{s_sudo::params::sudo_check_file}",
refreshonly => true,
subscribe => File["Sudo_Fragment_Directory"],
}
file { "Sudo_Check_File": # Secure the check file
path => "${s_sudo::params::sudo_check_file}",
mode => 644,
require => Exec["Assemble_Sudo_Fragments"];
}
### Validate sudo file before making live
Exec["Assemble_Sudo_Fragments"] ~> Exec["Validate_Check_File"] ->
File["Make_Sudo_File_Live"]
exec {"Validate_Check_File":
command => "visudo -cf $
{s_sudo::params::sudo_check_file}",
refreshonly => true,
}
file {"Make_Sudo_File_Live":
path => "/etc/sudoers",
source => "${s_sudo::params::sudo_check_file}",
mode => 440,
owner => root,
group => root,
}
}
======================
notice: /Stage[main]/S_sudo/File[Sudo_Fragment_Directory]/ensure:
created
info: /Stage[main]/S_sudo/File[Sudo_Fragment_Directory]: Scheduling
refresh of Exec[Assemble_Sudo_Fragments]
notice: /Stage[main]/S_sudo/File[Sudo_Header]/ensure: defined content
as '{md5}42b4c36c629f3a9c451d3dc783a851cb'
info: /Stage[main]/S_sudo/File[Sudo_Header]: Scheduling refresh of
Exec[Assemble_Sudo_Fragments]
notice: /Stage[main]/S_sudo/Exec[Assemble_Sudo_Fragments]: Triggered
'refresh' from 2 events
info: /Stage[main]/S_sudo/Exec[Assemble_Sudo_Fragments]: Scheduling
refresh of Exec[Validate_Check_File]
>>>>> ERROR >>>> err: /Stage[main]/S_sudo/Exec[Validate_Check_File]: Failed to call refresh: visudo -cf /tmp/sudo.check returned 1 instead of one of [0] at /etc/puppet/modules/environments/dev/s_sudo/manifests/init.pp:52
info: FileBucket adding {md5}f298d1064df9009a1603d76ed90ed90f
info: /Stage[main]/S_sudo/File[Make_Sudo_File_Live]: Filebucketed /etc/
sudoers to puppet with sum f298d1064df9009a1603d76ed90ed90f
notice: /Stage[main]/S_sudo/File[Make_Sudo_File_Live]/content: content
changed '{md5}f298d1064df9009a1603d76ed90ed90f' to '{md5}
42b4c36c629f3a9c451d3dc783a851cb'
Al @ Lab42
I personally don't use (and don't see much value added, but I'm open to alternative opinions) the "new"
Exec["Assemble_Sudo_Fragments"] ~> Exec["Validate_Check_File"] -> File["Make_Sudo_File_Live"]
I would rather try adding the require argument here:
file {"Make_Sudo_File_Live":
path => "/etc/sudoers",
source => "${s_sudo::params::sudo_check_file}",
mode => 440,
owner => root,
group => root,
}
Alessandro
Jon Jaroker
I relocated the dependency declaration into the native types and also
removed two stray semi-colons. After reviewing the dot graph, I also
made "Secure Check File" a dependency of "Validate Check File". The
updated class is below.
Puppet still does not honor the "Validate Check File" -> "Make Sudo
File Live" dependency, even with these changes.
The sudo check failed because my sudo template was missing an EOL
character. I am able to prevent the failure by appending '<% %>' to
the bottom of the template.
Thank you for checking the class. I think it is correct and this
dependency problem is a bug.
Regards,
Jon
==================
class s_sudo (
$wheel_req_password = true
) inherits s_sudo::params {
### Assemble the sudo check file from fragments
file { "Sudo_Fragment_Directory":
path => "${s_sudo::params::sudo_fragment_directory}",
ensure => directory,
purge => true,
recurse => true,
}
file {"Sudo_Header":
path => "${s_sudo::params::sudo_header_file}",
content => template("s_sudo/00-sudobase.erb"),
exec { "Assemble_Sudo_Fragments":
command => "/bin/cat $
{s_sudo::params::sudo_check_file}",
subscribe => File["Sudo_Fragment_Directory"],
}
file { 'Secure_Check_File': # Secure the check file
mode => 644,
}
exec {"Validate_Check_File":
command => "visudo -cf $
{s_sudo::params::sudo_check_file}",
refreshonly => true,
require => File['Secure_Check_File'],
> }
>
> Alessandro
jcbollinger
On Jul 21, 7:25 am, Jon Jaroker <goo...@jaroker.com> wrote:
> Hello Alessandro,
>
> I relocated the dependency declaration into the native types and also
> removed two stray semi-colons. After reviewing the dot graph, I also
> made "Secure Check File" a dependency of "Validate Check File". The
> updated class is below.
>
> Puppet still does not honor the "Validate Check File" -> "Make Sudo
> File Live" dependency, even with these changes.
>
> The sudo check failed because my sudo template was missing an EOL
> character. I am able to prevent the failure by appending '<% %>' to
> the bottom of the template.
>
> Thank you for checking the class. I think it is correct and this
> dependency problem is a bug.
appreciate a bug report. Before submitting one, however, it would be
worthwhile to check whether the catalog the node applies (incorrectly)
is indeed the result of compiling the manifest you show. There are
several ways in which the node might end up trying to apply a
different catalog, whether an old, cached one or one built from a
different version of the manifest. Just insert a Notice resource into
the class, and make sure it is reflected in the node's log.
It might also be amusing to test whether the problem remains if you
change Exec['Validate_Check_File'] to have refreshonly => false. I
don't see much else in your class that seems a promising explanation
for why a feature that generally works reliably to fail for you.
Don't mistake me: I think it *should* work as you expect with
refreshonly => true; I'm just saying that there's a little dark corner
there where a bug might hide.
Good luck,
John
vagn scott
> Hello, I was wondering if anyone can spot the mistake I am making in
> the file-fragments pattern below.
>
fragments directory, including the head fragment.
Use naming conventions to establish order.
Then the exec that validates should use a command like
cat $dir/* > $check && visudoers -cf $check && cat $check >
/etc/sudoers
get it all done in a one-liner that early-outs on error. $check is just
file { $check: ensure => file, mode => 600, owner => root, }
to make sure it is there with the right properties.
For what it's worth, here is a simple sudo class.
It works on distros that provide the /etc/sudoers.d directory.
Tested on debian squeeze.
--vagn
define sudo::sudoer() {
$username = "$name"
include sudo
file { "/etc/sudoers.d/$username":
content => "$username ALL=(ALL) ALL\n",
mode => 440, owner => root, group => root,
require => Package[ "sudo" ],
}
}
define sudo::nopasswd() {
$username = "$name"
include sudo
file { "/etc/sudoers.d/$username":
content => "$username ALL=NOPASSWD: ALL\n",
mode => 440, owner => root, group => root,
require => Package[ "sudo" ],
}
}
class sudo() {
package { "sudo":
ensure => installed,
}
file { "/usr/bin/sus":
content => "if [ $# -eq 0 ] ; then exec sudo su - ;
else exec sudo \"$@\" ; fi",
mode => 775, owner => root, group => root,
require => Package[ "sudo" ],
}
}
jcbollinger
On Jul 21, 11:38 pm, vagn scott <vagnsc...@gmail.com> wrote:
> On 07/20/2011 09:37 PM, Jon Jaroker wrote:> Hello, I was wondering if anyone can spot the mistake I am making in
> > the file-fragments pattern below.
>
> You probably want to drop ALL your fragments into the
> fragments directory, including the head fragment.
supposed that he omitted all that so as to provide a smaller failure
case.
John
vagn scott
> Well yes, but that's not relevant to the OP's problem. I had in fact
> supposed that he omitted all that so as to provide a smaller failure
> case.
The OP's problem is that he is not including the header
fragment in "Assemble_Sudo_Fragments". It is easy to miss
because
1. the code is noisy, he should get rid of those long interpolations
in the resources
2. he is handling the header fragment outside of the fragment directory,
complicating the design.
I didn't spot the logic error until I rewrote the thing:
class s_sudo ( $wheel_req_password = true)
inherits s_sudo::params
{
$dir = "${s_sudo::params::sudo_fragment_directory}"
$hdr = "${s_sudo::params::sudo_header_file}"
$hdr_tt = "s_sudo/00-sudobase.erb"
$check = "${s_sudo::params::sudo_check_file}"
file {"Sudoer_File":
path => "/etc/sudoers",
ensure => file,
mode => 440,
owner => root,
group => root,
}
file { "Sudo_Fragment_Directory":
path => "${dir}",
ensure => directory,
purge => true,
recurse => true,
}
file { "Sudo_Check_File":
path => "${check}",
ensure => file,
mode => 644,
}
file {"Sudo_Header":
path => "${hdr}",
content => template($hdr_tt),
}
exec { "Assemble_Sudo_Fragments":
command => "/bin/cat ${hdr} ${dir}/* > ${check}",
# <=== error was here
refreshonly => true,
subscribe => File[
"Sudoer_File",
"Sudo_Fragment_Directory",
"Sudo_Check_File",
"Sudo_Header",
],
notify => Exec["Check_And_Instantiate"],
}
exec {"Check_And_Instantiate":
command => "visudo -cf ${check} && cat ${check} >
/etc/sudoers",
refreshonly => true,
}
}
jcbollinger
On Jul 22, 8:45 am, vagn scott <vagnsc...@gmail.com> wrote:
> On 07/22/2011 09:08 AM, jcbollinger wrote:
>
> > Well yes, but that's not relevant to the OP's problem. I had in fact
> > supposed that he omitted all that so as to provide a smaller failure
> > case.
>
> The OP's problem is that he is not including the header
> fragment in "Assemble_Sudo_Fragments". It is easy to miss
> because
>
> 1. the code is noisy, he should get rid of those long interpolations
> in the resources
> 2. he is handling the header fragment outside of the fragment directory,
> complicating the design.
>
> I didn't spot the logic error until I rewrote the thing:
the Puppet behavior that he asked about, which is:
1. File["Make_Sudo_File_Live"] formally requires
Exec["Validate_Check_File"]
2. Application of Exec["Validate_Check_File"] fails on the client
3. Puppet applies File["Make_Sudo_File_Live"] anyway.
Puppet should not, and typically doesn't, apply resources that require
a failed resource. The content of the file managed by
File["Make_Sudo_File_Live"] is not directly relevant.
John
vagn scott
> Puppet should not, and typically doesn't, apply resources that require
> a failed resource. The content of the file managed by
> File["Make_Sudo_File_Live"] is not directly relevant.
>
I wonder what would happen if he spelled his
dependency chain like this:
### Validate sudo file before making live
Exec["Assemble_Sudo_Fragments"] ~> Exec["Validate_Check_File"] ~>
File["Make_Sudo_File_Live"]
Note a string of ~>, rather than a mixed string.
Is ~> -> even sensible? Shoudn't puppet turn that
2nd dependency from -> into ~>?
--
vagn
jcbollinger
On Jul 25, 9:47 am, vagn scott <vagnsc...@gmail.com> wrote:
> On 07/25/2011 08:47 AM, jcbollinger wrote:
>
> > Puppet should not, and typically doesn't, apply resources that require
> > a failed resource. The content of the file managed by
> > File["Make_Sudo_File_Live"] is not directly relevant.
>
> I wonder what would happen if he spelled his
> dependency chain like this:
>
> ### Validate sudo file before making live
> Exec["Assemble_Sudo_Fragments"] ~> Exec["Validate_Check_File"] ~>
> File["Make_Sudo_File_Live"]
and in that case it might indeed be triggered by the mixing of
relationship operators.
> Note a string of ~>, rather than a mixed string.
> Is ~> -> even sensible? Shoudn't puppet turn that
> 2nd dependency from -> into ~>?
operators are OK (it gives an example mixing "->" and "<-"), and that
chains are interpreted pairwise. Thus the OP's chain should be
equivalent to two separate statements:
Exec["Assemble_Sudo_Fragments"] ~> Exec["Validate_Check_File"]
version is potentially confusing to humans. Even though the correct
interpretation is documented, however, it is possible that Puppet is
buggy here.
My money is still on the problem relating to the Exec being refresh-
only, triggered by something about the particular pattern of
relationships involved. But we may never know, as it seems the OP may
have bowed out.
John