Re: Brainstorming ideas for define-configuration

[Liliana Marie Prikler]

Hi,

Am Donnerstag, dem 09.03.2023 um 02:28 +0000 schrieb Bruno Victal:
> After spending some time with old and new Guix services, I'd like to
> suggest some potential improvements to our define-configuration
> macro:
> 
> 
> User-specified sanitizer support
> =====================================================================
> ==========
> 
> The sanitizers should be integrated with the type. Otherwise, they
> are tedious to use and appear verbose when repeatedly applied to
> multiple fields.
> 
> --8<---------------cut here---------------start------------->8---
> ;; Suggestion #1
> ;; The procedure could return a sanitized value. Upon failure, there
> are
> ;; the following options:
> ;;    - The procedure returns only a special value (akin to %unset-
> value)
> ;;      and an error message, as a pair.
> ;;      Exception raising is done by define-sanitized macro behind
> the scenes
> ;;      which makes the procedure easier to write.
> ;;    - The procedure raises an exception. There would be no
> consistency
> ;;      on the message formats, however, except for any agreed
> convention.
> ;;      This would involve some code duplication.
> ;; Cons: too specific, not extensible.
> 
> (define-sanitized typename procedure
>   (prefix ...))
> 
> 
> ;; Suggestion #2
> ;; A user-supplied procedure ('procname' below) would work just like
> the
> ;; procedure in option #1.
> ;; There is some similiarity to the Guix record-type*.
> ;; This could be extended more easily in the future should it be
> required.
> (define-type typename        ; maybe call this 'define-configuration-
> type' ?
>   (sanitizer procname)
>   (maybe-type? #t)
>   ;; The properties below are service specific.
>   ;; If this is implemented with Guix record-type* then we could have
> a
>   ;; module containing generic types and do something along the lines
> of:
>   ;; (define-type foo-ip-address
>   ;;    (inherit generic-ip-address)
>   ;;    (serializer ...))
>   (serializer procname)      ; define-type/no-serialization = sets
> this field to #f ?
>   (prefix ...))
> --8<---------------cut here---------------end--------------->8---
Rather than creating yet another syntax, I think we should instead
extend define-configuration to support this use-case in a backwards-
compatible manner.

As for error handling, there are basically two options:
1. Let the sanitizer raise any exception and do regular stack 
   unwinding.
   1b. Provide a source-location to the sanitizer so that it can use
       it in case of an error to provide better fix hints.
2. Let the sanitizer raise any exception, catch it and enrich it with
   the source-location.  Also possibly cross-check the resulting value
   so that e.g. #f is not used if it's not a maybe type or a boolean.

> The original motivation for this proposal stems from the attempts to
> resolve issue #61570. There, one potential fix was to handle the user
> and group fields similarly to the way greetd service handles them.
> There is some opportunity for generalization here, types that might
> be useful elsewhere, such as a port number or a host name, could be
> placed in a separate module.
I think you are blending several concerns together here, which is fine
insofar as sanitizers are powerful enough to mix those but perhaps not
the wisest idea from a software engineering perspective.  Coercing an
account name into a user account is a distinct operation from checking
whether an integer is indeed a port – the latter is a simple range
check that can already be performed with the existing framework.

> Record Validator
> =====================================================================
> ==========
> 
> There is also a need to validate records. Matching fields alone do
> not actually ensure that the configuration is coherent and usable.
> For example, some fields may be mutually incompatible with others.
I smell bad code ahead.

> We could provide procedures that validate each record type within
> define-configuration itself instead of validating the value at
> runtime (i.e. within the body of the service-type).
> 
> --8<---------------cut here---------------start------------->8---
> ;; the common case
> (define-configuration foo-configuration
>   (name
>    string
>    "Lorem ipsum...")
> 
>   ;; ...
> 
>   (validator procname))
> 
> ;; [bonus] Simpler configurations that only care for mutually-
> exclusive fields
> (define-configuration foo-configuration
>   (name
>    string
>    "Lorem ipsum...")
> 
>   (title
>    string
>    "Lorem ipsum..."
>    (conflicts 'name)))
> --8<---------------cut here---------------end--------------->8---
Instead of providing both a name field and a title field, you might
provide a field that can either be a name or a title or allow an even
more powerful value type as long as it makes sense.

> Comments regarding literal placement
> ---------------------------------------------------------------------
> ----------
> 
> Does the placement order matter for the extra fields/literals for
> define-configuration?  Can they be placed arbitrarily or only at the
> bottom? (where custom-serializer is specified)
To keep backwards compatibility, I'd use the place where custom-
serializer currently lies to add these new customizations, but allow
arbitrary order within them.

Using only sanitizer and serializer for the sake of simplicity, the
following forms would be allowed:

- (name type doc): Implicit serializer
- (name type doc serializer): Explicit serializer with deprecation
warning
- (name type doc (serialize serializer)): Explicit serializer, new form
- (name type doc (sanitize sanitizer)): Explicit sanitization, implicit
  serialization
- (name type doc (sanitize ...) (serialize ...)): Sanitization and 
  serialization explicit.

> Another point, extra parameters given to define-configuration should
> never clash with field names. For example, it should be possible to
> name a field 'prefix', 'location' or similar without causing issues
> like #59423.
> 
> 
> Coalesced documentation
> =====================================================================
> ==========
> 
> Currently, we manually edit the texinfo output from configuration-
> >documentation if we're unsatisfied with the generated result. For
> instance, substituting @item with an @itemx marker for fields whose
> documentation is similar.
> 
> Instead, we could embed hints in define-configuration that affect the
> texinfo generation in smarter ways.
> 
> In the long term, guix.texi should source some portions of the
> documentation directly from the code base. The current workflow
> involving copy and paste the output from the evaluation of
> configuration->documentation carries the unnecessary risk that future
> documentation patches are done against guix.texi rather than the .scm
> file from where it was generated. (issue #60582)
> 
> Snippet based on mympd-ip-acl (gnu/services/audio.scm):
> --8<---------------cut here---------------start------------->8---
> (define-configuration/no-serialization mympd-ip-acl
>   (allow
>    (list-of-string '())
>    "Allowed/Disallowed IP addresses.")
> 
>   (deny
>    (list-of-string '())
>    (documentation 'allow)))  ; group with field 'allow
> 
> ;;; --- texi output below ---
> 
> @c %start of fragment
> @deftp {Data Type} mympd-ip-acl
> Available @code{mympd-ip-acl} fields are:
> 
> @table @asis
> @item @code{allow}
> @itemx @code{deny} (default: @code{()}) (type: list-of-string)
> Allowed/Disallowed IP addresses.
> 
> @end table
> @end deftp
> @c %end of fragment
> --8<---------------cut here---------------end--------------->8---
Sounds good, but you'd have to take extra caution here to not end up in
weird situations.  Consider for example:

(define-configuration micromanaged-acl
  (allow
   (list-of-string '())
   "Allowed/Disallowed IP addresses.")

  (ask
   (list-of-string '())
   "IP addresses which are allowed only after manual confirmation.")

  (deny
   (list-of-string '())
   (documentation 'allow)))  ; group with field 'allow

> Serializer access to other fields
> =====================================================================
> ==========
> 
> Serialization procedures generally only have access to the values of
> its own field. That may be insufficient in some cases as whether a
> field can be serialized or how that is done, for example, can depend
> on the value of other fields.
That sounds like a very, very bad idea.  Instead of having values that
provide no complete information of their own, group them into records.

> mympd-service-type is one example where each serialized field depends
> on the value of another field. Our standard serializer procedures
> were useless for that case.
Well, I'd for one send a patch upstream to separate configuration from
whatever is going on in the "working directory" – that pattern's
hazardous as fuck already.  Other than that, it wouldn't even strictly
be necessary to depend on this or rather, you are actually abusing the
serialization feature to implement a service type that wasn't meant to
be implemented in this manner.

> As a side note, the 'this-record' macro does not work with
> define-configuration which made it impossible to use
> (custom-serializer) for the same effect.
> 
> Instead, serializer procedures could take additional keyword
> arguments:
> --8<---------------cut here---------------start------------->8---
> (define* (serialize-string field-name value (#:key config))
>    (let ((baz-value (assoc-ref config 'baz)))
>       (string-append baz-value "/" value)
>       ...))
> --8<---------------cut here---------------end--------------->8---
Note that for the mympd-case you'd have to repeat the same boilerplate
over and over, hardly making any improvement over how the service is
currently written.

> Inheritable record-type definition
> =====================================================================
> ==========
> 
> The openvpn-service pairs in (gnu services vpn) define a special
> macro define-split-configuration with the purpose to avoid code
> duplication since the service pairs have multiple fields in common.
> 
> Perhaps in a similar vein to SRFI-136, we could have:
> 
> --8<---------------cut here---------------start------------->8---
> (define-configuration openvpn-common-configuration
>   (openvpn
>     (file-like openvpn)
>     "The OpenVPN package.")
>   ;; ...
> )
> 
> (define-configuration openvpn-server-configuration openvpn-common-
> configuration
>   (tls-auth
>     (tls-auth-server #f)
>     "Add an additional layer of HMAC authentication on top of the TLS
> control
> channel to protect against DoS attacks.")
>   ;; ...
> )
> 
> ;;; or through a literal/keyword approach
> 
> (define-configuration openvpn-server-configuration
>   (tls-auth
>     (tls-auth-server #f)
>     "Add an additional layer of HMAC authentication on top of the TLS
> control
> channel to protect against DoS attacks.")
> 
>   (parent openvpn-common-configuration))
> 
> --8<---------------cut here---------------end--------------->8---
Note that with the keyword approach you can no longer name fields
"parent", which might not be what you intended.

> Generic serialize-configuration
> =====================================================================
> ==========
> 
> The procedure serialize-configuration inherently assumes that the
> serialized configuration must be a single string. This assumption
> needn't always hold, especially if the service in question is not a
> shepherd service.
> 
> We could possibly make this procedure a bit more "generic", maybe
> picking some ideas from SRFI-171 Transducers.
> 
> Another improvement to this procedure is to eliminate (or make
> optional) the second parameter, the configuration fields. It's
> unclear what value it brings since one can't use fields from another
> configuration type here.  An argument can be made for selectively
> serializing some of the fields but then it would be more practical to
> make this an optional parameter.
I think it's fine to assume the most common case and let the users
handle their own uncommon beasts.

> PS.
> =====================================================================
> ==========
> 
> Kind of a late realization, but couldn't many of the items above be
> satisfied by improvements to define-record-type* instead?
Isn't the point that started this the realization that define-record-
type* has a feature that's currently inaccessible through define-
configuration?  🙂️

Cheers