code review

code review

[Catonano]

[-- Attachment #1: Type: text/plain, Size: 8706 bytes --]

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

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

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))))))))

(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)
            )))

[-- Attachment #2: Type: text/html, Size: 12576 bytes --]

Re: code review

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 11669 bytes --]

On Mon, 11 Sep 2017 22:10:22 +0200
Catonano <catonano@gmail.com> 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.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

Re: code review

[Catonano]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

2017-09-12 21:35 GMT+02:00 Christopher Baines <mail@cbaines.net>:

> On Mon, 11 Sep 2017 22:10:22 +0200
> Catonano <catonano@gmail.com> wrote:
>


Hi Chris,

thank you for your review. And I apologize for the long delay.

I tried to follow some of your suggestions here
https://gitlab.com/humanitiesNerd/guix-hacks/blob/trytonservice/gnu/services/trytond.scm

I didn't try to implement the abstraction for the postgres connections

The postgres service right now uses the operating system user "postgres" in
order to connect under the "postgres" role, so avoiding to use a password

I did the same with the "tryton" operating system user and the "tryton" role

This assumes that the postgres service and thhe Tryton service are rnning
on the same server and this could be restrictive in some situations

Honestly I wasn't willing to wrap my mind around
I just want to have a minimum Tryton service running

Postgres commands for connecting safely to a remote postgres server

I just want to have a minimum Tryton service running

I hope this is ok

Anyway, now I should try to write some system tests or these 2 services I
wrote
Any patch would be welcome ;-)

Thanks

[-- Attachment #2: Type: text/html, Size: 1984 bytes --]

Re: code review

[Catonano]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

2017-09-12 21:35 GMT+02:00 Christopher Baines <mail@cbaines.net>:

> On Mon, 11 Sep 2017 22:10:22 +0200
> Catonano <catonano@gmail.com> 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.
>

Yes, I'll work on this approach from now on


>
> 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.
>

I attempted this but my trytond-postgres-role-service-type doesn't extend
shepherd-root-service-type

I chose not to extend shepherd-root-service-type because the trytond role
doesn't require a daemon running
It has to be created one off (una tantum) so, I tought, a simple activation
will do

But now, the trytond-service-type (in charge of running the trytond daemon)
should require trytond-postgres-role among its dependencies but there is NO
sheperd service provisioning a postgres role

So this is what I end up with (when testing the system with the marionette
machinery)

srfi/srfi-1.scm:640:9: In procedure for-each:
srfi/srfi-1.scm:640:9: Throw to key `srfi-34' with args `(#<condition
&message [message: "service 'trytond' requires 'trytond-postgres-role',
which is not provided by any service"] 44bd120>)'.
make: *** [Makefile:5295: check-system] Error 1


Should I make the trytond-postgres-role a sheperd service ?
Admittedly I don't like the idea

So, I'm thinking, I'll get back to creating the postgres role upon
activation of the trytond-service-type

I failed to foresee this issue so I wasted some work :-/

The branch is here, should anyone want to take a look (pay attention, I
rebase carelessly)
https://gitlab.com/humanitiesNerd/guix-hacks

[-- Attachment #2: Type: text/html, Size: 4215 bytes --]