From a3161566f52f8da4847bd750df572053adc9f2ff Mon Sep 17 00:00:00 2001
From: Callum Banbery <callum.banbery@onthebeach.co.uk>
Date: Sun, 21 Mar 2021 08:06:31 +0000
Subject: [PATCH 1/3] Fix certbot VCS package installation

As referenced in #240 the official installation method was changed
in December 2020, deprecating the use of the `certbot-auto` script,
and recommending `snap` as the primary package source.

As `snap` isn't yet a builtin package provider in Puppet and I don't
fancy creating a dependency on a third party module, nor do I want
to add a load of `exec` commands for managing snap packages I chose
to fix the VCS install method, and since it's used in the CI there's
less to change.

Since we can no longer use the `certbot-auto` script we should
follow the setup instructions provided by Letsencrypt's development
guide https://certbot.eff.org/docs/contributing.html.  Their guide
simply states to call a python script which configures the virtual
environment, and since the script is gone we no longer need the
VENV_PATH environment variable.

As the command needs to create a python virtual environment we
need to add the required package to the VCS dependencies, and from
testing I found there was no need to call the initialisation command
when installing from a package source, so I've made it conditional
to the VCS installation.

I've also taken the opportunity to update the version of `certbot`
that's checked out during VCS installation since `0.39.0` is now
around 1.5 years old.  Version `1.7.0` is the latest version that
is supported by Python 3.5 which is the lowest Python3 version
that's installed by default via package managers.
---
 REFERENCE.md                              | 24 ++++++++++++------
 data/Debian-family.yaml                   |  5 ++++
 data/RedHat-family.yaml                   |  4 +++
 manifests/certonly.pp                     |  5 ++--
 manifests/init.pp                         | 26 +++++++++-----------
 manifests/install.pp                      |  7 +++---
 spec/acceptance/letsencrypt_spec.rb       |  2 +-
 spec/classes/letsencrypt_install_spec.rb  | 30 ++++++++++++++++-------
 spec/classes/letsencrypt_spec.rb          | 14 +++++------
 spec/defines/letsencrypt_certonly_spec.rb | 26 ++++++++++----------
 templates/renew-script.sh.erb             |  2 +-
 11 files changed, 86 insertions(+), 59 deletions(-)
 create mode 100644 data/Debian-family.yaml

diff --git a/REFERENCE.md b/REFERENCE.md
index c3459d5d..14be60e3 100644
--- a/REFERENCE.md
+++ b/REFERENCE.md
@@ -70,19 +70,19 @@ The path to the letsencrypt installation.
 
 Default value: '/opt/letsencrypt'
 
-##### `venv_path`
+##### `environment`
 
-Data type: `Any`
+Data type: `Array`
 
-virtualenv path for vcs-installed Certbot
+An optional array of environment variables.
 
-Default value: '/opt/letsencrypt/.venv'
+Default value: []
 
-##### `environment`
+##### `vcs_dependencies`
 
 Data type: `Array`
 
-An optional array of environment variables (in addition to VENV_PATH)
+An array of packages on which the VCS installation method depends.
 
 Default value: []
 
@@ -100,7 +100,7 @@ Data type: `String`
 
 The Git ref (tag, sha, branch) to check out when installing the client with the `vcs` method.
 
-Default value: 'v0.39.0'
+Default value: 'v1.7.0'
 
 ##### `package_name`
 
@@ -381,6 +381,14 @@ Name of package to use when installing the client with the `package` method.
 
 Default value: $letsencrypt::package_name
 
+##### `vcs_dependencies`
+
+Data type: `Array`
+
+An array of packages on which the VCS installation method depends.
+
+Default value: []
+
 ### letsencrypt::plugin::dns_rfc2136
 
 This class installs and configures the Let's Encrypt dns-rfc2136 plugin.
@@ -613,7 +621,7 @@ Default value: []
 
 Data type: `Array[String[1]]`
 
-An optional array of environment variables (in addition to VENV_PATH).
+An optional array of environment variables.
 
 Default value: []
 
diff --git a/data/Debian-family.yaml b/data/Debian-family.yaml
new file mode 100644
index 00000000..a90d8650
--- /dev/null
+++ b/data/Debian-family.yaml
@@ -0,0 +1,5 @@
+---
+letsencrypt::vcs_dependencies:
+  - 'git'
+  - 'python3'
+  - 'python3-venv'
diff --git a/data/RedHat-family.yaml b/data/RedHat-family.yaml
index 404b411c..912e9a78 100644
--- a/data/RedHat-family.yaml
+++ b/data/RedHat-family.yaml
@@ -2,3 +2,7 @@
 letsencrypt::configure_epel: true
 letsencrypt::plugin::dns_rfc2136::package_name: 'python2-certbot-dns-rfc2136'
 letsencrypt::plugin::dns_route53::package_name: 'python2-certbot-dns-route53'
+letsencrypt::vcs_dependencies:
+  - 'git'
+  - 'python3'
+  - 'python3-virtualenv'
diff --git a/manifests/certonly.pp b/manifests/certonly.pp
index b639fc95..0290b2da 100644
--- a/manifests/certonly.pp
+++ b/manifests/certonly.pp
@@ -17,7 +17,7 @@
 #   element will be used for all subsequent domains.
 # @param letsencrypt_command Command to run letsencrypt
 # @param additional_args An array of additional command line arguments to pass to the `letsencrypt-auto` command.
-# @param environment  An optional array of environment variables (in addition to VENV_PATH).
+# @param environment  An optional array of environment variables.
 # @param key_size Size for the RSA public key
 # @param manage_cron
 #   Indicating whether or not to schedule cron job for renewal.
@@ -157,7 +157,6 @@
   ]).filter | $arg | { $arg =~ NotUndef and $arg != [] }
   $command = join($_command, ' ')
 
-  $execution_environment = ["VENV_PATH=${letsencrypt::venv_path}",] + $environment
   $verify_domains = join(unique($domains), '\' \'')
 
   if $ensure == 'present' {
@@ -170,7 +169,7 @@
     command     => $command,
     *           => $exec_ensure,
     path        => $facts['path'],
-    environment => $execution_environment,
+    environment => $environment,
     provider    => 'shell',
     require     => [
       Class['letsencrypt'],
diff --git a/manifests/init.pp b/manifests/init.pp
index d72707b2..3337ce79 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -14,8 +14,7 @@
 #   The email address to use to register with Let's Encrypt. This takes
 #   precedence over an 'email' setting defined in $config.
 # @param path The path to the letsencrypt installation.
-# @param venv_path virtualenv path for vcs-installed Certbot
-# @param environment An optional array of environment variables (in addition to VENV_PATH)
+# @param environment An optional array of environment variables.
 # @param repo A Git URL to install the Let's encrypt client from.
 # @param version The Git ref (tag, sha, branch) to check out when installing the client with the `vcs` method.
 # @param package_name Name of package and command to use when installing the client with the `package` method.
@@ -28,6 +27,7 @@
 # @param manage_config A feature flag to toggle the management of the letsencrypt configuration file.
 # @param manage_install A feature flag to toggle the management of the letsencrypt client installation.
 # @param manage_dependencies A feature flag to toggle the management of the letsencrypt dependencies.
+# @param vcs_dependencies Array of packages to install before executing vcs installation.
 # @param configure_epel A feature flag to include the 'epel' class and depend on it for package installation.
 # @param install_method Method to install the letsencrypt client, either package or vcs.
 # @param agree_tos A flag to agree to the Let's Encrypt Terms of Service.
@@ -60,10 +60,9 @@
   Boolean $configure_epel,
   Optional[String] $email                = undef,
   String $path                           = '/opt/letsencrypt',
-  $venv_path                             = '/opt/letsencrypt/.venv',
   Array $environment                     = [],
   String $repo                           = 'https://github.com/certbot/certbot.git',
-  String $version                        = 'v0.39.0',
+  String $version                        = 'v1.7.0',
   String $package_name                   = 'certbot',
   $package_ensure                        = 'installed',
   String $package_command                = 'certbot',
@@ -75,6 +74,7 @@
   Boolean $manage_config                 = true,
   Boolean $manage_install                = true,
   Boolean $manage_dependencies           = true,
+  Array[String] $vcs_dependencies        = [],
   Enum['package', 'vcs'] $install_method = 'package',
   Boolean $agree_tos                     = true,
   Boolean $unsafe_registration           = false,
@@ -91,18 +91,16 @@
 ) {
   if $manage_install {
     contain letsencrypt::install # lint:ignore:relative_classname_inclusion
-    Class['letsencrypt::install'] ~> Exec['initialize letsencrypt']
     Class['letsencrypt::install'] -> Class['letsencrypt::renew']
-  }
 
-  $command = $install_method ? {
-    'package' => $package_command,
-    'vcs'     => "${venv_path}/bin/letsencrypt",
+    if $install_method == 'vcs' {
+      Class['letsencrypt::install'] ~> Exec['initialize letsencrypt']
+    }
   }
 
-  $command_init = $install_method ? {
+  $command = $install_method ? {
     'package' => $package_command,
-    'vcs'     => "${path}/letsencrypt-auto",
+    'vcs'     => "${path}/venv3/bin/certbot",
   }
 
   if $manage_config {
@@ -112,11 +110,11 @@
 
   contain letsencrypt::renew
 
-  # TODO: do we need this command when installing from package?
   exec { 'initialize letsencrypt':
-    command     => "${command_init} -h",
+    command     => 'python3 tools/venv3.py',
+    cwd         => $path,
     path        => $facts['path'],
-    environment => concat(["VENV_PATH=${venv_path}"], $environment),
+    environment => $environment,
     refreshonly => true,
   }
 
diff --git a/manifests/install.pp b/manifests/install.pp
index 0ba33134..89ec144a 100644
--- a/manifests/install.pp
+++ b/manifests/install.pp
@@ -7,6 +7,7 @@
 # @param path The path to the letsencrypt installation.
 # @param repo A Git URL to install the Let's encrypt client from.
 # @param version The Git ref (tag, sha, branch) to check out when installing the client with the `vcs` method.
+# @param vcs_dependencies Array of packages to install before executing vcs installation.
 # @param package_ensure The value passed to `ensure` when installing the client with the `package` method.
 # @param package_name Name of package to use when installing the client with the `package` method.
 #
@@ -20,12 +21,12 @@
   String $path                           = $letsencrypt::path,
   String $repo                           = $letsencrypt::repo,
   String $version                        = $letsencrypt::version,
+  Array[String] $vcs_dependencies        = $letsencrypt::vcs_dependencies,
 ) {
   if $install_method == 'vcs' {
     if $manage_dependencies {
-      $dependencies = ['python', 'git']
-      ensure_packages($dependencies)
-      Package[$dependencies] -> Vcsrepo[$path]
+      ensure_packages($vcs_dependencies)
+      Package[$vcs_dependencies] -> Vcsrepo[$path]
     }
 
     vcsrepo { $path:
diff --git a/spec/acceptance/letsencrypt_spec.rb b/spec/acceptance/letsencrypt_spec.rb
index 7c30e7b0..6a5cef80 100644
--- a/spec/acceptance/letsencrypt_spec.rb
+++ b/spec/acceptance/letsencrypt_spec.rb
@@ -55,7 +55,7 @@ class { 'letsencrypt' :
       its(:content) { is_expected.to match %r{email = letsregister@example.com} }
     end
 
-    describe file('/opt/letsencrypt/.venv/bin/certbot') do
+    describe file('/opt/letsencrypt/venv3/bin/certbot') do
       it { is_expected.to be_file }
       it { is_expected.to be_owned_by 'root' }
       it { is_expected.to be_grouped_into 'root' }
diff --git a/spec/classes/letsencrypt_install_spec.rb b/spec/classes/letsencrypt_install_spec.rb
index 46a807cc..838aa852 100644
--- a/spec/classes/letsencrypt_install_spec.rb
+++ b/spec/classes/letsencrypt_install_spec.rb
@@ -11,8 +11,9 @@
         manage_dependencies: true,
         path: '/opt/letsencrypt',
         repo: 'https://github.com/certbot/certbot.git',
-        version: 'v0.30.2',
-        package_name: 'letsencrypt'
+        version: 'v1.7.0',
+        package_name: 'letsencrypt',
+        vcs_dependencies: vcs_dependencies,
       }
     end
     let(:additional_params) { {} }
@@ -21,6 +22,14 @@
       let :facts do
         facts
       end
+      let(:vcs_dependencies) do
+        case facts[:osfamily]
+        when 'Debian'
+          ['git', 'python3', 'python3-venv']
+        when 'RedHat', 'OpenBSD', 'FreeBSD'
+          ['git', 'python3', 'python3-virtualenv']
+        end
+      end
 
       describe 'with install_method => package' do
         let(:additional_params) { { install_method: 'package' } }
@@ -29,8 +38,9 @@
 
         it 'contains the correct resources' do
           is_expected.not_to contain_vcsrepo('/opt/letsencrypt')
-          is_expected.not_to contain_package('python')
-          is_expected.not_to contain_package('git')
+          vcs_dependencies.each do |package|
+            is_expected.not_to contain_package(package)
+          end
 
           is_expected.to contain_package('letsencrypt').with_ensure('installed')
         end
@@ -64,9 +74,10 @@
 
         it 'contains the correct resources' do
           is_expected.to contain_vcsrepo('/opt/letsencrypt').with(source: 'https://github.com/certbot/certbot.git',
-                                                                  revision: 'v0.30.2')
-          is_expected.to contain_package('python')
-          is_expected.to contain_package('git')
+                                                                  revision: 'v1.7.0')
+          vcs_dependencies.each do |package|
+            is_expected.to contain_package(package)
+          end
 
           is_expected.not_to contain_package('letsencrypt')
         end
@@ -93,8 +104,9 @@
           let(:additional_params) { { install_method: 'vcs', manage_dependencies: false } }
 
           it 'does not contain the dependencies' do
-            is_expected.not_to contain_package('git')
-            is_expected.not_to contain_package('python')
+            vcs_dependencies.each do |package|
+              is_expected.not_to contain_package(package)
+            end
           end
         end
       end
diff --git a/spec/classes/letsencrypt_spec.rb b/spec/classes/letsencrypt_spec.rb
index 1a5e6154..0bc5533b 100644
--- a/spec/classes/letsencrypt_spec.rb
+++ b/spec/classes/letsencrypt_spec.rb
@@ -32,8 +32,7 @@
                    manage_install: true,
                    manage_dependencies: true,
                    repo: 'https://github.com/certbot/certbot.git',
-                   version: 'v0.39.0').
-              that_notifies('Exec[initialize letsencrypt]').
+                   version: 'v1.7.0').
               that_comes_before('Class[letsencrypt::renew]')
             is_expected.to contain_exec('initialize letsencrypt')
             is_expected.to contain_class('letsencrypt::config').that_comes_before('Exec[initialize letsencrypt]')
@@ -92,7 +91,9 @@
               is_expected.to contain_package('letsencrypt').with(name: 'py27-certbot')
               is_expected.to contain_file('/usr/local/etc/letsencrypt').with(ensure: 'directory')
             else
-              is_expected.to contain_class('letsencrypt::install').with(install_method: 'vcs')
+              is_expected.to contain_class('letsencrypt::install').with(install_method: 'vcs').
+                that_notifies('Exec[initialize letsencrypt]').
+                that_comes_before('Class[letsencrypt::renew]')
               is_expected.to contain_file('/etc/letsencrypt').with(ensure: 'directory')
             end
           end
@@ -102,13 +103,13 @@
           let(:additional_params) { { path: '/usr/lib/letsencrypt', install_method: 'vcs' } }
 
           it { is_expected.to contain_class('letsencrypt::install').with_path('/usr/lib/letsencrypt') }
-          it { is_expected.to contain_exec('initialize letsencrypt').with_command('/usr/lib/letsencrypt/letsencrypt-auto -h') }
+          it { is_expected.to contain_exec('initialize letsencrypt').with_command('python3 tools/venv3.py') }
         end
 
         describe 'with custom environment variables' do
           let(:additional_params) { { environment: ['FOO=bar', 'FIZZ=buzz'] } }
 
-          it { is_expected.to contain_exec('initialize letsencrypt').with_environment(['VENV_PATH=/opt/letsencrypt/.venv', 'FOO=bar', 'FIZZ=buzz']) }
+          it { is_expected.to contain_exec('initialize letsencrypt').with_environment(['FOO=bar', 'FIZZ=buzz']) }
         end
 
         describe 'with custom repo' do
@@ -162,14 +163,13 @@
           let(:additional_params) { { install_method: 'package', package_command: 'letsencrypt' } }
 
           it { is_expected.to contain_class('letsencrypt::install').with_install_method('package') }
-          it { is_expected.to contain_exec('initialize letsencrypt').with_command('letsencrypt -h') }
         end
 
         describe 'with install_method => vcs' do
           let(:additional_params) { { install_method: 'vcs' } }
 
           it { is_expected.to contain_class('letsencrypt::install').with_install_method('vcs') }
-          it { is_expected.to contain_exec('initialize letsencrypt').with_command('/opt/letsencrypt/letsencrypt-auto -h') }
+          it { is_expected.to contain_exec('initialize letsencrypt').with_command('python3 tools/venv3.py') }
         end
 
         describe 'with custom config directory' do
diff --git a/spec/defines/letsencrypt_certonly_spec.rb b/spec/defines/letsencrypt_certonly_spec.rb
index 63028b71..b4e61bcc 100644
--- a/spec/defines/letsencrypt_certonly_spec.rb
+++ b/spec/defines/letsencrypt_certonly_spec.rb
@@ -172,7 +172,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_command('"/var/lib/puppet/letsencrypt/renew-foo.example.com.sh"').with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a apache --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a apache --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with hook' do
@@ -228,7 +228,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_hour(13).with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with manage_cron and out of range defined cron_hour (integer)' do
@@ -255,7 +255,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_hour('00').with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with manage_cron and defined cron_hour (array)' do
@@ -269,7 +269,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_hour([1, 13]).with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with manage_cron and defined cron_minute (integer)' do
@@ -283,7 +283,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_minute(15).with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with manage_cron and out of range defined cron_hour (integer)' do
@@ -310,7 +310,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_minute('15').with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with manage_cron and defined cron_minute (array)' do
@@ -324,7 +324,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_minute([0, 30]).with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with manage_cron and ensure absent' do
@@ -354,7 +354,7 @@ class { 'letsencrypt::plugin::dns_route53':
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_file('/tmp/custom_vardir/letsencrypt').with_ensure('directory') }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_command '"/tmp/custom_vardir/letsencrypt/renew-foo.example.com.sh"' }
-        it { is_expected.to contain_file('/tmp/custom_vardir/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a apache --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/tmp/custom_vardir/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a apache --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with custom plugin and manage cron and cron_success_command' do
@@ -370,7 +370,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_command '"/var/lib/puppet/letsencrypt/renew-foo.example.com.sh"' }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\n(echo before) && letsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a apache --cert-name 'foo.example.com' -d 'foo.example.com' && (echo success)\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\n(echo before) && letsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a apache --cert-name 'foo.example.com' -d 'foo.example.com' && (echo success)\n") }
       end
 
       context 'without plugin' do
@@ -401,7 +401,7 @@ class { 'letsencrypt::plugin::dns_route53':
         let(:params) { { environment: ['FOO=bar', 'FIZZ=buzz'] } }
 
         it { is_expected.to compile.with_all_deps }
-        it { is_expected.to contain_exec('letsencrypt certonly foo.example.com').with_environment(['VENV_PATH=/opt/letsencrypt/.venv', 'FOO=bar', 'FIZZ=buzz']) }
+        it { is_expected.to contain_exec('letsencrypt certonly foo.example.com').with_environment(['FOO=bar', 'FIZZ=buzz']) }
       end
 
       context 'with custom environment variables and manage_cron' do
@@ -409,7 +409,7 @@ class { 'letsencrypt::plugin::dns_route53':
         let(:params) { { environment: ['FOO=bar', 'FIZZ=buzz'], manage_cron: true } }
 
         it { is_expected.to compile.with_all_deps }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_content "#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nexport FOO=bar\nexport FIZZ=buzz\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n" }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_content "#!/bin/sh\nexport FOO=bar\nexport FIZZ=buzz\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n" }
       end
 
       context 'with manage cron and suppress_cron_output' do\
@@ -421,7 +421,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with_command('"/var/lib/puppet/letsencrypt/renew-foo.example.com.sh"').with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com' > /dev/null 2>&1\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com' > /dev/null 2>&1\n") }
       end
 
       context 'with manage cron and custom day of month' do
@@ -433,7 +433,7 @@ class { 'letsencrypt::plugin::dns_route53':
 
         it { is_expected.to compile.with_all_deps }
         it { is_expected.to contain_cron('letsencrypt renew cron foo.example.com').with(monthday: [1, 15]).with_ensure('present') }
-        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nexport VENV_PATH=/opt/letsencrypt/.venv\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
+        it { is_expected.to contain_file('/var/lib/puppet/letsencrypt/renew-foo.example.com.sh').with_ensure('file').with_content("#!/bin/sh\nletsencrypt --keep-until-expiring --text --agree-tos --non-interactive certonly --rsa-key-size 4096 -a standalone --cert-name 'foo.example.com' -d 'foo.example.com'\n") }
       end
 
       context 'with custom config_dir' do
diff --git a/templates/renew-script.sh.erb b/templates/renew-script.sh.erb
index 8ff4b80d..a77cde55 100644
--- a/templates/renew-script.sh.erb
+++ b/templates/renew-script.sh.erb
@@ -1,5 +1,5 @@
 #!/bin/sh
-<%- @execution_environment.each do |environment| -%>
+<%- @environment.each do |environment| -%>
 export <%= environment %>
 <%- end -%>
 <%= @cron_cmd %>

From 6aaa82d3f288dc7ee2bf0b5eefe110f1c41db861 Mon Sep 17 00:00:00 2001
From: Callum Banbery <callum.banbery@onthebeach.co.uk>
Date: Mon, 22 Mar 2021 11:04:21 +0000
Subject: [PATCH 2/3] Make sure vcs_dependencies cannot contain an empty string

Since the dependencies array is used to install packages it makes
sense to ensure a package name has been provided, an empty string
would be redundant as it would mean no package is downloaded.
---
 manifests/init.pp    | 2 +-
 manifests/install.pp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/manifests/init.pp b/manifests/init.pp
index 3337ce79..e7f8a291 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -74,7 +74,7 @@
   Boolean $manage_config                 = true,
   Boolean $manage_install                = true,
   Boolean $manage_dependencies           = true,
-  Array[String] $vcs_dependencies        = [],
+  Array[String[1]] $vcs_dependencies     = [],
   Enum['package', 'vcs'] $install_method = 'package',
   Boolean $agree_tos                     = true,
   Boolean $unsafe_registration           = false,
diff --git a/manifests/install.pp b/manifests/install.pp
index 89ec144a..b1646de0 100644
--- a/manifests/install.pp
+++ b/manifests/install.pp
@@ -21,7 +21,7 @@
   String $path                           = $letsencrypt::path,
   String $repo                           = $letsencrypt::repo,
   String $version                        = $letsencrypt::version,
-  Array[String] $vcs_dependencies        = $letsencrypt::vcs_dependencies,
+  Array[String[1]] $vcs_dependencies     = $letsencrypt::vcs_dependencies,
 ) {
   if $install_method == 'vcs' {
     if $manage_dependencies {

From dc8999f86ceaf53490cd08a4fb29ee6053891431 Mon Sep 17 00:00:00 2001
From: Callum Banbery <callum.banbery@onthebeach.co.uk>
Date: Mon, 22 Mar 2021 11:36:12 +0000
Subject: [PATCH 3/3] Restrict Exec initialize letsencrypt based on
 install_method

Since the letsencrpyt initialisation is only required when installing
from VCS it stands to reason to limit the resource declaration
based on the value of the parameter.
---
 manifests/init.pp                         | 19 ++++++++++++-------
 spec/classes/letsencrypt_spec.rb          |  7 ++++---
 spec/defines/letsencrypt_certonly_spec.rb |  1 -
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/manifests/init.pp b/manifests/init.pp
index e7f8a291..ab0496e4 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -105,17 +105,22 @@
 
   if $manage_config {
     contain letsencrypt::config # lint:ignore:relative_classname_inclusion
-    Class['letsencrypt::config'] -> Exec['initialize letsencrypt']
+
+    if $install_method == 'vcs' {
+      Class['letsencrypt::config'] -> Exec['initialize letsencrypt']
+    }
   }
 
   contain letsencrypt::renew
 
-  exec { 'initialize letsencrypt':
-    command     => 'python3 tools/venv3.py',
-    cwd         => $path,
-    path        => $facts['path'],
-    environment => $environment,
-    refreshonly => true,
+  if $install_method == 'vcs' {
+    exec { 'initialize letsencrypt':
+      command     => 'python3 tools/venv3.py',
+      cwd         => $path,
+      path        => $facts['path'],
+      environment => $environment,
+      refreshonly => true,
+    }
   }
 
   # Used in letsencrypt::certonly Exec["letsencrypt certonly ${title}"]
diff --git a/spec/classes/letsencrypt_spec.rb b/spec/classes/letsencrypt_spec.rb
index 0bc5533b..99fac956 100644
--- a/spec/classes/letsencrypt_spec.rb
+++ b/spec/classes/letsencrypt_spec.rb
@@ -34,8 +34,7 @@
                    repo: 'https://github.com/certbot/certbot.git',
                    version: 'v1.7.0').
               that_comes_before('Class[letsencrypt::renew]')
-            is_expected.to contain_exec('initialize letsencrypt')
-            is_expected.to contain_class('letsencrypt::config').that_comes_before('Exec[initialize letsencrypt]')
+            is_expected.to contain_class('letsencrypt::config')
             is_expected.to contain_class('letsencrypt::renew').
               with(pre_hook_commands: [],
                    post_hook_commands: [],
@@ -94,6 +93,8 @@
               is_expected.to contain_class('letsencrypt::install').with(install_method: 'vcs').
                 that_notifies('Exec[initialize letsencrypt]').
                 that_comes_before('Class[letsencrypt::renew]')
+              is_expected.to contain_exec('initialize letsencrypt')
+              is_expected.to contain_class('letsencrypt::config').that_comes_before('Exec[initialize letsencrypt]')
               is_expected.to contain_file('/etc/letsencrypt').with(ensure: 'directory')
             end
           end
@@ -107,7 +108,7 @@
         end
 
         describe 'with custom environment variables' do
-          let(:additional_params) { { environment: ['FOO=bar', 'FIZZ=buzz'] } }
+          let(:additional_params) { { install_method: 'vcs', environment: ['FOO=bar', 'FIZZ=buzz'] } }
 
           it { is_expected.to contain_exec('initialize letsencrypt').with_environment(['FOO=bar', 'FIZZ=buzz']) }
         end
diff --git a/spec/defines/letsencrypt_certonly_spec.rb b/spec/defines/letsencrypt_certonly_spec.rb
index b4e61bcc..b63a56f0 100644
--- a/spec/defines/letsencrypt_certonly_spec.rb
+++ b/spec/defines/letsencrypt_certonly_spec.rb
@@ -29,7 +29,6 @@
           it { is_expected.to contain_ini_setting('/etc/letsencrypt/cli.ini email foo@example.com') }
           it { is_expected.to contain_ini_setting('/etc/letsencrypt/cli.ini server https://acme-v02.api.letsencrypt.org/directory') }
         end
-        it { is_expected.to contain_exec('initialize letsencrypt') }
         it { is_expected.to contain_exec('letsencrypt certonly foo.example.com') }
         it { is_expected.to contain_exec('letsencrypt certonly foo.example.com').with_unless "/usr/local/sbin/letsencrypt-domain-validation #{pathprefix}/etc/letsencrypt/live/foo.example.com/cert.pem 'foo.example.com'" }
       end