Thread: Convert sepgsql tests to TAP
The sepgsql tests have not been integrated into the Meson build system yet. I propose to fix that here. One problem there was that the tests use a very custom construction where a top-level shell script internally calls make. I have converted this to a TAP script that does the preliminary checks and then calls pg_regress directly, without make. This seems to get the job done. Also, once you have your SELinux environment set up as required, the test now works fully automatically; you don't have to do any manual prep work. The whole thing is guarded by PG_TEST_EXTRA=sepgsql now. Some comments and questions: - Do we want to keep the old way to run the test? I don't know all the testing scenarios that people might be interested in, but of course it would also be good to cut down on the duplication in the test files. - Strangely, there was apparently so far no way to get to the build directory from a TAP script. They only ever want to read files from the source directory. So I had to add that. - If you go through the pre-test checks in contrib/sepgsql/test_sepgsql, I have converted most of these checks to the Perl script. Some of the checks are obsolete, because they check whether the database has been correctly initialized, which is now done by the TAP script anyway. One check that I wasn't sure about is the # 'psql' command must be executable from test domain The old test was checking the installation tree, which I guess could be set up in random ways. But do we need this kind of check if we are using a temporary installation? As mentioned in the patch, the documentation needs to be updated. This depends on the outcome of the question above whether we want to keep the old tests in some way.
Attachment
I took a quick look at the patch and I like that we standardize things a bit. But one thing I am not a fan of are all the use of sed and awk in the Perl script. I would prefer if that logic happened all in Perl, especially since we have some of it in Perl (e.g. chomp). Also I wonder if we should not use IPC::Run to do the tests since we already depend on it for the other TAP tests. I have not yet set up an VM with selinux to try the patch out for real but will do so later. On 5/13/24 8:16 AM, Peter Eisentraut wrote: > - Do we want to keep the old way to run the test? I don't know all the > testing scenarios that people might be interested in, but of course it > would also be good to cut down on the duplication in the test files. I cannot see why. Having two ways to run the tests seems only like a bad thing to me. > - If you go through the pre-test checks in contrib/sepgsql/test_sepgsql, > I have converted most of these checks to the Perl script. Some of the > checks are obsolete, because they check whether the database has been > correctly initialized, which is now done by the TAP script anyway. One > check that I wasn't sure about is the > > # 'psql' command must be executable from test domain > > The old test was checking the installation tree, which I guess could be > set up in random ways. But do we need this kind of check if we are > using a temporary installation? Yeah, that does not seem necessary. Andreas
On 7/24/24 4:31 PM, Andreas Karlsson wrote: > I have not yet set up an VM with selinux to try the patch out for real > but will do so later. I almost got the tests running but it required way too many manual steps to just get there and I gave up after just getting segfaults. I had to edit sepgsql-regtest.te because sepgsql-regtest.pp would not build otherwise on Debian bookworm, but after I had done that instead of getting test failures as I expected I just got segfaults. Maybe those are caused by an incorrect sepgsql-regtest.pp but this was not nice at all to try to get running for someone like me who does not know selinux well. Peter, what did you do to get the tests running? And should we fix these tests to make them more user friendly? Andreas
On 24.07.24 18:29, Andreas Karlsson wrote: > On 7/24/24 4:31 PM, Andreas Karlsson wrote: >> I have not yet set up an VM with selinux to try the patch out for real >> but will do so later. > > I almost got the tests running but it required way too many manual steps > to just get there and I gave up after just getting segfaults. I had to > edit sepgsql-regtest.te because sepgsql-regtest.pp would not build > otherwise on Debian bookworm, but after I had done that instead of > getting test failures as I expected I just got segfaults. Maybe those > are caused by an incorrect sepgsql-regtest.pp but this was not nice at > all to try to get running for someone like me who does not know selinux > well. > > Peter, what did you do to get the tests running? And should we fix these > tests to make them more user friendly? In my experience, the tests (both the old and the proposed new) only work on Red Hat-like platforms. I had also tried on Debian but decided that it won't work.
On 24.07.24 16:31, Andreas Karlsson wrote: > I took a quick look at the patch and I like that we standardize things a > bit. But one thing I am not a fan of are all the use of sed and awk in > the Perl script. I would prefer if that logic happened all in Perl, > especially since we have some of it in Perl (e.g. chomp). Also I wonder > if we should not use IPC::Run to do the tests since we already depend on > it for the other TAP tests. In principle yes, but here I tried not rewriting the tests too much but just port them to a newer environment. I think the adjustments you describe could be done as a second step. (I don't really have any expertise in sepgsql or selinux, I'm just doing this to reduce the dependency on makefiles for testing. So I'm trying to use as light a touch as possible.)
On 7/24/24 6:31 PM, Peter Eisentraut wrote: > On 24.07.24 18:29, Andreas Karlsson wrote: >> Peter, what did you do to get the tests running? And should we fix >> these tests to make them more user friendly? > > In my experience, the tests (both the old and the proposed new) only > work on Red Hat-like platforms. I had also tried on Debian but decided > that it won't work. Thanks, will try to run them on Rocky Linux when I have calmed down a bit. :) Andreas
Peter Eisentraut <peter@eisentraut.org> writes:
> In my experience, the tests (both the old and the proposed new) only
> work on Red Hat-like platforms. I had also tried on Debian but decided
> that it won't work.
Yeah, Red Hat is pretty much the only vendor that has pushed SELinux
far enough to be usable by non-wizards. I'm not surprised if there
are outright bugs in other distros' versions of it, as AFAIK
nobody else turns it on by default.
regards, tom lane
On 7/24/24 12:36, Tom Lane wrote: > Peter Eisentraut <peter@eisentraut.org> writes: >> In my experience, the tests (both the old and the proposed new) only >> work on Red Hat-like platforms. I had also tried on Debian but decided >> that it won't work. > > Yeah, Red Hat is pretty much the only vendor that has pushed SELinux > far enough to be usable by non-wizards. I'm not surprised if there > are outright bugs in other distros' versions of it, as AFAIK > nobody else turns it on by default. I tried some years ago to get it working on my Debian-derived Linux Mint desktop and gave up. I think SELinux is a really good tool on RHEL variants, but I don't think many people use it on anything else. As Tom says, perhaps there are a few wizards out there though... -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 7/24/24 6:33 PM, Peter Eisentraut wrote: > On 24.07.24 16:31, Andreas Karlsson wrote: >> I took a quick look at the patch and I like that we standardize things >> a bit. But one thing I am not a fan of are all the use of sed and awk >> in the Perl script. I would prefer if that logic happened all in Perl, >> especially since we have some of it in Perl (e.g. chomp). Also I >> wonder if we should not use IPC::Run to do the tests since we already >> depend on it for the other TAP tests. > > In principle yes, but here I tried not rewriting the tests too much but > just port them to a newer environment. I think the adjustments you > describe could be done as a second step. That reasoning makes a lot of sense and I am in agreement. Cleaning that up is best for another patch. And managed to get the tests running on Rocky Linux 9 with both autotools and meson and everything work as it should. So I have two comments: 1) As I said earlier I think we should remove the old code. 2) If we remove the old code I think the launcher script can be merged into the TAP test instead of being a separate shell script. But I am fine if you think that is also something for a separate commit. I like this kind of clean up patch. Good work! :) Andreas
Andreas Karlsson <andreas@proxel.se> writes:
> 1) As I said earlier I think we should remove the old code.
I agree that carrying two versions of the test doesn't seem great.
However, a large part of the purpose of test_sepgsql is to help
people debug their sepgsql setup, which is why it goes to great
lengths to print helpful error messages. I'm worried that making
it into a TAP test will degrade the usefulness of that, simply
because the TAP infrastructure is pretty damn unfriendly when it
comes to figuring out why a test failed. You have to know where
to even look for the test logfile, and then you have to ignore
a bunch of useless-to-you chatter. I'm not sure if there is much
we can do to improve that. (Although if we could, it would
yield benefits across the whole tree.)
OTOH, I suspect there are so few people using sepgsql that this
doesn't matter too much. Probably most of them will be advanced
hackers who won't blink at digging through a TAP log. We should
update the docs to explain that though.
regards, tom lane
On 7/24/24 10:35 PM, Tom Lane wrote: > Andreas Karlsson <andreas@proxel.se> writes: >> 1) As I said earlier I think we should remove the old code. > > I agree that carrying two versions of the test doesn't seem great. > However, a large part of the purpose of test_sepgsql is to help > people debug their sepgsql setup, which is why it goes to great > lengths to print helpful error messages. I'm worried that making > it into a TAP test will degrade the usefulness of that, simply > because the TAP infrastructure is pretty damn unfriendly when it > comes to figuring out why a test failed. You have to know where > to even look for the test logfile, and then you have to ignore > a bunch of useless-to-you chatter. I'm not sure if there is much > we can do to improve that. (Although if we could, it would > yield benefits across the whole tree.) For me personally the output from when running it with meson was good enough while the output when running with autotools was usable but annoying to work with. Meson's integration with TAP is pretty good. But with that said I am a power user and developer used to both meson and autotools. Unclear what skill we should expect from the target audience of test_sepgsql. Andreas