From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Baines Subject: Re: code review Date: Tue, 12 Sep 2017 20:35:19 +0100 Message-ID: <20170912203519.31bb6943@cbaines.net> References: Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; boundary="Sig_/.R9Tk2QPQigYLrc_.UqXI/g"; protocol="application/pgp-signature" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:48352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1drqy2-0000wC-Gn for guix-devel@gnu.org; Tue, 12 Sep 2017 15:35:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1drqxz-0008Cz-88 for guix-devel@gnu.org; Tue, 12 Sep 2017 15:35:30 -0400 Received: from mira.cbaines.net ([2a01:7e00::f03c:91ff:fe69:8da9]:43555) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1drqxy-0008Ce-UQ for guix-devel@gnu.org; Tue, 12 Sep 2017 15:35:27 -0400 In-Reply-To: List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Catonano Cc: guix-devel --Sig_/.R9Tk2QPQigYLrc_.UqXI/g Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Mon, 11 Sep 2017 22:10:22 +0200 Catonano wrote: > I could use some code review on my Trytond service hypothesis >=20 > As far as I understand, there are 2 steps that need to be done in > order for a Trytond service to be usable. >=20 > 1) as the "postgres" role (that is as the operating system "postgres" > user), create a "tryton" role >=20 > 2) as the tryton user (hence under the tryton role) run the Tryton > initialization script (trytond-admin -c -d name> --all) >=20 > 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.=20 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 . 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/datab= ases/postgresql.scm#L24 =20 > I borrowed some code from the postgresql service code and somewhat > edited it >=20 > But I feel like an amateur neurosurgeon :-/ >=20 > I could really use a review of this code >=20 > Here it is >=20 > (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 > )) >=20 > ;;; Commentary: > ;;; > ;;; Trytond based services. Mainly Trytond and GNUHealth for now > ;;; > ;;; Code: >=20 > (define-record-type* > trytond-configuration make-trytond-configuration > trytond-configuration? > (trytond trytond-configuration-trytond ; > (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) > ) >=20 >=20 > (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 =3D 'postgresql://trytond:password@/'\n" > "uri =3D 'postgresql://trytond:@/'\n" ;; is this > string gonna work ? > "path =3D /var/lib/trytond")) >=20 > (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"))))) >=20 > (define trytond-activation > (match-lambda > (($ trytond postgresql locale config-file > data-directory) > #~(begin > (use-modules (guix build utils) > (gnu packages database) > (ice-9 match)) >=20 > (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)) >=20 >=20 > ;; 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)))))))) >=20 >=20 >=20 >=20 > ;; 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 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)))))))) >=20 > (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)))))) >=20 > (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. >=20 > 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. --Sig_/.R9Tk2QPQigYLrc_.UqXI/g Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAlm4NvdfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE 9XcB0A/+M76bIpVBK1Ep7zJSmIg5edUBU0RJjdz+v2WDBzpMxsB3XnQLZUv9HEu3 iE8tTExGbG+pdKfpfXkykCZbG5QbuuvoiCNhc+9CX3Wap/bQodcby0mcE0TsEtiH kFw3nH75ekIMh2ks5s7SRV0ezFpmwCPYnd65J+8v6tJ7wThyohV4ZPAixH2Vshif e/Ekfg0Gd7WsHbzGKbSc8G4rGrn5GQqNBQSoyy4CR2k2LRZ4/QrSM/SeLROvtb/U wcoPTaO2ikVJYFq2mTeHEjDXc5T3z4k7Vthq4yBsYOVqQCcnDUAymns0HW5X58aO s+ZvMX4Mm0ghZA9f0DERsrtGB01JpVCccRVeIBeMyWvUjqynrTC352aOKrZdapS1 ND7LdG1RJMh4w3h8S0IhyhYTFLY2T3B1Z1Y7DFcQBqVmovlm4KV1FLgnMv3loCIe s8Iuu7fMEv4EEdLJy0dZZFwBHCqd+7221Vsami9j7k+ctE+LFobBb/0QgI9FMTmd Z6xecOOnGbFVQqU7YZPS3FddV6OHU/iDYhJAKnu4i7WuApjvt82IvbgccX9FSFbT b9AulNi0C2ubWFNDDfxSTc3iMvCLHgyp7kIKFYkfr+FW9du+ukGFZJ36Q6KIPZnh h9/iFaUCKI8tUL0PdDoO8wz2UVUbBLrMfAgr0NsVU1Qr7H4XVMo= =LC7M -----END PGP SIGNATURE----- --Sig_/.R9Tk2QPQigYLrc_.UqXI/g--