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 <[email protected]> Co-authored-by: Dylan Sprayberry <[email protected]>
singer-io · Jan 10, 2023 · e40018f · e40018f
1 parent b7467f6
commit e40018f
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 4 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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)