From e40018f45e25b52bbd3eb7140b450df85e0e7b25 Mon Sep 17 00:00:00 2001 From: Harsh <80324346+hpatel41@users.noreply.github.com> Date: Wed, 11 Jan 2023 03:04:37 +0530 Subject: [PATCH] TDL-20459, TDL-21606: Remove backoff for Access Denied error (#55) * updated error doc link for regex and removed backoff for Access Denied * resolved pylint error * added parameterized in config.yml file * updated the giveup condition * updated regex doc link * Changelog, Version Bump, Remove listfiles pre-role-assumption * Revert role assumption change to unbreak tap-tester Co-authored-by: Dylan Sprayberry Co-authored-by: Dylan Sprayberry <28106103+dsprayberry@users.noreply.github.com> --- .circleci/config.yml | 2 +- CHANGELOG.md | 5 ++ setup.py | 4 +- tap_s3_csv/s3.py | 13 ++++- .../test_giveup_for_access_denied_error.py | 50 +++++++++++++++++++ 5 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 tests/unittests/test_giveup_for_access_denied_error.py diff --git a/.circleci/config.yml b/.circleci/config.yml index 057b1d8..040010f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -19,7 +19,7 @@ jobs: name: 'Unit Tests' command: | source /usr/local/share/virtualenvs/tap-s3-csv/bin/activate - pip install nose coverage + pip install nose coverage parameterized nosetests --with-coverage --cover-erase --cover-package=tap_s3_csv --cover-html-dir=htmlcov tests/unittests coverage html - store_test_results: diff --git a/CHANGELOG.md b/CHANGELOG.md index bda963c..384fb7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 1.3.7 + * Remove Backoff for Access Denied errors + * Add unitttest for access denied giveup + * [#55](https://github.com/singer-io/tap-s3-csv/pull/55) + ## 1.3.6 * Implement request timeout [#46](https://github.com/singer-io/tap-s3-csv/pull/46) * Updgrade SDK version [#51](https://github.com/singer-io/tap-s3-csv/pull/51) diff --git a/setup.py b/setup.py index 65ab2bb..c951d34 100644 --- a/setup.py +++ b/setup.py @@ -3,7 +3,7 @@ from setuptools import setup setup(name='tap-s3-csv', - version='1.3.6', + version='1.3.7', description='Singer.io tap for extracting CSV files from S3', author='Stitch', url='https://singer.io', @@ -18,7 +18,7 @@ ], extras_require={ 'dev': [ - 'ipdb==0.11' + 'ipdb' ] }, entry_points=''' diff --git a/tap_s3_csv/s3.py b/tap_s3_csv/s3.py index 84032b5..f728724 100644 --- a/tap_s3_csv/s3.py +++ b/tap_s3_csv/s3.py @@ -40,6 +40,16 @@ # timeout request after 300 seconds REQUEST_TIMEOUT = 300 +def is_access_denied_error(error): + """ + This function checks whether the URLError contains 'Access Denied' substring + and return boolean values accordingly, to decide whether to backoff or not. + """ + # retry if the error string contains 'Access Denied' + if 'Access Denied' in str(error) or 'AccessDenied' in str(error): + return True + return False + def retry_pattern(fnc): @backoff.on_exception(backoff.expo, (ConnectTimeoutError, ReadTimeoutError), @@ -50,6 +60,7 @@ def retry_pattern(fnc): ClientError, max_tries=5, on_backoff=log_backoff_attempt, + giveup=is_access_denied_error, # Giveup if we do not have the access factor=10) @functools.wraps(fnc) def wrapper(*args, **kwargs): @@ -419,7 +430,7 @@ def get_input_files_for_table(config, table_spec, modified_since=None): raise ValueError( ("search_pattern for table `{}` is not a valid regular " "expression. See " - "https://docs.python.org/3.5/library/re.html#regular-expression-syntax").format(table_spec['table_name']), + "https://docs.python.org/3.9/library/re.html#regular-expression-syntax").format(table_spec['table_name']), pattern) from e LOGGER.info( diff --git a/tests/unittests/test_giveup_for_access_denied_error.py b/tests/unittests/test_giveup_for_access_denied_error.py new file mode 100644 index 0000000..b3182ca --- /dev/null +++ b/tests/unittests/test_giveup_for_access_denied_error.py @@ -0,0 +1,50 @@ +import unittest +from unittest import mock +from tap_s3_csv.s3 import PageIterator +from botocore.exceptions import ClientError +from parameterized import parameterized + +def get(*args, **kwargs): + """Function to raise an appropriate error as per the arguments""" + if kwargs.get("access_denied_error"): + raise ClientError({"Error": {"Message": "Access Denied", "Code": "AccessDenied"}}, "ListObjectsV2") + elif kwargs.get("AccessDenied_error"): + raise ClientError({"Error": {"Message": "You arr not authorized to perform this action", "Code": "AccessDenied"}}, "ListObjectsV2") + else: + raise ClientError({"Error": {"Message": "Test Error", "Code": "TestError"}}, "ListObjectsV2") + +class TestGiveUpForAccessDeniedError(unittest.TestCase): + """Test case to verify we giveup when we encounter Access Denied error""" + + @parameterized.expand([ + ["giveup_for_access_denied_error", {"access_denied_error": True}, 1], + ["giveup_for_AccessDenied_error", {"AccessDenied_error": True}, 1], + ["not_giveup", {}, 5], + ]) + @mock.patch("time.sleep") + def test_giveup_for_access_denied_error(self, name, test_data, expected_data, mocked_sleep): + + mocked_method = mock.Mock() + mocked_method.side_effect = get + + # Create PageIterator object + page_iter = PageIterator( + method=mocked_method, + input_token=None, + output_token=None, + more_results=None, + result_keys=None, + non_aggregate_keys=None, + limit_key=None, + max_items=None, + starting_token=None, + page_size=None, + op_kwargs=None, + ) + + with self.assertRaises(ClientError) as e: + # Function call + page_iter._make_request(test_data) + + # Verify the call count + self.assertEqual(mocked_method.call_count, expected_data)