Re: Allow file inclusion in pg_hba and pg_ident files - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: Allow file inclusion in pg_hba and pg_ident files |
Date | |
Msg-id | Y2IgaH5YzIq2b+iR@paquier.xyz Whole thread Raw |
In response to | Re: Allow file inclusion in pg_hba and pg_ident files (Julien Rouhaud <rjuju123@gmail.com>) |
Responses |
Re: Allow file inclusion in pg_hba and pg_ident files
|
List | pgsql-hackers |
On Fri, Oct 28, 2022 at 11:49:54AM +0800, Julien Rouhaud wrote: > To be honest I'd rather not to. It's excessively annoying to work on those > tests (I spent multiple days trying to make it as clean and readable as > possible), and splitting it to only test the current infrastructure will need > some substantial efforts. Well, I'd really like to split things as much as possible, beginning with some solid basics before extending its capabilities. This reduces the odds of introducing issues in-between, particularly in areas as sensible as authentication that involves not-yet-logged-in users. > But more importantly, the next commit that will add tests for file inclusion > will then be totally unmaintainable and unreadable, so that's IMO even worse. > I think it will probably either be the current file overwritten or a new one > written from scratch if some changes are done in the simplified test, and I'm > not volunteering to do that. Not sure about that either. Anyway, the last patch posted on this thread does not apply and the CF bot fails, so it needed a rebase. I have first noticed that your patch included some doc fixes independent of this thread, so I have applied that as e765028 and backpatched it down to 15. The TAP test needs to be renamed to 0004, and it was missing from meson.build, hence the CI was not testing it. I have spent some time starting at the logic today of the whole, and GetConfFilesInDir() is really the first thing that stood out. I am not sure that it makes much sense to keep that in guc-file.c, TBH, once we feed it into hba.c. Perhaps we could put the refactored routine (and AbsoluteConfigLocation as a side effect) into a new file in misc/? As of HEAD, tokenize_inc_file() is the place where we handle a list of tokens included with an '@' file, appending the existing set of AuthTokens into the list we are building by grabbing a copy of these before deleting the line memory context. Your patch proposes a different alternative, which is to pass down the memory context created in tokenize_auth_file() down to the callers with tokenize_file_with_context() dealing with all the internals. process_included_auth_file() is different extension of that. This may come down to a matter of taste, but could be be cleaner to take an approach similar to tokenize_inc_file() and just create a copy of the AuthToken list coming from a full file and append it to the result rather than passing around the memory context for the lines? This would lead to some simplifications, it seems, at least with the number of arguments passed down across the layers. The addition of a check for the depth in two places seems unnecessary, and it looks like we should do this kind of check in only one place. I have not tried yet, but if we actually move the AllocateFile() and FreeFile() calls within tokenize_auth_file(), it looks like we may be able to get to a simpler logic without the need of the with_context() flavor (even no process_included_auth_file required)? That could make the interface easier to follow as a whole, while limiting the presence of AllocateFile() and FreeFile() to a single code path, impacting open_inc_file() that relies on what the patch uses for SecondaryAuthFile and IncludedAuthFile (we could attempt to use the same error message everywhere as well, as one could expect that expanded and included files have different names which is enough to guess which one is an inclusion and which one is a secondary). Attached are two patches: the first one is a rebase of what you have posted, and the second one are some changes I did while playing with the logic. In the second one, except for conffiles.{h.c}, the changes are just a POC but that's to show the areas that I am planning to rework, and your tests pass with it. I still need to think about all that and reconsider the design of the interface that would fit with the tokenization of the inclusions, without that many subroutines to do the work as it makes the code harder to follow. Well, that's to say that I am not staying idle :) -- Michael
Attachment
pgsql-hackers by date: