Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix connection parameter handling #190

Merged
merged 5 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions trollmoves/movers.py
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,27 @@ class S3Mover(Mover):
changing the filename. The new destination filename will be the last part
of the provided destination following the last slash ('/').

In the Trollmoves Server config, which is in .ini format, the connection parameters
and other dictionary-like items can be defined with douple underscore format::

connection_parameters__secret = secret
connection_parameters__client_kwargs__endpoint_url = https://endpoint.url
connection_parameters__client_kwargs__verify = false

will result in a nested dictionary item::

{
'connection_parameters': {
'secret': 'secret',
'client_kwargs': {
'endpoint_url': 'https://endpoint.url',
'verify': False
}
}
}

Note that boolean values are converted. Numeric values are handled where they are used.

"""

def copy(self):
Expand Down
47 changes: 46 additions & 1 deletion trollmoves/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@
file_cache_lock = Lock()
START_TIME = datetime.datetime.utcnow()

CONNECTION_CONFIG_ITEMS = ["connection_uptime", "ssh_key_filename", "ssh_connection_timeout", "ssh_private_key_file"]


class RequestManager(Thread):
"""Manage requests."""
Expand Down Expand Up @@ -165,7 +167,8 @@ def _move_file(self, pathname, message, rel_path):
return_message = None
try:
destination = move_it(pathname, message.data['destination'],
self._attrs, rel_path=rel_path,
self._attrs["connection_parameters"],
rel_path=rel_path,
backup_targets=message.data.get('backup_targets', None))
message.data['destination'] = destination
except Exception as err:
Expand Down Expand Up @@ -584,6 +587,8 @@ def _read_ini_config(filename):
_parse_nameserver(res[section], cp_[section])
_parse_addresses(res[section])
_parse_delete(res[section], cp_[section])
res[section] = _create_config_sub_dicts(res[section])
res[section] = _form_connection_parameters_dict(res[section])
if not _check_origin_and_listen(res, section):
continue
if not _check_topic(res, section):
Expand Down Expand Up @@ -622,6 +627,46 @@ def _parse_delete(conf, raw_conf):
conf["delete"] = val


def _create_config_sub_dicts(original):
# Take a copy so we can modify the values if necessary
res = dict(original.items())
for key in original.keys():
parts = key.split("__")
if len(parts) > 1:
_create_dicts(res, parts, original[key])
del res[key]
return res


def _create_dicts(res, parts, val):
cur = res
for part in parts[:-1]:
if part not in cur:
cur[part] = {}
cur = cur[part]
cur[parts[-1]] = _check_bool(val)


def _check_bool(val):
if val.lower() in ["0", "false"]:
return False
elif val.lower() in ["1", "true"]:
return True
return val


def _form_connection_parameters_dict(original):
# Take a copy so we can modify the values if necessary
res = dict(original.items())
if "connection_parameters" not in res:
res["connection_parameters"] = {}
for key in original.keys():
if key in CONNECTION_CONFIG_ITEMS:
res["connection_parameters"][key] = original[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have a deprecation warning here? to motivate people to move to the __ syntax?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Maybe 🤔 I'll see think about this and see how to document this in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning added in e158f3a

del res[key]
return res


def _check_origin_and_listen(res, section):
if ("origin" not in res[section]) and ('listen' not in res[section]):
LOGGER.warning("Incomplete section %s: add an 'origin' or 'listen' item.", section)
Expand Down
12 changes: 6 additions & 6 deletions trollmoves/tests/test_movers.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,14 @@ def test_s3_copy_file_to_base_using_connection_parameters(S3FileSystem):
"""Test copying to base of S3 bucket."""
# Get the connection parameters:
config = yaml.safe_load(test_yaml_s3_connection_params)
attrs = config['target-s3-example1']['connection_parameters']
attrs = config['target-s3-example1']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we be passing only the connection parameters here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Forgot to revert this part, done in 53a4bc3


s3_mover = _get_s3_mover(ORIGIN, "s3://data-bucket/", **attrs)
assert s3_mover.attrs['client_kwargs'] == {'endpoint_url': 'https://minio-server.mydomain.se:9000',
'verify': False}
assert s3_mover.attrs['secret'] == 'my-super-secret-key'
assert s3_mover.attrs['key'] == 'my-access-key'
assert s3_mover.attrs['use_ssl'] is True
assert s3_mover.attrs['connection_parameters']['client_kwargs'] == {
'endpoint_url': 'https://minio-server.mydomain.se:9000', 'verify': False}
assert s3_mover.attrs['connection_parameters']['secret'] == 'my-super-secret-key'
assert s3_mover.attrs['connection_parameters']['key'] == 'my-access-key'
assert s3_mover.attrs['connection_parameters']['use_ssl'] is True

s3_mover.copy()

Expand Down
45 changes: 41 additions & 4 deletions trollmoves/tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,17 +170,54 @@ def test_empty_init_arguments_does_not_crash_add(self):
Deleter(dict()).add('bla')


config_file = b"""
CONFIG_INI = b"""
[eumetcast-hrit-0deg]
origin = /local_disk/tellicast/received/MSGHRIT/H-000-{nominal_time:%Y%m%d%H%M}-{compressed:_<2s}
request_port = 9094
publisher_port = 9010
info = sensor=seviri;variant=0DEG
topic = /1b/hrit-segment/0deg
delete = False
# Everything below this should end up in connection_parameters dict
connection_uptime = 30
ssh_key_filename = id_rsa.pub
ssh_private_key_file = id_rsa
ssh_connection_timeout = 30
connection_parameters__secret = secret
connection_parameters__client_kwargs__endpoint_url = https://endpoint.url
connection_parameters__client_kwargs__verify = false
"""


def test_read_config_ini_with_dicts():
"""Test reading a config in ini format when dictionary values should be created."""
from trollmoves.server import read_config

with NamedTemporaryFile(suffix=".ini") as config_file:
config_file.write(CONFIG_INI)
config_file.flush()
config = read_config(config_file.name)
eumetcast = config["eumetcast-hrit-0deg"]
assert "origin" in eumetcast
assert "request_port" in eumetcast
assert "publisher_port" in eumetcast
assert "info" in eumetcast
assert "topic" in eumetcast
assert "delete" in eumetcast
expected_conn_params = {
"secret": "secret",
"client_kwargs": {
"endpoint_url": "https://endpoint.url",
"verify": False,
},
"connection_uptime": "30",
"ssh_key_filename": "id_rsa.pub",
"ssh_private_key_file": "id_rsa",
"ssh_connection_timeout": "30",
}
assert eumetcast["connection_parameters"] == expected_conn_params


class TestMoveItServer:
"""Test the move it server."""

Expand All @@ -195,7 +232,7 @@ def test_reloads_config_crashes_when_config_file_does_not_exist(self):
def test_reloads_config_on_example_config(self, fake_publisher):
"""Test that config can be reloaded with basic example."""
with NamedTemporaryFile() as temporary_config_file:
temporary_config_file.write(config_file)
temporary_config_file.write(CONFIG_INI)
config_filename = temporary_config_file.name
cmd_args = parse_args(["--port", "9999", config_filename])
server = MoveItServer(cmd_args)
Expand All @@ -206,7 +243,7 @@ def test_reloads_config_on_example_config(self, fake_publisher):
def test_reloads_config_calls_reload_config(self, mock_reload_config, mock_publisher):
"""Test that config file can be reloaded."""
with NamedTemporaryFile() as temporary_config_file:
temporary_config_file.write(config_file)
temporary_config_file.write(CONFIG_INI)
config_filename = temporary_config_file.name
cmd_args = parse_args(["--port", "9999", config_filename])
server = MoveItServer(cmd_args)
Expand All @@ -218,7 +255,7 @@ def test_reloads_config_calls_reload_config(self, mock_reload_config, mock_publi
def test_signal_reloads_config_calls_reload_config(self, mock_reload_config, mock_publisher):
"""Test that config file can be reloaded through signal."""
with NamedTemporaryFile() as temporary_config_file:
temporary_config_file.write(config_file)
temporary_config_file.write(CONFIG_INI)
config_filename = temporary_config_file.name
cmd_args = parse_args([config_filename])
client = MoveItServer(cmd_args)
Expand Down