bug#46961: Nginx and certbot cervices don't play well togther

["Clément Lassieur" <clement@lassieur.org>]

Removing guix-devel.

On Tue, Jan 30 2024, Carlo Zancanaro wrote:

> +            (define (file-contains? file string)
> +              (string-contains (call-with-input-file file
> +                                 get-string-all)
> +                               string))
> +
> +            (define (connection-error?)
> +              (file-contains? "/var/log/letsencrypt/letsencrypt.log"
> +                              "Failed to establish a new connection"))
> +
> +            (let ((script-code 0))
>                (for-each
>                 (match-lambda
>                   ((name . command)
>                    (begin
>                      (format #t "Acquiring or renewing certificate: ~a~%" name)

Here we could add ‘(force-output)’, because otherwise those logs arrive
after the certbot logs, and it's hard to understand anything.

> -                    (set! code (or (apply system* command) code)))))
> -               '#$commands) code)))))))
> +                    (unless (zero? (status:exit-val (apply system* command)))
> +                      ;; Certbot errors are always exit code 1, but we'd like
> +                      ;; to separate connection errors from other error types.
> +                      (if (connection-error?)
> +                          ;; If we have a connection error, then bail early
> +                          ;; with exit code 2. We don't expect this to
> +                          ;; resolve within the timespan of this script.

Could we have a (log + force-output) here too?  (I imagine within a
‘begin’)
> +                          (exit 2)
> +                          ;; If we have any other type of error, then continue
> +                          ;; but exit with a failing status code in the end.

and here?
> +                          (set! script-code 1))))))

And maybe a log also in case the command succeeds.  (So that would mean
to replace ‘unless’ with ‘if’).

> +               '#$commands)
> +              (exit script-code))))))))
>  
> +               (let loop ((attempt 0))
> +                 (let ((code (status:exit-val
> +                              (system* #$(certbot-command config)))))
> +                   (cond
> +                    ((and (= code 2)      ; Exit code 2 means connection error
> +                          (< attempt 12)) ; 12 * 10 seconds = 2 minutes
                                                                 ^------
This comment is not true because certbot takes time to execute (around
15s on my vm).  I don't think there is a need to be that precise.  Maybe
you can just add in in the let form, as in (let ((code ...)
                                                 (max-attempts 12)).

> +                     (sleep 10)
> +                     (loop (1+ attempt)))
> +                    ((zero? code)
> +                     ;; Success!
> +                     #t)
> +                    (else
> +                     ;; Failure.
> +                     #f))))))

Also could you update the example in the docs?

From the doc:
>> @defvar certbot-service-type
>> A service type for the @code{certbot} Let's Encrypt client.  Its value
>> must be a @code{certbot-configuration} record as in this example:
>> 
>> @lisp
>> (define %certbot-deploy-hook
>>   (program-file "certbot-deploy-hook.scm"
>>     (with-imported-modules '((gnu services herd))
>>       #~(begin
>>           (use-modules (gnu services herd))
>>           (with-shepherd-action 'nginx ('reload) result result)))))
                   ^
This part isn't useful anymore.  However, we could add a
nginx-service-type and a dhcp-client-service-type so that people have an
idea of what the minimal config is, maybe like I did in my first review:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46961#23.

>> (service certbot-service-type
>>          (certbot-configuration
>>           (email "foo@@example.net")
>>           (certificates
>>            (list
>>             (certificate-configuration
>>              (domains '("example.net" "www.example.net"))
>>              (deploy-hook %certbot-deploy-hook))
>>             (certificate-configuration
>>              (domains '("bar.example.net")))))))
>> @end lisp

We are almost there, thanks!
Clément