[PATCH] Better cleanup in TLS tests for -13beta2
Hi,
This patch removes two temporary files that are not removed. In
Debian, repeated builds fail. We do not allow builds from modified
sources.
The first file was clearly an oversight. It was created separately. I
am not sure why the loop over @keys did not remove the second.
For the record, the error message from Debian is below. Lines 25-27
show the files that were left behind.
Kind regards,
Felix Lechner
lechner@4bba56c5a8a8:~/postgresql$ debuild
dpkg-buildpackage -us -uc -ui
dpkg-buildpackage: info: source package postgresql-13
dpkg-buildpackage: info: source version 13~beta2-1
dpkg-buildpackage: info: source distribution experimental
dpkg-buildpackage: info: source changed by Christoph Berg <myon@debian.org>
dpkg-source --before-build .
dpkg-buildpackage: info: host architecture amd64
fakeroot debian/rules clean
dh clean
debian/rules override_dh_auto_clean
make[1]: Entering directory '/home/lechner/postgresql'
rm -rf build
make[1]: Leaving directory '/home/lechner/postgresql'
dh_autoreconf_clean
dh_clean
dpkg-source -b .
dpkg-source: info: using source format '3.0 (quilt)'
dpkg-source: info: building postgresql-13 using existing
./postgresql-13_13~beta2.orig.tar.bz2
dpkg-source: info: using patch list from debian/patches/series
dpkg-source: warning: ignoring deletion of file configure, use
--include-removal to override
dpkg-source: warning: ignoring deletion of file
src/include/pg_config.h.in, use --include-removal to override
dpkg-source: warning: ignoring deletion of file
doc/src/sgml/man-stamp, use --include-removal to override
dpkg-source: warning: ignoring deletion of file
doc/src/sgml/html-stamp, use --include-removal to override
dpkg-source: info: local changes detected, the modified files are:
postgresql/src/test/ssl/ssl/client_tmp.key
postgresql/src/test/ssl/ssl/client_wrongperms_tmp.key
dpkg-source: error: aborting due to unexpected upstream changes, see
/tmp/postgresql-13_13~beta2-1.diff.gy3ajb
dpkg-source: info: you can integrate the local changes with dpkg-source --commit
dpkg-buildpackage: error: dpkg-source -b . subprocess returned exit status 2
debuild: fatal error at line 1182:
dpkg-buildpackage -us -uc -ui failed
Attachments:
cleanup-certificates.patchtext/x-patch; charset=US-ASCII; name=cleanup-certificates.patchDownload
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -544,6 +544,8 @@ test_connect_fails($common_connstr, "ssl
qr/SSL error/, "intermediate client certificate is missing");
# clean up
+unlink("ssl/client_wrongperms_tmp.key");
+unlink("ssl/client_tmp.key");
foreach my $key (@keys)
{
unlink("ssl/${key}_tmp.key");
On 29 Jun 2020, at 17:52, Felix Lechner <felix.lechner@lease-up.com> wrote:
This patch removes two temporary files that are not removed. In
Debian, repeated builds fail. We do not allow builds from modified
sources.
Aha, nice catch!
The first file was clearly an oversight. It was created separately. I
am not sure why the loop over @keys did not remove the second.
That's because it's created in the 002_scram.pl testsuite as well, but not
cleaned up there.
The proposed patch admittedly seems a bit like a hack, and the client_tmo.key
handling is wrong as mentioned above. I propose that we instead add the key to
the @keys array and have clean up handle all files uniformly. The attached
fixes both of the files.
cheers ./daniel
Attachments:
tls_tests_cleanup.patchapplication/octet-stream; name=tls_tests_cleanup.patch; x-unix-mode=0644Download
From bfd7f017594ed3d3a7ccd757f175d4eb9c7591c4 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 29 Jun 2020 20:59:41 +0200
Subject: [PATCH] Fix file removal in SSL tests
---
src/test/ssl/t/001_ssltests.pl | 4 +++-
src/test/ssl/t/002_scram.pl | 3 +++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index a454bb0274..7a18af70b7 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -52,9 +52,11 @@ foreach my $key (@keys)
# Also make a copy of that explicitly world-readable. We can't
# necessarily rely on the file in the source tree having those
-# permissions.
+# permissions. Also make sure to add to @keys to include it in
+# the clean up phase.
copy("ssl/client.key", "ssl/client_wrongperms_tmp.key");
chmod 0644, "ssl/client_wrongperms_tmp.key";
+push @keys, 'client_wrongperms';
#### Set up the server.
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index ee6e26d732..12a53a390c 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -99,4 +99,7 @@ test_connect_fails(
qr/channel binding required, but server authenticated client without channel binding/,
"Cert authentication and channel_binding=require");
+# clean up
+unlink("ssl/client_tmp.key");
+
done_testing($number_of_tests);
--
2.21.1 (Apple Git-122.3)
Daniel Gustafsson <daniel@yesql.se> writes:
The proposed patch admittedly seems a bit like a hack, and the client_tmo.key
handling is wrong as mentioned above. I propose that we instead add the key to
the @keys array and have clean up handle all files uniformly. The attached
fixes both of the files.
Hmm ... so I guess my reaction to both of these is "what guarantees
that we get to the part of the script that does the unlinks?".
I've certainly seen lots of TAP tests fail to complete. Could we
do the cleanup in an END block or the like? (I'm a poor enough
Perl programmer to be uncertain what's the best way, but I know
Perl has constructs like that.)
regards, tom lane
On 29 Jun 2020, at 21:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hmm ... so I guess my reaction to both of these is "what guarantees
that we get to the part of the script that does the unlinks?".
I've certainly seen lots of TAP tests fail to complete. Could we
do the cleanup in an END block or the like? (I'm a poor enough
Perl programmer to be uncertain what's the best way, but I know
Perl has constructs like that.)
If execution calls die() during testing, then we wont reach the clean up
portion at the end but we would if we did it as part of END which is (unless my
memory is too fogged) guaranteed to be the last code to run before the
interpreter exits.
That being said, we do retain temporary files on such failures on purpose in
our TestLib since 88802e068017bee8cea7a5502a712794e761c7b5 and a few follow-up
commits since, should these be handled differently? They are admittedly less
"unknown" as compared to other files as they are copies, but famous last words
have been spoken about bugs that can never happen.
cheers ./daniel
Daniel Gustafsson <daniel@yesql.se> writes:
On 29 Jun 2020, at 21:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hmm ... so I guess my reaction to both of these is "what guarantees
that we get to the part of the script that does the unlinks?".
That being said, we do retain temporary files on such failures on purpose in
our TestLib since 88802e068017bee8cea7a5502a712794e761c7b5 and a few follow-up
commits since, should these be handled differently? They are admittedly less
"unknown" as compared to other files as they are copies, but famous last words
have been spoken about bugs that can never happen.
Oh, good point. Objection withdrawn.
regards, tom lane
On Mon, Jun 29, 2020 at 03:51:48PM -0400, Tom Lane wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
That being said, we do retain temporary files on such failures on purpose in
our TestLib since 88802e068017bee8cea7a5502a712794e761c7b5 and a few follow-up
commits since, should these be handled differently? They are admittedly less
"unknown" as compared to other files as they are copies, but famous last words
have been spoken about bugs that can never happen.Oh, good point. Objection withdrawn.
I looked at the patch, and can confirm that client_wrongperms_tmp.key
remains around after running 001_ssltests.pl, and client_tmp.key after
running 002_scram.pl. The way the patch does its cleanup looks fine
to me, so I'll apply and backpatch where necessary, if there are no
objections of course.
--
Michael
On Tue, Jun 30, 2020 at 01:13:39PM +0900, Michael Paquier wrote:
I looked at the patch, and can confirm that client_wrongperms_tmp.key
remains around after running 001_ssltests.pl, and client_tmp.key after
running 002_scram.pl. The way the patch does its cleanup looks fine
to me, so I'll apply and backpatch where necessary, if there are no
objections of course.
I found one problem when testing with parallel jobs once we apply this
patch (say PROVE_FLAGS="-j 4"): the tests of 001 and 002 had the idea
to use the same file name client_tmp.key, so it was possible to easily
fail the tests if for example 002 removes the temporary client key
copy that 001 needs, or vice-versa. 001 takes longer than 002, so the
removal would likely be done by the latter, not the former. And it
was even logically possible to fail in the case where 001 removes the
file and 002 needs it, though very unlikely because 002 needs this
file for a very short amount of time and one test case. I have fixed
this issue by just making 002 use a different file name, as we do in
001 for the case of the wrong permissions, and applied the patch down
to 13.
--
Michael