From: Hunter Haugen <hunter@puppetlabs.com>
Date: Wed, 5 Mar 2014 19:06:43 +0000 (-0800)
Subject: Merge pull request #249 from ghoneycutt/define_apt_source_in_hiera
X-Git-Tag: 1.5.0~26
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=de095a173fc3ba7a8238e442af92637cd5a20627;hp=46606c9a2b1c9b2b29aabcbbfe40312f54d00757;p=puppet-modules%2Fpuppetlabs-apt.git

Merge pull request #249 from ghoneycutt/define_apt_source_in_hiera

Add ability to specify hash of apt sources in hiera
---

diff --git a/.travis.yml b/.travis.yml
index a6cfda6..5a26d90 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -5,9 +5,6 @@ branches:
 language: ruby
 bundler_args: --without development
 script: bundle exec rake spec SPEC_OPTS='--format documentation'
-after_success:
-  - git clone -q git://github.com/puppetlabs/ghpublisher.git .forge-release
-  - .forge-release/publish
 rvm:
   - 1.8.7
   - 1.9.3
@@ -19,12 +16,6 @@ env:
     - PUPPET_GEM_VERSION="~> 3.2.0"
     - PUPPET_GEM_VERSION="~> 3.3.0"
     - PUPPET_GEM_VERSION="~> 3.4.0"
-  global:
-  - PUBLISHER_LOGIN=puppetlabs
-  - secure: |-
-      ipB/CV1rVSTXU9ZDuzrFOlzJrRmJob36tKns2xszuH4r9s5P9qivNAngRGdV
-      msb69xvOlzQykM0WRF+4kJ6TZ7AbMiDI+VZ8GDtsRaU5/q3BpsvFe8aato+6
-      QeyFtBG62OsosTEhGws4mqiFsPDu3dHlakuJc9zevlTuhNwbKSs=
 matrix:
   fast_finish: true
   exclude:
diff --git a/README.md b/README.md
index 5828d80..581277b 100644
--- a/README.md
+++ b/README.md
@@ -29,7 +29,6 @@ Setup
     * NOTE: Setting the `purge_sources_list` and `purge_sources_list_d` parameters to 'true' will destroy any existing content that was not declared with Puppet. The default for these parameters is 'false'.
 * system repositories
 * authentication keys
-* wget (optional)
 
 ### Beginning with APT
 
@@ -81,9 +80,31 @@ Forces a package to be installed from a specific release.  This class is particu
       require => Apt::Source['debian_unstable'],
     }
 
+### apt_key
+
+A native Puppet type and provider for managing GPG keys for APT is provided by
+this module.
+
+    apt_key { 'puppetlabs':
+      ensure => 'present',
+      id     => '4BD6EC30',
+    }
+
+You can additionally set the following attributes:
+
+ * `source`: HTTP, HTTPS or FTP location of a GPG key or path to a file on the
+             target host;
+ * `content`: Instead of pointing to a file, pass the key in as a string;
+ * `server`: The GPG key server to use. It defaults to *keyserver.ubuntu.com*;
+ * `keyserver_options`: Additional options to pass to `--keyserver`.
+
+Because it is a native type it can be used in and queried for with MCollective.
+
 ### apt::key
 
-Adds a key to the list of keys used by APT to authenticate packages.
+Adds a key to the list of keys used by APT to authenticate packages. This type
+uses the aforementioned `apt_key` native type. As such it no longer requires
+the wget command that the old implementation depended on.
 
     apt::key { 'puppetlabs':
       key        => '4BD6EC30',
@@ -95,8 +116,6 @@ Adds a key to the list of keys used by APT to authenticate packages.
       key_source => 'http://pkg.jenkins-ci.org/debian/jenkins-ci.org.key',
     }
 
-Note that use of `key_source` requires wget to be installed and working.
-
 ### apt::pin
 
 Adds an apt pin for a certain release.
@@ -119,6 +138,45 @@ If you wish to pin a number of packages you may specify the packages as a space
 delimited string using the `packages` attribute or pass in an array of package
 names.
 
+### apt::hold
+
+When you wish to hold a package in Puppet is should be done by passing in
+'held' as the ensure attribute to the package resource. However, a lot of
+public modules do not take this into account and generally do not work well
+with an ensure of 'held'.
+
+There is an additional issue that when Puppet is told to hold a package, it
+will hold it at the current version installed, there is no way to tell it in
+one go to install a specific version and then hold that version without using
+an exec resource that wraps `dpkg --set-selections` or `apt-mark`.
+
+At first glance this could also be solved by just passing the version required
+to the ensure attribute but that only means that Puppet will install that
+version once it processes that package. It does not inform apt that we want
+this package to be held. In other words; if another package somehow wants to
+upgrade this one (because of a version requirement in a dependency), apt
+should not allow it.
+
+In order to solve this you can use apt::hold. It's implemented by creating
+a preferences file with a priority of 1001, meaning that under normal
+circumstances this preference will always win. Because the priority is > 1000
+apt will interpret this as 'this should be the version installed and I am
+allowed to downgrade the current package if needed'.
+
+With this you can now set a package's ensure attribute to 'latest' but still
+get the version specified by apt::hold. You can do it like this:
+
+    apt::hold { 'vim':
+      version => '2:7.3.547-7',
+    }
+
+Since you might just want to hold Vim at version 7.3 and not care about the
+rest you can also pass in a version with a glob:
+
+    apt::hold { 'vim':
+      version => '2:7.3.*',
+    }
+
 ### apt::ppa
 
 Adds a ppa repository using `add-apt-repository`.
@@ -260,3 +318,4 @@ A lot of great people have contributed to this module. A somewhat current list f
 * Spencer Krum <spencer@puppetlabs.com>
 * William Van Hevelingen <blkperl@cat.pdx.edu> <wvan13@gmail.com>
 * Zach Leslie <zach@puppetlabs.com>
+* Daniele Sluijters <github@daenney.net>
diff --git a/manifests/hold.pp b/manifests/hold.pp
new file mode 100644
index 0000000..c4016c6
--- /dev/null
+++ b/manifests/hold.pp
@@ -0,0 +1,54 @@
+# == Define apt::hold
+#
+# This defined type allows you to hold a package based on the version you
+# require. It's implemented by dropping an apt preferences file pinning the
+# package to the version you require.
+#
+# === Parameters
+#
+# [*version*]
+#   The version at which you wish to pin a package.
+#
+#   This can either be the full version, such as 4:2.11.8.1-5, or
+#   a partial version, such as 4:2.11.*
+#
+# [*package*]
+#   _default_: +$title+, the title/name of the resource.
+#
+#   Name of the package that apt is to hold.
+#
+# [*priority*]
+#   _default_: +1001+
+#
+#   The default priority of 1001 causes this preference to always win. By
+#   setting the priority to a number greater than 1000 apt will always install
+#   this version even if it means downgrading the currently installed version.
+define apt::hold(
+  $version,
+  $ensure   = 'present',
+  $package  = $title,
+  $priority = 1001,
+){
+
+  validate_string($title)
+  validate_re($ensure, ['^present|absent',])
+  validate_string($package)
+  validate_string($version)
+
+  if ! is_integer($priority) {
+    fail('$priority must be an integer')
+  }
+
+  if $ensure == 'present' {
+    ::apt::pin { "hold ${package} at ${version}":
+      packages => $package,
+      version  => $version,
+      priority => $priority,
+    }
+  } else {
+    ::apt::pin { "hold ${package} at ${version}":
+      ensure => 'absent',
+    }
+  }
+
+}
diff --git a/manifests/key.pp b/manifests/key.pp
index c78bf65..9ccbfcb 100644
--- a/manifests/key.pp
+++ b/manifests/key.pp
@@ -1,90 +1,121 @@
+# == Define: apt::key
+#
+# The apt::key defined type allows for keys to be added to apt's keyring
+# which is used for package validation. This defined type uses the apt_key
+# native type to manage keys. This is a simple wrapper around apt_key with
+# a few safeguards in place.
+#
+# === Parameters
+#
+# [*key*]
+#   _default_: +$title+, the title/name of the resource
+#
+#   Is a GPG key ID. This key ID is validated with a regex enforcing it
+#   to only contain valid hexadecimal characters, be precisely 8 or 16
+#   characters long and optionally prefixed with 0x.
+#
+# [*ensure*]
+#   _default_: +present+
+#
+#   The state we want this key in, may be either one of:
+#   * +present+
+#   * +absent+
+#
+# [*key_content*]
+#   _default_: +undef+
+#
+#   This parameter can be used to pass in a GPG key as a
+#   string in case it cannot be fetched from a remote location
+#   and using a file resource is for other reasons inconvenient.
+#
+# [*key_source*]
+#   _default_: +undef+
+#
+#   This parameter can be used to pass in the location of a GPG
+#   key. This URI can take the form of a:
+#   * +URL+: ftp, http or https
+#   * +path+: absolute path to a file on the target system.
+#
+# [*key_server*]
+#   _default_: +undef+
+#
+#   The keyserver from where to fetch our GPG key. It defaults to
+#   undef which results in apt_key's default keyserver being used,
+#   currently +keyserver.ubuntu.com+.
+#
+# [*key_options*]
+#   _default_: +undef+
+#
+#   Additional options to pass on to `apt-key adv --keyserver-options`.
 define apt::key (
-  $key = $title,
-  $ensure = present,
-  $key_content = false,
-  $key_source = false,
-  $key_server = 'keyserver.ubuntu.com',
-  $key_options = false
+  $key         = $title,
+  $ensure      = present,
+  $key_content = undef,
+  $key_source  = undef,
+  $key_server  = undef,
+  $key_options = undef,
 ) {
 
-  include apt::params
-
-  $upkey = upcase($key)
-  # trim the key to the last 8 chars so we can match longer keys with apt-key list too
-  $trimmedkey = regsubst($upkey, '^.*(.{8})$', '\1')
+  validate_re($key, ['\A(0x)?[0-9a-fA-F]{8}\Z', '\A(0x)?[0-9a-fA-F]{16}\Z'])
+  validate_re($ensure, ['\Aabsent|present\Z',])
 
   if $key_content {
-    $method = 'content'
-  } elsif $key_source {
-    $method = 'source'
-  } elsif $key_server {
-    $method = 'server'
+    validate_string($key_content)
   }
 
-  # This is a hash of the parts of the key definition that we care about.
-  # It is used as a unique identifier for this instance of apt::key. It gets
-  # hashed to ensure that the resource name doesn't end up being pages and
-  # pages (e.g. in the situation where key_content is specified).
-  $digest = sha1("${upkey}/${key_content}/${key_source}/${key_server}/")
-
-  # Allow multiple ensure => present for the same key to account for many
-  # apt::source resources that all reference the same key.
-  case $ensure {
-    present: {
-
-      anchor { "apt::key/${title}": }
+  if $key_source {
+    validate_re($key_source, ['\Ahttps?:\/\/', '\Aftp:\/\/', '\A\/\w+'])
+  }
 
-      if defined(Exec["apt::key ${upkey} absent"]) {
-        fail("Cannot ensure Apt::Key[${upkey}] present; ${upkey} already ensured absent")
-      }
+  if $key_server {
+    if !is_domain_name($key_server) {
+      fail('$key_server must be a valid domain name')
+    }
+  }
 
-      if !defined(Anchor["apt::key ${upkey} present"]) {
-        anchor { "apt::key ${upkey} present": }
-      }
+  if $key_options {
+    validate_string($key_options)
+  }
 
-      if $key_options{
-        $options_string = "--keyserver-options ${key_options}"
-      }
-      else{
-        $options_string = ''
+  case $ensure {
+    present: {
+      if defined(Anchor["apt_key ${key} absent"]){
+        fail("key with id ${key} already ensured as absent")
       }
 
-      if !defined(Exec[$digest]) {
-        $digest_command = $method ? {
-          'content' => "echo '${key_content}' | /usr/bin/apt-key add -",
-          'source'  => "wget -q '${key_source}' -O- | apt-key add -",
-          'server'  => "apt-key adv --keyserver '${key_server}' ${options_string} --recv-keys '${upkey}'",
-        }
-        exec { $digest:
-          command   => $digest_command,
-          path      => '/bin:/usr/bin',
-          unless    => "/usr/bin/apt-key list | /bin/grep '${trimmedkey}'",
-          logoutput => 'on_failure',
-          before    => Anchor["apt::key ${upkey} present"],
-        }
+      if !defined(Anchor["apt_key ${key} present"]) {
+        apt_key { $title:
+          ensure            => $ensure,
+          id                => $key,
+          source            => $key_source,
+          content           => $key_content,
+          server            => $key_server,
+          keyserver_options => $key_options,
+        } ->
+        anchor { "apt_key ${key} present": }
       }
-
-      Anchor["apt::key ${upkey} present"] -> Anchor["apt::key/${title}"]
-
     }
-    absent: {
 
-      if defined(Anchor["apt::key ${upkey} present"]) {
-        fail("Cannot ensure Apt::Key[${upkey}] absent; ${upkey} already ensured present")
+    absent: {
+      if defined(Anchor["apt_key ${key} present"]){
+        fail("key with id ${key} already ensured as present")
       }
 
-      exec { "apt::key ${upkey} absent":
-        command   => "apt-key del '${upkey}'",
-        path      => '/bin:/usr/bin',
-        onlyif    => "apt-key list | grep '${trimmedkey}'",
-        user      => 'root',
-        group     => 'root',
-        logoutput => 'on_failure',
+      if !defined(Anchor["apt_key ${key} absent"]){
+        apt_key { $title:
+          ensure            => $ensure,
+          id                => $key,
+          source            => $key_source,
+          content           => $key_content,
+          server            => $key_server,
+          keyserver_options => $key_options,
+        } ->
+        anchor { "apt_key ${key} absent": }
       }
     }
 
     default: {
-      fail "Invalid 'ensure' value '${ensure}' for aptkey"
+      fail "Invalid 'ensure' value '${ensure}' for apt::key"
     }
   }
 }
diff --git a/manifests/params.pp b/manifests/params.pp
index b35bb1c..5ef3ccd 100644
--- a/manifests/params.pp
+++ b/manifests/params.pp
@@ -21,7 +21,7 @@ class apt::params {
     }
     'ubuntu': {
       case $::lsbdistcodename {
-        'hardy','maverick','natty','oneiric','precise': {
+        'precise','trusty': {
           $backports_location = 'http://us.archive.ubuntu.com/ubuntu'
           $ppa_options = '-y'
         }
diff --git a/manifests/pin.pp b/manifests/pin.pp
index 9cb4a8e..0d4d742 100644
--- a/manifests/pin.pp
+++ b/manifests/pin.pp
@@ -68,7 +68,17 @@ define apt::pin(
     ''      => "${preferences_d}/${name}.pref",
     default => "${preferences_d}/${order}-${name}.pref",
   }
-  file { "${name}.pref":
+
+  # According to man 5 apt_preferences:
+  # The files have either no or "pref" as filename extension
+  # and only contain alphanumeric, hyphen (-), underscore (_) and period
+  # (.) characters. Otherwise APT will print a notice that it has ignored a
+  # file, unless that file matches a pattern in the
+  # Dir::Ignore-Files-Silently configuration list - in which case it will
+  # be silently ignored.
+  $file_name = regsubst($title, '[^0-9a-z\-_\.]', '_', 'IG')
+
+  file { "${file_name}.pref":
     ensure  => $ensure,
     path    => $path,
     owner   => root,
diff --git a/manifests/source.pp b/manifests/source.pp
index bc93ad9..196fc92 100644
--- a/manifests/source.pp
+++ b/manifests/source.pp
@@ -8,10 +8,10 @@ define apt::source(
   $repos             = 'main',
   $include_src       = true,
   $required_packages = false,
-  $key               = false,
+  $key               = undef,
   $key_server        = 'keyserver.ubuntu.com',
-  $key_content       = false,
-  $key_source        = false,
+  $key_content       = undef,
+  $key_source        = undef,
   $pin               = false,
   $architecture      = undef
 ) {
@@ -69,7 +69,7 @@ define apt::source(
   }
 
   # We do not want to remove keys when the source is absent.
-  if ($key != false) and ($ensure == 'present') {
+  if $key and ($ensure == 'present') {
     apt::key { "Add key: ${key} from Apt::Source ${title}":
       ensure      => present,
       key         => $key,
diff --git a/spec/acceptance/pin_spec.rb b/spec/acceptance/pin_spec.rb
index 09a8d18..07fc607 100644
--- a/spec/acceptance/pin_spec.rb
+++ b/spec/acceptance/pin_spec.rb
@@ -150,7 +150,7 @@ describe 'apt::pin define', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfa
 
       describe file('/etc/apt/preferences.d/vim-puppet.pref') do
         it { should be_file }
-        it { should contain 'Pin: origin "testrelease"' }
+        it { should contain 'Pin: origin testrelease' }
       end
     end
   end
diff --git a/spec/defines/hold_spec.rb b/spec/defines/hold_spec.rb
new file mode 100644
index 0000000..731e218
--- /dev/null
+++ b/spec/defines/hold_spec.rb
@@ -0,0 +1,100 @@
+require 'spec_helper'
+describe 'apt::hold' do
+  let :facts do {
+      :osfamily   => 'Debian',
+      :lsbdistid  => 'Debian',
+      :lsbrelease => 'wheezy',
+  } end
+
+  let :title do
+    'vim'
+  end
+
+  let :default_params do {
+    :version => '1.1.1',
+  } end
+
+  describe 'default params' do
+    let :params do default_params end
+
+    it 'creates an apt preferences file' do
+      should contain_apt__hold(title).with({
+        :ensure   => 'present',
+        :package  => title,
+        :version  => params[:version],
+        :priority => 1001,
+      })
+
+      should contain_apt__pin("hold #{title} at #{params[:version]}").with({
+        :ensure   => 'present',
+        :packages => title,
+        :version  => params[:version],
+        :priority => 1001,
+      })
+    end
+  end
+
+  describe 'ensure => absent' do
+    let :params do default_params.merge({:ensure => 'absent',}) end
+
+    it 'creates an apt preferences file' do
+      should contain_apt__hold(title).with({
+        :ensure   => params[:ensure],
+      })
+
+      should contain_apt__pin("hold #{title} at #{params[:version]}").with({
+        :ensure   => params[:ensure],
+      })
+    end
+  end
+
+  describe 'priority => 990' do
+    let :params do default_params.merge({:priority => 990,}) end
+
+    it 'creates an apt preferences file' do
+      should contain_apt__hold(title).with({
+        :ensure   => 'present',
+        :package  => title,
+        :version  => params[:version],
+        :priority => params[:priority],
+      })
+
+      should contain_apt__pin("hold #{title} at #{params[:version]}").with({
+        :ensure   => 'present',
+        :packages => title,
+        :version  => params[:version],
+        :priority => params[:priority],
+      })
+    end
+  end
+
+  describe 'validation' do
+    context 'version => {}' do
+      let :params do { :version => {}, } end
+      it 'should fail' do
+        expect { subject }.to raise_error(/is not a string/)
+      end
+    end
+
+    context 'ensure => bananana' do
+      let :params do default_params.merge({:ensure => 'bananana',}) end
+      it 'should fail' do
+        expect { subject }.to raise_error(/does not match/)
+      end
+    end
+
+    context 'package => []' do
+      let :params do default_params.merge({:package => [],}) end
+      it 'should fail' do
+        expect { subject }.to raise_error(/is not a string/)
+      end
+    end
+
+    context 'priority => bananana' do
+      let :params do default_params.merge({:priority => 'bananana',}) end
+      it 'should fail' do
+        expect { subject }.to raise_error(/must be an integer/)
+      end
+    end
+  end
+end
diff --git a/spec/defines/key_spec.rb b/spec/defines/key_spec.rb
index 4ba7b87..a85d171 100644
--- a/spec/defines/key_spec.rb
+++ b/spec/defines/key_spec.rb
@@ -1,124 +1,285 @@
 require 'spec_helper'
+
 describe 'apt::key', :type => :define do
   let(:facts) { { :lsbdistid => 'Debian' } }
+  GPG_KEY_ID = '4BD6EC30'
+
   let :title do
-    '8347A27F'
+    GPG_KEY_ID
   end
 
-  let :default_params do
-    {
-      :key         => title,
-      :ensure      => 'present',
-      :key_server  => "keyserver.ubuntu.com",
-      :key_source  => false,
-      :key_content => false
-    }
-  end
+  describe 'normal operation' do
+    describe 'default options' do
+      it 'contains the apt::key' do
+        should contain_apt__key(title).with({
+          :key    => title,
+          :ensure => 'present',
+        })
+      end
+      it 'contains the apt_key' do
+        should contain_apt_key(title).with({
+          :id                => title,
+          :ensure            => 'present',
+          :source            => nil,
+          :server            => nil,
+          :content           => nil,
+          :keyserver_options => nil,
+        })
+      end
+      it 'contains the apt_key present anchor' do
+        should contain_anchor("apt_key #{title} present")
+      end
+    end
+
+    describe 'title and key =>' do
+      let :title do
+        'puppetlabs'
+      end
+
+      let :params do {
+        :key => GPG_KEY_ID,
+      } end
+
+      it 'contains the apt::key' do
+        should contain_apt__key(title).with({
+          :key    => GPG_KEY_ID,
+          :ensure => 'present',
+        })
+      end
+      it 'contains the apt_key' do
+        should contain_apt_key(title).with({
+          :id                => GPG_KEY_ID,
+          :ensure            => 'present',
+          :source            => nil,
+          :server            => nil,
+          :content           => nil,
+          :keyserver_options => nil,
+        })
+      end
+      it 'contains the apt_key present anchor' do
+        should contain_anchor("apt_key #{GPG_KEY_ID} present")
+      end
+    end
 
-  [{},
-    {
-      :ensure  => 'absent'
-    },
-    {
-      :ensure  => 'random'
-    },
-    {
-      :key_source => 'ftp://ftp.example.org/key',
-    },
-    {
-      :key_content => 'deadbeef',
-    }
-  ].each do |param_set|
-
-    let :param_hash do
-      param_hash = default_params.merge(param_set)
-      param_hash[:key].upcase! if param_hash[:key]
-      param_hash
+    describe 'ensure => absent' do
+      let :params do {
+        :ensure => 'absent',
+      } end
+
+      it 'contains the apt::key' do
+        should contain_apt__key(title).with({
+          :key          => title,
+          :ensure       => 'absent',
+        })
+      end
+      it 'contains the apt_key' do
+        should contain_apt_key(title).with({
+          :id                => title,
+          :ensure            => 'absent',
+          :source            => nil,
+          :server            => nil,
+          :content           => nil,
+          :keyserver_options => nil,
+        })
+      end
+      it 'contains the apt_key absent anchor' do
+        should contain_anchor("apt_key #{title} absent")
+      end
     end
 
-    let :params do
-      param_set
+    describe 'key_content =>' do
+      let :params do {
+        :key_content => 'GPG key content',
+      } end
+
+      it 'contains the apt::key' do
+        should contain_apt__key(title).with({
+          :key         => title,
+          :ensure      => 'present',
+          :key_content => params[:key_content],
+        })
+      end
+      it 'contains the apt_key' do
+        should contain_apt_key(title).with({
+          :id                => title,
+          :ensure            => 'present',
+          :source            => nil,
+          :server            => nil,
+          :content           => params[:key_content],
+          :keyserver_options => nil,
+        })
+      end
+      it 'contains the apt_key present anchor' do
+        should contain_anchor("apt_key #{title} present")
+      end
     end
 
-    let :digest do
-      str = String.new
-      str << param_hash[:key].to_s         << '/'
-      str << param_hash[:key_content].to_s << '/'
-      str << param_hash[:key_source].to_s  << '/'
-      str << param_hash[:key_server].to_s  << '/'
-      Digest::SHA1.hexdigest(str)
+    describe 'key_source =>' do
+      let :params do {
+        :key_source => 'http://apt.puppetlabs.com/pubkey.gpg',
+      } end
+
+      it 'contains the apt::key' do
+        should contain_apt__key(title).with({
+          :key         => title,
+          :ensure      => 'present',
+          :key_source => params[:key_source],
+        })
+      end
+      it 'contains the apt_key' do
+        should contain_apt_key(title).with({
+          :id                => title,
+          :ensure            => 'present',
+          :source            => params[:key_source],
+          :server            => nil,
+          :content           => nil,
+          :keyserver_options => nil,
+        })
+      end
+      it 'contains the apt_key present anchor' do
+        should contain_anchor("apt_key #{title} present")
+      end
+    end
+
+    describe 'key_server =>' do
+      let :params do {
+        :key_server => 'pgp.mit.edu',
+      } end
+
+      it 'contains the apt::key' do
+        should contain_apt__key(title).with({
+          :key         => title,
+          :ensure      => 'present',
+          :key_server  => 'pgp.mit.edu',
+        })
+      end
+      it 'contains the apt_key' do
+        should contain_apt_key(title).with({
+          :id                => title,
+          :ensure            => 'present',
+          :source            => nil,
+          :server            => params[:key_server],
+          :content           => nil,
+          :keyserver_options => nil,
+        })
+      end
+      it 'contains the apt_key present anchor' do
+        should contain_anchor("apt_key #{title} present")
+      end
     end
 
-    describe "when #{param_set == {} ? "using default" : "specifying"} define parameters" do
-
-      it {
-        if [:present, 'present', :absent, 'absent'].include? param_hash[:ensure]
-          should contain_apt__params
-        end
-      }
-
-      it {
-        if [:present, 'present'].include? param_hash[:ensure]
-          should_not contain_exec("apt::key #{param_hash[:key]} absent")
-          should contain_anchor("apt::key #{param_hash[:key]} present")
-          should contain_exec(digest).with({
-            "path"    => "/bin:/usr/bin",
-            "unless"  => "/usr/bin/apt-key list | /bin/grep '#{param_hash[:key]}'"
-          })
-        elsif [:absent, 'absent'].include? param_hash[:ensure]
-          should_not contain_anchor("apt::key #{param_hash[:key]} present")
-          should contain_exec("apt::key #{param_hash[:key]} absent").with({
-            "path"    => "/bin:/usr/bin",
-            "onlyif"  => "apt-key list | grep '#{param_hash[:key]}'",
-            "command" => "apt-key del '#{param_hash[:key]}'"
-          })
-        else
-          expect { should raise_error(Puppet::Error) }
-        end
-      }
-
-      it {
-        if [:present, 'present'].include? param_hash[:ensure]
-          if param_hash[:key_content]
-            should contain_exec(digest).with({
-              "command" => "echo '#{param_hash[:key_content]}' | /usr/bin/apt-key add -"
-            })
-          elsif param_hash[:key_source]
-            should contain_exec(digest).with({
-              "command" => "wget -q '#{param_hash[:key_source]}' -O- | apt-key add -"
-            })
-          elsif param_hash[:key_server]
-            should contain_exec(digest).with({
-              "command" => "apt-key adv --keyserver '#{param_hash[:key_server]}' --recv-keys '#{param_hash[:key]}'"
-            })
-          end
-        end
-      }
+    describe 'key_options =>' do
+      let :params do {
+        :key_options => 'debug',
+      } end
 
+      it 'contains the apt::key' do
+        should contain_apt__key(title).with({
+          :key          => title,
+          :ensure       => 'present',
+          :key_options  => 'debug',
+        })
+      end
+      it 'contains the apt_key' do
+        should contain_apt_key(title).with({
+          :id                => title,
+          :ensure            => 'present',
+          :source            => nil,
+          :server            => nil,
+          :content           => nil,
+          :keyserver_options => params[:key_options],
+        })
+      end
+      it 'contains the apt_key present anchor' do
+        should contain_anchor("apt_key #{title} present")
+      end
     end
   end
 
-  [{ :ensure => 'present' }, { :ensure => 'absent' }].each do |param_set|
-    describe "should correctly handle duplicate definitions" do
+  describe 'validation' do
+    context 'invalid key' do
+      let :title do
+        'Out of rum. Why? Why are we out of rum?'
+      end
+      it 'fails' do
+        expect { subject }.to raise_error(/does not match/)
+      end
+    end
 
-      let :pre_condition do
-        "apt::key { 'duplicate': key => '#{title}'; }"
+    context 'invalid source' do
+      let :params do {
+        :key_source => 'afp://puppetlabs.com/key.gpg',
+      } end
+      it 'fails' do
+        expect { subject }.to raise_error(/does not match/)
       end
+    end
 
-      let(:params) { param_set }
+    context 'invalid content' do
+      let :params do {
+        :key_content => [],
+      } end
+      it 'fails' do
+        expect { subject }.to raise_error(/is not a string/)
+      end
+    end
 
-      it {
-        if param_set[:ensure] == 'present'
-          should contain_anchor("apt::key #{title} present")
-          should contain_apt__key(title)
-          should contain_apt__key("duplicate")
-        elsif param_set[:ensure] == 'absent'
-          expect { should raise_error(Puppet::Error) }
-        end
-      }
+    context 'invalid server' do
+      let :params do {
+        :key_server => 'two bottles of rum',
+      } end
+      it 'fails' do
+        expect { subject }.to raise_error(/must be a valid domain name/)
+      end
+    end
 
+    context 'invalid keyserver_options' do
+      let :params do {
+        :key_options => {},
+      } end
+      it 'fails' do
+        expect { subject }.to raise_error(/is not a string/)
+      end
     end
   end
 
-end
+  describe 'duplication' do
+    context 'two apt::key resources for same key, different titles' do
+      let :pre_condition do
+        "apt::key { 'duplicate': key => #{title}, }"
+      end
 
+      it 'contains two apt::key resources' do
+        should contain_apt__key('duplicate').with({
+          :key    => title,
+          :ensure => 'present',
+        })
+        should contain_apt__key(title).with({
+          :key    => title,
+          :ensure => 'present',
+        })
+      end
+
+      it 'contains only a single apt_key' do
+        should contain_apt_key('duplicate').with({
+          :id                => title,
+          :ensure            => 'present',
+          :source            => nil,
+          :server            => nil,
+          :content           => nil,
+          :keyserver_options => nil,
+        })
+        should_not contain_apt_key(title)
+      end
+    end
+
+    context 'two apt::key resources, different ensure' do
+      let :pre_condition do
+        "apt::key { 'duplicate': key => #{title}, ensure => 'absent', }"
+      end
+      it 'informs the user of the impossibility' do
+        expect { subject }.to raise_error(/already ensured as absent/)
+      end
+    end
+  end
+end
diff --git a/spec/defines/pin_spec.rb b/spec/defines/pin_spec.rb
index f1ca437..6438e8c 100644
--- a/spec/defines/pin_spec.rb
+++ b/spec/defines/pin_spec.rb
@@ -105,4 +105,16 @@ describe 'apt::pin', :type => :define do
       }
     end
   end
+
+  describe 'resource title with invalid chars' do
+    context 'spaces' do
+      let(:title) { 'oh my god this is not valid' }
+      it { should contain_file('oh_my_god_this_is_not_valid.pref') }
+    end
+
+    context '#$&*$' do
+      let(:title) { 'so && many $* invalid @! things' }
+      it { should contain_file('so____many____invalid____things.pref') }
+    end
+  end
 end
diff --git a/spec/defines/source_spec.rb b/spec/defines/source_spec.rb
index 9da8b23..34b3942 100644
--- a/spec/defines/source_spec.rb
+++ b/spec/defines/source_spec.rb
@@ -1,6 +1,9 @@
 require 'spec_helper'
+
 describe 'apt::source', :type => :define do
   let(:facts) { { :lsbdistid => 'Debian' } }
+  GPG_KEY_ID = '4BD6EC30'
+
   let :title do
     'my_source'
   end
@@ -14,7 +17,7 @@ describe 'apt::source', :type => :define do
       :include_src        => true,
       :required_packages  => false,
       :key                => false,
-      :key_server         => 'keyserver.ubuntu.com',
+      :key_server         => false,
       :key_content        => false,
       :key_source         => false,
       :pin                => false
@@ -28,15 +31,14 @@ describe 'apt::source', :type => :define do
       :repos              => 'security',
       :include_src        => false,
       :required_packages  => 'apache',
-      :key                => 'key_name',
+      :key                => GPG_KEY_ID,
       :key_server         => 'keyserver.debian.com',
       :pin                => '600',
       :key_content        => 'ABCD1234'
     },
     {
-      :key                => 'key_name',
+      :key                => GPG_KEY_ID,
       :key_server         => 'keyserver.debian.com',
-      :key_content        => false,
     },
     {
       :ensure             => 'absent',
@@ -135,13 +137,16 @@ describe 'apt::source', :type => :define do
       }
 
       it {
+        key_server  = param_hash[:key_server]  || nil
+        key_content = param_hash[:key_content] || nil
+        key_source  = param_hash[:key_source]  || nil
         if param_hash[:key]
           should contain_apt__key("Add key: #{param_hash[:key]} from Apt::Source #{title}").with({
             "key"         => param_hash[:key],
             "ensure"      => :present,
-            "key_server"  => param_hash[:key_server],
-            "key_content" => param_hash[:key_content],
-            "key_source"  => param_hash[:key_source],
+            "key_server"  => key_server,
+            "key_content" => key_content,
+            "key_source"  => key_source,
             "before"      => "File[#{title}.list]"
           })
         else