diff --git a/.release b/.release index 9d87499b239181b47cedcb9f21a78a1ed501eba8..28d4da95f2febc7f9e326f85992595249dc86cde 100644 --- a/.release +++ b/.release @@ -1,2 +1,2 @@ -release=0.5.2 -tag=lmcbaseclasses-0.5.2 +release=0.5.3 +tag=lmcbaseclasses-0.5.3 diff --git a/README.md b/README.md index f928bf117275cc31dff8762c4c6e9a05e71fea80..a275cebef637a127990d2e2bee952dc9d3282c06 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,12 @@ The lmc-base-classe repository contains set of eight classes as mentioned in SKA ## Version History +#### 0.5.3 +- Setting `loggingTargets` attribute to empty list no longer raises exception. +- Change syslog targets in `loggingTargets` attribute to a full URL so that remote syslog servers can be specified. + For example, `"syslog::udp://server.domain:514"`, would send logs to `server.domain` via UDP port 514. + Specifying a path without a protocol, like `"syslog::/var/log"`, is deprecated. + #### 0.5.2 - Change ska_logger dependency to use ska-namespaced package (v0.3.0). No change to usage. @@ -251,8 +257,8 @@ current_targets = proxy.loggingTargets new_targets = list(current_targets) + ["file::/tmp/my.log"] proxy.loggingTargets = new_targets -# disable all additional targets (empty list breaks, so include an empty string!) -proxy.loggingTargets = [''] +# disable all additional targets +proxy.loggingTargets = [] ``` Currently there are three types of targets implemented: @@ -271,12 +277,21 @@ the Tango name. E.g., "my/test/device" would get the file "my_test_device.log". Currently, we using a `logging.handlers.RotatingFileHandler` with a 1 MB limit and just 2 backups. This could be modified in future. -For syslog, the syslog target address details must be provided after the `::`. -This string is what ever you would pass to `logging.handlers.SysLogHandler`'s `address` -argument. E.g. `proxy.loggingTargets = ["syslog::/dev/log"]`. +For syslog, the syslog target address details must be provided after the `::` as a URL. +The following types are supported: +- File, `file://<path>` + - E.g., for `/dev/log` use `file:///dev/log`. + - If the protocol is omitted, it is assumed to be `file://`. Note: this is deprecated. + Support will be removed in v0.6.0. +- Remote UDP server, `udp://<hostname>:<port>` + - E.g., for `server.domain` on UDP port 514 use `udp://server.domain:514`. +- Remote TCP server, `tcp://<hostname>:<port>` + - E.g., for `server.domain` on TCP port 601 use `tcp://server.domain:601`. + +Example of usage: `proxy.loggingTargets = ["syslog::udp://server.domain:514"]`. If you want file and syslog targets, you could do something like: -`proxy.loggingTargets = ["file::/tmp/my.log", "syslog::/dev/log"]`. +`proxy.loggingTargets = ["file::/tmp/my.log", "syslog::udp://server.domain:514"]`. **Note:** There is a limit of 3 additional handlers. That the maximum length of the spectrum attribute. We could change this if there is a reasonable use diff --git a/src/ska/base/base_device.py b/src/ska/base/base_device.py index ae62822c56dfd7b428063c4f737ee14909516322..1886c1ee22edec105853ef5020350247363093bd 100644 --- a/src/ska/base/base_device.py +++ b/src/ska/base/base_device.py @@ -15,8 +15,13 @@ import json import logging import logging.handlers import os +import socket import sys import threading +import warnings + +from urllib.parse import urlparse +from urllib.request import url2pathname # Tango imports import tango @@ -75,7 +80,7 @@ class LoggingUtils: :param target: List of candidate logging target strings, like '<type>[::<name>]' - Empty and whitespace-only strings are ignored. + Empty and whitespace-only strings are ignored. Can also be None. :param device_name: TANGO device name, like 'domain/family/member', used @@ -91,29 +96,94 @@ class LoggingUtils: "syslog": None} valid_targets = [] - for target in targets: - target = target.strip() - if not target: - continue - if "::" in target: - target_type, target_name = target.split("::", 1) - else: - target_type = target - target_name = None - if target_type not in default_target_names: - raise LoggingTargetError( - "Invalid target type: {} - options are {}".format( - target_type, list(default_target_names.keys()))) - if not target_name: - target_name = default_target_names[target_type] - if not target_name: - raise LoggingTargetError( - "Target name required for type {}".format(target_type)) - valid_target = "{}::{}".format(target_type, target_name) - valid_targets.append(valid_target) + if targets: + for target in targets: + target = target.strip() + if not target: + continue + if "::" in target: + target_type, target_name = target.split("::", 1) + else: + target_type = target + target_name = None + if target_type not in default_target_names: + raise LoggingTargetError( + "Invalid target type: {} - options are {}".format( + target_type, list(default_target_names.keys()))) + if not target_name: + target_name = default_target_names[target_type] + if not target_name: + raise LoggingTargetError( + "Target name required for type {}".format(target_type)) + valid_target = "{}::{}".format(target_type, target_name) + valid_targets.append(valid_target) return valid_targets + @staticmethod + def get_syslog_address_and_socktype(url): + """Parse syslog URL and extract address and socktype parameters for SysLogHandler. + + :param url: + Universal resource locator string for syslog target. Three types are supported: + file path, remote UDP server, remote TCP server. + - Output to a file: 'file://<path to file>' + Example: 'file:///dev/log' will write to '/dev/log' + - Output to remote server over UDP: 'udp://<hostname>:<port>' + Example: 'udp://syslog.com:514' will send to host 'syslog.com' on UDP port 514 + - Output to remote server over TCP: 'tcp://<hostname>:<port>' + Example: 'tcp://rsyslog.com:601' will send to host 'rsyslog.com' on TCP port 601 + For backwards compatibility, if the protocol prefix is missing, the type is + interpreted as file. This is deprecated. + - Example: '/dev/log' is equivalent to 'file:///dev/log' + + :return: (address, socktype) + For file types: + - address is the file path as as string + - socktype is None + For UDP and TCP: + - address is tuple of (hostname, port), with hostname a string, and port an integer. + - socktype is socket.SOCK_DGRAM for UDP, or socket.SOCK_STREAM for TCP. + + :raises: LoggingTargetError for invalid url string + """ + address = None + socktype = None + parsed = urlparse(url) + if parsed.scheme in ["file", ""]: + address = url2pathname(parsed.netloc + parsed.path) + socktype = None + if not address: + raise LoggingTargetError( + "Invalid syslog URL - empty file path from '{}'".format(url) + ) + if parsed.scheme == "": + warnings.warn( + "Specifying syslog URL without protocol is deprecated, " + "use 'file://{}' instead of '{}'".format(url, url), + DeprecationWarning, + ) + elif parsed.scheme in ["udp", "tcp"]: + if not parsed.hostname: + raise LoggingTargetError( + "Invalid syslog URL - could not extract hostname from '{}'".format(url) + ) + try: + port = int(parsed.port) + except (TypeError, ValueError): + raise LoggingTargetError( + "Invalid syslog URL - could not extract integer port number from '{}'".format( + url + ) + ) + address = (parsed.hostname, port) + socktype = socket.SOCK_DGRAM if parsed.scheme == "udp" else socket.SOCK_STREAM + else: + raise LoggingTargetError( + "Invalid syslog URL - expected file, udp or tcp protocol scheme in '{}'".format(url) + ) + return address, socktype + @staticmethod def create_logging_handler(target): """Create a Python log handler based on the target type (console, file, syslog) @@ -136,8 +206,11 @@ class LoggingUtils: handler = logging.handlers.RotatingFileHandler( log_file_name, 'a', LOG_FILE_SIZE, 2, None, False) elif target_type == "syslog": + address, socktype = LoggingUtils.get_syslog_address_and_socktype(target_name) handler = logging.handlers.SysLogHandler( - address=target_name, facility=logging.handlers.SysLogHandler.LOG_SYSLOG) + address=address, + facility=logging.handlers.SysLogHandler.LOG_SYSLOG, + socktype=socktype) else: raise LoggingTargetError( "Invalid target type requested: '{}' in '{}'".format(target_type, target)) diff --git a/src/ska/base/release.py b/src/ska/base/release.py index 65992ce6db7eb209e9df9b3418ac77015a132dde..c73a1b82254042b1245f868a3cc2b808b0861564 100644 --- a/src/ska/base/release.py +++ b/src/ska/base/release.py @@ -7,7 +7,7 @@ """Release information for lmc-base-classes Python Package""" name = """lmcbaseclasses""" -version = "0.5.2" +version = "0.5.3" version_info = version.split(".") description = """A set of generic base devices for SKA Telescope.""" author = "SKA India and SARAO" diff --git a/tests/test_base_device.py b/tests/test_base_device.py index 51e932959e9e764c92214078ade7e12b07a4b0fa..2f48f882df37fddb7768d5696c5e4913b7d989a8 100644 --- a/tests/test_base_device.py +++ b/tests/test_base_device.py @@ -16,6 +16,8 @@ import pytest # PROTECTED REGION ID(SKABaseDevice.test_additional_imports) ENABLED START # import logging +import socket + from unittest import mock from tango import DevFailed, DevState from ska.base.control_model import ( @@ -32,6 +34,7 @@ from ska.base.base_device import ( class TestLoggingUtils: @pytest.fixture(params=[ + (None, []), ([""], []), ([" \n\t "], []), (["console"], ["console::cout"]), @@ -41,7 +44,9 @@ class TestLoggingUtils: (["file"], ["file::my_dev_name.log"]), (["file::"], ["file::my_dev_name.log"]), (["file::/tmp/dummy"], ["file::/tmp/dummy"]), - (["syslog::some/address"], ["syslog::some/address"]), + (["syslog::some/path"], ["syslog::some/path"]), + (["syslog::file://some/path"], ["syslog::file://some/path"]), + (["syslog::protocol://somehost:1234"], ["syslog::protocol://somehost:1234"]), (["console", "file"], ["console::cout", "file::my_dev_name.log"]), ]) def good_logging_targets(self, request): @@ -61,18 +66,58 @@ class TestLoggingUtils: dev_name = "my/dev/name" return targets_in, dev_name - def test_sanitise_logging_targets_success(self, good_logging_targets): targets_in, dev_name, expected = good_logging_targets actual = LoggingUtils.sanitise_logging_targets(targets_in, dev_name) assert actual == expected - def test_sanitise_logging_targets_fail(self, bad_logging_targets): targets_in, dev_name = bad_logging_targets with pytest.raises(LoggingTargetError): LoggingUtils.sanitise_logging_targets(targets_in, dev_name) + @pytest.fixture(params=[ + ("deprecated/path", ["deprecated/path", None]), + ("file:///abs/path", ["/abs/path", None]), + ("file://relative/path", ["relative/path", None]), + ("file://some/spaced%20path", ["some/spaced path", None]), + ("udp://somehost.domain:1234", [("somehost.domain", 1234), socket.SOCK_DGRAM]), + ("udp://127.0.0.1:1234", [("127.0.0.1", 1234), socket.SOCK_DGRAM]), + ("tcp://somehost:1234", [("somehost", 1234), socket.SOCK_STREAM]), + ("tcp://127.0.0.1:1234", [("127.0.0.1", 1234), socket.SOCK_STREAM]), + ]) + def good_syslog_url(self, request): + url, (expected_address, expected_socktype) = request.param + return url, (expected_address, expected_socktype) + + @pytest.fixture(params=[ + None, + "", + "file://", + "udp://", + "udp://somehost", + "udp://somehost:", + "udp://somehost:not_integer_port", + "udp://:1234", + "tcp://", + "tcp://somehost", + "tcp://somehost:", + "tcp://somehost:not_integer_port", + "tcp://:1234", + "invalid://somehost:1234" + ]) + def bad_syslog_url(self, request): + return request.param + + def test_get_syslog_address_and_socktype_success(self, good_syslog_url): + url, (expected_address, expected_socktype) = good_syslog_url + actual_address, actual_socktype = LoggingUtils.get_syslog_address_and_socktype(url) + assert actual_address == expected_address + assert actual_socktype == expected_socktype + + def test_get_syslog_address_and_socktype_fail(self, bad_syslog_url): + with pytest.raises(LoggingTargetError): + LoggingUtils.get_syslog_address_and_socktype(bad_syslog_url) @mock.patch('logging.handlers.SysLogHandler') @mock.patch('logging.handlers.RotatingFileHandler') @@ -101,7 +146,22 @@ class TestLoggingUtils: assert handler == mock_file_handler() handler.setFormatter.assert_called_once_with(mock_formatter) - handler = LoggingUtils.create_logging_handler("syslog::some/address") + handler = LoggingUtils.create_logging_handler("syslog::udp://somehost:1234") + mock_syslog_handler.assert_called_once_with( + address=("somehost", 1234), + facility=mock_syslog_handler.LOG_SYSLOG, + socktype=socket.SOCK_DGRAM + ) + assert handler == mock_syslog_handler() + handler.setFormatter.assert_called_once_with(mock_formatter) + + mock_syslog_handler.reset_mock() + handler = LoggingUtils.create_logging_handler("syslog::file:///tmp/path") + mock_syslog_handler.assert_called_once_with( + address="/tmp/path", + facility=mock_syslog_handler.LOG_SYSLOG, + socktype=None + ) assert handler == mock_syslog_handler() handler.setFormatter.assert_called_once_with(mock_formatter) @@ -299,15 +359,19 @@ class TestSKABaseDevice(object): # test adding file and syslog targets (already have console) mocked_creator.reset_mock() tango_context.device.loggingTargets = [ - "console::cout", "file::/tmp/dummy", "syslog::some/address"] + "console::cout", "file::/tmp/dummy", "syslog::udp://localhost:514"] assert tango_context.device.loggingTargets == ( - "console::cout", "file::/tmp/dummy", "syslog::some/address") + "console::cout", "file::/tmp/dummy", "syslog::udp://localhost:514") assert mocked_creator.call_count == 2 mocked_creator.assert_has_calls( [mock.call("file::/tmp/dummy"), - mock.call("syslog::some/address")], + mock.call("syslog::udp://localhost:514")], any_order=True) + # test clearing all targets (note: PyTango returns None for empty spectrum attribute) + tango_context.device.loggingTargets = [] + assert tango_context.device.loggingTargets is None + mocked_creator.reset_mock() with pytest.raises(DevFailed): tango_context.device.loggingTargets = ["invalid::type"]