Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

(#15841,#13542) PMT tarball install & embed Minitar gem #1508

Closed
wants to merge 3 commits into from

9 participants

@mruzicka
Owner

This pull request should really be two separate ones but because of it its
gradual changes it ended up as a single large one. It introduces
two principal changes:

  • Adds support to PMT for installing/upgrading puppet modules
    from local tarballs. This support depends on a new forge API
    introduced in:
    puppetlabs/puppet-forge-api#32
    In addition to the posibility of installing modules from tarballs a new
    algorithm for module dependency resolution is also introduced.
    The algorithm is capable of backtracking so is able to find resolutions
    even in situations in which the original algorithm would have failed.
    Many of the data structures used by the depedecy resolution code have
    been changed but care has been taken to preserve backward compatiblity
    of any externally visible data structures.
    Also the installer and upgrader applications have been greatly unified,
    so they now share the majority of their code.

  • Embeds the minitar gem into Puppet while changing its namespace to:
    Puppet::Util::Archive::Tar::Minitar
    and makes use of it at various places in the PMT code instead of
    calling external tar commands.

The two additional commits in this pull request update and add acceptance
& unit tests to keep up with the code changes, they are deliberately kept
separate to make the review of this pull request somewhat easier.

@domcleal
Collaborator

The new rubygems dependency is probably OK I think, but it will have packaging implications - the spec file will need updating too for instance. Today Puppet will function with or without rubygems (see Puppet::Util::Rubygems for instance).

@domcleal domcleal commented on the diff
lib/puppet/module_tool/applications/installer.rb
((96 lines not shown))
results[:result] = :success
- results[:installed_modules] = @graph
@domcleal Collaborator

This will be an incompatible change in the JSON/YAML output of these commands, so should probably be left in. I haven't delved into @graph, is it unchanged?

@mruzicka Owner

Could we perhaps, include both? Like so:
results[:installed_modules] = results[:affected_modules] = @graph
the first for backwards compatibility and the second as its name better describes the content of the graph and to unify it with what the upgrade does

@mruzicka Owner

The basic structure of the graph is unchanged, (the method to print it dind't need any adjustments for instance) but there are changes. The same (or more) information is present in each graph node, the structure of the nodes is different though.

@domcleal Collaborator

@mruzicka yes, I think including both would be ideal, thanks. If you're feeling keen, it could be listed for removal in 4.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@domcleal domcleal commented on the diff
lib/puppet/module_tool/applications/upgrader.rb
((124 lines not shown))
end
end
-
- results[:result] = :success
- results[:base_dir] = @graph.first[:path]
@domcleal Collaborator

I like that install_dir here is now consistent with the install action, but would suggest leaving base_dir in place too for compatibility.

@mruzicka Owner

again how about setting both?

@domcleal Collaborator

ditto, that'd be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/puppet/forge.rb
@@ -89,6 +89,21 @@ def remote_dependency_info(author, mod_name, version)
end
end
+ def multiple_remote_dependency_info(modules)
@domcleal Collaborator

Some YARD on the expected response would be useful here, looks like it's the same as remote_dependency_info but combining results for multiple modules?

@mruzicka Owner

yes, it is the same as in the simple query scenario, I'll add the doc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@domcleal domcleal commented on the diff
lib/puppet/forge.rb
@@ -89,6 +89,21 @@ def remote_dependency_info(author, mod_name, version)
end
end
+ def multiple_remote_dependency_info(modules)
+ query_string = ''
+ modules.each do |mod|
+ query_string << "&module[]=#{mod.first}&version[]=#{mod.last}"
+ end
+ response = repository.make_http_request('/api/v1/releases.json?' << query_string[1..-1])
@domcleal Collaborator

@mhrivnak FYI, it looks like support for this may need adding to Pulp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/puppet/forge/errors.rb
@@ -14,6 +14,18 @@ def multiline
end
end
+ # This class just adds the possibility to specify the multiline message
+ class VerboseForgeError < ForgeError
+ def initialize(message, mutliline_message, original=nil)
+ super(message, original)
+ @mutliline_message = mutliline_message
@adrienthebo Owner

Typo, mutliline vs multiline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adrienthebo
Owner

This fails pretty hard on Centos 5.8:

[root@localhost ~]# puppet module install branan/eight_hundred
/usr/lib/ruby/site_ruby/1.8/puppet/module_tool/applications/builder.rb:1:in `require': no such file to load -- rubygems (LoadError)
    from /usr/lib/ruby/site_ruby/1.8/puppet/module_tool/applications/builder.rb:1
    from /usr/lib/ruby/site_ruby/1.8/puppet/module_tool/applications.rb:6:in `require'
    from /usr/lib/ruby/site_ruby/1.8/puppet/module_tool/applications.rb:6
    from /usr/lib/ruby/site_ruby/1.8/puppet/module.rb:3:in `require'
    from /usr/lib/ruby/site_ruby/1.8/puppet/module.rb:3
    from /usr/lib/ruby/site_ruby/1.8/puppet/parser/files.rb:1:in `require'
    from /usr/lib/ruby/site_ruby/1.8/puppet/parser/files.rb:1
    from /usr/lib/ruby/site_ruby/1.8/puppet/parser/templatewrapper.rb:1:in `require'
     ... 13 levels...
    from /usr/lib/ruby/site_ruby/1.8/puppet/util/command_line.rb:12:in `require'
    from /usr/lib/ruby/site_ruby/1.8/puppet/util/command_line.rb:12
    from /usr/bin/puppet:3:in `require'
    from /usr/bin/puppet:3
[root@localhost ~]# cat /etc/redhat-release 
CentOS release 5.8 (Final)
[root@localhost ~]# ruby --version
ruby 1.8.5 (2006-08-25) [i386-linux]
[root@localhost ~]# which gem
/usr/bin/which: no gem in (/usr/kerberos/sbin:/usr/local/sbin:/usr/sbin:/sbin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/home/vagrant/bin:/root/bin)
@adrienthebo adrienthebo commented on the diff
acceptance/tests/modules/install/already_installed.rb
@@ -56,10 +56,10 @@
on master, puppet("module install pmtacceptance-nginx --force") do
assert_output <<-OUTPUT
\e[mNotice: Preparing to install into /etc/puppet/modules ...\e[0m
- \e[mNotice: Downloading from https://forge.puppetlabs.com ...\e[0m
@adrienthebo Owner

This line is duplicated a ton of times across these tests; does it make sense to extract this information to some sort of shared fixture data?

@mruzicka Owner
mruzicka added a note

The tests would need to manage the puppet.conf file (or pass the --module_repository option to all commands) to control which repository is used. We could stop stubbing the forge.puppetlabs.com at that point too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adrienthebo
Owner

It looks like Github ate my comment on the commit, but 753d7fb has a lot of changes (866 additions, 353 deletions) but doesn't have any unit tests for new behavior.

In addition the travis-ci build has 24 failed tests, as demonstrated in (https://gist.github.com/adrienthebo/5061785).

@mruzicka
Owner

The code has a hard dependency on the rubygems tar code. So how do I ensure the rubygems library is loaded in the bundle environment? Is there a way to have bundler load it in a way that doesn't interfere with it?
Note that it would be possible to use the minitar implementation instead but again it would be a hard dependency, i.e. the code would not run without it.

I completely forgot about the unit test, I'll update them and add new ones.

@mruzicka
Owner

There is another alternative to using minitar or rubygems tar code and that is including the Tar handling code directly in puppet - at least the rubygems tar code is very simple only a handful of classes really.

@joshcooper
Owner

@mruzicka I recently discovered rubygems is not present on rhel6 that we use in acceptance testing, i.e. require 'rubygems' will fail. Also using minitar is not a hard-dependency provided you wrap access to it by first checking whether the Puppet.features.minitar? is available or not, then fall back to shelling out to tar/gtar.

@jeffmccune
Owner

There is another alternative to using minitar or rubygems tar code and that is including the Tar handling code directly in puppet - at least the rubygems tar code is very simple only a handful of classes really.

I think this would work well, provided the classes are properly namespaced to avoid collisions and monkey patching and the classes work on all of our platforms including windows.

@mruzicka Would you mind amending this change set and re-pushing?

-Jeff

@haus
Owner

@mruzicka if going down the road of including the tar code in a utility namespace of puppet, I would want to know the source of the tar code and other things like how often does upstream get updated (do they see a lot of security events), and if our plan is to fork it or to keep it up to date with upstream.

@mruzicka
Owner

@haus the code we are talking about can be found here: https://github.com/rubygems/rubygems/tree/master/lib/rubygems/package
It hasn't received any significant updates in 2 years, IMO such a frequency of updates will probably be the norm for the code, given the tar format itself is fairly stable.
My thinking is that we should just copy the files we need:
tar_reader/entry.rb
tar_header.rb
tar_reader.rb
tar_writer.rb
and possibly:
tar_test_case.rb
and adjust them to suit our needs (change the namespace, drop requires on the rest of the rubygems code, add support for symliks), which would probably make them difficult to update. I.e. I don't see us updating them from the upstream.

@mruzicka
Owner

@jeffmccune I'm ready to start looking into that, which namespace should I use for the tar code?

@joshcooper
Owner

@mruzicka I think you'll find the minitar code would be easier to vendor, especially as it already provides library like methods for extracting, e.g. you call a method and pass it a block that gets called once for each extracted file. The tar code in rubygems was a bit more entangled with rubygems specific implementation.

@mruzicka
Owner

@joshcooper @jeffmccune @zaphod42 I was able to replace the rubygems tar code with the minitar implementation, it turns out the minitar is not that much different and moreover better suited for embedding as it has no dependencies (although it contains more code we probably would not use).
So we could embed it with minimal changes which means it should be possible to update the code easily from the upstream.

The question is what namespace should I embed it under?
(It's current namespace is Archive::Tar::Minitar)

@haus the minitar code can be found here: https://github.com/atoulme/minitar/tree/master/lib/archive/tar
It appears to be similarly stable as the rubygems tar code, IMO again due the stability of the tar format itself.

@jeffmccune
Owner
@pvande

Inside of Puppet::Util is customary. For example, Trollop is in
puppet/util/command_line/trollop with a class name of
Puppet::Util::CommandLine::Trollop

We could similarly use Puppet::Util::Archive::Tar::Minitar with a path of
lib/puppet/util/archive/tar/minitar

How will this change if and when we extract the module tool into an independent module?

@jeffmccune
Owner

How will this change if and when we extract the module tool into an independent module?

I think in this situation the module should vendor the Minitar using the puppet_x and PuppetX conventions we've established in 14149. An example of migrating existing code to this convention can be found at https://github.com/puppetlabs/puppetlabs-registry/pull/18/commits

Please let me know if this doesn't fully answer your question or there's more information I can provide.

@mruzicka
Owner

I've added 3 new commits which bring the following changes:

  • add the requested YARD and fix the typo mentioned in one of the previous comments
  • replace the rubygems tar code with minitar implementation, which in turn is embedded into puppet at the Puppet::Util::Archive::Tar::Minitar namespace.
  • improve backward compatibility of the data structure returned by the PMT install/upgrade commands
  • fix the unit tests broken by the PMT install/upgrade code changes

AFAIKT that should address all the comments on this pull request, except for the request to add new unit tests,
which I'll add in another commit.

@adrienthebo
Owner

Tests are passing and the use of minitar looks good by me, :+1: when we get unit test coverage.

@joshcooper joshcooper commented on the diff
lib/puppet/module_tool/applications/unpacker.rb
((17 lines not shown))
+ (destination_file_path.length == 2 || destination_file_path[2..2] == '/')
+ then
+ raise ArgumentError, "tar entry outside of the module directory: #{entry.full_name}"
+ end
+ destination_file = build_dir + destination_file
+ destination_directory = destination_file.dirname
+ destination_directory.mkpath() unless destination_directory.directory?
+
+ mode = entry.mode
+ case
+ when entry.directory?
+ destination_file.mkdir(mode) unless destination_file.directory?
+ destination_file.chmod(mode)
+ when entry.file?
+ destination_file.unlink() if (destination_file.exist? || destination_file.symlink?)
+ destination_file.open(File::WRONLY|File::CREAT|File::EXCL, mode) do |f|
@joshcooper Owner

@mruzicka I think we want to use puppet's built-in Puppet::Util.replace_file method to ensure we write files securely and atomically (we write to a tempfile and then rename)

@mruzicka Owner
mruzicka added a note

This is in fact a bit paranoid as the entire module is unpacked to an empty temporary directory so we should never be overwriting any files unless the tar contains duplicate entries, in which case I just make sure the last one wins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@joshcooper joshcooper commented on the diff
lib/puppet/module_tool/applications/unpacker.rb
@@ -35,18 +36,44 @@ def extract_module_to_install_dir
build_dir.mkpath
begin
begin
- if Facter.value('osfamily') == "Solaris"
- # Solaris tar is not as safe and works differently, so we prefer
- # gnutar instead.
- if Puppet::Util.which('gtar')
- Puppet::Util::Execution.execute("gtar xzf #{@filename} -C #{build_dir}")
- else
- raise RuntimeError, "Cannot find the command 'gtar'. Make sure GNU tar is installed, and is in your PATH."
+ Zlib::GzipReader.open(@filename) do |gzip|
+ Puppet::Util::Archive::Tar::Minitar::Reader.open(gzip) do |tar|
+ tar.each do |entry|
+ destination_file = Pathname.new(entry.full_name).cleanpath
+ if destination_file.absolute? ||
+ (destination_file_path = destination_file.to_s).start_with?('..') &&
@joshcooper Owner

@mruzicka I'm not sure this is sufficient, what about a malicious path of ./../../? The rubygems' tar code has similar logic for checking that we don't escape the module directory, how do they handle it?

@mruzicka Owner
mruzicka added a note

The Pathanme.cleanpath is supposed to canonicalize such malicious paths so the test for .. should be sufficient.
The rubygems' tar code uses similar approach utilizing the File.expand_path to canonicalize the path and subsequently making sure the start of the path matches the expected install directory, see here:
https://github.com/rubygems/rubygems/blob/master/lib/rubygems/package.rb#L368-L378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@joshcooper joshcooper commented on the diff
lib/puppet/util/archive/tar/minitar/command.rb
((3 lines not shown))
+module Puppet
+module Util
+
+#--
+# Archive::Tar::Baby 0.5.2
+# Copyright 2004 Mauricio Julio Ferna'ndez Pradier and Austin Ziegler
+# This is free software with ABSOLUTELY NO WARRANTY.
+#
+# This program is based on and incorporates parts of RPA::Package from
+# rpa-base (lib/rpa/package.rb and lib/rpa/util.rb) by Mauricio and has been
+# adapted to be more generic by Austin.
+#
+# This file contains an adaptation of Ruby/ProgressBar by Satoru
+# Takabayashi <satoru@namazu.org>, copyright 2001 - 2004.
+#
+# It is licensed under the GNU General Public Licence or Ruby's licence.
@joshcooper Owner

@mruzicka can you check with the release team (@haus or @stahnma) to ensure the license is compatible with puppet's?

@haus Owner
haus added a note

The ruby license is Apache compatible, it should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@haus
Owner

We're working on a document to explain the preferred/accepted method and workflow of vendoring code in our projects. We'll have it ready for review in the next couple of days. Please hold on merging this until we have that solidified. Thanks!

mruzicka added some commits
mruzicka (#15841,#13542) PMT install/upgrade from a local tarball & embed Minitar
This patch should really be two separate ones but because of it its
gradual changes it ended up as a single large one. It introduces
two principal changes:
* Adds support to PMT for installing/upgrading puppet modules
  from local tarballs. This support depends on a new forge API
  introduced in:
    https://github.com/puppetlabs/puppet-forge-api/pull/32
  In addition to the posibility of installing modules from tarballs a new
  algorithm for module dependency resolution is also introduced.
  The algorithm is capable of backtracking so is able to find resolutions
  even in situations in which the original algorithm would have failed.
  Many of the data structures used by the depedecy resolution code have
  been changed but care has been taken to preserve backward compatiblity
  of any externally visible data structures.
  Also the installer and upgrader applications have been greatly unified,
  so they now share the majority of their code.

* Embeds the minitar gem into Puppet while changing its namespace to:
    Puppet::Util::Archive::Tar::Minitar
  and makes use of it at various places in the PMT code instead of
  calling external tar utilities.
6c7276c
mruzicka (#13542) New and updated acceptance tests for the PMT tarball install
This patch adds new and updates original acceptance tests to keep up
with the changes in PMT which now uses the ebeded minitar to work with
the module packages instead of calling external tar commands and has
gained the capability to install modules from tarballs.
0cabc5e
mruzicka (#13542) New and updated unit tests for the PMT tarball install
This patch adds new and updates the original unit tests for the PMT
install/upgrade code which now uses the ebeded minitar to work with
the module packages instead of calling external tar commands and has
gained the capability to install modules from tarballs.
ad7e651
@mruzicka
Owner

I've rebased this pull request on current master and while at it I have removed all references to the use of the rubygems' tar code as the pull request ended up embedding Minitar instead.

@joshcooper
Owner

@mruzicka (cc @khussey @adreyer @zaphod42 @rcoleman) Sorry for the back and forth on this. We have some concerns about the changes made to the vendored minitar to handle symbolic links and permissions. At the same time, we need to get puppet module tool on windows into Puppet 3.2 RC, which will go into PE3. So we've decided to continue using native commands (tar, gzip) for most platforms, but fall back to minitar on windows. This way we are not blocked on making minitar work on all platforms.

I am working on a branch to add minitar support on windows will be submitting a PR today, so I am going to close this PR. Can you split out your changes to install from local tarball changes(#13542) and submit a new PR?

@joshcooper joshcooper closed this
@ryanycoleman
Owner

Hi @joshcooper, I'm trying to figure out where this work stands. Did you end up making a new PR?

@ryanycoleman

@joshcooper, @ahpook, Could someone point me at the new PR for this work? I'm specifically interested in whether it's still a candidate for Puppet destined for PE 3.

@ahpook
Owner

@rcoleman the minitar on windows support is in as a targeted fix , see pull #1603 via redmine 11276

@ryanycoleman
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 4, 2013
  1. (#15841,#13542) PMT install/upgrade from a local tarball & embed Minitar

    mruzicka authored
    This patch should really be two separate ones but because of it its
    gradual changes it ended up as a single large one. It introduces
    two principal changes:
    * Adds support to PMT for installing/upgrading puppet modules
      from local tarballs. This support depends on a new forge API
      introduced in:
        https://github.com/puppetlabs/puppet-forge-api/pull/32
      In addition to the posibility of installing modules from tarballs a new
      algorithm for module dependency resolution is also introduced.
      The algorithm is capable of backtracking so is able to find resolutions
      even in situations in which the original algorithm would have failed.
      Many of the data structures used by the depedecy resolution code have
      been changed but care has been taken to preserve backward compatiblity
      of any externally visible data structures.
      Also the installer and upgrader applications have been greatly unified,
      so they now share the majority of their code.
    
    * Embeds the minitar gem into Puppet while changing its namespace to:
        Puppet::Util::Archive::Tar::Minitar
      and makes use of it at various places in the PMT code instead of
      calling external tar utilities.
  2. (#13542) New and updated acceptance tests for the PMT tarball install

    mruzicka authored
    This patch adds new and updates original acceptance tests to keep up
    with the changes in PMT which now uses the ebeded minitar to work with
    the module packages instead of calling external tar commands and has
    gained the capability to install modules from tarballs.
  3. (#13542) New and updated unit tests for the PMT tarball install

    mruzicka authored
    This patch adds new and updates the original unit tests for the PMT
    install/upgrade code which now uses the ebeded minitar to work with
    the module packages instead of calling external tar commands and has
    gained the capability to install modules from tarballs.
Something went wrong with that request. Please try again.