From 5caddb7f4a8d8d201f807ed0997c98668f5ff2a6 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 3 Jan 2019 17:50:15 +0100 Subject: [PATCH 1/8] Update affected tests. --- st2api/tests/unit/controllers/v1/test_packs.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2api/tests/unit/controllers/v1/test_packs.py b/st2api/tests/unit/controllers/v1/test_packs.py index 1ccd4ea439..8c5d9f338c 100644 --- a/st2api/tests/unit/controllers/v1/test_packs.py +++ b/st2api/tests/unit/controllers/v1/test_packs.py @@ -530,14 +530,14 @@ def test_packs_register_endpoint(self, mock_get_packs): {'packs': ['dummy_pack_1'], 'types': ['action']}) self.assertEqual(resp.status_int, 200) - self.assertEqual(resp.json, {'actions': 1, 'runners': 18}) + self.assertEqual(resp.json, {'actions': 1, 'runners': 15}) # Verify that plural name form also works resp = self.app.post_json('/v1/packs/register', {'packs': ['dummy_pack_1'], 'types': ['actions']}) self.assertEqual(resp.status_int, 200) - self.assertEqual(resp.json, {'actions': 1, 'runners': 18}) + self.assertEqual(resp.json, {'actions': 1, 'runners': 15}) # Register single resource from a single pack specified multiple times - verify that # resources from the same pack are only registered once From cffe8f268d44b71a408995767d3f5aab6bfe6fc1 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 3 Jan 2019 17:53:33 +0100 Subject: [PATCH 2/8] Use remove_windows_and_cloudslang_runner st2-packages branch. --- .circleci/config.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 78558480f2..caac1664e6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -121,6 +121,7 @@ jobs: - ST2_PACKAGES: "st2" - ST2_CHECKOUT: 0 - ST2_GITDIR: /tmp/st2 + - CIRCLE_BRANCH: "remove_windows_and_cloudslang_runner" - BASH_ENV: ~/.buildenv steps: - checkout From b6d11da06d019945f63b95d209b3715f939d171b Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 7 Jan 2019 14:15:01 +0100 Subject: [PATCH 3/8] Fix two issues in FileWatchSensor: 1. Mitigate / avoid race when calling add_file() before tailer has been initialized 2. Fix a bug with failing to dispatch a trigger due to code using invalid trigger type reference (it uses trigger ref instead of trigger type ref) --- contrib/linux/sensors/file_watch_sensor.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contrib/linux/sensors/file_watch_sensor.py b/contrib/linux/sensors/file_watch_sensor.py index d0a74c71a9..547d4306ef 100644 --- a/contrib/linux/sensors/file_watch_sensor.py +++ b/contrib/linux/sensors/file_watch_sensor.py @@ -1,5 +1,7 @@ import os +import eventlet + from logshipper.tail import Tail from st2reactor.sensor.base import Sensor @@ -10,6 +12,7 @@ def __init__(self, sensor_service, config=None): super(FileWatchSensor, self).__init__(sensor_service=sensor_service, config=config) self._trigger = None + self._trigger_type_ref = 'linux.file_watch.line' self._logger = self._sensor_service.get_logger(__name__) self._tail = None @@ -42,6 +45,9 @@ def add_trigger(self, trigger): if not self._trigger: raise Exception('Trigger %s did not contain a ref.' % trigger) + # Wait a bit to avoid initialization race in logshipper library + eventlet.sleep(1.0) + self._tail.add_file(filename=file_path) self._logger.info('Added file "%s"' % (file_path)) @@ -61,7 +67,7 @@ def remove_trigger(self, trigger): self._logger.info('Removed file "%s"' % (file_path)) def _handle_line(self, file_path, line): - trigger = self._trigger + trigger = self._trigger_type_ref payload = { 'file_path': file_path, 'file_name': os.path.basename(file_path), From c8f45a85aacf7e00bfe7695f175bc3ea46c72a71 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 7 Jan 2019 14:23:53 +0100 Subject: [PATCH 4/8] Fix validate_trigger_payload() method so it also works if a trigger reference and not trigger type reference is passed to it. --- contrib/linux/sensors/file_watch_sensor.py | 3 +-- st2common/st2common/validators/api/reactor.py | 13 ++++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/contrib/linux/sensors/file_watch_sensor.py b/contrib/linux/sensors/file_watch_sensor.py index 547d4306ef..3768e7f4a6 100644 --- a/contrib/linux/sensors/file_watch_sensor.py +++ b/contrib/linux/sensors/file_watch_sensor.py @@ -12,7 +12,6 @@ def __init__(self, sensor_service, config=None): super(FileWatchSensor, self).__init__(sensor_service=sensor_service, config=config) self._trigger = None - self._trigger_type_ref = 'linux.file_watch.line' self._logger = self._sensor_service.get_logger(__name__) self._tail = None @@ -67,7 +66,7 @@ def remove_trigger(self, trigger): self._logger.info('Removed file "%s"' % (file_path)) def _handle_line(self, file_path, line): - trigger = self._trigger_type_ref + trigger = self._trigger payload = { 'file_path': file_path, 'file_name': os.path.basename(file_path), diff --git a/st2common/st2common/validators/api/reactor.py b/st2common/st2common/validators/api/reactor.py index 0e0f4397bc..875417b51d 100644 --- a/st2common/st2common/validators/api/reactor.py +++ b/st2common/st2common/validators/api/reactor.py @@ -113,7 +113,7 @@ def validate_trigger_payload(trigger_type_ref, payload, throw_on_inexistent_trig """ This function validates trigger payload parameters for system and user-defined triggers. - :param trigger_type_ref: Reference of a trigger type or a trigger dictionary object. + :param trigger_type_ref: Reference of a trigger type / trigger / trigger dictionary object. :type trigger_type_ref: ``str`` :param payload: Trigger payload. @@ -144,9 +144,20 @@ def validate_trigger_payload(trigger_type_ref, payload, throw_on_inexistent_trig # System trigger payload_schema = SYSTEM_TRIGGER_TYPES[trigger_type_ref]['payload_schema'] else: + # 1. First assume we received TriggerType reference trigger_type_db = triggers.get_trigger_type_db(trigger_type_ref) + if not trigger_type_db: + # 2. If TriggerType was not found, assume we received a Trigger reference # Trigger doesn't exist in the database + trigger_db = triggers.get_trigger_db_by_ref(trigger_type_ref) + + if trigger_db: + trigger_type_db = triggers.get_trigger_type_db(trigger_db.type) + else: + trigger_type_db = None + + if not trigger_type_db: if throw_on_inexistent_trigger: msg = ('Trigger type with reference "%s" doesn\'t exist in the database' % (trigger_type_ref)) From 2e113d155539ecb68c1dc7de5d9c2f876457d647 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 7 Jan 2019 14:27:33 +0100 Subject: [PATCH 5/8] Revert "Use remove_windows_and_cloudslang_runner st2-packages branch." This reverts commit 22bb9a06756924d340fd8c674c4e3f4eefd7d4ce. --- .circleci/config.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index caac1664e6..78558480f2 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -121,7 +121,6 @@ jobs: - ST2_PACKAGES: "st2" - ST2_CHECKOUT: 0 - ST2_GITDIR: /tmp/st2 - - CIRCLE_BRANCH: "remove_windows_and_cloudslang_runner" - BASH_ENV: ~/.buildenv steps: - checkout From a78c317ccc2e81108f67187b31d695f8fa35e23c Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 7 Jan 2019 14:34:57 +0100 Subject: [PATCH 6/8] Update inaccurate comment. --- st2common/st2common/services/trigger_dispatcher.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/services/trigger_dispatcher.py b/st2common/st2common/services/trigger_dispatcher.py index 14850a595d..92a46825ae 100644 --- a/st2common/st2common/services/trigger_dispatcher.py +++ b/st2common/st2common/services/trigger_dispatcher.py @@ -40,7 +40,7 @@ def dispatch(self, trigger, payload=None, trace_tag=None, throw_on_validation_er """ Method which dispatches the trigger. - :param trigger: Reference to the TriggerType (.). + :param trigger: Reference to the TriggerTypeDB (.) or TriggerDB object. :type trigger: ``str`` :param payload: Trigger payload. @@ -64,7 +64,7 @@ def dispatch_with_context(self, trigger, payload=None, trace_context=None, """ Method which dispatches the trigger. - :param trigger: Reference to the TriggerType (.). + :param trigger: Reference to the TriggerTypeDB (.) or TriggerDB object. :type trigger: ``str`` :param payload: Trigger payload. From 2390a5a44323bf9e06098fdb398b5081d2ce704c Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 7 Jan 2019 15:59:32 +0100 Subject: [PATCH 7/8] Determine if the trigger_type_ref is actually a trigger ref based if the value is a valid UUID4. --- st2common/st2common/validators/api/reactor.py | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/st2common/st2common/validators/api/reactor.py b/st2common/st2common/validators/api/reactor.py index 875417b51d..35cbcb9d69 100644 --- a/st2common/st2common/validators/api/reactor.py +++ b/st2common/st2common/validators/api/reactor.py @@ -14,8 +14,9 @@ # limitations under the License. from __future__ import absolute_import -import six +import six +import uuid from oslo_config import cfg from apscheduler.triggers.cron import CronTrigger @@ -144,20 +145,25 @@ def validate_trigger_payload(trigger_type_ref, payload, throw_on_inexistent_trig # System trigger payload_schema = SYSTEM_TRIGGER_TYPES[trigger_type_ref]['payload_schema'] else: - # 1. First assume we received TriggerType reference - trigger_type_db = triggers.get_trigger_type_db(trigger_type_ref) + # We assume Trigger ref and not TriggerType ref is passed in if second + # part (trigger name) is a valid UUID version 4 + try: + trigger_uuid = uuid.UUID(trigger_type_ref.split('.')[-1]) + except ValueError: + is_trigger_db = False + else: + is_trigger_db = (trigger_uuid.version == 4) - if not trigger_type_db: - # 2. If TriggerType was not found, assume we received a Trigger reference - # Trigger doesn't exist in the database + if is_trigger_db: trigger_db = triggers.get_trigger_db_by_ref(trigger_type_ref) if trigger_db: - trigger_type_db = triggers.get_trigger_type_db(trigger_db.type) - else: - trigger_type_db = None + trigger_type_ref = trigger_db.type + + trigger_type_db = triggers.get_trigger_type_db(trigger_type_ref) if not trigger_type_db: + # Trigger doesn't exist in the database if throw_on_inexistent_trigger: msg = ('Trigger type with reference "%s" doesn\'t exist in the database' % (trigger_type_ref)) From 290e7f1fa568f5074a71003fc72901334fb77c87 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 7 Jan 2019 16:04:00 +0100 Subject: [PATCH 8/8] Add a test case for it. --- st2reactor/tests/unit/test_sensor_service.py | 53 ++++++++++++++++---- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/st2reactor/tests/unit/test_sensor_service.py b/st2reactor/tests/unit/test_sensor_service.py index d3058b77f0..214b28dc69 100644 --- a/st2reactor/tests/unit/test_sensor_service.py +++ b/st2reactor/tests/unit/test_sensor_service.py @@ -28,9 +28,14 @@ } -class TriggerTypeMock(object): - def __init__(self, schema={}): - self.payload_schema = schema +class TriggerTypeDBMock(object): + def __init__(self, schema=None): + self.payload_schema = schema or {} + + +class TriggerDBMock(object): + def __init__(self, type=None): + self.type = type class SensorServiceTestCase(unittest2.TestCase): @@ -54,7 +59,7 @@ def tearDown(self): cfg.CONF.system.validate_trigger_payload = self.validate_trigger_payload @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock(TEST_SCHEMA))) + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) def test_dispatch_success_valid_payload_validation_enabled(self): cfg.CONF.system.validate_trigger_payload = True @@ -75,7 +80,33 @@ def test_dispatch_success_valid_payload_validation_enabled(self): self.assertEqual(self._dispatched_count, 1) @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock(TEST_SCHEMA))) + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) + @mock.patch('st2common.services.triggers.get_trigger_db_by_ref', + mock.MagicMock(return_value=TriggerDBMock(type='trigger-type-ref'))) + def test_dispatch_success_with_validation_enabled_trigger_reference(self): + # Test a scenario where a Trigger ref and not TriggerType ref is provided + cfg.CONF.system.validate_trigger_payload = True + + # define a valid payload + payload = { + 'name': 'John Doe', + 'age': 25, + 'career': ['foo, Inc.', 'bar, Inc.'], + 'married': True, + 'awards': {'2016': ['hoge prize', 'fuga prize']}, + 'income': 50000 + } + + self.assertEqual(self._dispatched_count, 0) + + # dispatching a trigger + self.sensor_service.dispatch('pack.86582f21-1fbc-44ea-88cb-0cd2b610e93b', payload) + + # This assumed that the target tirgger dispatched + self.assertEqual(self._dispatched_count, 1) + + @mock.patch('st2common.services.triggers.get_trigger_type_db', + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) def test_dispatch_success_with_validation_disabled_and_invalid_payload(self): """ Tests that an invalid payload still results in dispatch success with default config @@ -108,7 +139,7 @@ def test_dispatch_success_with_validation_disabled_and_invalid_payload(self): self.assertEqual(self._dispatched_count, 1) @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock(TEST_SCHEMA))) + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) def test_dispatch_failure_caused_by_incorrect_type(self): # define a invalid payload (the type of 'age' is incorrect) payload = { @@ -131,7 +162,7 @@ def test_dispatch_failure_caused_by_incorrect_type(self): self.assertEqual(self._dispatched_count, 1) @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock(TEST_SCHEMA))) + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) def test_dispatch_failure_caused_by_lack_of_required_parameter(self): # define a invalid payload (lack of required property) payload = { @@ -149,7 +180,7 @@ def test_dispatch_failure_caused_by_lack_of_required_parameter(self): self.assertEqual(self._dispatched_count, 1) @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock(TEST_SCHEMA))) + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) def test_dispatch_failure_caused_by_extra_parameter(self): # define a invalid payload ('hobby' is extra) payload = { @@ -162,7 +193,7 @@ def test_dispatch_failure_caused_by_extra_parameter(self): self.assertEqual(self._dispatched_count, 0) @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock(TEST_SCHEMA))) + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) def test_dispatch_success_with_multiple_type_value(self): payload = { 'name': 'John Doe', @@ -180,7 +211,7 @@ def test_dispatch_success_with_multiple_type_value(self): self.assertEqual(self._dispatched_count, 2) @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock(TEST_SCHEMA))) + mock.MagicMock(return_value=TriggerTypeDBMock(TEST_SCHEMA))) def test_dispatch_success_with_null(self): payload = { 'name': 'John Doe', @@ -193,7 +224,7 @@ def test_dispatch_success_with_null(self): self.assertEqual(self._dispatched_count, 1) @mock.patch('st2common.services.triggers.get_trigger_type_db', - mock.MagicMock(return_value=TriggerTypeMock())) + mock.MagicMock(return_value=TriggerTypeDBMock())) def test_dispatch_success_without_payload_schema(self): # the case trigger has no property self.sensor_service.dispatch('trigger-name', {})