Subject: Re: [bug#75144] [PATCH] machine: Implement 'hetzner-environment-type'.

From: Ludovic Courtès <[email-masked]>
Date: Thu, 16 Jan 2025 22:26:18 +0100

Hello Roman,

Roman Scherer <[email-masked]> skribis:

> * gnu/machine/hetzner.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> * guix/ssh.scm (open-ssh-session): Add stricthostkeycheck option.
> * doc/guix.texi (Invoking guix deploy): Add documentation for
> 'hetzner-configuration'.
>
> Change-Id: Idc17dbc33279ecbf3cbfe2c53d7699140f8b9f41

Thumbs up for this big piece of work, one that I think is important for
the project!  ‘guix deploy’ is a great idea but it desperately needs
more backends like this one.

I’m not familiar with Hetzner so I’ll comment on more general aspects.
Chris, perhaps you can provide feedback on Hetzner-specific issues?  I
think we could put this backend to good use for Guix infra since a few
services are running at Hetzner.

> +@deftp {Data Type} hetzner-configuration
> +This is the data type describing the server that should be created for a
> +machine with an @code{environment} of @code{hetzner-environment-type}.

Could you add a sentence providing more context like:

  It allows you to configure deployment to a @acronym{VPS, virtual
  private server} hosted by @uref{https://www.hetzner.com, Hetzner}.

> +@item @code{authorize?} (default: @code{#t})
> +If true, the coordinator's public signing key

“coordinator” has nothing to do here I guess.

> +@item @code{labels} (default: @code{'()})
> +A user defined alist of key/value pairs attached to the server. Keys and
> +values must be strings. For more information, see
> +@uref{https://docs.hetzner.cloud/#labels, Labels}.

Maybe add a short example?

> +@item @code{location} (default: @code{"fsn1"})
> +The name of a @uref{https://docs.hetzner.com/cloud/general/locations,
> +location} to create the server in.

Maybe add: “For example, @code{"fsn1"} corresponds to the Hetzner site
in Falkenstein, Germany, while @code{"sin"} corresponds to its site in
Singapore.”

> +@item @code{server-type} (default: @code{"cx42"})
> +The name of the
> +@uref{https://docs.hetzner.com/cloud/servers/overview#server-types,
> +server type} this server should be created with.

Likewise, an example would be elcome.

> +@item @code{ssh-key}
> +The path to the SSH private key to use to authenticate with the remote
> +host.

s/path to/file name of/

> +The following example shows the definition of 2 machines that are

s/2/two/

> +vCPUs and 32 GB of RAM on the @code{aarch64} architecture, the second

s/@code{aarch64}/AArch64/

> +shared vCPUs and 32 GB of RAM on the @code{x86_64} architecture.

Drop @code.

> +@lisp
> +(use-modules (gnu machine)
> +             (gnu machine hetzner))
> +
> +(list (machine
> +       (operating-system %hetzner-os-arm)
> +       (environment hetzner-environment-type)
> +       (configuration (hetzner-configuration
> +                       (server-type "cax41")
> +                       (ssh-key "/home/charlie/.ssh/id_rsa"))))
> +      (machine
> +       (operating-system %hetzner-os-x86)
> +       (environment hetzner-environment-type)
> +       (configuration (hetzner-configuration
> +                       (server-type "cpx51")
> +                       (ssh-key "/home/charlie/.ssh/id_rsa")))))

Nice!

> +API key} should provision 2 machines for you.

s/2/two/

> +  #:use-module (ice-9 receive)

The code base preferable uses SRFI-71 for multiple-value returns.

> +      (raise (formatted-message
> +              (G_ "Expected a list of Hetzner API responses")))))

Messages should start with a lower-case letter (for all the messages in
this file).

Please add the file to ‘po/guix/POTFILES.in’ so that it’s actually
subject to translation.

> +(define (hetzner-api-response-read port)
> +  "Read the Hetzner API response from PORT."
> +  (let* ((response (read-response port))
> +         (body (read-response-body response)))
> +    (hetzner-api-response
> +     (body (json-string->scm (bytevector->string body "UTF-8")))

Just ‘string->utf8’ (shorter).

More importantly: instead of ‘json-string->scm’ (which gives an alist,
leading to ‘assoc-ref’ calls all over the code base along with free-form
alists, which is very error-prone), could you use ‘define-json-mapping’?

In essence it’s like ‘define-record-type’ but it additionally define how
to map a JSON dictionary to a Scheme record.  There are several examples
in Guix, such as (guix swh).

For clarity, it might be useful to move all the hetzner-api-* bits to a
separate module, for example (gnu machine hetzner http).  WDYT?

The rest of the code looks nice to me (modulo alists :-)) but that’s
about all I can say.  It’s quite a significant body of code.  What would
you suggest to prevent bitrot and support maintenance?  Are there parts
of it that could be usefully tested automatically, possibly by mocking
part of the Hetzner API?  Or are there tips on how you tested it that
could be written down in the file itself?

Could you move the (guix ssh) bits to a separate patch?

> +++ b/guix/ssh.scm
> @@ -103,7 +103,8 @@ (define* (open-ssh-session host #:key user port identity
>                             host-key
>                             (compression %compression)
>                             (timeout 3600)
> -                           (connection-timeout 10))
> +                           (connection-timeout 10)
> +                           (stricthostkeycheck #t))
>    "Open an SSH session for HOST and return it.  IDENTITY specifies the file
>  name of a private key to use for authenticating with the host.  When USER,
>  PORT, or IDENTITY are #f, use default values or whatever '~/.ssh/config'

Please update the docstring.

Rather ‘strict-host-key-check?’ to match naming conventions, even if
Guile-SSH calls it that way.

> @@ -137,7 +138,8 @@ (define* (open-ssh-session host #:key user port identity
>  
>                                 ;; Speed up RPCs by creating sockets with
>                                 ;; TCP_NODELAY.
> -                               #:nodelay #t)))
> +                               #:nodelay #t
> +                               #:stricthostkeycheck stricthostkeycheck)))

Not sure what this does actually.  Looks like the main part is the
“when stricthostkeycheck” condition that comes below, no?

Could you send a second version?

Thank you!

Ludo’.