Thread: Disabling src/test/[ssl|ldap] when not building with SSL/LDAP support
Hi all, In order to run tests consistently on the whole tree, I use a simple alias which tests also things like src/test/ssl and src/test/ldap on the way. Lately, I am getting annoyed by $subject when working on OpenSSL stuff as sometimes I need to test things with and without SSL support to make sure that a patch is rightly shaped. However if configure is built without SSL support then src/test/ssl just fails. The same applies to src/test/ldap. Could it be possible to disable them using something like the attached if a build is done without SSL and/or LDAP? Thanks, -- Michael
Attachment
Michael Paquier <michael.paquier@gmail.com> writes:
> In order to run tests consistently on the whole tree, I use a simple
> alias which tests also things like src/test/ssl and src/test/ldap on the
> way.
> Lately, I am getting annoyed by $subject when working on OpenSSL stuff
> as sometimes I need to test things with and without SSL support to make
> sure that a patch is rightly shaped. However if configure is built
> without SSL support then src/test/ssl just fails. The same applies to
> src/test/ldap.
> Could it be possible to disable them using something like the attached
> if a build is done without SSL and/or LDAP?
That seems like possibly not such a great idea. If somebody were using
a run of src/test/ssl to check their build, they would now get no
notification if they'd forgotten to build --with-openssl.
Perhaps we could introduce some new targets, "check-if-enabled" or so,
that act like you want.
regards, tom lane
On Thu, Feb 08, 2018 at 09:28:10AM -0500, Tom Lane wrote: > Michael Paquier <michael.paquier@gmail.com> writes: > That seems like possibly not such a great idea. If somebody were using > a run of src/test/ssl to check their build, they would now get no > notification if they'd forgotten to build --with-openssl. > > Perhaps we could introduce some new targets, "check-if-enabled" or so, > that act like you want. Per this argument, we need to do something even for check and installcheck anyway, no? Except that what you are suggesting is to make the tests fail instead of silently being bypassed. Copying an expression you used recently, this boils down to not spend CPU cycles for nothing. The TAP tests showing in red all over your screen don't show any useful information either as one may be building with SSL support, and still getting failures because he/she is working on an SSL-related feature. I prefer making the tests personally not fail, as when building without SSL one needs to move down to run ./configure again, so he likely knows what is is doing. Bypassing them also has the advantage to not do failure check dances, particularly in bash when using temporarily set +e/-e to avoid a problem, so this makes things easier for most cases. -- Michael
Attachment
On Sat, Feb 10, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Feb 08, 2018 at 09:28:10AM -0500, Tom Lane wrote: >> Michael Paquier <michael.paquier@gmail.com> writes: >> That seems like possibly not such a great idea. If somebody were using >> a run of src/test/ssl to check their build, they would now get no >> notification if they'd forgotten to build --with-openssl. >> >> Perhaps we could introduce some new targets, "check-if-enabled" or so, >> that act like you want. > > Per this argument, we need to do something even for check and > installcheck anyway, no? Except that what you are suggesting is to make > the tests fail instead of silently being bypassed. Copying an > expression you used recently, this boils down to not spend CPU cycles > for nothing. The TAP tests showing in red all over your screen don't > show any useful information either as one may be building with SSL > support, and still getting failures because he/she is working on an > SSL-related feature. > > I prefer making the tests personally not fail, as when building without > SSL one needs to move down to run ./configure again, so he likely knows > what is is doing. Bypassing them also has the advantage to not do > failure check dances, particularly in bash when using temporarily set > +e/-e to avoid a problem, so this makes things easier for most cases. One complication with the LDAP tests is that src/test/ldap/t/001_auth.pl has greater requirements than --with-ldap. For example, on a Debian system you probably only need the libldap2-dev package installed for --with-ldap to build, but 001_auth.pl also requires the slapd package to be installed (the OpenLDAP server). On some other systems including macOS and maybe AIX, a conforming LDAP client library (maybe OpenLDAP or maybe some other implementation) is part of the base system but I don't think slapd is. I agree that it would be nice if the build farm (and my unofficial patch tester for that matter) could automatically test the LDAP stuff when running on a suitable system, but I think it would need to be based not just on ifeq ($(with_ldap),yes) as you showed. I think the test would need a way to report that it can't find slapd so it can't run, but not consider that to be a hard failure. Or something like that... It might also be a good idea if you could tell 001_auth.pl where slapd and openssl (used by 001_auth.pl to make certificates) are through environment variables, in case anyone wants to run those tests against an installation other than one in the hardcoded paths we told it about for Linux, macOS (with Brew) and FreeBSD. -- Thomas Munro http://www.enterprisedb.com
On Sat, Feb 10, 2018 at 4:32 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > I agree that it would be nice if the build farm (and my unofficial > patch tester for that matter) could automatically test the LDAP stuff > when running on a suitable system, but I think it would need to be > based not just on ifeq ($(with_ldap),yes) as you showed. I think the > test would need a way to report that it can't find slapd so it can't > run, but not consider that to be a hard failure. Or something like > that... Hmm. I guess I just changed the subject a bit there to running those tests from check-world, if possible, by default. But as src/test/Makefile says, those test suites "are not secure to run on a multi-user system". You're talking about making the tests not fail if you tried to run them directly (not from check-world, but by direct invocation) when you didn't build with the right options. I take back what I said: it's probably better if you run those tests explicitly when you know you have the prerequisites installed and you're OK with the security implications (for example I should probably just do that on those Travis CI patch tester builds). Sorry for the noise. -- Thomas Munro http://www.enterprisedb.com
On Sat, Feb 10, 2018 at 04:44:38PM +1300, Thomas Munro wrote: > On Sat, Feb 10, 2018 at 4:32 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: >> I agree that it would be nice if the build farm (and my unofficial >> patch tester for that matter) could automatically test the LDAP stuff >> when running on a suitable system, but I think it would need to be >> based not just on ifeq ($(with_ldap),yes) as you showed. I think the >> test would need a way to report that it can't find slapd so it can't >> run, but not consider that to be a hard failure. Or something like >> that... > > Hmm. I guess I just changed the subject a bit there to running those > tests from check-world, if possible, by default. But as > src/test/Makefile says, those test suites "are not secure to run on a > multi-user system". Yeah, I am not advocating that. Note that the SSL tests could become part of the default if we have one day a way do so SSL with a Unix domain socket. There were some discussions a couple of years back but, if I recall correctly, the arguing has stuck about the way to shape the HBA entries for the configuration. There are also few use cases so the enthusiasm has not lasted long. > You're talking about making the tests not fail if > you tried to run them directly (not from check-world, but by direct > invocation) when you didn't build with the right options. I take back > what I said: it's probably better if you run those tests explicitly > when you know you have the prerequisites installed and you're OK with > the security implications (for example I should probably just do that > on those Travis CI patch tester builds). Sorry for the noise. Yes, you need to go into the test's paths to run them as well. The problem is also that the information provided by the TAP tests exploding because the build does not support those is pretty useless. It is true that you could filter things by using TestLib::check_pg_config, but do we really want to spend cycles for that when a Makefile check can do the job better and in a cheaper way? -- Michael
Attachment
On Sat, Feb 10, 2018 at 2:02 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > I agree that it would be nice if the build farm (and my unofficial > patch tester for that matter) could automatically test the LDAP stuff > when running on a suitable system, but I think it would need to be > based not just on ifeq ($(with_ldap),yes) as you showed. I think the > test would need a way to report that it can't find slapd so it can't > run, but not consider that to be a hard failure. Or something like > that... I think we could do that in the buildfarm by providing an extra_tap_tests config setting. It would be up to the owner to ensure that there was a suitable test environment available. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Disabling src/test/[ssl|ldap] when not building with SSL/LDAPsupport
From
Peter Eisentraut
Date:
I would like to see a global setting of some kind that specifies which extra tests are allowed to be run. This could be a configure argument or an environment/make variable. Rather than make it a list of tests, specify which facilities you have available, e.g., PG_EXTRA_TEST_FACILTITIES='tcpip openssl slapd' Then you could combine that with the actual build configuration to skip tests that the build doesn't support. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Feb 12, 2018 at 10:22:13PM -0500, Peter Eisentraut wrote: > I would like to see a global setting of some kind that specifies which > extra tests are allowed to be run. This could be a configure argument > or an environment/make variable. Rather than make it a list of tests, > specify which facilities you have available, e.g., > > PG_EXTRA_TEST_FACILTITIES='tcpip openssl slapd' > > Then you could combine that with the actual build configuration to skip > tests that the build doesn't support. Hm. Wouldn't it be enough to just spread the use of TestLib::check_pg_config and use SKIP on the tests where things cannot be run then? I see no need to invent an extra facility if all the control is already in pg_config.h. For slapd, you can actually just check if it can be executed in the PATH defined at the top of 001_auth.pl in $slapd, and skip the tests if the thing cannot be run. We have enough flexibility in perl and TAP to allow that to work reliably. -- Michael
Attachment
Re: Disabling src/test/[ssl|ldap] when not building with SSL/LDAPsupport
From
Peter Eisentraut
Date:
On 2/12/18 23:00, Michael Paquier wrote: > Hm. Wouldn't it be enough to just spread the use of > TestLib::check_pg_config and use SKIP on the tests where things cannot > be run then? I see no need to invent an extra facility if all the > control is already in pg_config.h. For slapd, you can actually just > check if it can be executed in the PATH defined at the top of > 001_auth.pl in $slapd, and skip the tests if the thing cannot be run. > We have enough flexibility in perl and TAP to allow that to work > reliably. Maybe that would work. We still need a way to configure whether we want to run tests that open TCP/IP listen sockets. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 16, 2018 at 03:32:46PM -0500, Peter Eisentraut wrote: > Maybe that would work. > > We still need a way to configure whether we want to run tests that open > TCP/IP listen sockets. For this an environment variable seems suited to me. Say if PG_TAP_ALLOW_INSECURE is set, then the tests said "insecure" are allowed to run. If the tests are tried to be run, then they are just skipped with a nice log message to tell you about it. The cool part about this design is that all the tests that are not part of check-world could be integrated in it. Most developers don't run regression tests on a shared resource. Let me think about a full-fledged patch first. Documentation also matters a lot. -- Michael
Attachment
On Sat, Feb 17, 2018 at 08:34:41AM +0900, Michael Paquier wrote: > For this an environment variable seems suited to me. Say if > PG_TAP_ALLOW_INSECURE is set, then the tests said "insecure" are allowed > to run. If the tests are tried to be run, then they are just skipped > with a nice log message to tell you about it. The cool part about this > design is that all the tests that are not part of check-world could be > integrated in it. Most developers don't run regression tests on a > shared resource. Let me think about a full-fledged patch first. > Documentation also matters a lot. Attached is what I have finished with. I have gathered the feedback from everybody on this thread and I think that the result can satisfy all the requirements mentioned: - 0001 is a small patch which makes the SSL and LDAP test suite fail immediately if the build's ./configure is not set up with necessary build options. This uses TestLib::check_pg_config to do the checks. - 0002 introduces a new environment variable which can be used to decide if an extra test suite can be used or not. I have named it PROVE_EXTRA_ALLOWED, and can be used as such: make -C src/test check PROVE_EXTRA_ALLOWED=3D'ssl ldap' This includes also some documentation. Note that with default settings the tests are skipped to keep things secure. 0002 is close to the logic mentioned by Peter E just upthread. To be more consistent with PROVE_TESTS and PROVE_FLAGS I also chose a prove variable as that's related to TAP at the end. I am of course open to better names. -- Michael
Attachment
Re: Disabling src/test/[ssl|ldap] when not building with SSL/LDAPsupport
From
Peter Eisentraut
Date:
On 2/17/18 08:48, Michael Paquier wrote: > Attached is what I have finished with. I have gathered the feedback > from everybody on this thread and I think that the result can satisfy > all the requirements mentioned: > - 0001 is a small patch which makes the SSL and LDAP test suite fail > immediately if the build's ./configure is not set up with necessary > build options. This uses TestLib::check_pg_config to do the checks. I'm not sure why we need that. The tests will presumably fail anyway. > - 0002 introduces a new environment variable which can be used to decide > if an extra test suite can be used or not. I have named it > PROVE_EXTRA_ALLOWED, and can be used as such: > make -C src/test check PROVE_EXTRA_ALLOWED=3D'ssl ldap' > This includes also some documentation. Note that with default settings > the tests are skipped to keep things secure. I think sticking this into the Perl code is too complicated and inflexible. We might want to use the same mechanism for non-TAP tests as well. What I had in mind would consist of something like this in src/test/Makefile: ifeq ($(with_ldap),yes) ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE))) SUBDIRS += ldap endif endif -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Feb 24, 2018 at 10:51:22AM -0500, Peter Eisentraut wrote:
> On 2/17/18 08:48, Michael Paquier wrote:
>> Attached is what I have finished with. I have gathered the feedback
>> from everybody on this thread and I think that the result can satisfy
>> all the requirements mentioned:
>> - 0001 is a small patch which makes the SSL and LDAP test suite fail
>> immediately if the build's ./configure is not set up with necessary
>> build options. This uses TestLib::check_pg_config to do the checks.
>
> I'm not sure why we need that. The tests will presumably fail anyway.
Sure. But then I think that it would be nice to show up on screen the
reason why the test failed if possible. As of now if SSL is missing the
whole run shows in red without providing much useful information.
Instead of 0001 as shaped previously, why not using as well diag to show
the failure on the screen?
For example the following block at the top of each test:
if (!check_pg_config("#define USE_OPENSSL 1"))
{
diag "SSL tests not supported without support in build";
die;
}
Would result in this output:
t/001_ssltests.pl .. # SSL tests not supported without support in build
# Looks like your test exited with 255 before it could output anything.
t/001_ssltests.pl .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 40/40 subtests
I would find that more helful to make the difference between an
implementation failure and a test failing because of a missing
dependency.
> I think sticking this into the Perl code is too complicated and
> inflexible. We might want to use the same mechanism for non-TAP tests
> as well.
>
> What I had in mind would consist of something like this in
> src/test/Makefile:
>
> ifeq ($(with_ldap),yes)
> ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE)))
> SUBDIRS += ldap
> endif
> endif
OK. So let's call it PG_TEST_EXTRA_INSECURE or PG_TEST_EXTRA, which can
take 'ldap', 'ssl' or 'ldap,ssl' as valid values. Seeing that
documented is really necessary in my opinion. Any idea for a better
name?
--
Michael
Attachment
Re: Disabling src/test/[ssl|ldap] when not building with SSL/LDAPsupport
From
Peter Eisentraut
Date:
On 2/24/18 18:29, Michael Paquier wrote:
> Sure. But then I think that it would be nice to show up on screen the
> reason why the test failed if possible. As of now if SSL is missing the
> whole run shows in red without providing much useful information.
> Instead of 0001 as shaped previously, why not using as well diag to show
> the failure on the screen?
>
> For example the following block at the top of each test:
> if (!check_pg_config("#define USE_OPENSSL 1"))
> {
> diag "SSL tests not supported without support in build";
> die;
> }
I think BAIL_OUT() is intended for this.
>> What I had in mind would consist of something like this in
>> src/test/Makefile:
>>
>> ifeq ($(with_ldap),yes)
>> ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE)))
>> SUBDIRS += ldap
>> endif
>> endif
>
> OK. So let's call it PG_TEST_EXTRA_INSECURE or PG_TEST_EXTRA, which can
> take 'ldap', 'ssl' or 'ldap,ssl' as valid values. Seeing that
> documented is really necessary in my opinion. Any idea for a better
> name?
I don't have a great idea about the name. The value should be
space-separated to work better with make functions.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Feb 28, 2018 at 10:02:37AM -0500, Peter Eisentraut wrote:
> On 2/24/18 18:29, Michael Paquier wrote:
>> Sure. But then I think that it would be nice to show up on screen the
>> reason why the test failed if possible. As of now if SSL is missing the
>> whole run shows in red without providing much useful information.
>> Instead of 0001 as shaped previously, why not using as well diag to show
>> the failure on the screen?
>>
>> For example the following block at the top of each test:
>> if (!check_pg_config("#define USE_OPENSSL 1"))
>> {
>> diag "SSL tests not supported without support in build";
>> die;
>> }
>
> I think BAIL_OUT() is intended for this.
That's a better idea. I have reworked that in 0001. What do you think?
This has the same effect as diag(), which shows information directly to
the screen, and that's the behavior I was looking for.
>>> What I had in mind would consist of something like this in
>>> src/test/Makefile:
>>>
>>> ifeq ($(with_ldap),yes)
>>> ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE)))
>>> SUBDIRS += ldap
>>> endif
>>> endif
>>
>> OK. So let's call it PG_TEST_EXTRA_INSECURE or PG_TEST_EXTRA, which can
>> take 'ldap', 'ssl' or 'ldap,ssl' as valid values. Seeing that
>> documented is really necessary in my opinion. Any idea for a better
>> name?
>
> I don't have a great idea about the name. The value should be
> space-separated to work better with make functions.
I have concluded about using the most simple name: PG_TEST_EXTRA. A
documentation attempt is added as well. This is unfortunately close to
EXTRA_TESTS. It would be tempting to directly reuse EXTRA_TESTS as
well, however this is used only for the core regression tests now, so a
separated variable seems adapted to me.
Thoughts and reviews are welcome.
--
Michael
Attachment
Re: Disabling src/test/[ssl|ldap] when not building with SSL/LDAPsupport
From
Peter Eisentraut
Date:
On 3/2/18 03:23, Michael Paquier wrote: > That's a better idea. I have reworked that in 0001. What do you think? > This has the same effect as diag(), which shows information directly to > the screen, and that's the behavior I was looking for. I ended up using plan skip_all, which seems to be intended for this purpose, according to the documentation. > I have concluded about using the most simple name: PG_TEST_EXTRA. A > documentation attempt is added as well. This is unfortunately close to > EXTRA_TESTS. It would be tempting to directly reuse EXTRA_TESTS as > well, however this is used only for the core regression tests now, so a > separated variable seems adapted to me. Committed. Very nice. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Mar 03, 2018 at 08:58:18AM -0500, Peter Eisentraut wrote: > On 3/2/18 03:23, Michael Paquier wrote: >> That's a better idea. I have reworked that in 0001. What do you think? >> This has the same effect as diag(), which shows information directly to >> the screen, and that's the behavior I was looking for. > > I ended up using plan skip_all, which seems to be intended for this > purpose, according to the documentation. Yes, that's what I got as well first but... >> I have concluded about using the most simple name: PG_TEST_EXTRA. A >> documentation attempt is added as well. This is unfortunately close to >> EXTRA_TESTS. It would be tempting to directly reuse EXTRA_TESTS as >> well, however this is used only for the core regression tests now, so a >> separated variable seems adapted to me. > > Committed. Very nice. Thanks for the commit, Peter, PG_TEST_EXTRA is already set is my environment. -- Michael