From dfd3b4eed4710566e267310daf3c1c35c33b6922 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Wed, 23 Apr 2025 08:35:06 +0200 Subject: [PATCH 1/5] Remove tests that can easily be done by pg_regrss Some things tested in 001_basic.pl can easily be tested by pg_regress, so let's remove them from the TAP test. - Checking the extension version - Verify that tde_heap tables can't be created without principal key set - Select from table right after insert - Dropping a tde_heap table - Dropping the extension --- contrib/pg_tde/t/001_basic.pl | 18 ------------------ contrib/pg_tde/t/expected/001_basic.out | 20 -------------------- 2 files changed, 38 deletions(-) diff --git a/contrib/pg_tde/t/001_basic.pl b/contrib/pg_tde/t/001_basic.pl index c9619b104e9f2..c8f3f3067752f 100644 --- a/contrib/pg_tde/t/001_basic.pl +++ b/contrib/pg_tde/t/001_basic.pl @@ -16,17 +16,6 @@ PGTDE::psql($node, 'postgres', 'CREATE EXTENSION IF NOT EXISTS pg_tde;'); -PGTDE::psql($node, 'postgres', - 'SELECT extname, extversion FROM pg_extension WHERE extname = \'pg_tde\';' -); - -PGTDE::psql($node, 'postgres', - 'CREATE TABLE test_enc(id SERIAL,k INTEGER,PRIMARY KEY (id)) USING tde_heap;' -); - -PGTDE::append_to_result_file("-- server restart"); -$node->restart; - PGTDE::psql($node, 'postgres', "SELECT pg_tde_add_database_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');" ); @@ -42,9 +31,6 @@ PGTDE::psql($node, 'postgres', 'INSERT INTO test_enc (k) VALUES (\'foobar\'),(\'barfoo\');'); -PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id ASC;'); - -PGTDE::append_to_result_file("-- server restart"); $node->restart; PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id ASC;'); @@ -63,10 +49,6 @@ $strings .= `strings $tablefile | grep foo`; PGTDE::append_to_result_file($strings); -PGTDE::psql($node, 'postgres', 'DROP TABLE test_enc;'); - -PGTDE::psql($node, 'postgres', 'DROP EXTENSION pg_tde;'); - $node->stop; # Compare the expected and out file diff --git a/contrib/pg_tde/t/expected/001_basic.out b/contrib/pg_tde/t/expected/001_basic.out index c1e385741b99f..c8d7a974ae603 100644 --- a/contrib/pg_tde/t/expected/001_basic.out +++ b/contrib/pg_tde/t/expected/001_basic.out @@ -1,14 +1,4 @@ CREATE EXTENSION IF NOT EXISTS pg_tde; -SELECT extname, extversion FROM pg_extension WHERE extname = 'pg_tde'; - extname | extversion ----------+------------ - pg_tde | 1.0-rc -(1 row) - -CREATE TABLE test_enc(id SERIAL,k INTEGER,PRIMARY KEY (id)) USING tde_heap; -psql::1: ERROR: principal key not configured -HINT: create one using pg_tde_set_key before using encrypted tables --- server restart SELECT pg_tde_add_database_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per'); pg_tde_add_database_key_provider_file --------------------------------------- @@ -30,16 +20,6 @@ SELECT * FROM test_enc ORDER BY id ASC; 2 | barfoo (2 rows) --- server restart -SELECT * FROM test_enc ORDER BY id ASC; - id | k -----+-------- - 1 | foobar - 2 | barfoo -(2 rows) - TABLEFILE FOUND: yes CONTAINS FOO (should be empty): -DROP TABLE test_enc; -DROP EXTENSION pg_tde; From 21dc7e2ad1263235f1310d4aa5d399353533626e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Wed, 23 Apr 2025 08:47:06 +0200 Subject: [PATCH 2/5] Do not assert that test scaffolding works Assume that everything that isn't the test subject works and don't assert that it does. These things can easily be tested by pg_regress. Also prefer q{} over '' for SQL strings as that will keep escaping to a minimum and make it easier to read. --- contrib/pg_tde/t/001_basic.pl | 22 +++++++++------------- contrib/pg_tde/t/expected/001_basic.out | 15 --------------- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/contrib/pg_tde/t/001_basic.pl b/contrib/pg_tde/t/001_basic.pl index c8f3f3067752f..38382675e0721 100644 --- a/contrib/pg_tde/t/001_basic.pl +++ b/contrib/pg_tde/t/001_basic.pl @@ -14,22 +14,18 @@ $node->append_conf('postgresql.conf', "shared_preload_libraries = 'pg_tde'"); $node->start; -PGTDE::psql($node, 'postgres', 'CREATE EXTENSION IF NOT EXISTS pg_tde;'); - -PGTDE::psql($node, 'postgres', - "SELECT pg_tde_add_database_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');" +$node->safe_psql('postgres', q{CREATE EXTENSION IF NOT EXISTS pg_tde}); +$node->safe_psql('postgres', + q{SELECT pg_tde_add_database_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per')} ); - -PGTDE::psql($node, 'postgres', - "SELECT pg_tde_set_key_using_database_key_provider('test-db-key','file-vault');" +$node->safe_psql('postgres', + q{SELECT pg_tde_set_key_using_database_key_provider('test-db-key','file-vault')} ); - -PGTDE::psql($node, 'postgres', - 'CREATE TABLE test_enc(id SERIAL,k VARCHAR(32),PRIMARY KEY (id)) USING tde_heap;' +$node->safe_psql('postgres', + q{CREATE TABLE test_enc(id SERIAL,k VARCHAR(32),PRIMARY KEY (id)) USING tde_heap} ); - -PGTDE::psql($node, 'postgres', - 'INSERT INTO test_enc (k) VALUES (\'foobar\'),(\'barfoo\');'); +$node->safe_psql('postgres', + q{INSERT INTO test_enc (k) VALUES ('foobar'),('barfoo')}); $node->restart; diff --git a/contrib/pg_tde/t/expected/001_basic.out b/contrib/pg_tde/t/expected/001_basic.out index c8d7a974ae603..edb7188ea4dc5 100644 --- a/contrib/pg_tde/t/expected/001_basic.out +++ b/contrib/pg_tde/t/expected/001_basic.out @@ -1,18 +1,3 @@ -CREATE EXTENSION IF NOT EXISTS pg_tde; -SELECT pg_tde_add_database_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per'); - pg_tde_add_database_key_provider_file ---------------------------------------- - 1 -(1 row) - -SELECT pg_tde_set_key_using_database_key_provider('test-db-key','file-vault'); - pg_tde_set_key_using_database_key_provider --------------------------------------------- - -(1 row) - -CREATE TABLE test_enc(id SERIAL,k VARCHAR(32),PRIMARY KEY (id)) USING tde_heap; -INSERT INTO test_enc (k) VALUES ('foobar'),('barfoo'); SELECT * FROM test_enc ORDER BY id ASC; id | k ----+-------- From b94a41fc785a36d090794d3117651a2f35c04558 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Wed, 23 Apr 2025 08:59:11 +0200 Subject: [PATCH 3/5] Use Test::More assertions Instead of our home brewed assertion system we use regular Test::More assertions as these will document what is being tested and give a comprehensive error message if a test fail. Also use the "standard" test scrip pre-amble from src/test/perl/README --- contrib/pg_tde/t/001_basic.pl | 35 +++++++------------------ contrib/pg_tde/t/expected/001_basic.out | 10 ------- 2 files changed, 10 insertions(+), 35 deletions(-) delete mode 100644 contrib/pg_tde/t/expected/001_basic.out diff --git a/contrib/pg_tde/t/001_basic.pl b/contrib/pg_tde/t/001_basic.pl index 38382675e0721..c1cf751ae43df 100644 --- a/contrib/pg_tde/t/001_basic.pl +++ b/contrib/pg_tde/t/001_basic.pl @@ -1,13 +1,11 @@ #!/usr/bin/perl use strict; -use warnings; -use File::Basename; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; use Test::More; -use lib 't'; -use pgtde; -PGTDE::setup_files_dir(basename($0)); my $node = PostgreSQL::Test::Cluster->new('main'); $node->init; @@ -29,29 +27,16 @@ $node->restart; -PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id ASC;'); +is($node->safe_psql('postgres', q{SELECT * FROM test_enc ORDER BY id ASC}), + "1|foobar\n2|barfoo", 'tde_heap table can be read after server restart'); -# Verify that we can't see the data in the file -my $tablefile = $node->safe_psql('postgres', 'SHOW data_directory;'); -$tablefile .= '/'; -$tablefile .= - $node->safe_psql('postgres', 'SELECT pg_relation_filepath(\'test_enc\');'); +my $tablefile = $node->data_dir . '/' + . $node->safe_psql('postgres', q{SELECT pg_relation_filepath('test_enc')}); +my $tablefilecontents = slurp_file($tablefile); -my $strings = 'TABLEFILE FOUND: '; -$strings .= `(ls $tablefile >/dev/null && echo yes) || echo no`; -PGTDE::append_to_result_file($strings); - -$strings = 'CONTAINS FOO (should be empty): '; -$strings .= `strings $tablefile | grep foo`; -PGTDE::append_to_result_file($strings); +unlike($tablefilecontents, qr/foo/, + 'table file does not contain plaintext data'); $node->stop; -# Compare the expected and out file -my $compare = PGTDE->compare_results(); - -is($compare, 0, - "Compare Files: $PGTDE::expected_filename_with_path and $PGTDE::out_filename_with_path files." -); - done_testing(); diff --git a/contrib/pg_tde/t/expected/001_basic.out b/contrib/pg_tde/t/expected/001_basic.out deleted file mode 100644 index edb7188ea4dc5..0000000000000 --- a/contrib/pg_tde/t/expected/001_basic.out +++ /dev/null @@ -1,10 +0,0 @@ -SELECT * FROM test_enc ORDER BY id ASC; - id | k -----+-------- - 1 | foobar - 2 | barfoo -(2 rows) - -TABLEFILE FOUND: yes - -CONTAINS FOO (should be empty): From 856e6e859491a4c0a8917a0b53ac0730eba5cc9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Wed, 23 Apr 2025 09:03:18 +0200 Subject: [PATCH 4/5] Reformat SQL to make it easier to read Just because these are tests we don't have to make the SQL hard to read. --- contrib/pg_tde/t/001_basic.pl | 44 +++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/contrib/pg_tde/t/001_basic.pl b/contrib/pg_tde/t/001_basic.pl index c1cf751ae43df..eb0db23a58d90 100644 --- a/contrib/pg_tde/t/001_basic.pl +++ b/contrib/pg_tde/t/001_basic.pl @@ -13,22 +13,46 @@ $node->start; $node->safe_psql('postgres', q{CREATE EXTENSION IF NOT EXISTS pg_tde}); -$node->safe_psql('postgres', - q{SELECT pg_tde_add_database_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per')} +$node->safe_psql( + 'postgres', q{ + SELECT pg_tde_add_database_key_provider_file( + provider_name => 'file-vault', + file_path => '/tmp/pg_tde_test_keyring.per' + ) + } ); -$node->safe_psql('postgres', - q{SELECT pg_tde_set_key_using_database_key_provider('test-db-key','file-vault')} +$node->safe_psql( + 'postgres', q{ + SELECT pg_tde_set_key_using_database_key_provider( + key_name => 'test-db-key', + provider_name => 'file-vault' + ) + } ); -$node->safe_psql('postgres', - q{CREATE TABLE test_enc(id SERIAL,k VARCHAR(32),PRIMARY KEY (id)) USING tde_heap} +$node->safe_psql( + 'postgres', q{ + CREATE TABLE test_enc( + id SERIAL, + k VARCHAR(32), + PRIMARY KEY (id) + ) USING tde_heap + } +); +$node->safe_psql( + 'postgres', q{ + INSERT INTO test_enc (k) VALUES ('foobar'), ('barfoo') + } ); -$node->safe_psql('postgres', - q{INSERT INTO test_enc (k) VALUES ('foobar'),('barfoo')}); $node->restart; -is($node->safe_psql('postgres', q{SELECT * FROM test_enc ORDER BY id ASC}), - "1|foobar\n2|barfoo", 'tde_heap table can be read after server restart'); +is( $node->safe_psql( + 'postgres', q{ + SELECT * FROM test_enc ORDER BY id ASC + } + ), + "1|foobar\n2|barfoo", + 'tde_heap table can be read after server restart'); my $tablefile = $node->data_dir . '/' . $node->safe_psql('postgres', q{SELECT pg_relation_filepath('test_enc')}); From 7f475e9676f8fdf6f31da53e6539404d6a1f38cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Wed, 23 Apr 2025 15:18:31 +0200 Subject: [PATCH 5/5] Add wrapper to make Andreas happy This is by no means the final wrapper we should use, but should hide a peeve for Andreas so he can see the rest of this branch :D . --- contrib/pg_tde/t/001_basic.pl | 32 +++++++++++++++++++------------- contrib/pg_tde/t/pgtde.pm | 27 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/contrib/pg_tde/t/001_basic.pl b/contrib/pg_tde/t/001_basic.pl index eb0db23a58d90..6cd741ca4c216 100644 --- a/contrib/pg_tde/t/001_basic.pl +++ b/contrib/pg_tde/t/001_basic.pl @@ -6,31 +6,35 @@ use PostgreSQL::Test::Utils; use Test::More; +use FindBin; +use lib $FindBin::RealBin; + +use pgtde; my $node = PostgreSQL::Test::Cluster->new('main'); $node->init; $node->append_conf('postgresql.conf', "shared_preload_libraries = 'pg_tde'"); $node->start; -$node->safe_psql('postgres', q{CREATE EXTENSION IF NOT EXISTS pg_tde}); -$node->safe_psql( - 'postgres', q{ +PGTDE::do_psql($node, 'postgres', q{CREATE EXTENSION IF NOT EXISTS pg_tde}); +PGTDE::do_psql( + $node, 'postgres', q{ SELECT pg_tde_add_database_key_provider_file( provider_name => 'file-vault', file_path => '/tmp/pg_tde_test_keyring.per' ) } ); -$node->safe_psql( - 'postgres', q{ +PGTDE::do_psql( + $node, 'postgres', q{ SELECT pg_tde_set_key_using_database_key_provider( key_name => 'test-db-key', provider_name => 'file-vault' ) } ); -$node->safe_psql( - 'postgres', q{ +PGTDE::do_psql( + $node, 'postgres', q{ CREATE TABLE test_enc( id SERIAL, k VARCHAR(32), @@ -38,24 +42,26 @@ ) USING tde_heap } ); -$node->safe_psql( - 'postgres', q{ +PGTDE::do_psql( + $node, 'postgres', q{ INSERT INTO test_enc (k) VALUES ('foobar'), ('barfoo') } ); $node->restart; -is( $node->safe_psql( - 'postgres', q{ +is( PGTDE::do_psql( + $node, 'postgres', q{ SELECT * FROM test_enc ORDER BY id ASC } ), "1|foobar\n2|barfoo", 'tde_heap table can be read after server restart'); -my $tablefile = $node->data_dir . '/' - . $node->safe_psql('postgres', q{SELECT pg_relation_filepath('test_enc')}); +my $tablefile = + $node->data_dir . '/' + . PGTDE::do_psql($node, 'postgres', + q{SELECT pg_relation_filepath('test_enc')}); my $tablefilecontents = slurp_file($tablefile); unlike($tablefilecontents, qr/foo/, diff --git a/contrib/pg_tde/t/pgtde.pm b/contrib/pg_tde/t/pgtde.pm index 16b98c392a5b4..a2a3a8af79557 100644 --- a/contrib/pg_tde/t/pgtde.pm +++ b/contrib/pg_tde/t/pgtde.pm @@ -81,4 +81,31 @@ sub compare_results return compare($expected_filename_with_path, $out_filename_with_path); } +sub do_psql +{ + my ($node, $dbname, $sql, %params) = @_; + + local %ENV = $node->_get_env(); + + my ($stdout, $stderr); + my $ret = $node->psql( + $dbname, $sql, + %params, + stdout => \$stdout, + stderr => \$stderr, + on_error_die => 0, + on_error_stop => 1); + + # psql can emit stderr from NOTICEs etc + if ($stderr ne "") + { + diag("#### Begin standard error\n"); + diag($stderr); + diag("\n#### End standard error\n"); + } + + die() if $ret; + + return $stdout; +} 1;