Re: code review

[Christopher Baines]

On Mon, 11 Sep 2017 22:10:22 +0200
Catonano <address@hidden> wrote:

> I could use some code review on my Trytond service hypothesis
> 
> As far as I understand, there are 2 steps that need to be done in
> order for a Trytond service to be usable.
> 
> 1) as the "postgres" role (that is as the operating system "postgres"
> user), create a "tryton" role
> 
> 2) as the tryton user (hence under the tryton role) run the Tryton
> initialization script (trytond-admin -c <config file> -d <database
> name> --all)
> 
> I feel like I should extend the postgresql service in order to create
> te trytond role but I don't know how

I don't think there is any current approach for doing this.

Service extension could be one way of doing it. It's similar to how
other services in Guix interact with each other. 

However, I believe creating a role can only be done when the PostgreSQL
server is running, which means that you'd need to defer creating the
role until that point. Keeping this in a single shepherd service might
not be easy to implement, and even if you did, there are some usability
issues, e.g. if you add a new service to the system, and that service
also extends PostgreSQL, then you might run in to trouble creating the
role for the new service, without needlessly restarting the PostgreSQL
service in the process.

One approach I've implemented and used [1] is to do create roles for
PostgreSQL when you start the service that uses that role.

I also remember Ludo suggesting using additional services to handle
this kind of setup, which I understood to be something like a
tryton-postgresql-role shepherd service, which the tryton shepherd
service would require.

1: https://github.com/alphagov/govuk-guix

> Also, I create the trytond role with no password. But what if someone
> wants to use this service for a real server ?
>
> The role password should be a parameter somehow. Again, I'm not sure
> how

It looks to be configurable at the moment, as someone using the service
can pass through there own value for the config-file field in the
<trytond-configuration>.

On the subject of database connections, again, when trying to make
something work with the govuk-guix project, I ended up making record
types for different database connections (e.g. [2]). This makes it easy
to work with as data, and then convert to a URI when you need one. I'd
be interested in seeing this in Guix, as I think it would be really
useful when trying to write services that use databases.

2:
https://github.com/alphagov/govuk-guix/blob/master/gds/services/utils/databases/postgresql.scm#L24
 
> I borrowed some code from the postgresql service code and somewhat
> edited it
> 
> But I feel like an amateur neurosurgeon :-/
> 
> I could really use a review of this code
> 
> Here it is
> 
> (define-module (gnu services trytond)
>   #:use-module (gnu services)
>   #:use-module (gnu services shepherd)
>   #:use-module (gnu system shadow)
>   #:use-module (gnu packages admin)
>  ;; do I really need to access the postgresql package, here ?
>   #:use-module (gnu packages databases)
>   #:use-module (gnu packages tryton)
>   #:use-module (guix modules)
>   #:use-module (guix records)
>   #:use-module (guix gexp)
>   #:use-module (ice-9 match)
>   #:export (trytond-configuration
>             trytond-configuration?
>             trytond-service
>             trytond-service-type
>             ))
> 
> ;;; Commentary:
> ;;;
> ;;; Trytond based services. Mainly Trytond and GNUHealth for now
> ;;;
> ;;; Code:
> 
> (define-record-type* <trytond-configuration>
>   trytond-configuration make-trytond-configuration
>   trytond-configuration?
>   (trytond     trytond-configuration-trytond ;<package>
>                (default trytond))
> ;; do I really need to access the postgresql package, here ?
>   (postgresql  postgresql-configuration-trytond
>                (default postgresql))
>   (locale         trytond-configuration-locale
>                   (default "en_US.utf8"))
>   (config-file    trytond-configuration-file)
>   (data-directory trytond-configuration-data-directory)
>   )
> 
> 
> (define %default-trytond-config
>   (mixed-text-file "trytond.conf"
>                    "[database]\n"
>                    ;; how do I connect with a role that has no
> password  ? ;; I create the trytond role without the password
>                    ;; but what if someone wants to use this service
> for a real server ?
>                    ;; the password should be a parameter, somehow
>                    ;;"uri = 'postgresql://trytond:password@/'\n"
>                    "uri = 'postgresql://trytond:@/'\n" ;; is this
> string gonna work ?
>                    "path = /var/lib/trytond"))
> 
> (define %trytond-accounts
>   (list (user-group (name "trytond") (system? #t))
>         (user-account
>          (name "trytond")
>          (group "trytond")
>          (system? #t)
>          (comment "Trytond server user")
>          (home-directory "/var/empty")
>          (shell (file-append shadow "/sbin/nologin")))))
> 
> (define trytond-activation
>   (match-lambda
>     (($ <trytond-configuration> trytond postgresql locale config-file
> data-directory)
>      #~(begin
>          (use-modules (guix build utils)
>                                 (gnu packages database)
>                                 (ice-9 match))
> 
>          (let ((trytond-user (getpwnam "trytond"))
>                (postgres-user (getpwnam "postgres"))
>                (create-the-trytond-role (string-append #$postgresql
>                                        "/bin/createuser"
>                                        "trytond"
>                                         "-d")) ;; the role can create
> new DBs
>                (run-the-trytond-init-script (string-append #$trytond
>                                       "/bin/trytond-admin"
>                                       "-c"
>                                       #$config-file
>                                       "-d"
>                                       "trytondb" ;;the database name
>                                       "--all"))
>                (trytond-initscript-args
>                 (append
>                  (if #$locale
>                      (list (string-append "-l " #$locale))
>                      '()))))
>            ;; Create data directory.
>            (mkdir-p #$data-directory)
>            (chown #$data-directory
>                   (passwd:uid trytond-user)
>                   (passwd:gid trytond-user))
> 
> 
>            ;; Drop privileges and create the tryton role in a new
>            ;; process.  Wait for it to finish before proceeding.
>            ;; shouldn't this be done by extending the postgresql
> service ? ;; but how ?
>            (match (primitive-fork)
>              (0
>               ;; Exit with a non-zero status code if an exception is
> thrown. (dynamic-wind
>                 (const #t)
>                 (lambda ()
>                   (setgid (passwd:gid postgres-user))
>                   (setuid (passwd:uid postgres-user))
>                   (primitive-exit
>                    (apply system*
>                           ;; shouldn't this be done by an extension
> to the postgresql service ?
>                           ;; but how ?
>                           create-the-trytond-role)))
>                 (lambda ()
>                   (primitive-exit 1))))
>              (pid (waitpid pid))))))))
> 
> 
> 
> 
>            ;; Drop privileges and run the trytond init script in a new
>            ;; process.  Wait for it to finish before proceeding.
>            (match (primitive-fork)
>              (0
>               ;; Exit with a non-zero status code if an exception is
> thrown. (dynamic-wind
>                 (const #t)
>                 (lambda ()
>                   (setgid (passwd:gid trytond-user))
>                   (setuid (passwd:uid trytond-user))
>                   (primitive-exit
>                    (apply system*
>                           run-the-trytond-init-script
>                           trytond-initscript-args)))
>                 (lambda ()
>                   (primitive-exit 1))))
>              (pid (waitpid pid))))))))

As discussed above, I wouldn't expect this to always work, as the
activation script might run before the PostgreSQL service has started.
I'm guessing it works for you though?

> (define trytond-shepherd-service
>   (match-lambda
>     (($ <trytond-configuration> trytond locale config-file)
>      (let ((start-script
>             ;; Wrapper script that switches to the 'trytond' user
> before ;; launching daemon.
>             (program-file "start-trytond"
>                           #~(let ((user (getpwnam "trytond"))
>                                   (trytond (string-append #$trytond
>                                                            "/bin/trytond")))
>                               (setgid (passwd:gid user))
>                               (setuid (passwd:uid user))
>                               (system* trytond
>                                        (string-append "-c "
>                                                       #$config-file))))))
>        (list (shepherd-service
>               (provision '(trytond))
>               (documentation "Run the Trytond daemon.")
>               (requirement '(user-processes loopback postgresql))
>               ;; why do I require the postgrresql service if I don't
> use it ?
>               (start #~(make-forkexec-constructor #$start-script))
>               (stop #~(make-kill-destructor))))))))
> 
> (define trytond-service-type
>   (service-type (name 'trytond)
>                 (extensions
>      ;; how is the postgresql service meant to be extended ?
>                  (list (service-extension shepherd-root-service-type
>                                           trytond-shepherd-service)
>                        (service-extension activation-service-type
>                                           trytond-activation)
>                        (service-extension account-service-type
>                                           (const
> %trytond-accounts))))))
> 
> (define* (trytond-service #:key (trytond trytond)
>                           (postgresql postgresql)
>                              ;;(port 5432) ;; The port is in the
> config file (locale "en_US.utf8")
>                           (config-file %default-trytond-config)
>                           ;;(data-directory "/var/lib/trytond/data")
>                           )
>   "Return a service that runs @var{trytond}, the Tryton server
> component.
> 
> The Trytond daemon loads its runtime configuration from
> @var{config-file} and relies on the PostgreSQL service for data
> storage." (service trytond-service-type
>            (trytond-configuration
>             (trytond trytond)
>             (postgresql postgresql) ;; sigh
>             ;;(port port)
>             (locale locale)
>             (config-file config-file)
>             ;;(data-directory data-directory)
>             )))

I'd remove trytond-service, in favour of giving the service-type a
default-value. There are some examples of services which do this.

All in all, awesome work :)

In case you're not already, I'd strongly suggest using system tests to
help develop this service, as I've found them useful when writing other
services.

pgpHs5MUXFIoX.pgp
Description: OpenPGP digital signature