Обсуждение: Add test case for sslinfo

On Mon, Jul 8, 2019 at 2:59 PM Hao Wu <hawu@pivotal.io> wrote:
> I see there is no test case for sslinfo. I have added a test case for it in my project.

Hi Hao Wu,

Thanks!  I see that you created a CF entry
https://commitfest.postgresql.org/24/2203/.  While I was scanning
through the current CF looking for trouble, this one popped in front
of my eyes, so here's some quick feedback even though it's in the next
CF:

+#!/bin/bash

I don't think we can require that script interpreter.

This failed[1] with permissions errors:

+cp: cannot create regular file '/server.crt': Permission denied

It looks like that's because the script assumes that PGDATA is set.

I wonder if we want to include more SSL certificates, or if we want to
use the same set of fixed certificates (currently under
src/test/ssl/ssl) for all tests like this.  I don't have a strong
opinion on that, but I wanted to mention that policy decision.  (There
is also a test somewhere that creates a new one on the fly.)

[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/555576601

-- 
Thomas Munro
https://enterprisedb.com

On Mon, Jul 08, 2019 at 02:11:34PM +0800, Hao Wu wrote:
> Thank you for your quick response! I work on greenplum, and I didn't see
> this folder(src/test/ssl/ssl) before.
> I will add more certificates to test and resend again.

Not having duplicates would be nice.

> Do you have any suggestion about the missing PGDATA? Since the test needs
> to configure postgresql.conf, maybe there are other ways to determine this
> environment.

+REGRESS = sslinfo
+REGRESS_OPT = --temp-config=$(top_srcdir)/contrib/sslinfo/sslinfo.conf

When it comes to custom configuration files in the regression tests,
you should always have NO_INSTALLCHECK = 1 in the Makefile because
there is no guarantee that that the running server will have the
configuration you want when running an installcheck.

+echo "preparing CRTs and KEYs"
+cp -f data/root.crt   $PGDATA/
+cp -f data/server.crt $PGDATA/
+cp -f data/server.key $PGDATA/
+chmod 400 $PGDATA/server.key
+chmod 644 $PGDATA/server.crt
+chmod 644 $PGDATA/root.crt
Using a TAP test here would be more adapted.  Another idea would be to
add that directly into src/test/ssl/ and enforce the installation of
with EXTRA_INSTALL when running the tests.

+-- start_ignore
+\! bash config.bash clean
+\! pg_ctl restart 2>&1 >/dev/null
+-- end_ignore
Please, no...
--
Michael

On 2019-07-08 10:18, Michael Paquier wrote:
> On Mon, Jul 08, 2019 at 02:11:34PM +0800, Hao Wu wrote:
>> Thank you for your quick response! I work on greenplum, and I didn't see
>> this folder(src/test/ssl/ssl) before.
>> I will add more certificates to test and resend again.
> 
> Not having duplicates would be nice.

I think sslinfo should be tested as an extension of src/test/ssl/
instead of its own test suite.  There are too many complications that we
would otherwise have to solve again.

You might want to review commit f60a0e96778854ed0b7fd4737488ba88022e47bd
and how it adds test cases.  You can't just hardcode a specific output
since different installations might report TLS 1.2 vs 1.3, different
ciphers etc.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hao Wu,

Are you submitting an updated version of this patch soon?

Thanks,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services