Fix "x-amz-server-side-encryption header is not supported for this operation" errors

gen3workflow/aws/bucket.py

@@ -258,7 +258,6 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None:
     # The deny in this policy fires when the headers are present but wrong (e.g. trying not to use
     # KMS encryption, or trying to use a different KMS key). If the headers are absent, the request
     # is accepted and AWS falls back on the bucket's default encryption (set above).
-    # TODO: stop specifying the KMS key in the funnel config
     new_bucket_policy = {
         "Version": "2012-10-17",
         "Statement": [
@@ -301,8 +300,6 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None:
     else:
         logger.debug("Bucket policy is already up to date")
 
-    return kms_key_arn
-
 
 def enable_bucket_versioning(bucket_name: str) -> None:
     """
@@ -330,15 +327,15 @@ def enable_bucket_versioning(bucket_name: str) -> None:
         raise
 
 
-async def _create_user_bucket(user_id: str) -> Tuple[str, str]:
+async def _create_user_bucket(user_id: str) -> str:
     """
     Create an S3 bucket for the specified user and return information about the bucket.
 
     Args:
         user_id (str): The user's unique Gen3 ID
 
     Returns:
-        tuple: (bucket name, kms key ARN)
+        (str) bucket name
     """
     user_bucket_name = get_bucket_name_from_user_id(user_id)
     try:
@@ -402,9 +399,8 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str]:
         ChecksumAlgorithm="SHA256",
     )
 
-    kms_key_arn = None
     if config["KMS_ENCRYPTION_ENABLED"]:
-        kms_key_arn = setup_kms_encryption_on_bucket(user_bucket_name)
+        setup_kms_encryption_on_bucket(user_bucket_name)
     else:
         logger.warning(f"Disabling KMS encryption on bucket '{user_bucket_name}'")
         clients.s3_client.delete_bucket_encryption(Bucket=user_bucket_name)
@@ -414,7 +410,7 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str]:
         # Bucket versioning is necessary for S3Files
         enable_bucket_versioning(user_bucket_name)
 
-    return user_bucket_name, kms_key_arn
+    return user_bucket_name
 
 
 async def create_user_bucket(user_id: str) -> Tuple[str, str, str]:
@@ -435,9 +431,9 @@ async def create_user_bucket(user_id: str) -> Tuple[str, str, str]:
     retry_backoff_factor = 2
     for attempt in range(1, max_tries + 1):
         try:
-            bucket_info = await _create_user_bucket(user_id)
-            USER_BUCKET_CACHE.set(user_id, bucket_info)
-            return bucket_info
+            bucket_name = await _create_user_bucket(user_id)
+            USER_BUCKET_CACHE.set(user_id, bucket_name)
+            return bucket_name
         except ClientError as e:
             if (
                 e.response["Error"]["Code"]

gen3workflow/routes/s3.py

@@ -18,6 +18,9 @@
     HTTP_401_UNAUTHORIZED,
     HTTP_403_FORBIDDEN,
     HTTP_404_NOT_FOUND,
+    HTTP_408_REQUEST_TIMEOUT,
+    HTTP_429_TOO_MANY_REQUESTS,
+    HTTP_500_INTERNAL_SERVER_ERROR,
 )
 
 from gen3workflow import logger
@@ -445,16 +448,22 @@ async def s3_endpoint(path: str, request: Request):
                     logger.error(
                         f"Error from S3: {response.status_code} {response.text}"
                     )
-                    # do not retry in the case of a 403 error: authentication is done internally by
-                    # this function, so 403 errors are internal service errors
-                    if response.status_code != HTTP_403_FORBIDDEN:
+                    # in the case of a client-side (4xx) error (except `408 Request  Timeout` and
+                    # `429 Too Many Requests`), print debug logs and do not retry
+                    if (
+                        response.status_code >= HTTP_400_BAD_REQUEST
+                        and response.status_code < HTTP_500_INTERNAL_SERVER_ERROR
+                        and response.status_code
+                        not in [HTTP_408_REQUEST_TIMEOUT, HTTP_429_TOO_MANY_REQUESTS]
+                    ):
                         proceed = False
-                    # SignatureDoesNotMatch errors are a sign of a bug in this code => debug logs
-                    if "<Code>SignatureDoesNotMatch</Code>" in response.text:
                         logger.debug(f"Incoming headers:\n{in_headers}")
                         logger.debug(f"Outgoing headers:\n{out_headers}")
                         logger.debug(f"Canonical request:\n{canonical_request}")
                         logger.debug(f"String to sign:\n{string_to_sign}")
+                        logger.debug(f"Incoming query params:\n{request.query_params}")
+                        logger.debug(f"Outgoing query params:\n{query_params}")
+                        logger.debug(f"Outgoing body:\n{body}")
                 else:
                     logger.debug(f"Error from S3: {response.status_code}")
         except Exception as e:
@@ -484,7 +493,7 @@ async def s3_endpoint(path: str, request: Request):
 
     # Return the response from AWS S3.
     # - mask the details of 403 errors from the end user: authentication is done internally by this
-    # function, so 403 errors are internal service errors
+    # function, so 403 errors are internal service errors.
     # - return all the headers from the AWS response, except:
     #   - `x-amz-bucket-region` which for some reason causes this error for tasks ran through
     #     Nextflow: `The AWS Access Key Id you provided does not exist in our records`.

gen3workflow/routes/storage.py

@@ -40,17 +40,14 @@ async def storage_setup(
     # only users with access to create tasks should be able to setup their storage
     await auth.authorize("create", ["/services/workflow/gen3-workflow/tasks"])
 
-    bucket_name, kms_key_arn = await create_user_bucket(user_id)
+    bucket_name = await create_user_bucket(user_id)
     bucket_prefix = "ga4gh-tes"
     bucket_region = config["USER_BUCKETS_REGION"]
 
     storage_info = {
         "bucket": bucket_name,
         "workdir": f"s3://{bucket_name}/{bucket_prefix}",
         "region": bucket_region,
-        "kms_key_arn": (
-            kms_key_arn if config["KMS_ENCRYPTION_ENABLED"] and kms_key_arn else None
-        ),
     }
 
     if config["ENABLE_S3_FILES"]:
@@ -59,7 +56,7 @@ async def storage_setup(
             # Create S3 Files Filesystem ID if not exists
             fs_id = s3_files.setup_s3_filesystem(bucket_name)
             # NOTE: To avoid blocking `/storage/setup` call, setting s3 filesystem just returns
-            # the filesystem id and continue with the rest of the steps asynchronously
+            # the filesystem id and continues with the rest of the steps asynchronously
             background_tasks.add_task(s3_files.provision_mount_targets, fs_id)
 
         storage_info["s3files_filesystem_id"] = fs_id

tests/test_storage.py

@@ -62,7 +62,6 @@ async def test_storage_setup(
         "bucket": expected_bucket_name,
         "workdir": f"s3://{expected_bucket_name}/ga4gh-tes",
         "region": config["USER_BUCKETS_REGION"],
-        "kms_key_arn": kms_key_arn,
     }
 
     # check that the bucket was created after the call to `/storage/setup`