Re: Allow file inclusion in pg_hba and pg_ident files

Hi,

On Tue, Jul 19, 2022 at 03:13:12PM +0900, Michael Paquier wrote:
> On Mon, Jul 18, 2022 at 03:11:51PM +0800, Julien Rouhaud wrote:
>
> > I'm not really sure what should be done here.  The best compromise I can think
> > of is to split the tests in 3 parts:
> >
> > 1) view reporting with various inclusions using safe_psql()
>
> You mean in the case where the HBA and indent files can be loaded,
> so as it is possible to peek at the system views without the
> EXEC_BACKEND problem, right?

Yes, testing the general behavior when there are no errors in the auth files.

> > 2) log error reporting
>
> This one should be reliable and stable enough by parsing the logs of
> the backend, thanks to connect_ok() and connect_fails().

I meant testing the postgres logs reporting, something like generating entirely
bogus files, restarting, checking that the start failed and verify that the
logs are expected.  With a variation that if only the pg_ident.conf is bogus,
the server will (and should) still start, so both needs to be tested
separately.

> > 3) view reporting with various inclusions errors, using safe_psql()
> >
> > And when testing 3), detect first if we can still connect after introducing
> > errors.  If not, assume this is Windows / EXEC_BACKEND and give up here without
> > reporting an error.  Otherwise continue, and fail the test if we later can't
> > connect anymore.  As discussed previously, detecting that the build is using
> > the fork emulation code path doesn't seem worthwhile so guessing it from the
> > psql error may be a better approach.
>
> Yeah, we could do that.  Now we may also fail on other patterns, so we
> would need to make sure that a set of expected error outputs are the
> ones generated?  I'd be fine to give up testing the error output
> generated in the system views at the end.  Without a persistent
> connection state with the same kind of APIs as any of the drivers able
> to do so, that's going to be a PITA to maintain.

Yes, or just checking that the error is expected on the psql side as the server
will report it.

I've been working on all of that and came up with the attached v8.

- 0001: I modified things as discussed previously to report the real auth file
  names rather than the hardcoded "pg_ident.conf" and "pg_hba.conf" in the
  various log messages
- 0002 and 0003 are minor fixes to error logs more consistent
- 0004: the rule_number / mapping_number addition in the views in a separate
  commit
- 0005: the main file inclusion patch.  Only a few minor bugfix since
  previous version discovered thanks to the tests (a bit more about it after),
  and documentation tweaks based on previous discussions
- 0006: the pg_hba_matches() POC, no changes

About the regression tests:
I added a new 003_file_inclusion.pl in src/test/authentication TAP tests to do
things as previously described, ie test both extisting behavior and new
features added, for working and non working scenarios.  The view error
reporting are bypassed for Windows / EXEC_BACKEND build by detecting a
connection failure with the expected error reported by the server.  If the
error doesn't match, the rest of the tests are still skipped but overall test
will fail due to the mismatch in the reported error.

It's a bit troublesome to write such tests, as you need to keep in sync the
various rule and line number, format things differently depending on whether
you want to check the logs or the view with each new tests added and so on.

To make the tests easier to write (and to maintain) I added some wrapper
functions to add lines in the wanted files that allow to automatically
generates the wanted regex (for the logs) or output (for the view).  There are
still things to be careful about and a bit of duplication, but this way I can
easily modify any test, or add new ones, without the need to modify everything
around.  As written, it also uses the same included files for the log error
reporting and the view error reporting, so there's no need to define everything
twice.

The tests work as expected locally on a normal build and an EXEC_BACKEND build,
and also on the CI.