Re: Proposal: Save user's original authenticated identity for logging

On Fri, Apr 02, 2021 at 12:03:21AM +0000, Jacob Champion wrote:
> On Thu, 2021-04-01 at 10:21 +0900, Michael Paquier wrote:
> > This stuff can take advantage of 0d1a3343, and I
> > think that we should make the kerberos, ldap, authentication and SSL
> > test suites just use connect_ok() and connect_fails() from
> > PostgresNode.pm. They just need to be extended a bit with a new
> > argument for the log pattern check.
>
> v16, attached, migrates all tests in those suites to connect_ok/fails
> (in the first two patches), and also adds the log pattern matching (in
> the final feature patch).

Thanks. I have been looking at 0001 and 0002, and found the addition
of %params to connect_ok() and connect_fails() confusing first, as
this is only required for the 12th test of 001_password.pl (failure to
grab a password for md5_role not located in a pgpass file with
PGPASSWORD not set). Instead of falling into a trap where the tests
could remain stuck, I think that we could just pass down -w from
connect_ok() and connect_fails() to PostgresNode::psql.

This change made also the parameter handling of the kerberos tests
more confusing on two points:
- PostgresNode::psql uses a query as an argument, so there was a mix
between the query passed down within the set of parameters, but then
removed from the list.
- PostgresNode::psql uses already -XAt so there is no need to define
it again.

> A since-v15 diff is attached, but it should be viewed with suspicion
> since I've rebased on top of the new SSL tests at the same time.

That did not seem that suspicious to me ;)

Anyway, after looking at 0003, the main patch, it becomes quite clear
that the need to match logs depending on like() or unlike() is much
more elegant once we have use of parameters in connect_ok() and
connect_fails(), but I think that it is a mistake to pass down blindly
the parameters to psql and delete some of them on the way while
keeping the others. The existing code of HEAD only requires a SQL
query or some expected stderr or stdout output, so let's make all
that parameterized first.

Attached is what I have come up with as the first building piece,
which is basically a combination of 0001 and 0002, except that I
modified things so as the number of arguments remains minimal for all
the routines. This avoids the manipulation of the list of parameters
passed down to PostgresNode::psql. The arguments for the optional
query, the expected stdout and stderr are part of the parameter set
(0001 was not doing that). For the main patch, this will need to be
extended with two more parameters in each routine: log_like and
log_unlike to match for the log patterns, handled as arrays of
regexes. That's what 0003 is basically doing already.

As a whole, this is a consolidation of its own, so let's apply this
part first.
--
Michael