The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #8255

inconsistent handling of octal in file { mode => 'nnnn' }

Added by tgeeky - over 3 years ago. Updated about 3 years ago.

Status:ClosedStart date:07/06/2011
Priority:NormalDue date:
Assignee:tgeeky -% Done:

100%

Category:settingsEstimated time:1.00 hour
Target version:2.7.8
Affected Puppet version:2.7.1 Branch:https://github.com/jhelwig/puppet/tree/tickets/2.7.x/8255-use-string-mode-creating-file-setting-resources
Keywords:octal, permissions, genmanifest, defaults

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

I just noticed this is a repeat of bug [1756]. But for serveral reasons (including the much different version of puppet), I’m posting it separately.

I believe this merits priority High because it represents a major stumbling block for people using puppet for the first time, and a tool which doesn’t properly generate its own configuration (though it claims to in docs) represents a serious slight against the stability of the software in question. However, since this problem may only be restricted to the scope of —genmanifest (I suspect there is more inconsistent handling of octal in puppet), I leave it at Normal for now.

In an out of-of-the-box configuration, the following command will produce incorrect output:

puppet --genmanifest > /etc/puppet/manifests/site.pp

The resulting output contains decimal file modes like this one:

file { '/etc/puppet/ssl/private':
ensure   => 'directory',
backup   => 'false',
links    => 'follow',
loglevel => 'debug',
mode     => '488',
owner    => 'puppet',
}

Trying to run with this autogenerated manifest:

puppet agent --server puppet --verbose --test --debug trace

Fails:

...
err: Could not run Puppet configuration client: Parameter mode failed: File modes can only be octal numbers, not "488"

A one-liner fix was suggested by richardc, and tested by me (and it works, insfoar as the output is parseable now):

diff --git a/lib/puppet/util/settings/file_setting.rb b/lib/puppet/util/settings/file_setting.rb
index 776398e..c765a4b 100644
--- a/lib/puppet/util/settings/file_setting.rb
+++ b/lib/puppet/util/settings/file_setting.rb
@@ -91,7 +91,7 @@ class Puppet::Util::Settings::FileSetting < Puppet::Util::Settings::Setting
resource = Puppet::Resource.new(:file, path)
if Puppet[:manage_internal_file_permissions]
-      resource[:mode] = self.mode if self.mode
+      resource[:mode] = sprintf("%o", self.mode) if self.mode
if Puppet.features.root?
resource[:owner] = self.owner if self.owner

Much more detail is available in this [pastie].


Related issues

Related to Puppet - Bug #8192: puppet breaking setuid bit on group change Accepted 06/30/2011
Related to Puppet - Refactor #7274: notices use 3 digit modes instead of 4 digit modes Closed 04/28/2011

History

#1 Updated by tgeeky - over 3 years ago

  • Category set to installation
  • Status changed from Unreviewed to Ready For Checkin
  • Assignee set to tgeeky -
  • % Done changed from 90 to 100

OK. The original fix as proposed in the above pastie are not the best possible fix. I will use pastie links here, instead of typing the code in, because of syntax highlighting and whatnot.

This bug is really two problems. The first is the series of entries (somewhat poorly formatted) by Luke Caines. He introduced defaults that, in general, looked like this:

:yamldir => {:default => "$vardir/yaml", 
     :owner => "service", 
     :group => "service", 
     :mode  => "750",
     :desc  => "The directory in which YAML data is stored, usually in a subdirectory."},

The problem line is:

:mode => "750"

When using strings for :mode-lines, this is decimal.

The output with —genmanifest is:

file { '/var/lib/puppet/yaml':
    ensure   => 'directory',
    backup   => 'false',
    group    => 'puppet',
    links    => 'follow',
    loglevel => 'debug',
    mode     => '1356',
    owner    => 'puppet',
}

This isn’t a problem that stops puppet, because 1356 does not contain any non-octal digits. This is not the intended mode, obviously.

So, I wrote this [patch] which converts the defaults introduced in [bug#1138] to the format used for the rest of the file. These are bare ruby numbers, all in octal (starting with 0).

[The result (pastie)] is either good or bad, depending on your viewpoint. On the one hand, there are plenty of invalid modelines which were already there. On the other hand, they are consistently incorrect!

On advice from richardc, I tried (instead of implementing the sprintf ‘hack’) making all modelines strings. I even did this twice – once with singlequote strings, once with doublequote:

Happily, these produce correct modelines:

And they are identical:

diff genmanifest-strings.pp genmanifest-doublequotes.pp

< nothing! >

Now, [running these manifests] results in a correct-looking configuration.

Huzzah!

Now for the tounge-in-cheek part. Since we aren’t using a language like Haskell with strong typing, perhaps it would be best to pick a standard (from my investigation I assume only storing modelines as strings will do) – and to warn in other cases – especially because this can get through puppet and appear to work, but fail by setting permissions incorrectly.

#2 Updated by Michael Stahnke over 3 years ago

  • Target version set to 2.7.x

#3 Updated by James Turnbull over 3 years ago

  • Status changed from Ready For Checkin to Requires CLA to be signed

Hi – thanks for working through this. We need to do a couple of things before we can merge the code. First, we need to get you to sign a CLA (see the link at https://projects.puppetlabs.com/contributor_licenses/sign). Then it’d be awesome if you could have a read of our dev process (http://projects.puppetlabs.com/projects/puppet/wiki/Development_Development_Lifecycle) and it’d be great if you could submit your patch as a pull request or to the puppet-dev mailing list so we can give it a review! Thanks again!

#4 Updated by tgeeky - over 3 years ago

James Turnbull wrote:

Hi – thanks for working through this. We need to do a couple of things before we can merge the code. First, we need to get you to sign a CLA (see the link at https://projects.puppetlabs.com/contributor_licenses/sign). Then it’d be awesome if you could have a read of our dev process (http://projects.puppetlabs.com/projects/puppet/wiki/Development_Development_Lifecycle) and it’d be great if you could submit your patch as a pull request or to the puppet-dev mailing list so we can give it a review! Thanks again!

One issue is that my email address on github contains a +github filter, so for now I’ve included it in my puppet profile.

#5 Updated by tgeeky - over 3 years ago

  • Status changed from Requires CLA to be signed to Tests Insufficient

#6 Updated by tgeeky - over 3 years ago

I would like to do some work, generally, on the defaulting process. That work would need to include tests, but this is short enough I feel comfortable not including any beyond the diffs in this ticket.

#7 Updated by tgeeky - over 3 years ago

  • Category changed from installation to settings
  • Status changed from Tests Insufficient to In Topic Branch Pending Review
  • Branch set to https://github.com/puppetlabs/puppet/pull/17

#8 Updated by Jacob Helwig about 3 years ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient

Looking at the changes in the pull request, they seem to be papering over the problem, not actually solving it, since the problem will reoccur once someone forgets that the modes in defaults.rb need to be quoted strings, rather than bare octals. It seems like the best thing to do would be to fix the problem closer to the root, by changing the file type to store modes internally as strings whether they were provided as strings or octal numbers. This would mean that they would always be output correctly by Puppet::Resource#to_manifest. Looking through the revision history, it looks like commit:3e5927773c1dc7bc6e9af922fef09149d1599ef6 was trying to do exactly this.

#9 Updated by Jacob Helwig about 3 years ago

I believe that the better fix is to adjust lib/puppet/util/settings/file_setting.rb to make sure it always passes a string to the resource it creates in #to_resource.

Something like the following (though with tests):

diff --git i/lib/puppet/util/settings/file_setting.rb w/lib/puppet/util/settings/file_setting.rb
index f02a0c5..56c1a3d 100644
--- i/lib/puppet/util/settings/file_setting.rb
+++ w/lib/puppet/util/settings/file_setting.rb
@@ -91,7 +91,12 @@ class Puppet::Util::Settings::FileSetting < Puppet::Util::Settings::Setting
     resource = Puppet::Resource.new(:file, path)

     if Puppet[:manage_internal_file_permissions]
-      resource[:mode] = self.mode if self.mode
+      if self.mode
+        mode = self.mode
+        mode = mode.to_i(8) if mode.is_a?(String)
+        mode = mode.to_s(8)
+        resource[:mode] = mode
+      end

       # REMIND fails on Windows because chown/chgrp functionality not supported yet
       if Puppet.features.root? and !Puppet.features.microsoft_windows?

#10 Updated by Jacob Helwig about 3 years ago

  • Status changed from Code Insufficient to In Topic Branch Pending Review
  • Branch changed from https://github.com/puppetlabs/puppet/pull/17 to https://github.com/jhelwig/puppet/tree/tickets/2.7.x/8255-use-string-mode-creating-file-setting-resources

I’ve opened pull request 196 which modifies the #to_resource of Puppet::Util::Settings::FileSetting to munge values to always be strings.

(#8255) Always use string modes when creating resources from FileSetting settings

Since we're setting the 'is', rather than the 'should' on the
resource, the property's munge method is not automatically called for
us and we need to make sure we're always passing the stringified
version of the mode to the resource.

By doing this, we fix the problem where 'puppet --genmanifest' would
output the (base 10) integer version of the mode in the generated
manifest for settings where the mode was not specified as a string.

It would have been nice to re-use the munge from the mode property
directly, but there doesn't appear to be a good/clean way to do this
(especially without reaching into the internals of other objects).

Another alternative would have been to modify []= to call munge for
us, but this isn't really a change to be made in a point release,
especially not without very careful thought about the implications of
such a change.

#11 Updated by Josh Cooper about 3 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

This was merged in 2.7.x in commit https://github.com/puppetlabs/puppet/commit/aef93cee8ae2133f9b2c571b23c490ef3405e3fe

Note at the time the original bug report was filed, modes expressed as strings were not correctly converted. But between that time and now, that part was fixed in a different commit. For example, here a directory with a string mode is correctly converted:

:client_datadir => {
  :default => "$vardir/client_data", 
  :mode => "750", 
  :desc => "The directory in which serialized data is stored on the client."},

And the result of genmanifest:

file { '/Users/josh/.puppet/var/client_data':
  ensure   => 'directory',
  backup   => 'false',
  links    => 'follow',
  loglevel => 'debug',
  mode     => '750',
}

The part that was broken and is now fixed by this commit is the case where modes are expressed as fixnum’s:

:lastrunreport =>  { 
  :default => "$statedir/last_run_report.yaml",
  :mode => 0660,
  :desc => "Where puppet agent stores the last run report in yaml format."
},

Which genmanifest incorrectly converted to:

file { '/Users/josh/.puppet/var/state/last_run_report.yaml':
  ensure   => 'file',
  backup   => 'false',
  links    => 'follow',
  loglevel => 'debug',
  mode     => '432',
}

This commit fixes the fixnum case (mode => 0660) and doesn’t break the string cases (mode => ‘750’ and mode => ‘0750’)

#12 Updated by Matthaus Owens about 3 years ago

  • Status changed from Merged - Pending Release to Closed
  • Target version changed from 2.7.x to 2.7.8

Released in 2.7.8rc1

Also available in: Atom PDF