From 42d6f08b628bbea49fa4d96ef7f1b94c92f6dc10 Mon Sep 17 00:00:00 2001 From: ashwini-orchestral Date: Thu, 18 Feb 2021 16:19:53 +0000 Subject: [PATCH 1/4] Assert change - replaced assert with if and raised an appropriate exception --- .../action_chain_runner.py | 6 ++- .../python_runner/python_action_wrapper.py | 3 +- .../python_runner/python_runner.py | 3 +- .../st2api/controllers/v1/actionexecutions.py | 18 +++++--- st2api/st2api/controllers/v1/keyvalue.py | 18 +++++--- st2api/st2api/controllers/v1/triggers.py | 3 +- .../st2common/bootstrap/actionsregistrar.py | 3 +- .../st2common/bootstrap/aliasesregistrar.py | 3 +- .../st2common/bootstrap/configsregistrar.py | 3 +- .../st2common/bootstrap/policiesregistrar.py | 3 +- .../st2common/bootstrap/rulesregistrar.py | 3 +- .../st2common/bootstrap/sensorsregistrar.py | 3 +- .../st2common/bootstrap/triggersregistrar.py | 3 +- st2common/st2common/content/loader.py | 6 ++- st2common/st2common/content/utils.py | 3 +- st2common/st2common/models/api/keyvalue.py | 3 +- st2common/st2common/models/system/action.py | 3 +- st2common/st2common/rbac/types.py | 6 ++- st2common/st2common/runners/base.py | 9 ++-- st2common/st2common/services/trace.py | 3 +- st2common/st2common/services/triggers.py | 6 ++- st2common/st2common/transport/announcement.py | 6 ++- st2common/st2common/transport/reactor.py | 6 ++- st2common/st2common/util/crypto.py | 29 ++++++++----- st2common/st2common/util/green/shell.py | 3 +- st2common/st2common/util/sandboxing.py | 42 ++----------------- st2common/st2common/util/shell.py | 3 +- st2common/st2common/util/templating.py | 3 +- .../st2reactor/container/sensor_wrapper.py | 3 +- .../st2reactor/garbage_collector/base.py | 9 ++-- st2reactor/st2reactor/rules/enforcer.py | 3 +- 31 files changed, 119 insertions(+), 99 deletions(-) diff --git a/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py b/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py index 39cb873136..b28501a92a 100644 --- a/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py +++ b/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py @@ -824,8 +824,10 @@ def _format_action_exec_result(self, action_node, liveaction_db, created_at, upd :rtype: ``dict`` """ - assert isinstance(created_at, datetime.datetime) - assert isinstance(updated_at, datetime.datetime) + if not isinstance(created_at, datetime.datetime): + raise TypeError("created_at is not a datetime object") + if not isinstance(updated_at, datetime.datetime): + raise TypeError("updated_at is not a datetime object") result = {} diff --git a/contrib/runners/python_runner/python_runner/python_action_wrapper.py b/contrib/runners/python_runner/python_runner/python_action_wrapper.py index 119f6bdf84..844634d6a5 100644 --- a/contrib/runners/python_runner/python_runner/python_action_wrapper.py +++ b/contrib/runners/python_runner/python_runner/python_action_wrapper.py @@ -322,7 +322,8 @@ def _get_action_instance(self): LOG.debug('Received parameters: %s', parameters) - assert isinstance(parent_args, list) + if not isinstance(parent_args, list): + raise ValueError("Parent_args needs a list of values") obj = PythonActionWrapper(pack=args.pack, file_path=args.file_path, config=config, diff --git a/contrib/runners/python_runner/python_runner/python_runner.py b/contrib/runners/python_runner/python_runner/python_runner.py index fd412c890e..6be7914aa3 100644 --- a/contrib/runners/python_runner/python_runner/python_runner.py +++ b/contrib/runners/python_runner/python_runner/python_runner.py @@ -322,7 +322,8 @@ def _get_output_values(self, exit_code, stdout, stderr, timed_out): if ACTION_OUTPUT_RESULT_DELIMITER in stdout: split = stdout.split(ACTION_OUTPUT_RESULT_DELIMITER) - assert len(split) == 3 + if not len(split) == 3: + raise ValueError("Length should be 3") action_result = split[1].strip() stdout = split[0] + split[2] else: diff --git a/st2api/st2api/controllers/v1/actionexecutions.py b/st2api/st2api/controllers/v1/actionexecutions.py index b0aa4e9e1d..f5d3a359d4 100644 --- a/st2api/st2api/controllers/v1/actionexecutions.py +++ b/st2api/st2api/controllers/v1/actionexecutions.py @@ -378,13 +378,16 @@ def validate(self): 're-running task(s) for a workflow.') if self.parameters: - assert isinstance(self.parameters, dict) + if not isinstance(self.parameters, dict): + raise ValueError("Parameters needs to be a dictionary") if self.tasks: - assert isinstance(self.tasks, list) + if not isinstance(self.tasks, list): + raise ValueError("Tasks needs to be a list") if self.reset: - assert isinstance(self.reset, list) + if not isinstance(self.reset, list): + raise ValueError("Reset needs to be a list") if list(set(self.reset) - set(self.tasks)): raise ValueError('List of tasks to reset does not match the tasks to rerun.') @@ -405,13 +408,16 @@ def post(self, spec_api, id, requester_user, no_merge=False, show_secrets=False) 're-running task(s) for a workflow.') if spec_api.parameters: - assert isinstance(spec_api.parameters, dict) + if not isinstance(spec_api.parameters, dict): + raise ValueError("Parameters needs to be a list") if spec_api.tasks: - assert isinstance(spec_api.tasks, list) + if not isinstance(spec_api.tasks, list): + raise ValueError("Tasks needs to be a list") if spec_api.reset: - assert isinstance(spec_api.reset, list) + if not isinstance(spec_api.reset, list): + raise ValueError("reset needs to be a list") if list(set(spec_api.reset) - set(spec_api.tasks)): raise ValueError('List of tasks to reset does not match the tasks to rerun.') diff --git a/st2api/st2api/controllers/v1/keyvalue.py b/st2api/st2api/controllers/v1/keyvalue.py index eab8cb025a..d768500948 100644 --- a/st2api/st2api/controllers/v1/keyvalue.py +++ b/st2api/st2api/controllers/v1/keyvalue.py @@ -193,7 +193,8 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non raw_filters['scope'] = FULL_SYSTEM_SCOPE raw_filters['prefix'] = prefix - assert 'scope' in raw_filters + if 'scope'not in raw_filters: + raise KeyError("The key scope is not in raw_filters") kvp_apis_system = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, @@ -212,8 +213,10 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non else: raw_filters['prefix'] = user_scope_prefix - assert 'scope' in raw_filters - assert 'prefix' in raw_filters + if 'scope'not in raw_filters: + raise KeyError("The key scope is not found in raw_filters") + if 'prefix'not in raw_filters: + raise KeyError("The key prefix is not in raw_filters") kvp_apis_user = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, @@ -231,8 +234,10 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non prefix = get_key_reference(name=prefix or '', scope=scope, user=user) raw_filters['prefix'] = user_scope_prefix - assert 'scope' in raw_filters - assert 'prefix' in raw_filters + if 'scope' not in raw_filters: + raise KeyError("The key scope is not found in raw_filters") + if 'prefix' not in raw_filters: + raise KeyError("The key prefix is not in raw_filters") kvp_apis = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, @@ -243,7 +248,8 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non elif scope in [SYSTEM_SCOPE, FULL_SYSTEM_SCOPE]: raw_filters['prefix'] = prefix - assert 'scope' in raw_filters + if 'scope' not in raw_filters: + raise KeyError("The key scope is not found in raw_filters") kvp_apis = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, diff --git a/st2api/st2api/controllers/v1/triggers.py b/st2api/st2api/controllers/v1/triggers.py index 12c3f133ec..9577ad1354 100644 --- a/st2api/st2api/controllers/v1/triggers.py +++ b/st2api/st2api/controllers/v1/triggers.py @@ -324,7 +324,8 @@ def __init__(self, payload=None): def validate(self): if self.payload: - assert isinstance(self.payload, dict) + if not isinstance(self.payload, dict): + raise TypeError("The object is not a dict type") return True diff --git a/st2common/st2common/bootstrap/actionsregistrar.py b/st2common/st2common/bootstrap/actionsregistrar.py index c21788fb14..6839d9996a 100644 --- a/st2common/st2common/bootstrap/actionsregistrar.py +++ b/st2common/st2common/bootstrap/actionsregistrar.py @@ -210,7 +210,8 @@ def _register_actions_from_pack(self, pack, actions): def register_actions(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: - assert isinstance(packs_base_paths, list) + if not isinstance(packs_base_paths, list): + raise ValueError("Pack base paths needs to be a list") if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/aliasesregistrar.py b/st2common/st2common/bootstrap/aliasesregistrar.py index dbc9c3b0fc..c6c9863f37 100644 --- a/st2common/st2common/bootstrap/aliasesregistrar.py +++ b/st2common/st2common/bootstrap/aliasesregistrar.py @@ -191,7 +191,8 @@ def register_aliases(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: - assert isinstance(packs_base_paths, list) + if not isinstance(packs_base_paths, list): + raise TypeError("The object is not a List type") if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/configsregistrar.py b/st2common/st2common/bootstrap/configsregistrar.py index fc7e05eb98..708510f9db 100644 --- a/st2common/st2common/bootstrap/configsregistrar.py +++ b/st2common/st2common/bootstrap/configsregistrar.py @@ -149,7 +149,8 @@ def register_configs(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False, validate_configs=True): if packs_base_paths: - assert isinstance(packs_base_paths, list) + if not isinstance(packs_base_paths, list): + raise ValueError("Pack Base paths needs to be a list") if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/policiesregistrar.py b/st2common/st2common/bootstrap/policiesregistrar.py index b963eaf097..75e4170a9b 100644 --- a/st2common/st2common/bootstrap/policiesregistrar.py +++ b/st2common/st2common/bootstrap/policiesregistrar.py @@ -206,7 +206,8 @@ def register_policy_types(module): def register_policies(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: - assert isinstance(packs_base_paths, list) + if not isinstance(packs_base_paths, list): + raise TypeError("The object is not a list type") if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/rulesregistrar.py b/st2common/st2common/bootstrap/rulesregistrar.py index c50b0d5eae..9b5aef35ac 100644 --- a/st2common/st2common/bootstrap/rulesregistrar.py +++ b/st2common/st2common/bootstrap/rulesregistrar.py @@ -185,7 +185,8 @@ def _register_rules_from_pack(self, pack, rules): def register_rules(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: - assert isinstance(packs_base_paths, list) + if not isinstance(packs_base_paths, list): + raise ValueError("Pack base paths needs to be a list") if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/sensorsregistrar.py b/st2common/st2common/bootstrap/sensorsregistrar.py index 5181270d79..4e1a549a98 100644 --- a/st2common/st2common/bootstrap/sensorsregistrar.py +++ b/st2common/st2common/bootstrap/sensorsregistrar.py @@ -178,7 +178,8 @@ def _register_sensor_from_pack(self, pack, sensor): def register_sensors(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: - assert isinstance(packs_base_paths, list) + if not isinstance(packs_base_paths, list): + raise TypeError("The object is not a list type") if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/triggersregistrar.py b/st2common/st2common/bootstrap/triggersregistrar.py index 4f95a6d0a3..fdc457c9f8 100644 --- a/st2common/st2common/bootstrap/triggersregistrar.py +++ b/st2common/st2common/bootstrap/triggersregistrar.py @@ -154,7 +154,8 @@ def _register_trigger_from_pack(self, pack_base_path, pack, trigger): def register_triggers(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: - assert isinstance(packs_base_paths, list) + if not isinstance(packs_base_paths, list): + raise ValueError("Pack base paths needs to be a list") if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 0dfae4c0b6..c5c319ae53 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -61,7 +61,8 @@ def get_packs(self, base_dirs): directory. :rtype: ``dict`` """ - assert isinstance(base_dirs, list) + if not isinstance(base_dirs, list): + raise TypeError("Base dirs needs to be a list") result = {} for base_dir in base_dirs: @@ -88,7 +89,8 @@ def get_content(self, base_dirs, content_type): :rtype: ``dict`` """ - assert isinstance(base_dirs, list) + if not isinstance(base_dirs, list): + raise TypeError("Base dirs needs to be a list") if content_type not in self.ALLOWED_CONTENT_TYPES: raise ValueError('Unsupported content_type: %s' % (content_type)) diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index 3bd5e2b12c..147eea87d7 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -281,7 +281,8 @@ def get_pack_file_abs_path(pack_ref, file_path, resource_type=None, use_pack_cac path_components.append(normalized_file_path) result = os.path.join(*path_components) # pylint: disable=E1120 - assert normalized_file_path in result + if normalized_file_path not in result: + raise ValueError("This is not normalized path to prevent directory traversal") # Final safety check for common prefix to avoid traversal attack common_prefix = os.path.commonprefix([pack_base_path, result]) diff --git a/st2common/st2common/models/api/keyvalue.py b/st2common/st2common/models/api/keyvalue.py index 8365350ef7..8955baa031 100644 --- a/st2common/st2common/models/api/keyvalue.py +++ b/st2common/st2common/models/api/keyvalue.py @@ -188,7 +188,8 @@ def to_model(cls, kvp): raise ValueError(msg) # Additional safety check to ensure that the value hasn't been decrypted - assert value == original_value + if not value == original_value: + raise ValueError("Value doesn't match with original value") elif secret: cls._verif_key_is_set_up(name=name) diff --git a/st2common/st2common/models/system/action.py b/st2common/st2common/models/system/action.py index b5efe124f5..1487550c5e 100644 --- a/st2common/st2common/models/system/action.py +++ b/st2common/st2common/models/system/action.py @@ -177,7 +177,8 @@ def _get_command_string(self, cmd, args): :rtype: ``str`` """ - assert isinstance(args, (list, tuple)) + if not isinstance(args, (list, tuple)): + raise ValueError("Args needs to be a list and tuple") args = [quote_unix(arg) for arg in args] args = ' '.join(args) diff --git a/st2common/st2common/rbac/types.py b/st2common/st2common/rbac/types.py index 1c6b0ea352..566ffad4b3 100644 --- a/st2common/st2common/rbac/types.py +++ b/st2common/st2common/rbac/types.py @@ -184,7 +184,8 @@ def get_resource_type(cls, permission_type): return ResourceType.EXECUTION split = permission_type.split('_') - assert len(split) >= 2 + if not len(split) >= 2: + raise ValueError("Length should be greater than or equal to 2") return '_'.join(split[:-1]) @@ -196,7 +197,8 @@ def get_permission_name(cls, permission_type): :rtype: ``str`` """ split = permission_type.split('_') - assert len(split) >= 2 + if not len(split) >= 2: + raise ValueError("Length should be greater than or equal to 2") # Special case for PACK_VIEWS_INDEX_HEALTH if permission_type == PermissionType.PACK_VIEWS_INDEX_HEALTH: diff --git a/st2common/st2common/runners/base.py b/st2common/st2common/runners/base.py index 6a9656b9a1..e2954657f6 100644 --- a/st2common/st2common/runners/base.py +++ b/st2common/st2common/runners/base.py @@ -288,7 +288,8 @@ def pre_run(self): entry_point=self.entry_point, worktree_path=self.git_worktree_path) - assert(entry_point.startswith(self.git_worktree_path)) + if not (entry_point.startswith(self.git_worktree_path)): + raise ValueError("Value not matching") self.entry_point = entry_point @@ -375,8 +376,10 @@ def cleanup_git_worktree(self, worktree_path, pack_name, content_version): :rtype: ``bool`` """ # Safety check to make sure we don't remove something outside /tmp - assert(worktree_path.startswith('/tmp')) - assert(worktree_path.startswith('/tmp/%s' % (self.WORKTREE_DIRECTORY_PREFIX))) + if not (worktree_path.startswith('/tmp')): + raise ValueError("Value not matching") + if not (worktree_path.startswith('/tmp/%s' % (self.WORKTREE_DIRECTORY_PREFIX))): + raise ValueError("Value not matching") if self._debug: LOG.debug('Not removing git worktree "%s" because debug mode is enabled' % diff --git a/st2common/st2common/services/trace.py b/st2common/st2common/services/trace.py index 3eb92bd2f1..40a83123be 100644 --- a/st2common/st2common/services/trace.py +++ b/st2common/st2common/services/trace.py @@ -54,7 +54,8 @@ def _get_valid_trace_context(trace_context): """ Check if tarce_context is a valid type and returns a TraceContext object. """ - assert isinstance(trace_context, (TraceContext, dict)) + if not isinstance(trace_context, (TraceContext, dict)): + raise TypeError("Not a valid trace_context type") # Pretty much abuse the dynamic nature of python to make it possible to support # both dict and TraceContext types. diff --git a/st2common/st2common/services/triggers.py b/st2common/st2common/services/triggers.py index 6448aa2533..efc2909b00 100644 --- a/st2common/st2common/services/triggers.py +++ b/st2common/st2common/services/triggers.py @@ -253,7 +253,8 @@ def create_or_update_trigger_db(trigger, log_not_unique_error_as_debug=False): :param trigger: Trigger info. :type trigger: ``dict`` """ - assert isinstance(trigger, dict) + if not isinstance(trigger, dict): + raise ValueError("Trigger needs to be a dict object") existing_trigger_db = _get_trigger_db(trigger) @@ -406,7 +407,8 @@ def create_or_update_trigger_type_db(trigger_type, log_not_unique_error_as_debug :rtype: ``object`` """ - assert isinstance(trigger_type, dict) + if not isinstance(trigger_type, dict): + raise ValueError("trigger needs to be a dict object") trigger_type_api = TriggerTypeAPI(**trigger_type) trigger_type_api.validate() diff --git a/st2common/st2common/transport/announcement.py b/st2common/st2common/transport/announcement.py index 84c8bf27a7..cbcf3f77b6 100644 --- a/st2common/st2common/transport/announcement.py +++ b/st2common/st2common/transport/announcement.py @@ -65,8 +65,10 @@ def dispatch(self, routing_key, payload, trace_context=None): :param trace_context: Trace context to associate with Announcement. :type trace_context: ``TraceContext`` """ - assert isinstance(payload, (type(None), dict)) - assert isinstance(trace_context, (type(None), dict, TraceContext)) + if not isinstance(payload, (type(None), dict)): + raise TypeError("Object is not a dict type") + if not isinstance(trace_context, (type(None), dict, TraceContext)): + raise TypeError("Object is not a TraceContext type") payload = { 'payload': payload, diff --git a/st2common/st2common/transport/reactor.py b/st2common/st2common/transport/reactor.py index 613a1d08ed..2578b39901 100644 --- a/st2common/st2common/transport/reactor.py +++ b/st2common/st2common/transport/reactor.py @@ -93,8 +93,10 @@ def dispatch(self, trigger, payload=None, trace_context=None): :param trace_context: Trace context to associate with Trigger. :type trace_context: ``TraceContext`` """ - assert isinstance(payload, (type(None), dict)) - assert isinstance(trace_context, (type(None), TraceContext)) + if not isinstance(payload, (type(None), dict)): + raise TypeError("Object is not a dict type") + if not isinstance(trace_context, (type(None), TraceContext)): + raise TypeError("Object is not a TraceContext type") payload = { 'trigger': trigger, diff --git a/st2common/st2common/util/crypto.py b/st2common/st2common/util/crypto.py index 230c4ada8e..4559b0aee9 100644 --- a/st2common/st2common/util/crypto.py +++ b/st2common/st2common/util/crypto.py @@ -80,7 +80,8 @@ DEFAULT_AES_KEY_SIZE = 256 -assert DEFAULT_AES_KEY_SIZE >= MINIMUM_AES_KEY_SIZE +if not DEFAULT_AES_KEY_SIZE >= MINIMUM_AES_KEY_SIZE: + raise ValueError('Verify the key size :%s' % (DEFAULT_AES_KEY_SIZE)) class AESKey(object): @@ -206,15 +207,18 @@ def cryptography_symmetric_encrypt(encrypt_key, plaintext): NOTE: Header itself is unused, but it's added so the format is compatible with keyczar format. """ - assert isinstance(encrypt_key, AESKey), 'encrypt_key needs to be AESKey class instance' - assert isinstance(plaintext, (six.text_type, six.string_types, six.binary_type)), \ - 'plaintext needs to either be a string/unicode or bytes' + if not isinstance(encrypt_key, AESKey): + raise ValueError("Encrypt key needs to be an AESkey class instance") + if not isinstance(plaintext, (six.text_type, six.string_types, six.binary_type)): + raise TypeError("Plaintext needs to either be a string/unicode or bytes") aes_key_bytes = encrypt_key.aes_key_bytes hmac_key_bytes = encrypt_key.hmac_key_bytes - assert isinstance(aes_key_bytes, six.binary_type) - assert isinstance(hmac_key_bytes, six.binary_type) + if not isinstance(aes_key_bytes, six.binary_type): + raise TypeError("AESKey bytes matching with binary type") + if not isinstance(hmac_key_bytes, six.binary_type): + raise TypeError("HMACKey bytes matching with binary type") if isinstance(plaintext, (six.text_type, six.string_types)): # Convert data to bytes @@ -263,15 +267,18 @@ def cryptography_symmetric_decrypt(decrypt_key, ciphertext): NOTE 2: This function is loosely based on keyczar AESKey.Decrypt() (Apache 2.0 license). """ - assert isinstance(decrypt_key, AESKey), 'decrypt_key needs to be AESKey class instance' - assert isinstance(ciphertext, (six.text_type, six.string_types, six.binary_type)), \ - 'ciphertext needs to either be a string/unicode or bytes' + if not isinstance(decrypt_key, AESKey): + raise ValueError("Decrypt key needs to be an AESKey class instance") + if not isinstance(ciphertext, (six.text_type, six.string_types, six.binary_type)): + raise TypeError("Ciphertext needs to either be a string/unicode or bytes") aes_key_bytes = decrypt_key.aes_key_bytes hmac_key_bytes = decrypt_key.hmac_key_bytes - assert isinstance(aes_key_bytes, six.binary_type) - assert isinstance(hmac_key_bytes, six.binary_type) + if not isinstance(aes_key_bytes, six.binary_type): + raise TypeError("Key Bytes type not matching") + if not isinstance(hmac_key_bytes, six.binary_type): + raise TypeError("Value Type not matching") # Convert from hex notation ASCII string to bytes ciphertext = binascii.unhexlify(ciphertext) diff --git a/st2common/st2common/util/green/shell.py b/st2common/st2common/util/green/shell.py index 4fd71ef7cf..4f7c893e9c 100644 --- a/st2common/st2common/util/green/shell.py +++ b/st2common/st2common/util/green/shell.py @@ -91,7 +91,8 @@ def run_command(cmd, stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, """ LOG.debug('Entering st2common.util.green.run_command.') - assert isinstance(cmd, (list, tuple) + six.string_types) + if not isinstance(cmd, (list, tuple) + six.string_types): + raise TypeError("cmd must be a type of (list, tuple) string ") if (read_stdout_func and not read_stderr_func) or (read_stderr_func and not read_stdout_func): raise ValueError('Both read_stdout_func and read_stderr_func arguments need ' diff --git a/st2common/st2common/util/sandboxing.py b/st2common/st2common/util/sandboxing.py index 02871f7472..d7a921f466 100644 --- a/st2common/st2common/util/sandboxing.py +++ b/st2common/st2common/util/sandboxing.py @@ -20,7 +20,6 @@ from __future__ import absolute_import -import fnmatch import os import sys from distutils.sysconfig import get_python_lib @@ -28,7 +27,6 @@ from oslo_config import cfg from st2common.constants.pack import SYSTEM_PACK_NAMES -from st2common.content.utils import get_pack_base_path __all__ = [ 'get_sandbox_python_binary_path', @@ -117,7 +115,8 @@ def get_sandbox_python_path(inherit_from_parent=True, inherit_parent_virtualenv= site_packages_dir = get_python_lib() sys_prefix = os.path.abspath(sys.prefix) - assert sys_prefix in site_packages_dir + if sys_prefix not in site_packages_dir: + raise ValueError("Sys_prefix file not identified in directory") sandbox_python_path.append(site_packages_dir) @@ -134,45 +133,10 @@ def get_sandbox_python_path_for_python_action(pack, inherit_from_parent=True, Same as get_sandbox_python_path() function, but it's intended to be used for Python runner actions. """ - sandbox_python_path = get_sandbox_python_path( + return get_sandbox_python_path( inherit_from_parent=inherit_from_parent, inherit_parent_virtualenv=inherit_parent_virtualenv) - pack_base_path = get_pack_base_path(pack_name=pack) - virtualenv_path = get_sandbox_virtualenv_path(pack=pack) - - if virtualenv_path and os.path.isdir(virtualenv_path): - pack_virtualenv_lib_path = os.path.join(virtualenv_path, 'lib') - - virtualenv_directories = os.listdir(pack_virtualenv_lib_path) - virtualenv_directories = [dir_name for dir_name in virtualenv_directories if - fnmatch.fnmatch(dir_name, 'python*')] - - # Add the pack's lib directory (lib/python3.x) in front of the PYTHONPATH. - pack_actions_lib_paths = os.path.join(pack_base_path, 'actions', 'lib') - pack_virtualenv_lib_path = os.path.join(virtualenv_path, 'lib') - pack_venv_lib_directory = os.path.join(pack_virtualenv_lib_path, virtualenv_directories[0]) - - # Add the pack's site-packages directory (lib/python3.x/site-packages) - # in front of the Python system site-packages This is important because - # we want Python 3 compatible libraries to be used from the pack virtual - # environment and not system ones. - pack_venv_site_packages_directory = os.path.join(pack_virtualenv_lib_path, - virtualenv_directories[0], - 'site-packages') - - full_sandbox_python_path = [ - # NOTE: Order here is very important for imports to function correctly - pack_venv_lib_directory, - pack_venv_site_packages_directory, - pack_actions_lib_paths, - sandbox_python_path, - ] - - sandbox_python_path = ':'.join(full_sandbox_python_path) - - return sandbox_python_path - def get_sandbox_virtualenv_path(pack): """ diff --git a/st2common/st2common/util/shell.py b/st2common/st2common/util/shell.py index 5c4217594a..7083496c3d 100644 --- a/st2common/st2common/util/shell.py +++ b/st2common/st2common/util/shell.py @@ -74,7 +74,8 @@ def run_command(cmd, stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, :rtype: ``tuple`` (exit_code, stdout, stderr) """ - assert isinstance(cmd, (list, tuple) + six.string_types) + if not isinstance(cmd, (list, tuple) + six.string_types): + raise TypeError("cmd must be a type of (list, tuple) string ") if not env: env = os.environ.copy() diff --git a/st2common/st2common/util/templating.py b/st2common/st2common/util/templating.py index 9dc25d917c..da835c254b 100644 --- a/st2common/st2common/util/templating.py +++ b/st2common/st2common/util/templating.py @@ -40,7 +40,8 @@ def render_template(value, context=None): :param context: Template context. :type context: ``dict`` """ - assert isinstance(value, six.string_types) + if not isinstance(value, six.string_types): + raise TypeError("Value type does not match with string") context = context or {} env = get_jinja_environment(allow_undefined=False) # nosec diff --git a/st2reactor/st2reactor/container/sensor_wrapper.py b/st2reactor/st2reactor/container/sensor_wrapper.py index 56a37707d2..743083ac72 100644 --- a/st2reactor/st2reactor/container/sensor_wrapper.py +++ b/st2reactor/st2reactor/container/sensor_wrapper.py @@ -361,7 +361,8 @@ def _sanitize_trigger(self, trigger): trigger_types = args.trigger_type_refs trigger_types = trigger_types.split(',') if trigger_types else [] parent_args = json.loads(args.parent_args) if args.parent_args else [] - assert isinstance(parent_args, list) + if not isinstance(parent_args, list): + raise TypeError("Command line arguments passed to the parent process") obj = SensorWrapper(pack=args.pack, file_path=args.file_path, diff --git a/st2reactor/st2reactor/garbage_collector/base.py b/st2reactor/st2reactor/garbage_collector/base.py index 3261458677..db1d150d73 100644 --- a/st2reactor/st2reactor/garbage_collector/base.py +++ b/st2reactor/st2reactor/garbage_collector/base.py @@ -196,7 +196,8 @@ def _purge_action_executions(self): timestamp_str = isotime.format(dt=timestamp) LOG.info('Deleting action executions older than: %s' % (timestamp_str)) - assert timestamp < utc_now + if not timestamp < utc_now: + raise ValueError("utc_now violates the minimum time stamp") try: purge_executions(logger=LOG, timestamp=timestamp) @@ -216,7 +217,8 @@ def _purge_action_executions_output(self): timestamp_str = isotime.format(dt=timestamp) LOG.info('Deleting action executions output objects older than: %s' % (timestamp_str)) - assert timestamp < utc_now + if timestamp < utc_now: + raise ValueError("utc_now violates the minimum time stamp") try: purge_execution_output_objects(logger=LOG, timestamp=timestamp) @@ -239,7 +241,8 @@ def _purge_trigger_instances(self): timestamp_str = isotime.format(dt=timestamp) LOG.info('Deleting trigger instances older than: %s' % (timestamp_str)) - assert timestamp < utc_now + if timestamp < utc_now: + raise ValueError("utc_now violates the minimum time stamp") try: purge_trigger_instances(logger=LOG, timestamp=timestamp) diff --git a/st2reactor/st2reactor/rules/enforcer.py b/st2reactor/st2reactor/rules/enforcer.py index 594f157482..18a3f4edd9 100644 --- a/st2reactor/st2reactor/rules/enforcer.py +++ b/st2reactor/st2reactor/rules/enforcer.py @@ -160,7 +160,8 @@ def _update_trace(self): return None # This would signify some sort of coding error so assert. - assert trace_db + if not trace_db: + raise ValueError("tarcedb not found") trace_db = trace_service.add_or_update_given_trace_db( trace_db=trace_db, From fb54160ef65b56ed735a4eed2a941b4271c8ac7f Mon Sep 17 00:00:00 2001 From: ashwini-orchestral Date: Fri, 19 Feb 2021 12:59:06 +0000 Subject: [PATCH 2/4] Assertchanges - Code changes done as per review comments --- .../action_chain_runner.py | 4 +- .../python_runner/python_action_wrapper.py | 2 +- .../python_runner/python_runner.py | 2 +- .../st2api/controllers/v1/actionexecutions.py | 12 ++--- st2api/st2api/controllers/v1/keyvalue.py | 12 ++--- st2api/st2api/controllers/v1/triggers.py | 2 +- .../st2common/bootstrap/actionsregistrar.py | 2 +- .../st2common/bootstrap/aliasesregistrar.py | 2 +- .../st2common/bootstrap/configsregistrar.py | 2 +- .../st2common/bootstrap/policiesregistrar.py | 2 +- .../st2common/bootstrap/rulesregistrar.py | 2 +- .../st2common/bootstrap/sensorsregistrar.py | 2 +- .../st2common/bootstrap/triggersregistrar.py | 2 +- st2common/st2common/content/loader.py | 4 +- st2common/st2common/content/utils.py | 2 +- st2common/st2common/models/api/keyvalue.py | 2 +- st2common/st2common/models/system/action.py | 2 +- st2common/st2common/rbac/types.py | 4 +- st2common/st2common/runners/base.py | 7 +-- st2common/st2common/services/trace.py | 2 +- st2common/st2common/services/triggers.py | 4 +- st2common/st2common/transport/announcement.py | 4 +- st2common/st2common/transport/reactor.py | 4 +- st2common/st2common/util/crypto.py | 18 +++---- st2common/st2common/util/green/shell.py | 2 +- st2common/st2common/util/sandboxing.py | 49 +++++++++++++++---- st2common/st2common/util/shell.py | 2 +- st2common/st2common/util/templating.py | 2 +- .../st2reactor/container/sensor_wrapper.py | 2 +- .../st2reactor/garbage_collector/base.py | 6 +-- st2reactor/st2reactor/rules/enforcer.py | 4 +- 31 files changed, 99 insertions(+), 69 deletions(-) diff --git a/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py b/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py index b28501a92a..3e44cd614d 100644 --- a/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py +++ b/contrib/runners/action_chain_runner/action_chain_runner/action_chain_runner.py @@ -825,9 +825,9 @@ def _format_action_exec_result(self, action_node, liveaction_db, created_at, upd :rtype: ``dict`` """ if not isinstance(created_at, datetime.datetime): - raise TypeError("created_at is not a datetime object") + raise TypeError('The created_at is not a datetime object.') if not isinstance(updated_at, datetime.datetime): - raise TypeError("updated_at is not a datetime object") + raise TypeError('The updated_at is not a datetime object.') result = {} diff --git a/contrib/runners/python_runner/python_runner/python_action_wrapper.py b/contrib/runners/python_runner/python_runner/python_action_wrapper.py index 844634d6a5..eb8ccfda20 100644 --- a/contrib/runners/python_runner/python_runner/python_action_wrapper.py +++ b/contrib/runners/python_runner/python_runner/python_action_wrapper.py @@ -323,7 +323,7 @@ def _get_action_instance(self): LOG.debug('Received parameters: %s', parameters) if not isinstance(parent_args, list): - raise ValueError("Parent_args needs a list of values") + raise ValueError('The parent_args needs to be a list.') obj = PythonActionWrapper(pack=args.pack, file_path=args.file_path, config=config, diff --git a/contrib/runners/python_runner/python_runner/python_runner.py b/contrib/runners/python_runner/python_runner/python_runner.py index 6be7914aa3..bca7619a9c 100644 --- a/contrib/runners/python_runner/python_runner/python_runner.py +++ b/contrib/runners/python_runner/python_runner/python_runner.py @@ -323,7 +323,7 @@ def _get_output_values(self, exit_code, stdout, stderr, timed_out): if ACTION_OUTPUT_RESULT_DELIMITER in stdout: split = stdout.split(ACTION_OUTPUT_RESULT_DELIMITER) if not len(split) == 3: - raise ValueError("Length should be 3") + raise ValueError('The result length should be 3.') action_result = split[1].strip() stdout = split[0] + split[2] else: diff --git a/st2api/st2api/controllers/v1/actionexecutions.py b/st2api/st2api/controllers/v1/actionexecutions.py index f5d3a359d4..14cd2f2bf4 100644 --- a/st2api/st2api/controllers/v1/actionexecutions.py +++ b/st2api/st2api/controllers/v1/actionexecutions.py @@ -379,15 +379,15 @@ def validate(self): if self.parameters: if not isinstance(self.parameters, dict): - raise ValueError("Parameters needs to be a dictionary") + raise ValueError('The parameters needs to be a dictionary.') if self.tasks: if not isinstance(self.tasks, list): - raise ValueError("Tasks needs to be a list") + raise ValueError('The tasks needs to be a list.') if self.reset: if not isinstance(self.reset, list): - raise ValueError("Reset needs to be a list") + raise ValueError('The reset needs to be a list.') if list(set(self.reset) - set(self.tasks)): raise ValueError('List of tasks to reset does not match the tasks to rerun.') @@ -409,15 +409,15 @@ def post(self, spec_api, id, requester_user, no_merge=False, show_secrets=False) if spec_api.parameters: if not isinstance(spec_api.parameters, dict): - raise ValueError("Parameters needs to be a list") + raise ValueError('The parameters needs to be a dictionary.') if spec_api.tasks: if not isinstance(spec_api.tasks, list): - raise ValueError("Tasks needs to be a list") + raise ValueError('The tasks needs to be a list.') if spec_api.reset: if not isinstance(spec_api.reset, list): - raise ValueError("reset needs to be a list") + raise ValueError('The reset needs to be a list.') if list(set(spec_api.reset) - set(spec_api.tasks)): raise ValueError('List of tasks to reset does not match the tasks to rerun.') diff --git a/st2api/st2api/controllers/v1/keyvalue.py b/st2api/st2api/controllers/v1/keyvalue.py index d768500948..f1fb40d177 100644 --- a/st2api/st2api/controllers/v1/keyvalue.py +++ b/st2api/st2api/controllers/v1/keyvalue.py @@ -194,7 +194,7 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non raw_filters['prefix'] = prefix if 'scope'not in raw_filters: - raise KeyError("The key scope is not in raw_filters") + raise KeyError('The scope is not found in raw filters.') kvp_apis_system = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, @@ -214,9 +214,9 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non raw_filters['prefix'] = user_scope_prefix if 'scope'not in raw_filters: - raise KeyError("The key scope is not found in raw_filters") + raise KeyError('The scope is not found in raw filters.') if 'prefix'not in raw_filters: - raise KeyError("The key prefix is not in raw_filters") + raise KeyError('The prefix is not found in raw filters.') kvp_apis_user = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, @@ -235,9 +235,9 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non raw_filters['prefix'] = user_scope_prefix if 'scope' not in raw_filters: - raise KeyError("The key scope is not found in raw_filters") + raise KeyError('The scope is not found in raw filters.') if 'prefix' not in raw_filters: - raise KeyError("The key prefix is not in raw_filters") + raise KeyError('The prefix is not found in raw filters.') kvp_apis = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, @@ -249,7 +249,7 @@ def get_all(self, requester_user, prefix=None, scope=FULL_SYSTEM_SCOPE, user=Non raw_filters['prefix'] = prefix if 'scope' not in raw_filters: - raise KeyError("The key scope is not found in raw_filters") + raise KeyError('The scope is not found in raw filters.') kvp_apis = super(KeyValuePairController, self)._get_all( from_model_kwargs=from_model_kwargs, sort=sort, diff --git a/st2api/st2api/controllers/v1/triggers.py b/st2api/st2api/controllers/v1/triggers.py index 9577ad1354..7735cf9903 100644 --- a/st2api/st2api/controllers/v1/triggers.py +++ b/st2api/st2api/controllers/v1/triggers.py @@ -325,7 +325,7 @@ def __init__(self, payload=None): def validate(self): if self.payload: if not isinstance(self.payload, dict): - raise TypeError("The object is not a dict type") + raise TypeError('The payload has a value that is not a dictionary.') return True diff --git a/st2common/st2common/bootstrap/actionsregistrar.py b/st2common/st2common/bootstrap/actionsregistrar.py index 6839d9996a..2cf0c8ab44 100644 --- a/st2common/st2common/bootstrap/actionsregistrar.py +++ b/st2common/st2common/bootstrap/actionsregistrar.py @@ -211,7 +211,7 @@ def register_actions(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: if not isinstance(packs_base_paths, list): - raise ValueError("Pack base paths needs to be a list") + raise ValueError('The pack base paths has a value that is not a list.') if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/aliasesregistrar.py b/st2common/st2common/bootstrap/aliasesregistrar.py index c6c9863f37..3a0d132938 100644 --- a/st2common/st2common/bootstrap/aliasesregistrar.py +++ b/st2common/st2common/bootstrap/aliasesregistrar.py @@ -192,7 +192,7 @@ def register_aliases(packs_base_paths=None, pack_dir=None, use_pack_cache=True, if packs_base_paths: if not isinstance(packs_base_paths, list): - raise TypeError("The object is not a List type") + raise TypeError('The pack base paths has a value that is not a list.') if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/configsregistrar.py b/st2common/st2common/bootstrap/configsregistrar.py index 708510f9db..7eefeb6f6f 100644 --- a/st2common/st2common/bootstrap/configsregistrar.py +++ b/st2common/st2common/bootstrap/configsregistrar.py @@ -150,7 +150,7 @@ def register_configs(packs_base_paths=None, pack_dir=None, use_pack_cache=True, if packs_base_paths: if not isinstance(packs_base_paths, list): - raise ValueError("Pack Base paths needs to be a list") + raise ValueError('The pack base paths has a value that is not a list.') if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/policiesregistrar.py b/st2common/st2common/bootstrap/policiesregistrar.py index 75e4170a9b..928198e30b 100644 --- a/st2common/st2common/bootstrap/policiesregistrar.py +++ b/st2common/st2common/bootstrap/policiesregistrar.py @@ -207,7 +207,7 @@ def register_policies(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: if not isinstance(packs_base_paths, list): - raise TypeError("The object is not a list type") + raise TypeError('The pack base paths has a value that is not a list.') if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/rulesregistrar.py b/st2common/st2common/bootstrap/rulesregistrar.py index 9b5aef35ac..e43871ecde 100644 --- a/st2common/st2common/bootstrap/rulesregistrar.py +++ b/st2common/st2common/bootstrap/rulesregistrar.py @@ -186,7 +186,7 @@ def register_rules(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: if not isinstance(packs_base_paths, list): - raise ValueError("Pack base paths needs to be a list") + raise ValueError('The pack base paths has a value that is not a list.') if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/sensorsregistrar.py b/st2common/st2common/bootstrap/sensorsregistrar.py index 4e1a549a98..6bfe7673b2 100644 --- a/st2common/st2common/bootstrap/sensorsregistrar.py +++ b/st2common/st2common/bootstrap/sensorsregistrar.py @@ -179,7 +179,7 @@ def register_sensors(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: if not isinstance(packs_base_paths, list): - raise TypeError("The object is not a list type") + raise TypeError('The pack base paths has a value that is not a list.') if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/bootstrap/triggersregistrar.py b/st2common/st2common/bootstrap/triggersregistrar.py index fdc457c9f8..501a731975 100644 --- a/st2common/st2common/bootstrap/triggersregistrar.py +++ b/st2common/st2common/bootstrap/triggersregistrar.py @@ -155,7 +155,7 @@ def register_triggers(packs_base_paths=None, pack_dir=None, use_pack_cache=True, fail_on_failure=False): if packs_base_paths: if not isinstance(packs_base_paths, list): - raise ValueError("Pack base paths needs to be a list") + raise ValueError('The pack base paths has a value that is not a list.') if not packs_base_paths: packs_base_paths = content_utils.get_packs_base_paths() diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index c5c319ae53..232224c301 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -62,7 +62,7 @@ def get_packs(self, base_dirs): :rtype: ``dict`` """ if not isinstance(base_dirs, list): - raise TypeError("Base dirs needs to be a list") + raise TypeError('The base dirs has a value that is not a list.') result = {} for base_dir in base_dirs: @@ -90,7 +90,7 @@ def get_content(self, base_dirs, content_type): :rtype: ``dict`` """ if not isinstance(base_dirs, list): - raise TypeError("Base dirs needs to be a list") + raise TypeError('The base dirs has a value that is not a list.') if content_type not in self.ALLOWED_CONTENT_TYPES: raise ValueError('Unsupported content_type: %s' % (content_type)) diff --git a/st2common/st2common/content/utils.py b/st2common/st2common/content/utils.py index 147eea87d7..0d7fdb7c3f 100644 --- a/st2common/st2common/content/utils.py +++ b/st2common/st2common/content/utils.py @@ -282,7 +282,7 @@ def get_pack_file_abs_path(pack_ref, file_path, resource_type=None, use_pack_cac result = os.path.join(*path_components) # pylint: disable=E1120 if normalized_file_path not in result: - raise ValueError("This is not normalized path to prevent directory traversal") + raise ValueError('This is not a normalized path to prevent directory traversal.') # Final safety check for common prefix to avoid traversal attack common_prefix = os.path.commonprefix([pack_base_path, result]) diff --git a/st2common/st2common/models/api/keyvalue.py b/st2common/st2common/models/api/keyvalue.py index 8955baa031..2e47abc707 100644 --- a/st2common/st2common/models/api/keyvalue.py +++ b/st2common/st2common/models/api/keyvalue.py @@ -189,7 +189,7 @@ def to_model(cls, kvp): # Additional safety check to ensure that the value hasn't been decrypted if not value == original_value: - raise ValueError("Value doesn't match with original value") + raise ValueError("The value doesn't match with original value.") elif secret: cls._verif_key_is_set_up(name=name) diff --git a/st2common/st2common/models/system/action.py b/st2common/st2common/models/system/action.py index 1487550c5e..cf7101c95b 100644 --- a/st2common/st2common/models/system/action.py +++ b/st2common/st2common/models/system/action.py @@ -178,7 +178,7 @@ def _get_command_string(self, cmd, args): :rtype: ``str`` """ if not isinstance(args, (list, tuple)): - raise ValueError("Args needs to be a list and tuple") + raise ValueError('The args has a value that is not a list and tuple.') args = [quote_unix(arg) for arg in args] args = ' '.join(args) diff --git a/st2common/st2common/rbac/types.py b/st2common/st2common/rbac/types.py index 566ffad4b3..096ee3dfd1 100644 --- a/st2common/st2common/rbac/types.py +++ b/st2common/st2common/rbac/types.py @@ -185,7 +185,7 @@ def get_resource_type(cls, permission_type): split = permission_type.split('_') if not len(split) >= 2: - raise ValueError("Length should be greater than or equal to 2") + raise ValueError('The length should be greater than or equal to 2.') return '_'.join(split[:-1]) @@ -198,7 +198,7 @@ def get_permission_name(cls, permission_type): """ split = permission_type.split('_') if not len(split) >= 2: - raise ValueError("Length should be greater than or equal to 2") + raise ValueError('The length should be greater than or equal to 2.') # Special case for PACK_VIEWS_INDEX_HEALTH if permission_type == PermissionType.PACK_VIEWS_INDEX_HEALTH: diff --git a/st2common/st2common/runners/base.py b/st2common/st2common/runners/base.py index e2954657f6..7882840acf 100644 --- a/st2common/st2common/runners/base.py +++ b/st2common/st2common/runners/base.py @@ -289,7 +289,7 @@ def pre_run(self): worktree_path=self.git_worktree_path) if not (entry_point.startswith(self.git_worktree_path)): - raise ValueError("Value not matching") + raise ValueError("The entry point value does not match with git worktree path.") self.entry_point = entry_point @@ -377,9 +377,10 @@ def cleanup_git_worktree(self, worktree_path, pack_name, content_version): """ # Safety check to make sure we don't remove something outside /tmp if not (worktree_path.startswith('/tmp')): - raise ValueError("Value not matching") + raise ValueError('The worktree path does not match with /tmp.') if not (worktree_path.startswith('/tmp/%s' % (self.WORKTREE_DIRECTORY_PREFIX))): - raise ValueError("Value not matching") + raise ValueError('The worktree path does not match with /tmp/"%s".' % + (self.WORKTREE_DIRECTORY_PREFIX)) if self._debug: LOG.debug('Not removing git worktree "%s" because debug mode is enabled' % diff --git a/st2common/st2common/services/trace.py b/st2common/st2common/services/trace.py index 40a83123be..251abd8552 100644 --- a/st2common/st2common/services/trace.py +++ b/st2common/st2common/services/trace.py @@ -55,7 +55,7 @@ def _get_valid_trace_context(trace_context): Check if tarce_context is a valid type and returns a TraceContext object. """ if not isinstance(trace_context, (TraceContext, dict)): - raise TypeError("Not a valid trace_context type") + raise TypeError("The trace context has a value that is not a dictionary.") # Pretty much abuse the dynamic nature of python to make it possible to support # both dict and TraceContext types. diff --git a/st2common/st2common/services/triggers.py b/st2common/st2common/services/triggers.py index efc2909b00..0c4ab0fe19 100644 --- a/st2common/st2common/services/triggers.py +++ b/st2common/st2common/services/triggers.py @@ -254,7 +254,7 @@ def create_or_update_trigger_db(trigger, log_not_unique_error_as_debug=False): :type trigger: ``dict`` """ if not isinstance(trigger, dict): - raise ValueError("Trigger needs to be a dict object") + raise ValueError('The trigger has a value that is not a dictionary.') existing_trigger_db = _get_trigger_db(trigger) @@ -408,7 +408,7 @@ def create_or_update_trigger_type_db(trigger_type, log_not_unique_error_as_debug :rtype: ``object`` """ if not isinstance(trigger_type, dict): - raise ValueError("trigger needs to be a dict object") + raise ValueError('The trigger has a value that is not a dictionary.') trigger_type_api = TriggerTypeAPI(**trigger_type) trigger_type_api.validate() diff --git a/st2common/st2common/transport/announcement.py b/st2common/st2common/transport/announcement.py index cbcf3f77b6..fe22c78d59 100644 --- a/st2common/st2common/transport/announcement.py +++ b/st2common/st2common/transport/announcement.py @@ -66,9 +66,9 @@ def dispatch(self, routing_key, payload, trace_context=None): :type trace_context: ``TraceContext`` """ if not isinstance(payload, (type(None), dict)): - raise TypeError("Object is not a dict type") + raise TypeError('The payload has a value that is not a dictionary.') if not isinstance(trace_context, (type(None), dict, TraceContext)): - raise TypeError("Object is not a TraceContext type") + raise TypeError('The trace context has a value that is not a dictionary.') payload = { 'payload': payload, diff --git a/st2common/st2common/transport/reactor.py b/st2common/st2common/transport/reactor.py index 2578b39901..c015a756d9 100644 --- a/st2common/st2common/transport/reactor.py +++ b/st2common/st2common/transport/reactor.py @@ -94,9 +94,9 @@ def dispatch(self, trigger, payload=None, trace_context=None): :type trace_context: ``TraceContext`` """ if not isinstance(payload, (type(None), dict)): - raise TypeError("Object is not a dict type") + raise TypeError('The payload has a value that is not a dictionary.') if not isinstance(trace_context, (type(None), TraceContext)): - raise TypeError("Object is not a TraceContext type") + raise TypeError('The trace context has a value that is not of type TraceContext.') payload = { 'trigger': trigger, diff --git a/st2common/st2common/util/crypto.py b/st2common/st2common/util/crypto.py index 4559b0aee9..fe9ca28151 100644 --- a/st2common/st2common/util/crypto.py +++ b/st2common/st2common/util/crypto.py @@ -81,7 +81,7 @@ DEFAULT_AES_KEY_SIZE = 256 if not DEFAULT_AES_KEY_SIZE >= MINIMUM_AES_KEY_SIZE: - raise ValueError('Verify the key size :%s' % (DEFAULT_AES_KEY_SIZE)) + raise ValueError('Verify the key size :"%s".' % (DEFAULT_AES_KEY_SIZE)) class AESKey(object): @@ -208,17 +208,17 @@ def cryptography_symmetric_encrypt(encrypt_key, plaintext): """ if not isinstance(encrypt_key, AESKey): - raise ValueError("Encrypt key needs to be an AESkey class instance") + raise ValueError('Encrypted key needs to be an AESkey class instance.') if not isinstance(plaintext, (six.text_type, six.string_types, six.binary_type)): - raise TypeError("Plaintext needs to either be a string/unicode or bytes") + raise TypeError('Plaintext needs to either be a string/unicode or bytes.') aes_key_bytes = encrypt_key.aes_key_bytes hmac_key_bytes = encrypt_key.hmac_key_bytes if not isinstance(aes_key_bytes, six.binary_type): - raise TypeError("AESKey bytes matching with binary type") + raise TypeError('AESKey bytes does not match with binary type.') if not isinstance(hmac_key_bytes, six.binary_type): - raise TypeError("HMACKey bytes matching with binary type") + raise TypeError('HMACKey bytes does not match with binary type.') if isinstance(plaintext, (six.text_type, six.string_types)): # Convert data to bytes @@ -268,17 +268,17 @@ def cryptography_symmetric_decrypt(decrypt_key, ciphertext): NOTE 2: This function is loosely based on keyczar AESKey.Decrypt() (Apache 2.0 license). """ if not isinstance(decrypt_key, AESKey): - raise ValueError("Decrypt key needs to be an AESKey class instance") + raise ValueError('Decrypted key needs to be an AESKey class instance.') if not isinstance(ciphertext, (six.text_type, six.string_types, six.binary_type)): - raise TypeError("Ciphertext needs to either be a string/unicode or bytes") + raise TypeError('Ciphertext needs to either be a string/unicode or bytes.') aes_key_bytes = decrypt_key.aes_key_bytes hmac_key_bytes = decrypt_key.hmac_key_bytes if not isinstance(aes_key_bytes, six.binary_type): - raise TypeError("Key Bytes type not matching") + raise TypeError('The aes key bytes does not match with binary type.') if not isinstance(hmac_key_bytes, six.binary_type): - raise TypeError("Value Type not matching") + raise TypeError('The hmac key bytes does not match with binary type.') # Convert from hex notation ASCII string to bytes ciphertext = binascii.unhexlify(ciphertext) diff --git a/st2common/st2common/util/green/shell.py b/st2common/st2common/util/green/shell.py index 4f7c893e9c..dd5af4e85b 100644 --- a/st2common/st2common/util/green/shell.py +++ b/st2common/st2common/util/green/shell.py @@ -92,7 +92,7 @@ def run_command(cmd, stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, LOG.debug('Entering st2common.util.green.run_command.') if not isinstance(cmd, (list, tuple) + six.string_types): - raise TypeError("cmd must be a type of (list, tuple) string ") + raise TypeError('Command must be a type of list, tuple and string.') if (read_stdout_func and not read_stderr_func) or (read_stderr_func and not read_stdout_func): raise ValueError('Both read_stdout_func and read_stderr_func arguments need ' diff --git a/st2common/st2common/util/sandboxing.py b/st2common/st2common/util/sandboxing.py index d7a921f466..b2997da951 100644 --- a/st2common/st2common/util/sandboxing.py +++ b/st2common/st2common/util/sandboxing.py @@ -20,6 +20,7 @@ from __future__ import absolute_import +import fnmatch import os import sys from distutils.sysconfig import get_python_lib @@ -27,6 +28,7 @@ from oslo_config import cfg from st2common.constants.pack import SYSTEM_PACK_NAMES +from st2common.content.utils import get_pack_base_path __all__ = [ 'get_sandbox_python_binary_path', @@ -40,7 +42,6 @@ def get_sandbox_python_binary_path(pack=None): """ Return path to the Python binary for the provided pack. - :param pack: Pack name. :type pack: ``str`` """ @@ -59,10 +60,8 @@ def get_sandbox_python_binary_path(pack=None): def get_sandbox_path(virtualenv_path): """ Return PATH environment variable value for the sandboxed environment. - This function makes sure that virtualenv/bin directory is in the path and has precedence over the global PATH values. - Note: This function needs to be called from the parent process (one which is spawning a sandboxed process). """ @@ -87,16 +86,12 @@ def get_sandbox_path(virtualenv_path): def get_sandbox_python_path(inherit_from_parent=True, inherit_parent_virtualenv=True): """ Return PYTHONPATH environment variable value for the new sandboxed environment. - This function takes into account if the current (parent) process is running under virtualenv and other things like that. - Note: This function needs to be called from the parent process (one which is spawning a sandboxed process). - :param inherit_from_parent: True to inheir PYTHONPATH from the current process. :type inherit_from_parent: ``str`` - :param inherit_parent_virtualenv: True to inherit virtualenv path if the current process is running inside virtual environment. :type inherit_parent_virtualenv: ``str`` @@ -116,7 +111,7 @@ def get_sandbox_python_path(inherit_from_parent=True, inherit_parent_virtualenv= sys_prefix = os.path.abspath(sys.prefix) if sys_prefix not in site_packages_dir: - raise ValueError("Sys_prefix file not identified in directory") + raise ValueError('The file with prefix sys is not found in the directory.') sandbox_python_path.append(site_packages_dir) @@ -129,14 +124,48 @@ def get_sandbox_python_path_for_python_action(pack, inherit_from_parent=True, inherit_parent_virtualenv=True): """ Return sandbox PYTHONPATH for a particular Python runner action. - Same as get_sandbox_python_path() function, but it's intended to be used for Python runner actions. """ - return get_sandbox_python_path( + sandbox_python_path = get_sandbox_python_path( inherit_from_parent=inherit_from_parent, inherit_parent_virtualenv=inherit_parent_virtualenv) + pack_base_path = get_pack_base_path(pack_name=pack) + virtualenv_path = get_sandbox_virtualenv_path(pack=pack) + + if virtualenv_path and os.path.isdir(virtualenv_path): + pack_virtualenv_lib_path = os.path.join(virtualenv_path, 'lib') + + virtualenv_directories = os.listdir(pack_virtualenv_lib_path) + virtualenv_directories = [dir_name for dir_name in virtualenv_directories if + fnmatch.fnmatch(dir_name, 'python*')] + + # Add the pack's lib directory (lib/python3.x) in front of the PYTHONPATH. + pack_actions_lib_paths = os.path.join(pack_base_path, 'actions', 'lib') + pack_virtualenv_lib_path = os.path.join(virtualenv_path, 'lib') + pack_venv_lib_directory = os.path.join(pack_virtualenv_lib_path, virtualenv_directories[0]) + + # Add the pack's site-packages directory (lib/python3.x/site-packages) + # in front of the Python system site-packages This is important because + # we want Python 3 compatible libraries to be used from the pack virtual + # environment and not system ones. + pack_venv_site_packages_directory = os.path.join(pack_virtualenv_lib_path, + virtualenv_directories[0], + 'site-packages') + + full_sandbox_python_path = [ + # NOTE: Order here is very important for imports to function correctly + pack_venv_lib_directory, + pack_venv_site_packages_directory, + pack_actions_lib_paths, + sandbox_python_path, + ] + + sandbox_python_path = ':'.join(full_sandbox_python_path) + + return sandbox_python_path + def get_sandbox_virtualenv_path(pack): """ diff --git a/st2common/st2common/util/shell.py b/st2common/st2common/util/shell.py index 7083496c3d..957b9d54b0 100644 --- a/st2common/st2common/util/shell.py +++ b/st2common/st2common/util/shell.py @@ -75,7 +75,7 @@ def run_command(cmd, stdin=None, stdout=subprocess.PIPE, stderr=subprocess.PIPE, :rtype: ``tuple`` (exit_code, stdout, stderr) """ if not isinstance(cmd, (list, tuple) + six.string_types): - raise TypeError("cmd must be a type of (list, tuple) string ") + raise TypeError('Command must be a type of list, tuple and string.') if not env: env = os.environ.copy() diff --git a/st2common/st2common/util/templating.py b/st2common/st2common/util/templating.py index da835c254b..04a83d069d 100644 --- a/st2common/st2common/util/templating.py +++ b/st2common/st2common/util/templating.py @@ -41,7 +41,7 @@ def render_template(value, context=None): :type context: ``dict`` """ if not isinstance(value, six.string_types): - raise TypeError("Value type does not match with string") + raise TypeError('The template value needs to be of type string.') context = context or {} env = get_jinja_environment(allow_undefined=False) # nosec diff --git a/st2reactor/st2reactor/container/sensor_wrapper.py b/st2reactor/st2reactor/container/sensor_wrapper.py index 743083ac72..dacce0e692 100644 --- a/st2reactor/st2reactor/container/sensor_wrapper.py +++ b/st2reactor/st2reactor/container/sensor_wrapper.py @@ -362,7 +362,7 @@ def _sanitize_trigger(self, trigger): trigger_types = trigger_types.split(',') if trigger_types else [] parent_args = json.loads(args.parent_args) if args.parent_args else [] if not isinstance(parent_args, list): - raise TypeError("Command line arguments passed to the parent process") + raise TypeError('Command line arguments passed to the parent process must be a list.') obj = SensorWrapper(pack=args.pack, file_path=args.file_path, diff --git a/st2reactor/st2reactor/garbage_collector/base.py b/st2reactor/st2reactor/garbage_collector/base.py index db1d150d73..5bfb02090a 100644 --- a/st2reactor/st2reactor/garbage_collector/base.py +++ b/st2reactor/st2reactor/garbage_collector/base.py @@ -197,7 +197,7 @@ def _purge_action_executions(self): LOG.info('Deleting action executions older than: %s' % (timestamp_str)) if not timestamp < utc_now: - raise ValueError("utc_now violates the minimum time stamp") + raise ValueError('Calculated timestamp violates the utc now.') try: purge_executions(logger=LOG, timestamp=timestamp) @@ -218,7 +218,7 @@ def _purge_action_executions_output(self): LOG.info('Deleting action executions output objects older than: %s' % (timestamp_str)) if timestamp < utc_now: - raise ValueError("utc_now violates the minimum time stamp") + raise ValueError('Calculated timestamp violates the utc now.') try: purge_execution_output_objects(logger=LOG, timestamp=timestamp) @@ -242,7 +242,7 @@ def _purge_trigger_instances(self): LOG.info('Deleting trigger instances older than: %s' % (timestamp_str)) if timestamp < utc_now: - raise ValueError("utc_now violates the minimum time stamp") + raise ValueError('Calculated timestamp violates the utc now.') try: purge_trigger_instances(logger=LOG, timestamp=timestamp) diff --git a/st2reactor/st2reactor/rules/enforcer.py b/st2reactor/st2reactor/rules/enforcer.py index 18a3f4edd9..86b58da5d8 100644 --- a/st2reactor/st2reactor/rules/enforcer.py +++ b/st2reactor/st2reactor/rules/enforcer.py @@ -159,9 +159,9 @@ def _update_trace(self): LOG.exception('No Trace found for TriggerInstance %s.', self.trigger_instance.id) return None - # This would signify some sort of coding error so assert. + # This would signify some sort of coding error so raise ValueError. if not trace_db: - raise ValueError("tarcedb not found") + raise ValueError('Trace database not found.') trace_db = trace_service.add_or_update_given_trace_db( trace_db=trace_db, From 5bd2e2b2df48a90c02960e3892c5aed5fc66f7c2 Mon Sep 17 00:00:00 2001 From: ashwini-orchestral Date: Fri, 19 Feb 2021 15:13:39 +0000 Subject: [PATCH 3/4] error handling and logging error messages --- contrib/linux/actions/checks/check_processes.py | 4 ++-- contrib/linux/sensors/file_watch_sensor.py | 4 ++-- st2actions/st2actions/cmd/scheduler.py | 2 +- st2client/st2client/models/core.py | 7 +++++-- st2client/st2client/utils/terminal.py | 12 ++++++------ st2common/st2common/log.py | 4 ++-- st2common/st2common/runners/base.py | 5 +++-- st2common/st2common/util/jsonify.py | 4 ++-- st2reactor/st2reactor/container/sensor_wrapper.py | 5 +++-- 9 files changed, 26 insertions(+), 21 deletions(-) diff --git a/contrib/linux/actions/checks/check_processes.py b/contrib/linux/actions/checks/check_processes.py index b1ff1af0ae..eebba12cec 100755 --- a/contrib/linux/actions/checks/check_processes.py +++ b/contrib/linux/actions/checks/check_processes.py @@ -52,8 +52,8 @@ def process(self, criteria): cmdfh = open(self.procDir + "/" + p + "/cmdline") cmd = cmdfh.readline() pInfo[1] = cmd - except: - continue + except IOError: + print("Error: can't find file or read data.") finally: cmdfh.close() fh.close() diff --git a/contrib/linux/sensors/file_watch_sensor.py b/contrib/linux/sensors/file_watch_sensor.py index 2597d63926..8097d0b668 100644 --- a/contrib/linux/sensors/file_watch_sensor.py +++ b/contrib/linux/sensors/file_watch_sensor.py @@ -44,8 +44,8 @@ def cleanup(self): try: self._tail.notifier.stop() - except Exception: - pass + except Exception as e: + print("Error: tail notifier is still running.", e) def add_trigger(self, trigger): file_path = trigger['parameters'].get('file_path', None) diff --git a/st2actions/st2actions/cmd/scheduler.py b/st2actions/st2actions/cmd/scheduler.py index b3c972b654..238139068f 100644 --- a/st2actions/st2actions/cmd/scheduler.py +++ b/st2actions/st2actions/cmd/scheduler.py @@ -109,7 +109,7 @@ def _run_scheduler(): handler.shutdown() entrypoint.shutdown() except: - pass + LOG.exception('Unable to shutdown scheduler.') return 1 diff --git a/st2client/st2client/models/core.py b/st2client/st2client/models/core.py index 255c91534f..4ded531d9b 100644 --- a/st2client/st2client/models/core.py +++ b/st2client/st2client/models/core.py @@ -344,8 +344,11 @@ def delete_by_id(self, instance_id, **kwargs): resp_json = response.json() if resp_json: return resp_json - except: - pass + except Exception as e: + response.reason += ( + '\nUnable to retrieve detailed message ' + 'from the HTTP response. %s\n' % six.text_type(e) + ) return True diff --git a/st2client/st2client/utils/terminal.py b/st2client/st2client/utils/terminal.py index 555753fc95..63017cb034 100644 --- a/st2client/st2client/utils/terminal.py +++ b/st2client/st2client/utils/terminal.py @@ -63,8 +63,8 @@ def ioctl_GWINSZ(fd): for fd in (0, 1, 2): try: return ioctl_GWINSZ(fd)[1] - except Exception: - pass + except Exception as e: + print(e) # 3. try os.ctermid() try: @@ -73,8 +73,8 @@ def ioctl_GWINSZ(fd): return ioctl_GWINSZ(fd)[1] finally: os.close(fd) - except Exception: - pass + except Exception as e: + print(e) # 4. try `stty size` try: @@ -85,8 +85,8 @@ def ioctl_GWINSZ(fd): result = process.communicate() if process.returncode == 0: return tuple(int(x) for x in result[0].split())[1] - except Exception: - pass + except Exception as e: + print(e) # 5. return default fallback value return default diff --git a/st2common/st2common/log.py b/st2common/st2common/log.py index 5335af5f53..b624587e20 100644 --- a/st2common/st2common/log.py +++ b/st2common/st2common/log.py @@ -129,8 +129,8 @@ def find_caller(stack_info=False, stacklevel=1): sio.close() rv = (filename, f.f_lineno, co.co_name, sinfo) break - except Exception: - pass + except Exception as e: + print('Unable to find caller.', e) return rv diff --git a/st2common/st2common/runners/base.py b/st2common/st2common/runners/base.py index 7882840acf..86716d561d 100644 --- a/st2common/st2common/runners/base.py +++ b/st2common/st2common/runners/base.py @@ -391,8 +391,9 @@ def cleanup_git_worktree(self, worktree_path, pack_name, content_version): try: shutil.rmtree(worktree_path, ignore_errors=True) - except: - pass + except Exception: + msg = ('Unable to remove / cleanup the provided git worktree directory.') + LOG.exception(msg) return True diff --git a/st2common/st2common/util/jsonify.py b/st2common/st2common/util/jsonify.py index 1f47cec1b0..feb03f408b 100644 --- a/st2common/st2common/util/jsonify.py +++ b/st2common/st2common/util/jsonify.py @@ -73,8 +73,8 @@ def json_loads(obj, keys=None): for key in keys: try: obj[key] = json.loads(obj[key]) - except: - pass + except Exception as error: + print('Criteria pattern not valid JSON: "%s".', error) return obj diff --git a/st2reactor/st2reactor/container/sensor_wrapper.py b/st2reactor/st2reactor/container/sensor_wrapper.py index dacce0e692..411d0a681c 100644 --- a/st2reactor/st2reactor/container/sensor_wrapper.py +++ b/st2reactor/st2reactor/container/sensor_wrapper.py @@ -181,8 +181,9 @@ def __init__(self, pack, file_path, class_name, trigger_types, # 1. Parse the config with inherited parent args try: config.parse_args(args=self._parent_args) - except Exception: - pass + except Exception as e: + print('Failed to parse config using parent args (parent_args=%s): "%s".' % + (str(self._parent_args), six.text_type(e))) # 2. Establish DB connection username = cfg.CONF.database.username if hasattr(cfg.CONF.database, 'username') else None From 0cce47c369f46b92d9ad648686a7cd10c0bb3906 Mon Sep 17 00:00:00 2001 From: ashwini-orchestral Date: Fri, 19 Feb 2021 16:02:42 +0000 Subject: [PATCH 4/4] use yaml.safe_load instead of yaml.load --- st2common/st2common/util/spec_loader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/st2common/util/spec_loader.py b/st2common/st2common/util/spec_loader.py index 8ab926330f..c01bde8d20 100644 --- a/st2common/st2common/util/spec_loader.py +++ b/st2common/st2common/util/spec_loader.py @@ -77,7 +77,7 @@ def load_spec(module_name, spec_file, allow_duplicate_keys=False): # 1. Check for duplicate keys if not allow_duplicate_keys: - yaml.load(spec_string, UniqueKeyLoader) + yaml.safe_load(spec_string) # 2. Generate actual spec spec = yaml.safe_load(spec_string)