Rework letsencrypt::certonly for #175

[cFire]

WIP: Need to test it and update documentation etc.

• Added $ensure attribute
• Renamed $ensure_cron back to $manage_cron
• Returned $manage_cron back to a Boolean value
• Made execute of "letsencrypt certonly ${title}" conditional to $ensure == 'present'
• Added cleanup of directory for domain certs when $ensure == 'absent'
• Used global $ensure to signal desired state of cronjobs

[Dan33l]

Tests were failing due to timeout. I relaunched them and now its ok.

[Dan33l]

@cFire can you add [WIP] (Work In Progress) in the title and remove it when you have finished the work ?

[cFire]

The reason I've removed the WIP from the title is because it's ready for review (as far as I'm concerned). Sorry I did not explicitly mention that.

[Rathios]

Did some testing to see what this'd do out of curiosity/concern. I'd personally look at running an exec to have certbot itself delete the certificate as rm -rf'ing the directory isn't quite sufficient to clean it all up. There are still files left behind in /etc/letsencrypt/renewal and /etc/letsencrypt/archive.

/etc/letsencrypt/renewal/certone.test.rawr.no.conf
/etc/letsencrypt/archive/certone.test.rawr.no

Certbot is unhappy about them as well:

$ certbot certificates
...
Renewal configuration file /etc/letsencrypt/renewal/certone.test.rawr.no.conf produced an unexpected error: expected /etc/letsencrypt/live/certone.test.rawr.no/cert.pem to be a symlink. Skipping.

Creating a new certificate again with the same name without cleaning up these files makes it append a -0001 to it, still leaving the old files behind and generating a certificate name the module can't clean up by itself.

What about something like this?

exec { "letsencrypt cleanup ${title}":
  command => "${letsencrypt_command} delete --cert-name ${title}",
  path    => $facts['path'],
  onlyif  => "test -d '${config_dir}/live/${live_path_domain}'",
}

[cFire]

That's a much better idea. I'll look into implementing that over the coming week.

[cFire]

While working on implementing the delete function I noticed this;

$live_path_domain = regsubst($domains[0], '^\*\.', '')
$live_path = "${config_dir}/live/${live_path_domain}/cert.pem"

I think this is wrong when using a custom name like letsencrypt::certonly { "special_snowflake_${domain}": [...] } and will cause puppet to try and renew all certs every run.

Can someone please confirm I'm not seeing things that aren't there?
I think it should be

$live_path = "${config_dir}/live/${title}/cert.pem"

[cFire]

Pull request is ready for another round of reviews. All tests are passing and it seems to work well in my environments.

[Rathios]

@cFire The reason ${title} is not used directly in the file path is because certbot generates the file path based on the first domain in the certificate (from $domains[0]). You can set the resource title independently from this, so if you pass both domains and a title, the path will probably be wrong. Certbot will also drop the wildcard if the first domain has one when generating the file path, eg. *.example.com gets stored in /etc/letsencrypt/live/example.com, thus the regsubst() to match that behavior.

[Rathios]

@cFire I'm sorry, you were right - the sneaky feeling got me testing and certbot does use --cert-name to generate the file path and not $domains[0] / the first -d. It's easy to assume either since in some scenarios $title and $domains[0] will have the same value. So something like this should sort it? (With a comment to save others the same mistake)

# certbot uses --cert-name to generate the file path
$live_path_certname = regsubst($title, '^\*\.', '')
$live_path = "${config_dir}/live/${live_path_certname}/cert.pem"

[Dan33l]

Nice. Looks pretty good IMO.
I added a question in inline comment.

[cFire]

I'm mildly confused about what just happened, but besides ci needing to be re-triggered it seems to be in a correct state again.

[Dan33l]

I'm mildly confused about what just happened, but besides ci needing to be re-triggered it seems to be in a correct state again.

I relaunched the travis ci.
Can you squash yours commits ?

[cFire]

Well, that somehow broke absolutely everything. Stand by...

[cFire]

Fixed again. Used the wrong rebase strategy before.

[Dan33l]

A backward incompatible change was introduced by PR #161 . This PR #177 introduce an other change that permits a come back of manage_cron but introduce a new parameter ensure.

And so this PR is also backward incompatible.

[Dan33l]

@cFire thank you for the job.