SSL support for PostgreSQL hooks #331
|
@ -556,6 +556,37 @@ map:
|
|||
documentation for details. Note that format is
|
||||
ignored when the database name is "all".
|
||||
example: directory
|
||||
ssl_mode:
|
||||
type: str
|
||||
enum: ['disable', 'allow', 'prefer',
|
||||
'require', 'verify-ca', 'verify-full']
|
||||
desc: |
|
||||
SSL mode to use to connect to the database
|
||||
server. One of "disable", "allow", "prefer",
|
||||
"require", "verify-ca" or "verify-full".
|
||||
Defaults to "disable".
|
||||
example: require
|
||||
ssl_cert:
|
||||
type: str
|
||||
desc: |
|
||||
Path to a client certificate.
|
||||
example: "/root/.postgresql/postgresql.crt"
|
||||
ssl_key:
|
||||
type: str
|
||||
desc: |
|
||||
Path to a private client key.
|
||||
example: "/root/.postgresql/postgresql.key"
|
||||
ssl_root_cert:
|
||||
type: str
|
||||
desc: |
|
||||
Path to a root certificate containing a list of
|
||||
trusted certificate authorities.
|
||||
example: "/root/.postgresql/root.crt"
|
||||
ssl_crl:
|
||||
type: str
|
||||
s1shed marked this conversation as resolved
|
||||
desc: |
|
||||
Path to a certificate revocation list.
|
||||
example: "/root/.postgresql/root.crl"
|
||||
options:
|
||||
type: str
|
||||
desc: |
|
||||
|
@ -570,7 +601,8 @@ map:
|
|||
database dumps are added to your source directories at
|
||||
runtime, backed up, and removed afterwards. Requires
|
||||
pg_dump/pg_dumpall/pg_restore commands. See
|
||||
https://www.postgresql.org/docs/current/app-pgdump.html for
|
||||
https://www.postgresql.org/docs/current/app-pgdump.html and
|
||||
https://www.postgresql.org/docs/current/libpq-ssl.html for
|
||||
details.
|
||||
mysql_databases:
|
||||
seq:
|
||||
|
|
|
@ -15,6 +15,25 @@ def make_dump_path(location_config): # pragma: no cover
|
|||
)
|
||||
|
||||
|
||||
def make_extra_environment(database):
|
||||
'''
|
||||
Make the extra_environment dict from the given database configuration.
|
||||
'''
|
||||
extra = dict()
|
||||
if 'password' in database:
|
||||
extra['PGPASSWORD'] = database['password']
|
||||
extra['PGSSLMODE'] = database.get('ssl_mode', 'disable')
|
||||
if 'ssl_cert' in database:
|
||||
extra['PGSSLCERT'] = database['ssl_cert']
|
||||
if 'ssl_key' in database:
|
||||
extra['PGSSLKEY'] = database['ssl_key']
|
||||
if 'ssl_root_cert' in database:
|
||||
extra['PGSSLROOTCERT'] = database['ssl_root_cert']
|
||||
if 'ssl_crl' in database:
|
||||
extra['PGSSLCRL'] = database['ssl_crl']
|
||||
return extra
|
||||
|
||||
|
||||
def dump_databases(databases, log_prefix, location_config, dry_run):
|
||||
'''
|
||||
Dump the given PostgreSQL databases to a named pipe. The databases are supplied as a sequence of
|
||||
|
@ -56,7 +75,7 @@ def dump_databases(databases, log_prefix, location_config, dry_run):
|
|||
# format in a particular, a named destination is required, and redirection doesn't work.
|
||||
+ (('>', dump_filename) if dump_format != 'directory' else ())
|
||||
)
|
||||
extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None
|
||||
extra_environment = make_extra_environment(database)
|
||||
|
||||
logger.debug(
|
||||
'{}: Dumping PostgreSQL database {} to {}{}'.format(
|
||||
|
@ -141,7 +160,7 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run,
|
|||
+ (('--username', database['username']) if 'username' in database else ())
|
||||
+ (() if extract_process else (dump_filename,))
|
||||
)
|
||||
extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None
|
||||
extra_environment = make_extra_environment(database)
|
||||
|
||||
logger.debug(
|
||||
'{}: Restoring PostgreSQL database {}{}'.format(log_prefix, database['name'], dry_run_label)
|
||||
|
|
|
@ -29,7 +29,7 @@ def test_dump_databases_runs_pg_dump_for_each_database():
|
|||
'databases/localhost/{}'.format(name),
|
||||
),
|
||||
shell=True,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
witten
commented
Speaking of tests, it's awesome that you updated the existing tests. Do you think you'd be able to add new tests for the additional functionality you've added? I'm happy to help if it's not straight-forward. (This is the test file where any new unit tests on this code belong.) Speaking of tests, it's awesome that you updated the existing tests. Do you think you'd be able to add new tests for the additional functionality you've added? I'm happy to help if it's not straight-forward. (This is the test file where any new unit tests on this code belong.)
s1shed
commented
These changes were tested locally by modifying I can try to take a stab at writing tests for these new additions but might need some "hand holding" being as though I consider myself to be rather n00bish with Python and coding in general (and having close to 0 experience with writing tests). These changes were tested locally by modifying `pg_hba.conf` to make SSL/TLS mandatory and running borgmatic while manually changing the config file to set the various options, e.g. commenting out `ssl_mode` to ensure the default of `disabled` is set (and the dump/restore fails), setting `ssl_mode` to `verify-full` and seeing `pg_dump`/`pg_restore` complain about not providing the path to the root CA cert, setting `ssl_root_cert` to a non-existant path and seeing the dump/restore referring to this path, etc., so I am confident that these changes are at least *functionally correct*. Stylistically on the other hand…
I can try to take a stab at writing tests for these new additions but might need some "hand holding" being as though I consider myself to be rather n00bish with Python and coding in general (and having close to 0 experience with writing tests).
s1shed
commented
So...I created two new functions in the postgres unittest file. The changes I pushed last night:
With the changes I made this morning and the tests I just pushed:
Hopefully the tests look like what is needed? So...I created two new functions in the postgres unittest file.
The changes I pushed last night:
```
----------- coverage: platform linux, python 3.8.3-final-0 -----------
Name Stmts Miss Cover Missing
--------------------
borgmatic/execute.py 81 2 98% 104-105
borgmatic/hooks/postgresql.py 60 8 87% 64, 66, 68, 70, 161, 163, 165, 167
--------------------
TOTAL 1380 10 99%
42 files skipped due to complete coverage.
```
With the changes I made this morning and the tests I just pushed:
```
----------- coverage: platform linux, python 3.8.3-final-0 -----------
Name Stmts Miss Cover Missing
----------------------------------------------------
borgmatic/execute.py 81 2 98% 104-105
----------------------------------------------------
TOTAL 1372 2 99%
43 files skipped due to complete coverage.
```
Hopefully the tests look like what is needed?
witten
commented
This is looking great. I appreciate you making all the updates! The one change I would suggest is to make a dedicated unit test of just your new As part of that, it would probably make sense to mock out your new
Given that all of this is pretty minor, I'm happy to make these updates if you just want to be done with this pull request! Either way, please let me know. This is looking great. I appreciate you making all the updates!
The one change I would suggest is to make a dedicated unit test of just your new `make_extra_environment()` function. That way, you can test only that functionality by itself separate from all of the dump/restore functionality. That'll let you get rid of `test_dump_databases_runs_pg_dump_with_ssl_options()` and `test_restore_database_dump_runs_pg_restore_with_ssl_options()`, since you'd be testing the SSL options in your dediceated `test_make_extra_environment_does_whatever()`.
As part of that, it would probably make sense to mock out your new `make_extra_environment()` function in any of the existing dump/restore test functions.. So any given test just has a single actual function under test. An example of what that might look like:
```
def test_dump_databases_runs_pg_dump_with_options():
...
flexmock(module).should_receive('make_extra_environment').and_return({})
...
assert ... == ...
```
Given that all of this is pretty minor, I'm happy to make these updates if you just want to be done with this pull request! Either way, please let me know.
s1shed
commented
I came up with this which shows 100% coverage for Postgres and it passes but I have my doubts about this being correct.
If this is OK I can commit and submit. If it's still wrong, thanks in advance for your patience… I came up with this which shows 100% coverage for Postgres and it passes but I have my doubts about this being correct.
```diff
+def test_make_extra_environment():
+ database = {'name': 'foo',
+ 'ssl_mode': 'prefer',
+ 'ssl_cert': 'cert.crt',
+ 'ssl_key': 'key.key',
+ 'ssl_root_cert': 'root.crt',
+ 'ssl_crl': 'crl.crl'}
+ extra_env = module.make_extra_environment(database)
+ expected = {'PGSSLMODE': 'prefer',
+ 'PGSSLCERT': 'cert.crt',
+ 'PGSSLKEY': 'key.key',
+ 'PGSSLROOTCERT': 'root.crt',
+ 'PGSSLCRL': 'crl.crl'}
+ flexmock(module).should_receive('make_extra_environment').and_return(expected)
+ assert extra_env == expected
```
If this is OK I can commit and submit. If it's still wrong, thanks in advance for your patience…
witten
commented
The test function looks great except for the What I was actually suggesting about
That last line is the only addition. The idea is that in Thanks for your perseverence here! The test function looks great except for the `flexmock(module).should_receive('make_extra_environment').and_return(expected)` part. That's not needed in this test function because as it's written, the test function is testing that the mock returns what's expected, rather than testing that `make_extra_environment()` does! If you take that line out, then I think the test will do what you want.
What I was actually suggesting about `flexmock(module).should_receive('make_extra_environment')` was to add it within the `test_dump_databases_*` and `test_restore_database_*` functions only. Sorry if that wasn't clear. Expanding on the example above, the way that could go might be something like:
```python
def test_dump_databases_runs_pg_dump_with_options():
databases = [{'name': 'foo', 'options': '--stuff=such'}]
process = flexmock()
flexmock(module).should_receive('make_dump_path').and_return('')
flexmock(module.dump).should_receive('make_database_dump_filename').and_return(
'databases/localhost/foo'
)
flexmock(module.dump).should_receive('create_named_pipe_for_dump')
flexmock(module).should_receive('make_extra_environment').and_return({})
...
```
That last line is the only addition. The idea is that in `test_dump_databases_*()`, the only unit you want to test is `dump_databases()`... and therefore all other units that `dump_databases()` calls out to (like `make_extra_environment()` should get mocked out so that their implementation details don't impact the test.
Thanks for your perseverence here!
s1shed
commented
I rebased a "fixed" version into the previous commit which added the tests. Perhaps it's now correct. I genuinely appreciate the patience that you've demonstrated. :) I rebased a "fixed" version into the previous commit which added the tests. Perhaps it's now correct.
I genuinely appreciate the patience that you've demonstrated. :)
|
||||
run_to_completion=False,
|
||||
).and_return(process).once()
|
||||
|
||||
|
@ -74,7 +74,7 @@ def test_dump_databases_runs_pg_dump_with_hostname_and_port():
|
|||
'databases/database.example.org/foo',
|
||||
),
|
||||
shell=True,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
run_to_completion=False,
|
||||
).and_return(process).once()
|
||||
|
||||
|
@ -105,13 +105,34 @@ def test_dump_databases_runs_pg_dump_with_username_and_password():
|
|||
'databases/localhost/foo',
|
||||
),
|
||||
shell=True,
|
||||
extra_environment={'PGPASSWORD': 'trustsome1'},
|
||||
extra_environment={'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'},
|
||||
run_to_completion=False,
|
||||
).and_return(process).once()
|
||||
|
||||
assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [process]
|
||||
|
||||
|
||||
def test_make_extra_environment():
|
||||
database = {
|
||||
'name': 'foo',
|
||||
'ssl_mode': 'require',
|
||||
'ssl_cert': 'cert.crt',
|
||||
'ssl_key': 'key.key',
|
||||
'ssl_root_cert': 'root.crt',
|
||||
'ssl_crl': 'crl.crl',
|
||||
}
|
||||
expected = {
|
||||
'PGSSLMODE': 'require',
|
||||
'PGSSLCERT': 'cert.crt',
|
||||
'PGSSLKEY': 'key.key',
|
||||
'PGSSLROOTCERT': 'root.crt',
|
||||
'PGSSLCRL': 'crl.crl',
|
||||
}
|
||||
|
||||
extra_env = module.make_extra_environment(database)
|
||||
assert extra_env == expected
|
||||
|
||||
|
||||
def test_dump_databases_runs_pg_dump_with_directory_format():
|
||||
databases = [{'name': 'foo', 'format': 'directory'}]
|
||||
process = flexmock()
|
||||
|
@ -135,7 +156,7 @@ def test_dump_databases_runs_pg_dump_with_directory_format():
|
|||
'foo',
|
||||
),
|
||||
shell=True,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
run_to_completion=False,
|
||||
).and_return(process).once()
|
||||
|
||||
|
@ -151,6 +172,8 @@ def test_dump_databases_runs_pg_dump_with_options():
|
|||
)
|
||||
flexmock(module.dump).should_receive('create_named_pipe_for_dump')
|
||||
|
||||
flexmock(module).should_receive('make_extra_environment').and_return({'PGSSLMODE': 'disable'})
|
||||
|
||||
flexmock(module).should_receive('execute_command').with_args(
|
||||
(
|
||||
'pg_dump',
|
||||
|
@ -165,7 +188,7 @@ def test_dump_databases_runs_pg_dump_with_options():
|
|||
'databases/localhost/foo',
|
||||
),
|
||||
shell=True,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
run_to_completion=False,
|
||||
).and_return(process).once()
|
||||
|
||||
|
@ -184,7 +207,7 @@ def test_dump_databases_runs_pg_dumpall_for_all_databases():
|
|||
flexmock(module).should_receive('execute_command').with_args(
|
||||
('pg_dumpall', '--no-password', '--clean', '--if-exists', '>', 'databases/localhost/all'),
|
||||
shell=True,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
run_to_completion=False,
|
||||
).and_return(process).once()
|
||||
|
||||
|
@ -210,12 +233,12 @@ def test_restore_database_dump_runs_pg_restore():
|
|||
processes=[extract_process],
|
||||
output_log_level=logging.DEBUG,
|
||||
input_file=extract_process.stdout,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
borg_local_path='borg',
|
||||
).once()
|
||||
flexmock(module).should_receive('execute_command').with_args(
|
||||
('psql', '--no-password', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'),
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
).once()
|
||||
|
||||
module.restore_database_dump(
|
||||
|
@ -260,7 +283,7 @@ def test_restore_database_dump_runs_pg_restore_with_hostname_and_port():
|
|||
processes=[extract_process],
|
||||
output_log_level=logging.DEBUG,
|
||||
input_file=extract_process.stdout,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
borg_local_path='borg',
|
||||
).once()
|
||||
flexmock(module).should_receive('execute_command').with_args(
|
||||
|
@ -277,7 +300,7 @@ def test_restore_database_dump_runs_pg_restore_with_hostname_and_port():
|
|||
'--command',
|
||||
'ANALYZE',
|
||||
),
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
).once()
|
||||
|
||||
module.restore_database_dump(
|
||||
|
@ -306,7 +329,7 @@ def test_restore_database_dump_runs_pg_restore_with_username_and_password():
|
|||
processes=[extract_process],
|
||||
output_log_level=logging.DEBUG,
|
||||
input_file=extract_process.stdout,
|
||||
extra_environment={'PGPASSWORD': 'trustsome1'},
|
||||
extra_environment={'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'},
|
||||
borg_local_path='borg',
|
||||
).once()
|
||||
flexmock(module).should_receive('execute_command').with_args(
|
||||
|
@ -321,7 +344,7 @@ def test_restore_database_dump_runs_pg_restore_with_username_and_password():
|
|||
'--command',
|
||||
'ANALYZE',
|
||||
),
|
||||
extra_environment={'PGPASSWORD': 'trustsome1'},
|
||||
extra_environment={'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'},
|
||||
).once()
|
||||
|
||||
module.restore_database_dump(
|
||||
|
@ -340,11 +363,12 @@ def test_restore_database_dump_runs_psql_for_all_database_dump():
|
|||
processes=[extract_process],
|
||||
output_log_level=logging.DEBUG,
|
||||
input_file=extract_process.stdout,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
borg_local_path='borg',
|
||||
).once()
|
||||
flexmock(module).should_receive('execute_command').with_args(
|
||||
('psql', '--no-password', '--quiet', '--command', 'ANALYZE'), extra_environment=None
|
||||
('psql', '--no-password', '--quiet', '--command', 'ANALYZE'),
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
).once()
|
||||
|
||||
module.restore_database_dump(
|
||||
|
@ -383,12 +407,12 @@ def test_restore_database_dump_without_extract_process_restores_from_disk():
|
|||
processes=[],
|
||||
output_log_level=logging.DEBUG,
|
||||
input_file=None,
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
borg_local_path='borg',
|
||||
).once()
|
||||
flexmock(module).should_receive('execute_command').with_args(
|
||||
('psql', '--no-password', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'),
|
||||
extra_environment=None,
|
||||
extra_environment={'PGSSLMODE': 'disable'},
|
||||
).once()
|
||||
|
||||
module.restore_database_dump(
|
||||
|
|
For these various new options, their naming would be more in keeping with the rest of borgmatic's configuration options if they were more like:
ssl_mode
,ssl_key
,ssl_root_cert
, etc.Changed