Обсуждение: [PATCH] Find additional connection service files in pg_service.conf.d directory

I love centralizing connection service definitions in
<sysconfdir>/pg_service.conf, but for a
large enterprise, sometimes we have multiple sets of connection
service definitions
independently managed.

The convention widely adopted for this type of thing is to allow
multiple config files to be in
a directory, usually the '.d' version of the config filename. See, for example:

    http://blog.siphos.be/2013/05/the-linux-d-approach/
    https://lists.debian.org/debian-devel/2010/04/msg00352.html

This patch adds an extra search for service connection definitions if
the current places fail
to find the service (~/.pg_service.conf <sysconfdir>/pg_service.conf).
It will then search
every readable file in the directory <sysconfdir>/pg_service.conf.d.

What do you think?

Curt

On Fri, Jan 12, 2018 at 09:53:51AM -0500, Curt Tilmes wrote:
> This patch adds an extra search for service connection definitions if
> the current places fail
> to find the service (~/.pg_service.conf <sysconfdir>/pg_service.conf).
> It will then search
> every readable file in the directory <sysconfdir>/pg_service.conf.d.
>
> What do you think?

If you are looking for feedback regarding your patch, please be sure to
register it to the next commit fest:
https://commitfest.postgresql.org/17/

Here are as well some guidelines about patch submission and review
process:
https://wiki.postgresql.org/wiki/Submitting_a_Patch
--
Michael

Hi,

On 2018-01-12 09:53:51 -0500, Curt Tilmes wrote:
> I love centralizing connection service definitions in
> <sysconfdir>/pg_service.conf, but for a
> large enterprise, sometimes we have multiple sets of connection
> service definitions
> independently managed.
> 
> The convention widely adopted for this type of thing is to allow
> multiple config files to be in
> a directory, usually the '.d' version of the config filename. See, for example:
> 
> What do you think?

Seems like a reasonable idea to me.


> @@ -4492,10 +4493,14 @@ parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
>  {
>      const char *service = conninfo_getval(options, "service");
>      char        serviceFile[MAXPGPATH];
> +    char        serviceDirPath[MAXPGPATH];
>      char       *env;
>      bool        group_found = false;
>      int            status;
>      struct stat stat_buf;
> +    DIR           *serviceDir;
> +    struct dirent *direntry;
> +
>  
>      /*
>       * We have to special-case the environment variable PGSERVICE here, since
> @@ -4539,21 +4544,45 @@ next_file:
>      snprintf(serviceFile, MAXPGPATH, "%s/pg_service.conf",
>               getenv("PGSYSCONFDIR") ? getenv("PGSYSCONFDIR") : SYSCONFDIR);
>      if (stat(serviceFile, &stat_buf) != 0)
> +        goto conf_dir;
> +
> +    status = parseServiceFile(serviceFile, service, options, errorMessage, &group_found);
> +    if (group_found || status != 0)
> +        return status;
> +
> +conf_dir:
> +

This is already pretty crufty, can't we make this look a bit prettier,
rather than extending this approach?


> +    /*
> +     * Try every file in pg_service.conf.d/*
> +     */
> +    snprintf(serviceDirPath, MAXPGPATH, "%s/pg_service.conf.d",
> +             getenv("PGSYSCONFDIR") ? getenv("PGSYSCONFDIR") : SYSCONFDIR);
> +
> +    if (stat(serviceDirPath, &stat_buf) != 0 || !S_ISDIR(stat_buf.st_mode) ||
> +        (serviceDir = opendir(serviceDirPath)) == NULL)
>          goto last_file;

This looks a bit ugly.


> -    status = parseServiceFile(serviceFile, service, options, errorMessage, &group_found);
> -    if (status != 0)
> -        return status;
> +    while ((direntry = readdir(serviceDir)) != NULL)

So there's no really well defined order in which we parse these?


> +    {
> +        snprintf(serviceFile, MAXPGPATH, "%s/%s", serviceDirPath, direntry->d_name);
> +

In my experience with such .conf.d directories it's very useful to
filter names not matching a common pattern. Otherwise you end up with
editor tempfiles and such being used, which gets confusing.


> +        if (stat(serviceFile, &stat_buf) != 0 || !S_ISREG(stat_buf.st_mode) ||
> +            access(serviceFile, R_OK))
> +            continue;

This doesn't look particularly pretty, but ...


Greetings,

Andres Freund

On Thu, Mar 1, 2018 at 3:53 AM, Andres Freund <andres@anarazel.de> wrote:
>
> On 2018-01-12 09:53:51 -0500, Curt Tilmes wrote:
> > The convention widely adopted for this type of thing is to allow
> > multiple config files to be in a directory, usually the '.d' version of the config filename.
> > What do you think?
>
> Seems like a reasonable idea to me.

Thank you for reviewing!

> This is already pretty crufty, can't we make this look a bit prettier,
> rather than extending this approach?

My goal was to match the surrounding code style, so I simply copied
existing lines. I
think most of your comments (crufty, ugly) stem from the existing
code, not the patch.  I also
wanted to minimize any impact on the existing well-tested, deployed
code, just adding this
tiny bit of new functionality.  I'd rather not rewrite the existing,
working code just to make it
pretty.

Do you have any specific suggestions?  Would it help if I separated
the new code into its own
subroutine?

> So there's no really well defined order in which we parse these?

These are after the existing homedir/sysconfdir, but yes, once we fall
down to the '.d' directory,
we just keep trying until we find the specified service, and fail if
we never find it.

> In my experience with such .conf.d directories it's very useful to
> filter names not matching a common pattern. Otherwise you end up with
> editor tempfiles and such being used, which gets confusing.

Suggestions?  I'll make it skip over files prefixed with '.'.
Anything else you suggest?

Thanks,
Curt

On Thu, Mar 1, 2018 at 11:36 AM, Curt Tilmes <curt@tilmes.org> wrote:
> Do you have any specific suggestions?  Would it help if I separated
> the new code into its own subroutine?

I broke the new directory search out into its own subroutine, so even
less impact on existing code.

>> In my experience with such .conf.d directories it's very useful to
>> filter names not matching a common pattern. Otherwise you end up with
>> editor tempfiles and such being used, which gets confusing.
>
> Suggestions?  I'll make it skip over files prefixed with '.'.
> Anything else you suggest?

New patch skips over '.' dotfiles.

Curt

Hi,

On 2018-03-01 13:04:35 -0500, Curt Tilmes wrote:
>On 2018-03-01 11:36:01 -0500, Curt Tilmes wrote:
>> > This is already pretty crufty, can't we make this look a bit prettier,
>> > rather than extending this approach?
>> 
>> My goal was to match the surrounding code style, so I simply copied
>> existing lines.

Yea, I think you're definitely to blame here. But I think just
continuing on the same bad trend isn't a good idea ;)


>> Do you have any specific suggestions?  Would it help if I separated
>> the new code into its own
>> subroutine?
> 
> I broke the new directory search out into its own subroutine, so even
> less impact on existing code.

I do think that helps!


>> > So there's no really well defined order in which we parse these?
>> 
>> These are after the existing homedir/sysconfdir, but yes, once we fall
>> down to the '.d' directory,
>> we just keep trying until we find the specified service, and fail if
>> we never find it.

And within the directory which service file wins will be decided by
filesystem internals. That makes me a bit uncomfortable, this very well
might not be stable.  I think it might not be terrible idea to sort the
directory and process alphabetically?

>> > In my experience with such .conf.d directories it's very useful to
>> > filter names not matching a common pattern. Otherwise you end up with
>> > editor tempfiles and such being used, which gets confusing.
>> 
>> Suggestions?  I'll make it skip over files prefixed with '.'.
>> Anything else you suggest?
>
> New patch skips over '.' dotfiles.

I'd also insist that the file ending is ".conf".


Thanks for the quick update,

Andres Freund

On Thu, Mar 1, 2018 at 1:13 PM, Andres Freund <andres@anarazel.de> wrote:
> And within the directory which service file wins will be decided by
> filesystem internals. That makes me a bit uncomfortable, this very well
> might not be stable.  I think it might not be terrible idea to sort the
> directory and process alphabetically?

My preference would be not to do this.  I added a note to the doc about
this.

> I'd also insist that the file ending is ".conf".

New patch limits files to ".conf".

Curt

On Mon, Mar 5, 2018 at 7:45 AM, Andreas 'ads' Scherbaum
<adsmail@wars-nicht.de> wrote:
> On Thu, Mar 1, 2018 at 7:40 PM, Curt Tilmes <curt@tilmes.org> wrote:
>> On Thu, Mar 1, 2018 at 1:13 PM, Andres Freund <andres@anarazel.de> wrote:
> Looked over this patch and found that you limit the directory entries to
> files only.
> Would it make sense to allow links as well? That would allow other
> programs/distributions to place a link in the config directory and point to
> a service file in their own directory.

I call stat(2), which from my understanding for a link will follow the
link and return
the information from the file linked to, not the link itself (which
lstat(2) would do).

I tried it out and it seems to work fine for links.

Curt

Hello,

On Thu, Mar 01, 2018 at 01:40:10PM -0500, Curt Tilmes wrote:
> New patch limits files to ".conf".

Isn't it worth to check errno for stat(), opendir() and readdir() calls?

I think when some error occured in searchServiceFileDirectory() then the
error "definition of service \"%s\" not found" will be raised, because
group_found is false. It may confuse as it hides a real error. For
example, if permission is denied to open the directory.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Mon, Jun 18, 2018 at 07:17:14PM +0300, Arthur Zakirov wrote:
> Isn't it worth to check errno for stat(), opendir() and readdir()
> calls?

Checking for such errors is mandatory.  There are some cases where there
are filters based on errno like ENOENT, but if something unexpected is
happening the user should know about it.  Of course this depends on the
feature discussed and its properties..
--
Michael

On Mon, Jun 18, 2018 at 12:17 PM Arthur Zakirov
<a.zakirov@postgrespro.ru> wrote:
> I think when some error occured in searchServiceFileDirectory() then the
> error "definition of service \"%s\" not found" will be raised, because
> group_found is false. It may confuse as it hides a real error. For
> example, if permission is denied to open the directory.

Correct.  The patch follows the model of the existing code.  It keeps
looking for the
correct service until it either finds it or doesn't.

If you want to temporarily hide your pg_service.conf.d directory or
files by removing
permissions, it will work fine, correctly skipping over them.  That is
not considered
an error.  If there is a real syntax error in the file, just as for
the other files, that will
be reported as a syntax error.

If after looking in all the right places it doesn't find the service,
it will correctly report
that the service was not found.

Curt

Handy feature!

On 01/03/18 20:40, Curt Tilmes wrote:
> On Thu, Mar 1, 2018 at 1:13 PM, Andres Freund <andres@anarazel.de> wrote:
>> And within the directory which service file wins will be decided by
>> filesystem internals. That makes me a bit uncomfortable, this very well
>> might not be stable.  I think it might not be terrible idea to sort the
>> directory and process alphabetically?
> 
> My preference would be not to do this.  I added a note to the doc about
> this.

I also think we should process the files in alphabetical order. That's 
how most software with conf.d directories work.

Imagine that you place "good.conf" and "bad.conf" in the config dir, and 
they both accidentally contain a service called "foobar". Everything 
seems to work fine, because the filesystem happens to return good.conf 
first. Then you take a backup of your system with "tar", and restore it 
on another system. On that system, "bad.conf" happens to be returned 
first, and it doesn't work. That's nightmare to debug.

Should we also search ~/.pg_service.conf.d ? And a PGSERVICEDIR env 
variable, to mirror PGSERVICEFILE. I don't think many people would use 
those, but I feel like it would be good to have consistency, so that 
wherever we search for "pg_service.conf" file, we also search for 
"pg_service.conf.d" directory.

- Heikki

On Mon, Aug 06, 2018 at 11:20:31AM -0400, Curt Tilmes wrote:
> On Thu, Jul 19, 2018 at 5:39 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> I also think we should process the files in alphabetical order. That's
>> how most software with conf.d directories work.
>>
>
> In my experience, this is not a problem with such directories.  Sysadmins
> and operations folks who map in such definitions understand how to do it
> right.

This thread has been waiting for a new patch for some time now, so I am
marking the patch as returned with feedback.  If you can provide a newer
version, please feel free to send one and update the commit fest
application accordingly.
--
Michael