Re: Allow file inclusion in pg_hba and pg_ident files

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