Thread: [PATCH] Find additional connection service files in pg_service.conf.d directory
[PATCH] Find additional connection service files in pg_service.conf.d directory
From
Curt Tilmes
Date:
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
Attachment
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Michael Paquier
Date:
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
Attachment
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Andres Freund
Date:
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
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Curt Tilmes
Date:
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
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Curt Tilmes
Date:
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
Attachment
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Andres Freund
Date:
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
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Curt Tilmes
Date:
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
Attachment
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
"Andreas 'ads' Scherbaum"
Date:
On Thu, Mar 1, 2018 at 7:40 PM, Curt Tilmes <curt@tilmes.org> wrote:
Looked over this patch and found that you limit the directory entries to files only.On Thu, Mar 1, 2018 at 1:13 PM, Andres Freund <andres@anarazel.de> wrote:
> I'd also insist that the file ending is ".conf".
New patch limits files to ".conf".
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.
Regards,
--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Curt Tilmes
Date:
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
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Arthur Zakirov
Date:
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
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Michael Paquier
Date:
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
Attachment
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Curt Tilmes
Date:
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
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Heikki Linnakangas
Date:
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
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Curt Tilmes
Date:
On Thu, Jul 19, 2018 at 5:39 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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.
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.
Is there a qsort() routine already accessible from that source file, or do we have to link in yet another sort?
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.
In my experience with other software, I've not seen that to be the case.. Individuals will use the single file in their home dir, while sysadmins and operations folks have much more call to split things into multiple files. I would rather keep things simple than add unnecessary complexity for its own sake.
Curt
Re: [PATCH] Find additional connection service files inpg_service.conf.d directory
From
Michael Paquier
Date:
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