Hi Maxim,

On 4/25/25 3:27 PM, Maxim Cournoyer wrote:
> Hi,
>
> Giacomo Leidi<goodoldpaul@autistici.org> writes:
>
>> This commit adds a password-file to the postgresql-role field.  It
>> allows users to provision Postgres roles with a set password.
>>
>> * gnu/services/databases.scm (postgresql-role): Add password-file field;
>> (postgresql-role-configuration): add requirement field;
>> (postgresql-create-roles): add support for setting passwords from a
>> file without leaking passwords to the command line;
>> (postgresql-role-shepherd-service): add support for customizable
>> requirements.
>> * gnu/tests/databases.scm: Test it.
>> * doc/guix.texi: Document it.
> Looks useful!
>
> Nitpick: Each change can be a sentence, starting with a capitalized
> letter and ending with a period.
Should be fixed now.
>
>> Change-Id: I3aabaa10b0c5e826c5aa874e5649e25a3508a585
>> ---
>>   doc/guix.texi              | 15 +++++++++---
>>   gnu/services/databases.scm | 48 +++++++++++++++++++++++++++++++++-----
>>   gnu/tests/databases.scm    | 38 +++++++++++++++++++++++++++---
>>   3 files changed, 89 insertions(+), 12 deletions(-)
>>
>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index bee80cd4e2b..8b32557f76a 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -27588,9 +27588,10 @@ Database Services
>>   
>>   @lisp
>>   (service-extension postgresql-role-service-type
>> -                   (const (postgresql-role
>> -                           (name "alice")
>> -                           (create-database? #t))))
>> +                   (const (list
>> +                           (postgresql-role
>> +                            (name "alice")
>> +                            (create-database? #t)))))
> Are you sure this is correct?  If it's a bug fix, it's not listed in the
> changelog.

Yes I am sure, the service type has


(compose concatenate)


this is now mentioned in the changelog.

>>               postgresql-role?
>>               postgresql-role-name
>> +            postgresql-role-password-file
>>               postgresql-role-permissions
>>               postgresql-role-create-database?
>>               postgresql-role-configuration
>>               postgresql-role-configuration?
>>               postgresql-role-configuration-host
>> +            postgresql-role-configuration-requirement
>>               postgresql-role-configuration-roles
>>   
>>               postgresql-role-service-type
>> @@ -374,6 +378,8 @@ (define-record-type* <postgresql-role>
>>     postgresql-role make-postgresql-role
>>     postgresql-role?
>>     (name             postgresql-role-name) ;string
>> +  (password-file    postgresql-role-password-file  ;string
>> +                    (default #f))
>>     (permissions      postgresql-role-permissions
>>                       (default '(createdb login))) ;list
>>     (create-database? postgresql-role-create-database?  ;boolean
>> @@ -392,6 +398,8 @@ (define-record-type* <postgresql-role-configuration>
>>     postgresql-role-configuration?
>>     (host             postgresql-role-configuration-host ;string
>>                       (default "/var/run/postgresql"))
>> +  (requirement      postgresql-role-configuration-requirement ;list-of-symbols
>> +                    (default '()))
> as mentioned above, this should be 'shepherd-requirement'.
I know at least two examples that are already in master which use 
'requirement' (I may be biased as I authored both of them): the 
restic-backup-service-type and the oci-container-service-type. If you 
don't find this to be a blocker I would defer this change to a specific 
commit setting a convention for the whole codebase after an analysis of 
which services use simply 'requirement' and which use 
'shepherd-requirement'. Is it ok for you?
>>     (log              postgresql-role-configuration-log ;string
>>                       (default "/var/log/postgresql_roles.log"))
>>     (roles            postgresql-role-configuration-roles
>> @@ -409,19 +417,36 @@ (define (postgresql-create-roles config)
>>                                  permissions)
>>                      " ")))
>>   
>> +  (define (password-value role)
>> +    (string-append "password_" (postgresql-role-name role)))
>> +
>> +  (define (role->password-variable role)
>> +    (define file-name
>> +      (postgresql-role-password-file role))
> I'd maybe use a let here, just for stylistic preferences.
I changed the define to let
>
>> +    (if (string? file-name)
>> +        ;; This way passwords do not leak to the command line
> Please use complete sentences, including the trailing period, for
> stand-alone comments.
I added the period
>
>> +        #~(string-append "-v \"" #$(password-value role)
>> +                         "=$(" #+coreutils "/bin/cat " #$file-name ")\"")
> Are you sure this should use ungexp-native (#+) on coreutils?  If this
> runs in a snippet, it should use the regular ungexp (#$). It could also
> use file-append.  Only if it runs as part of a derivation build should
> it use #+.
this runs at system activation so in my understanding it should be #$ . 
Thank you for explaining the difference, I was not aware :)
>
>> +        ""))
>> +
>>     (define (roles->queries roles)
>>       (apply mixed-text-file "queries"
>>              (append-map
>>               (lambda (role)
>>                 (match-record role <postgresql-role>
>>                   (name permissions create-database? encoding collation ctype
>> -                      template)
>> +                      template password-file)
>>                   `("SELECT NOT(EXISTS(SELECT 1 FROM pg_catalog.pg_roles WHERE \
>>   rolname = '" ,name "')) as not_exists;\n"
>>   "\\gset\n"
>>   "\\if :not_exists\n"
>>   "CREATE ROLE \"" ,name "\""
>>   " WITH " ,(format-permissions permissions)
>> +,(if (and (string? password-file)
>> +          (not (string-null? password-file)))
> Since you already check for the string-null? case, perhaps the default
> password value should be "" instead of #f?  And then you could do away
> with the string? check (though ideally there could be a sanitizer to
> verify that a string is passed -- perhaps best leaving it for the day
> this gets ported to define-configuration, which auto-generates sanitizers
> based on the type specified for the field in its form.
in its ideal form (using define-configuration) this would be a 
'maybe-string' type, as it represents an optional path. if you don't 
find this a blocker I would leave this for a later change where we could 
use 'maybe-value-set?' and leave the type check to a sanitizer. What do 
you think?
>> +     (string-append
>> +      "\nPASSWORD :'" (password-value role) "'")
>> +     "")
>>   ";\n"
>>   ,@(if create-database?
>>         `("CREATE DATABASE \"" ,name "\""
>> @@ -436,20 +461,30 @@ (define (postgresql-create-roles config)
>>   
>>     (let ((host (postgresql-role-configuration-host config))
>>           (roles (postgresql-role-configuration-roles config)))
>> -    #~(let ((psql #$(file-append postgresql "/bin/psql")))
>> -        (list psql "-a" "-h" #$host "-f" #$(roles->queries roles)))))
>> +    (program-file "run-queries"
>> +      #~(let ((bash #$(file-append bash-minimal "/bin/bash"))
>> +              (psql #$(file-append postgresql "/bin/psql")))
>> +          (define command
>> +            (string-append
>> +             "set -e; exec " psql " -a -h " #$host " -f "
>> +             #$(roles->queries roles) " "
>> +             (string-join
>> +              (list
>> +               #$@(map role->password-variable roles))
>> +              " ")))
>> +          (execlp bash bash "-c" command)))))
>>   
>>   (define (postgresql-role-shepherd-service config)
>>     (match-record config <postgresql-role-configuration>
>> -    (log)
>> +    (log requirement)
>>       (list (shepherd-service
>> -           (requirement '(user-processes postgres))
>> +           (requirement `(user-processes postgres ,@requirement))
> I see two styles being used in Guix services: 'shepherd-requirement'
> extend the requirements, or overrides it, with the 'user-processes at
> least normally found in the default values.  I think at this point in
> time the later fits better in Guix (think of arguments such as #:modules
> and #:imported-modules, etc.  the user has full control on them, for the
> better and worst).
Thank you for pointing this out, I created a 
%postgresql-role-shepherd-requirement value and exported it to allow 
users which want to override the Guix defaults, while retaining them can 
simply append to this list.
>              
>>              (provision '(postgres-roles))
>>              (one-shot? #t)
>>              (start
>>               #~(lambda args
>>                   (zero? (spawn-command
>> -                        #$(postgresql-create-roles config)
>> +                        (list #$(postgresql-create-roles config))
> Why is this change now necessary?  I didn't follow.

the return value of postgresql-create-roles changed, before this change 
it returned a list containing a psql command line, after this change it 
returns a gexp which will evaluate to the entrypoint responsible for 
reading password files and then calling psql.

>
>>                           #:user "postgres"
>>                           #:group "postgres"
>>                           ;; XXX: As of Shepherd 1.0.2, #:log-file is not
>> @@ -468,6 +503,7 @@ (define postgresql-role-service-type
>>                             (match-record config <postgresql-role-configuration>
>>                               (host roles)
>>                               (postgresql-role-configuration
>> +                             (inherit config)
> and this one?  Was it a preexisting bug?
Yes, for example without this change we would lose the 'log' field's 
value upon extension.
>
>>                                (host host)
>>                                (roles (append roles extended-roles))))))
>>                   (default-value (postgresql-role-configuration))
>> diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm
>> index fd5041344b6..c5da603565d 100644
>> --- a/gnu/tests/databases.scm
>> +++ b/gnu/tests/databases.scm
>> @@ -142,6 +142,8 @@ (define %role-log-file
>>   
>>   (define %postgresql-os
>>     (simple-operating-system
>> +   (extra-special-file "/password"
>> +                       (plain-file "password" "hello"))
>>      (service postgresql-service-type
>>               (postgresql-configuration
>>                (postgresql postgresql)
>> @@ -158,6 +160,10 @@ (define %postgresql-os
>>                (roles
>>                 (list (postgresql-role
>>                        (name "root")
>> +                     (create-database? #t))
>> +                    (postgresql-role
>> +                     (name "alice")
>> +                     (password-file "/password")
>>                        (create-database? #t))))))))
>>   
>>   (define (run-postgresql-test)
>> @@ -230,14 +236,40 @@ (define (run-postgresql-test)
>>               (marionette-eval
>>                '(begin
>>                   (use-modules (gnu services herd)
>> +                             (srfi srfi-1)
>>                                (ice-9 popen))
>>                   (current-output-port
>>                    (open-file "/dev/console" "w0"))
>> +                (every
>> +                 (lambda (role)
>> +                   (let* ((port (open-pipe*
>> +                                 OPEN_READ
>> +                                 #$(file-append postgresql "/bin/psql")
>> +                                 "-tA" "-c"
>> +                                 (string-append
>> +                                  "SELECT 1 FROM pg_database WHERE"
>> +                                  " datname='" role "'")))
>> +                          (output (get-string-all port)))
>> +                     (close-pipe port)
>> +                     (string-contains output "1")))
>> +                 '("root" "alice")))
>> +             marionette))
>> +
>> +          (test-assert "database passwords are set"
>> +            (marionette-eval
>> +             '(begin
>> +                (use-modules (gnu services herd)
>> +                             (ice-9 match)
>> +                             (ice-9 popen))
>> +                (current-output-port
>> +                 (open-file "/dev/console" "w0"))
>> +                (setgid (passwd:gid (getpwnam "alice")))
>> +                (setuid (passwd:uid (getpw "alice")))
>> +                (setenv "PGPASSWORD" "hello")
>>                   (let* ((port (open-pipe*
>>                                 OPEN_READ
>> -                              #$(file-append postgresql "/bin/psql")
>> -                              "-tA" "-c" "SELECT 1 FROM pg_database WHERE
>> - datname='root'"))
>> +                              #$(file-append postgresql "/bin/psql") "-tA" "-c"
>> +                              "SELECT 1 FROM pg_database WHERE datname='alice'"))
> Nice to have tests!  Could you please send a v7 taking care of my
> comments?  Then I guess it'd be good to go.


I should have addressed all your comments, I'm about to send a v7. Thank 
you for your help!


cheers

giacomo