From e343196cc0b9bd3d63e028ae80371693a2a5b3ef Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Tue, 7 Jul 2026 13:38:14 -0500 Subject: [PATCH 01/33] WIP: Enable bucket versioning --- gen3workflow/aws_utils.py | 81 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/gen3workflow/aws_utils.py b/gen3workflow/aws_utils.py index bdafdb9..d82eaf1 100644 --- a/gen3workflow/aws_utils.py +++ b/gen3workflow/aws_utils.py @@ -45,6 +45,7 @@ def get_boto3_client(service_name: str, **kwargs): kms_client = get_boto3_client("kms", region_name=config["USER_BUCKETS_REGION"]) sts_client = get_boto3_client("sts") eks_client = get_boto3_client("eks", region_name=config["EKS_CLUSTER_REGION"]) +s3files_client = get_boto3_client("s3files", region_name=config["USER_BUCKETS_REGION"]) def get_safe_name_from_hostname( @@ -378,6 +379,83 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None: return kms_key_arn +def enable_bucket_versioning(bucket_name: str) -> None: + """ + Enable versioning on the specified S3 bucket. + + Args: + bucket_name (str): name of the bucket to enable versioning on + """ + try: + existing_versioning = s3_client.get_bucket_versioning(Bucket=bucket_name) + if existing_versioning.get("Status") != "Enabled": + s3_client.put_bucket_versioning( + Bucket=bucket_name, + VersioningConfiguration={"Status": "Enabled"}, + ) + logger.debug(f"Enabled versioning on bucket '{bucket_name}'") + else: + logger.debug(f"Bucket '{bucket_name}' already has versioning enabled") + except ClientError as e: + logger.error( + f"Failed to enable versioning on bucket '{bucket_name}': {e.response['Error']['Message']}" + ) + raise + + +def get_or_create_s3_files_system(bucket_name: str, region: str, role_arn: str) -> str: + """ + Checks if an Amazon S3 Files filesystem already exists for a bucket. + If yes, returns the FileSystemId. If no, creates it and returns the new ID. + """ + # Standard S3 Files buckets require full ARN strings + bucket_arn = f"arn:aws:s3:::{bucket_name}" + logger.debug(f"Checking for existing S3Files mounts assigned to: {bucket_name}...") + try: + # List all file systems managed in this region + paginator = s3files_client.get_paginator("list_file_systems") + for page in paginator.paginate(): + for fs in page.get("FileSystems", []): + # Match against the underlying bucket ARN + if fs.get("Bucket") == bucket_arn: + fs_id = fs["FileSystemId"] + logger.debug( + f"Found existing S3Files system! ID: {fs_id} (State: {fs['LifeCycleState']})" + ) + return fs_id + except ClientError as e: + logger.error( + f"Failed to list S3 Files filesystems: {e.response['Error']['Message']}" + ) + raise + + # If not found, create a new S3 Files filesystem + try: + response = s3files_client.create_file_system( + bucket=bucket_arn, + StorageType="S3", + prefix="funnel-temp-files", + StorageConfiguration={"BucketName": bucket_name}, + FileSystemTypeVersion="1.0", + RoleArn=role_arn, + Tags=[{"Key": "Name", "Value": get_safe_name_from_hostname(user_id=None)}], + ) + file_system_id = response["FileSystemId"] + logger.debug( + f"Created new S3 Files filesystem '{file_system_id}' for bucket '{bucket_name}'" + ) + return file_system_id + except ClientError as e: + logger.error( + f"Failed to create S3 Files filesystem for bucket '{bucket_name}': {e.response['Error']['Message']}" + ) + raise + + +def create_mount_target_for_s3_files(file_system_id: str, region: str) -> None: + pass + + async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: """ Create an S3 bucket for the specified user and return information about the bucket. @@ -454,6 +532,9 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: s3_client.delete_bucket_encryption(Bucket=user_bucket_name) s3_client.delete_bucket_policy(Bucket=user_bucket_name) + enable_bucket_versioning(user_bucket_name) + # Create S3 Files Filesystem ID + return user_bucket_name, kms_key_arn From efdeabb1d1c72da8281096c29760a1109c2e6adb Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Tue, 21 Jul 2026 10:50:44 -0500 Subject: [PATCH 02/33] [UNTESTED]Add initial s3files setup + separate out s3 files into a new aws module --- gen3workflow/aws/s3_files.py | 394 +++++++++++++++++++++++++++++++ gen3workflow/aws_utils.py | 64 +---- gen3workflow/config-default.yaml | 3 + gen3workflow/routes/storage.py | 7 +- 4 files changed, 410 insertions(+), 58 deletions(-) create mode 100644 gen3workflow/aws/s3_files.py diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py new file mode 100644 index 0000000..632ba76 --- /dev/null +++ b/gen3workflow/aws/s3_files.py @@ -0,0 +1,394 @@ +from botocore.exceptions import ClientError + + +from gen3workflow import logger +from gen3workflow.config import config +from aws_utils import s3files_client, eks_client, ec2_client + +# NFS port used for all communication between EKS pods and S3 Files mount targets. +NFS_PORT = 2049 + +# --------------------------------------------------------------------------- # +# File system management +# --------------------------------------------------------------------------- # + + +def get_s3_files_system(bucket_name: str) -> str | None: + """ + Look up an existing S3 Files file system for the given bucket. + + Args: + bucket_name: name of the source S3 bucket. + + Returns: + The file system ID (if exists). + """ + bucket_arn = f"arn:aws:s3:::{bucket_name}" + logger.debug( + f"Checking for an existing S3 Files file system for bucket '{bucket_name}'..." + ) + + try: + paginator = s3files_client.get_paginator("list_file_systems") + for page in paginator.paginate(): + for fs in page.get("FileSystems", []): + if fs.get("Bucket") == bucket_arn: + fs_id = fs["FileSystemId"] + logger.debug( + f"Found existing S3 Files file system '{fs_id}' (state: {fs['LifeCycleState']})" + ) + return fs_id + except ClientError as e: + logger.error( + f"Failed to list S3 Files file systems: {e.response['Error']['Message']}" + ) + raise + + return None + + +def get_filesystem_status(file_system_id: str): + # TODO: fetch file system status + list mount targets and get their statuses, and unify into a single + # usable status (e.g. "ready" only once the FS and all expected mount targets + # are in an available state). + + # TODO: What if new AZs are added to the node after initial bucket setup? Filesystem may need new mount targets in these AZs too. + # This should not be the responsibility of this function, but where to put it? + return "Not ready" + + +def _create_s3_files_system(bucket_name: str, role_arn: str) -> str: + """ + Creating S3 Files file system for the given bucket. + + Args: + bucket_name: name of the source S3 bucket. + role_arn: ARN of the IAM role S3 Files assumes to sync with the bucket. + + Returns: + The file system ID. + """ + bucket_arn = f"arn:aws:s3:::{bucket_name}" + + try: + response = s3files_client.create_file_system( + bucket=bucket_arn, + prefix="funnel-temp-files", + roleArn=role_arn, + tags=[{"Key": "app-name", "Value": "gen3-workflow"}], + ) + file_system_id = response["fileSystemId"] + logger.debug( + f"Created new S3 Files file system '{file_system_id}' for bucket '{bucket_name}'" + ) + return file_system_id + except ClientError as e: + logger.error( + f"Failed to create S3 Files file system for bucket '{bucket_name}': {e.response['Error']['Message']}" + ) + raise + + +# --------------------------------------------------------------------------- # +# Mount target management +# --------------------------------------------------------------------------- # + + +def create_mount_target_for_file_system( + file_system_id: str, subnet_id: str, mount_target_sg_id: str +) -> None: + """ + Create a mount target for the given file system in the given subnet. + + Args: + file_system_id: the S3 Files file system to attach the mount target to. + subnet_id: subnet where the mount target's ENI will be created in. + mount_target_sg_id: security group to attach to the mount target's ENI. + """ + try: + response = s3files_client.create_mount_target( + fileSystemId=file_system_id, + subnetId=subnet_id, + securityGroups=[mount_target_sg_id], + ) + logger.info( + "Created mount target %s in subnet %s (status=%s)", + response.get("mountTargetId"), + subnet_id, + response.get("status"), + ) + except ClientError as exc: + logger.error("Failed to create mount target in subnet %s: %s", subnet_id, exc) + raise + + +def list_mount_targets_for_file_system(file_system_id: str) -> list[dict]: + """ + List all mount targets currently provisioned for a file system. + + Returns: + A flat list of mount target dicts + [{ + 'mountTargetId': 'string', + 'availabilityZoneId': 'string', + 'fileSystemId': 'string', + 'status': 'available'|'creating'|'deleting'|'deleted'|'error'|'updating', + 'statusMessage': 'string', + 'ipv4Address': 'string', + 'ipv6Address': 'string', + 'networkInterfaceId': 'string', + 'ownerId': 'string', + 'subnetId': 'string', + 'vpcId': 'string' + },..] + } + """ + mount_targets = [] + try: + paginator = s3files_client.get_paginator("list_mount_targets") + for page in paginator.paginate(fileSystemId=file_system_id): + mount_targets.extend(page.get("mountTargets", [])) + except ClientError as e: + logger.error( + f"Failed to list mount targets for {file_system_id=}: {e.response['Error']['Message']}" + ) + raise + return mount_targets + + +# --------------------------------------------------------------------------- # +# Networking / security groups +# --------------------------------------------------------------------------- # + + +def _get_vpc_id() -> str: + """ + Return the VPC ID the EKS cluster's nodes run in. Used to determine which VPC + the S3 Files mount targets should be created in. + """ + cluster = eks_client.describe_cluster(name=config["EKS_CLUSTER_NAME"]) + return cluster["cluster"]["resourcesVpcConfig"]["vpcId"] + + +def _get_available_az_to_subnet(discovery_tag: str) -> dict[str, str]: + """ + Get one subnet ID per AZ where EKS pods can be scheduled, so that one S3 Files + mount target can be created per AZ. + """ + subnets = ec2_client.describe_subnets( + Filters=[{"Name": "tag:karpenter.sh/discovery", "Values": [discovery_tag]}], + )["Subnets"] + return {subnet["AvailabilityZoneId"]: subnet["SubnetId"] for subnet in subnets} + + +def _get_eks_security_groups() -> list[tuple[str, str]]: + """ + Return (sg_id, sg_name) for every security group that Karpenter could attach + to an EKS worker node in this cluster (workflow and jupyter node pools). + """ + security_groups = ec2_client.describe_security_groups( + Filters=[ + { + "Name": "tag:karpenter.sh/discovery", + "Values": [ + config["EKS_CLUSTER_NAME"], + f'{config["EKS_CLUSTER_NAME"]}-jupyter', + ], + } + ] + ) + return [ + (group["GroupId"], group["GroupName"]) + for group in security_groups["SecurityGroups"] + ] + + +def _get_or_create_security_groups(bucket_name: str, vpc_id: str) -> str: + """ + Ensure the mount target security group exists and has the correct bidirectional + NFS rules in place against every EKS worker security group: + + | Security group | Rule type | Protocol | Port | Source/destination | + |-----------------|-----------|----------|------|-------------------------------| + | Compute SG | Outbound | TCP | 2049 | Mount target security group | + | Mount target SG | Inbound | TCP | 2049 | Compute security group | + + Returns: + The mount target security group ID. + """ + compute_security_groups = _get_eks_security_groups() + + mount_target_sg_name = f"{bucket_name}-mount-target-sg" + existing = ec2_client.describe_security_groups( + Filters=[{"Name": "group-name", "Values": [mount_target_sg_name]}] + )["SecurityGroups"] + + if existing: + mount_target_sg_id = existing[0]["GroupId"] + logger.info( + "Security group '%s' already exists (%s)", + mount_target_sg_name, + mount_target_sg_id, + ) + else: + response = ec2_client.create_security_group( + GroupName=mount_target_sg_name, + Description="S3 Files mount target SG -- allows inbound NFS (2049) from Funnel compute SGs only.", + VpcId=vpc_id, + ) + mount_target_sg_id = response["GroupId"] + logger.info( + "Created security group '%s' (%s)", mount_target_sg_name, mount_target_sg_id + ) + + # Inbound: mount target SG allows NFS from every compute SG, idempotently. + try: + ec2_client.authorize_security_group_ingress( + GroupId=mount_target_sg_id, + IpPermissions=[ + { + "IpProtocol": "tcp", + "FromPort": NFS_PORT, + "ToPort": NFS_PORT, + "UserIdGroupPairs": [ + {"GroupId": sg_id} for sg_id, _ in compute_security_groups + ], + } + ], + ) + logger.info( + "Authorized inbound TCP/%s on %s from %s", + NFS_PORT, + mount_target_sg_id, + compute_security_groups, + ) + except ClientError as exc: + if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": + logger.info( + "Inbound TCP/%s on %s from %s already exists; skipping.", + NFS_PORT, + mount_target_sg_id, + compute_security_groups, + ) + else: + raise + + # Outbound: each compute SG allows NFS to the mount target SG, idempotently. + for compute_sg_id, compute_sg_name in compute_security_groups: + try: + ec2_client.authorize_security_group_egress( + GroupId=compute_sg_id, + IpPermissions=[ + { + "IpProtocol": "tcp", + "FromPort": NFS_PORT, + "ToPort": NFS_PORT, + "UserIdGroupPairs": [{"GroupId": mount_target_sg_id}], + } + ], + ) + logger.info( + "Authorized outbound TCP/%s on %s (%s) to %s", + NFS_PORT, + compute_sg_id, + compute_sg_name, + mount_target_sg_id, + ) + except ClientError as exc: + if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": + logger.info( + "Outbound TCP/%s on %s (%s) to %s already exists; skipping.", + NFS_PORT, + compute_sg_id, + compute_sg_name, + mount_target_sg_id, + ) + else: + raise + + return mount_target_sg_id + + +# --------------------------------------------------------------------------- # +# IAM roles +# --------------------------------------------------------------------------- # + + +def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: + """ + Get or create the IAM role that S3 Files itself assumes to read from / write to + the source bucket and manage its EventBridge sync rules. This is the `roleArn` + passed to `create_file_system`. + + NOTE: this is *not* the role attached to compute (EKS nodes) for mounting the + file system -- that's handled separately. + + Reference: https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-files-prereq-policies.html#s3-files-prereq-iam + + Returns: + ARN of the (new or existing) IAM role. + """ + # TODO: implementation + pass + + +# --------------------------------------------------------------------------- # +# Orchestration +# --------------------------------------------------------------------------- # + + +def setup_s3_filesystem(bucket_name: str) -> str: + """ + Ensure an S3 Files file system exists for `bucket_name`, with one mount target + per AZ the EKS cluster's nodes can run in, and the security groups needed for + pods to reach those mount targets over NFS. + + Returns: + The file system ID. + """ + + region = config["USER_BUCKETS_REGION"] + role_arn = _get_or_create_s3_files_bucket_role( + bucket_name=bucket_name, region=region + ) + + file_system_id = _create_s3_files_system( + bucket_name, region=region, role_arn=role_arn + ) + + available_az_to_subnet_mapping = _get_available_az_to_subnet( + discovery_tag=config["EKS_CLUSTER_NAME"] + ) + mount_targets = list_mount_targets_for_file_system(file_system_id) + + # ASSUMPTION: this implementation currently requires the S3 bucket and the EKS + # cluster to be in the same AWS region. We derive the mount target VPC from the + # current compute environment (EKS cluster's VPC), which only works because the + # S3 Files file system and its mount targets are expected to live in that same + # region/VPC as the cluster. + # + # TODO: Support buckets in a different region than compute. This will require: + # 1. Resolving the VPC (and mount target subnets) in the BUCKET's region rather + # than assuming it's the same as the compute VPC. + # 2. Setting up cross-region connectivity (VPC peering or Transit Gateway) + # between the compute VPC and the bucket-region VPC, including route table + # updates on both sides. + # 3. Allowing inbound NFS (TCP 2049) from EKS node/pod security group on the mount + # target security group in the bucket's region. + mount_target_vpc_id = _get_vpc_id() + + mount_target_sg_id = _get_or_create_security_groups( + bucket_name=bucket_name, vpc_id=mount_target_vpc_id + ) + + # Create new m.t.s of this file system ID for each missing az from the az_to_subnet_dict + az_with_mount_targets = {mt["availabilityZoneId"] for mt in mount_targets} + for az, subnet_id in available_az_to_subnet_mapping.items(): + if az in az_with_mount_targets: + continue + create_mount_target_for_file_system( + file_system_id=file_system_id, + subnet_id=subnet_id, + mount_target_sg_id=mount_target_sg_id, + ) + return file_system_id diff --git a/gen3workflow/aws_utils.py b/gen3workflow/aws_utils.py index d82eaf1..64a669a 100644 --- a/gen3workflow/aws_utils.py +++ b/gen3workflow/aws_utils.py @@ -10,6 +10,7 @@ from starlette.status import HTTP_400_BAD_REQUEST from gen3workflow import logger +from gen3workflow.aws.s3_files import get_s3_files_system, setup_s3_filesystem from gen3workflow.config import config USER_BUCKET_CACHE = SimpleCache(default_timeout=config["USER_BUCKET_CACHE_SECONDS"]) @@ -46,6 +47,7 @@ def get_boto3_client(service_name: str, **kwargs): sts_client = get_boto3_client("sts") eks_client = get_boto3_client("eks", region_name=config["EKS_CLUSTER_REGION"]) s3files_client = get_boto3_client("s3files", region_name=config["USER_BUCKETS_REGION"]) +ec2_client = get_boto3_client("ec2", region_name=config["USER_BUCKETS_REGION"]) def get_safe_name_from_hostname( @@ -403,59 +405,6 @@ def enable_bucket_versioning(bucket_name: str) -> None: raise -def get_or_create_s3_files_system(bucket_name: str, region: str, role_arn: str) -> str: - """ - Checks if an Amazon S3 Files filesystem already exists for a bucket. - If yes, returns the FileSystemId. If no, creates it and returns the new ID. - """ - # Standard S3 Files buckets require full ARN strings - bucket_arn = f"arn:aws:s3:::{bucket_name}" - logger.debug(f"Checking for existing S3Files mounts assigned to: {bucket_name}...") - try: - # List all file systems managed in this region - paginator = s3files_client.get_paginator("list_file_systems") - for page in paginator.paginate(): - for fs in page.get("FileSystems", []): - # Match against the underlying bucket ARN - if fs.get("Bucket") == bucket_arn: - fs_id = fs["FileSystemId"] - logger.debug( - f"Found existing S3Files system! ID: {fs_id} (State: {fs['LifeCycleState']})" - ) - return fs_id - except ClientError as e: - logger.error( - f"Failed to list S3 Files filesystems: {e.response['Error']['Message']}" - ) - raise - - # If not found, create a new S3 Files filesystem - try: - response = s3files_client.create_file_system( - bucket=bucket_arn, - StorageType="S3", - prefix="funnel-temp-files", - StorageConfiguration={"BucketName": bucket_name}, - FileSystemTypeVersion="1.0", - RoleArn=role_arn, - Tags=[{"Key": "Name", "Value": get_safe_name_from_hostname(user_id=None)}], - ) - file_system_id = response["FileSystemId"] - logger.debug( - f"Created new S3 Files filesystem '{file_system_id}' for bucket '{bucket_name}'" - ) - return file_system_id - except ClientError as e: - logger.error( - f"Failed to create S3 Files filesystem for bucket '{bucket_name}': {e.response['Error']['Message']}" - ) - raise - - -def create_mount_target_for_s3_files(file_system_id: str, region: str) -> None: - pass - - async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: """ Create an S3 bucket for the specified user and return information about the bucket. @@ -464,7 +413,7 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: user_id (str): The user's unique Gen3 ID Returns: - tuple: (bucket name, prefix where the user stores objects in the bucket, bucket region, kms key ARN) + tuple: (bucket name, kms key ARN, S3 files filesystem ID) """ user_bucket_name = get_bucket_name_from_user_id(user_id) try: @@ -533,9 +482,12 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: s3_client.delete_bucket_policy(Bucket=user_bucket_name) enable_bucket_versioning(user_bucket_name) - # Create S3 Files Filesystem ID + fs_id = get_s3_files_system(user_bucket_name) + if not fs_id: + # Create S3 Files Filesystem ID if not exists + fs_id = setup_s3_filesystem(user_bucket_name) - return user_bucket_name, kms_key_arn + return user_bucket_name, kms_key_arn, fs_id async def create_user_bucket(user_id: str) -> Tuple[str, str, str]: diff --git a/gen3workflow/config-default.yaml b/gen3workflow/config-default.yaml index 98a9bc2..99b88b4 100644 --- a/gen3workflow/config-default.yaml +++ b/gen3workflow/config-default.yaml @@ -28,6 +28,9 @@ USER_BUCKET_CACHE_SECONDS: 43200 # AWS S3 # ########## +# NOTE: Currently Gen3Workflow only supports bucket and the EKS cluster being present in the same region. +# This is due to a limitation in the implementation of underlying S3Files feature. +# Support shall be added in the future. USER_BUCKETS_REGION: us-east-1 # connect to another S3-compatible service than AWS S3 (default: AWS S3) diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index 0cf82c2..071c2d4 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -10,6 +10,7 @@ from gen3workflow import aws_utils, logger from gen3workflow.auth import Auth +from gen3workflow.aws.s3_files import get_filesystem_status from gen3workflow.config import config router = APIRouter(prefix="/storage") @@ -36,10 +37,10 @@ async def storage_setup(request: Request, auth=Depends(Auth)) -> dict: # 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 aws_utils.create_user_bucket(user_id) + bucket_name, kms_key_arn, fs_id = await aws_utils.create_user_bucket(user_id) bucket_prefix = "ga4gh-tes" bucket_region = config["USER_BUCKETS_REGION"] - + filesystem_status = get_filesystem_status(fs_id) try: await auth.grant_user_access_to_their_own_data( username=username, user_id=user_id @@ -55,6 +56,8 @@ async def storage_setup(request: Request, auth=Depends(Auth)) -> dict: "kms_key_arn": ( kms_key_arn if config["KMS_ENCRYPTION_ENABLED"] and kms_key_arn else None ), + "s3files_filesystem_id": fs_id, + "status": filesystem_status, } From 41926ab47a309fbbdb1ccc948b01fa0fdbf04e0a Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Tue, 21 Jul 2026 11:33:49 -0500 Subject: [PATCH 03/33] Update EKS security group filter to use EKS group name --- gen3workflow/aws/s3_files.py | 102 +++++++++++++++++------------------ 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 632ba76..0769a93 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -181,26 +181,23 @@ def _get_available_az_to_subnet(discovery_tag: str) -> dict[str, str]: return {subnet["AvailabilityZoneId"]: subnet["SubnetId"] for subnet in subnets} -def _get_eks_security_groups() -> list[tuple[str, str]]: +def _get_eks_security_group() -> tuple[str, str]: """ Return (sg_id, sg_name) for every security group that Karpenter could attach to an EKS worker node in this cluster (workflow and jupyter node pools). """ - security_groups = ec2_client.describe_security_groups( + eks_sg_name = f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg" + security_group = ec2_client.describe_security_groups( Filters=[ { - "Name": "tag:karpenter.sh/discovery", - "Values": [ - config["EKS_CLUSTER_NAME"], - f'{config["EKS_CLUSTER_NAME"]}-jupyter', - ], + "Name": "group-name", + "Values": [eks_sg_name], } ] - ) - return [ - (group["GroupId"], group["GroupName"]) - for group in security_groups["SecurityGroups"] - ] + )["SecurityGroups"][0] + if not security_group: + raise f"Missing eks security group {eks_sg_name}" + return security_group["GroupId"], security_group["GroupName"] def _get_or_create_security_groups(bucket_name: str, vpc_id: str) -> str: @@ -208,15 +205,15 @@ def _get_or_create_security_groups(bucket_name: str, vpc_id: str) -> str: Ensure the mount target security group exists and has the correct bidirectional NFS rules in place against every EKS worker security group: - | Security group | Rule type | Protocol | Port | Source/destination | - |-----------------|-----------|----------|------|-------------------------------| - | Compute SG | Outbound | TCP | 2049 | Mount target security group | - | Mount target SG | Inbound | TCP | 2049 | Compute security group | + | Security group | Rule type | Protocol | Port | Source/destination | + |-----------------|-----------|----------|------|-------------------------------| + | Compute SG | Outbound | TCP | 2049 | Mount target security group | + | Mount target SG | Inbound | TCP | 2049 | Compute security group | Returns: The mount target security group ID. """ - compute_security_groups = _get_eks_security_groups() + compute_security_group_id, compute_security_group_name = _get_eks_security_group() mount_target_sg_name = f"{bucket_name}-mount-target-sg" existing = ec2_client.describe_security_groups( @@ -250,61 +247,64 @@ def _get_or_create_security_groups(bucket_name: str, vpc_id: str) -> str: "IpProtocol": "tcp", "FromPort": NFS_PORT, "ToPort": NFS_PORT, - "UserIdGroupPairs": [ - {"GroupId": sg_id} for sg_id, _ in compute_security_groups - ], + "UserIdGroupPairs": [{"GroupId": compute_security_group_id}], } ], ) logger.info( - "Authorized inbound TCP/%s on %s from %s", + "Authorized inbound TCP/%s on %s (%s) from %s (%s)", NFS_PORT, + mount_target_sg_name, mount_target_sg_id, - compute_security_groups, + compute_security_group_name, + compute_security_group_id, ) except ClientError as exc: if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": logger.info( - "Inbound TCP/%s on %s from %s already exists; skipping.", + "Inbound TCP/%s on %s (%s) from %s (%s) already exists; skipping.", NFS_PORT, + mount_target_sg_name, mount_target_sg_id, - compute_security_groups, + compute_security_group_name, + compute_security_group_id, ) else: raise # Outbound: each compute SG allows NFS to the mount target SG, idempotently. - for compute_sg_id, compute_sg_name in compute_security_groups: - try: - ec2_client.authorize_security_group_egress( - GroupId=compute_sg_id, - IpPermissions=[ - { - "IpProtocol": "tcp", - "FromPort": NFS_PORT, - "ToPort": NFS_PORT, - "UserIdGroupPairs": [{"GroupId": mount_target_sg_id}], - } - ], - ) + try: + ec2_client.authorize_security_group_egress( + GroupId=compute_security_group_id, + IpPermissions=[ + { + "IpProtocol": "tcp", + "FromPort": NFS_PORT, + "ToPort": NFS_PORT, + "UserIdGroupPairs": [{"GroupId": mount_target_sg_id}], + } + ], + ) + logger.info( + "Authorized outbound TCP/%s on %s (%s) to %s (%s)", + NFS_PORT, + compute_security_group_id, + compute_security_group_name, + mount_target_sg_id, + mount_target_sg_name, + ) + except ClientError as exc: + if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": logger.info( - "Authorized outbound TCP/%s on %s (%s) to %s", + "Outbound TCP/%s on %s (%s) to %s (%s) already exists; skipping.", NFS_PORT, - compute_sg_id, - compute_sg_name, + compute_security_group_id, + compute_security_group_name, mount_target_sg_id, + mount_target_sg_name, ) - except ClientError as exc: - if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": - logger.info( - "Outbound TCP/%s on %s (%s) to %s already exists; skipping.", - NFS_PORT, - compute_sg_id, - compute_sg_name, - mount_target_sg_id, - ) - else: - raise + else: + raise return mount_target_sg_id From 6e9abd52643f28953e6933e7cdcbd0321350f2ef Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Tue, 21 Jul 2026 11:42:42 -0500 Subject: [PATCH 04/33] Update docstring --- gen3workflow/aws/s3_files.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 0769a93..6afd780 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -183,8 +183,8 @@ def _get_available_az_to_subnet(discovery_tag: str) -> dict[str, str]: def _get_eks_security_group() -> tuple[str, str]: """ - Return (sg_id, sg_name) for every security group that Karpenter could attach - to an EKS worker node in this cluster (workflow and jupyter node pools). + Return (sg_id, sg_name) for the EKS security group that Karpenter attaches + to an EKS worker node in this cluster. """ eks_sg_name = f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg" security_group = ec2_client.describe_security_groups( From 6e368cdefeb20d4bf4748ae2d1decfc3b3b9e6a7 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Tue, 21 Jul 2026 14:07:03 -0500 Subject: [PATCH 05/33] Add a TODO comment regarding security group creation, maybe can reduce aws sdk calls --- gen3workflow/aws/s3_files.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 6afd780..522f777 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -200,6 +200,8 @@ def _get_eks_security_group() -> tuple[str, str]: return security_group["GroupId"], security_group["GroupName"] +# TODO: Investigate if this can be moved to server startup logic or elsewhere? +# Since the `create` part is only needed once per deployed environment. def _get_or_create_security_groups(bucket_name: str, vpc_id: str) -> str: """ Ensure the mount target security group exists and has the correct bidirectional @@ -329,7 +331,7 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: ARN of the (new or existing) IAM role. """ # TODO: implementation - pass + return "arn:aws:iam::707767160287:role/Gen3WorkflowS3FilesPOC" # --------------------------------------------------------------------------- # From f4f2b759dcbaccb203223e91887093ec83a7ccee Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Tue, 21 Jul 2026 15:36:08 -0500 Subject: [PATCH 06/33] Fix minor bugs and add more TODOs. --- gen3workflow/aws/s3_files.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 522f777..8f4bd0c 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -73,9 +73,9 @@ def _create_s3_files_system(bucket_name: str, role_arn: str) -> str: try: response = s3files_client.create_file_system( bucket=bucket_arn, - prefix="funnel-temp-files", + prefix="funnel-temp-files/", roleArn=role_arn, - tags=[{"Key": "app-name", "Value": "gen3-workflow"}], + tags=[{"key": "app-name", "value": "gen3-workflow"}], ) file_system_id = response["fileSystemId"] logger.debug( @@ -201,8 +201,8 @@ def _get_eks_security_group() -> tuple[str, str]: # TODO: Investigate if this can be moved to server startup logic or elsewhere? -# Since the `create` part is only needed once per deployed environment. -def _get_or_create_security_groups(bucket_name: str, vpc_id: str) -> str: +# Since the `create` part is only needed once per cluster. +def _get_or_create_security_groups(vpc_id: str) -> str: """ Ensure the mount target security group exists and has the correct bidirectional NFS rules in place against every EKS worker security group: @@ -217,7 +217,7 @@ def _get_or_create_security_groups(bucket_name: str, vpc_id: str) -> str: """ compute_security_group_id, compute_security_group_name = _get_eks_security_group() - mount_target_sg_name = f"{bucket_name}-mount-target-sg" + mount_target_sg_name = "gen3wf-s3files-mount-target-sg" existing = ec2_client.describe_security_groups( Filters=[{"Name": "group-name", "Values": [mount_target_sg_name]}] )["SecurityGroups"] @@ -354,9 +354,7 @@ def setup_s3_filesystem(bucket_name: str) -> str: bucket_name=bucket_name, region=region ) - file_system_id = _create_s3_files_system( - bucket_name, region=region, role_arn=role_arn - ) + file_system_id = _create_s3_files_system(bucket_name, role_arn=role_arn) available_az_to_subnet_mapping = _get_available_az_to_subnet( discovery_tag=config["EKS_CLUSTER_NAME"] From 0248c3d799104c83a1a3c5954caf87b51389318f Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 10:32:58 -0500 Subject: [PATCH 07/33] Add file system status implementation and waiter --- gen3workflow/aws/s3_files.py | 158 +++++++++++++++++++++++------------ 1 file changed, 104 insertions(+), 54 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 8f4bd0c..b001a6a 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -1,5 +1,5 @@ from botocore.exceptions import ClientError - +import time from gen3workflow import logger from gen3workflow.config import config @@ -47,13 +47,43 @@ def get_s3_files_system(bucket_name: str) -> str | None: return None -def get_filesystem_status(file_system_id: str): - # TODO: fetch file system status + list mount targets and get their statuses, and unify into a single - # usable status (e.g. "ready" only once the FS and all expected mount targets +def get_filesystem_status(file_system_id: str) -> tuple[str | None, str | None]: + """ + Fetch the status of an S3 Files file system. + + Args: + file_system_id: The ID of the file system to check. + + Returns: + A tuple of (status, status_message): + - (status, status_message) on success, e.g. ("AVAILABLE", None) + - (None, error_message) if the file system doesn't exist or the + lookup fails. + """ + if not file_system_id: + return None, "file_system_id must not be empty" + + try: + fs = s3files_client.get_file_system(fileSystemId=file_system_id) + except s3files_client.exceptions.FileSystemNotFound: + return None, f"File system with file_system_id={file_system_id} does not exist" + except ClientError as e: + return None, f"Failed to fetch file system {file_system_id}: {e}" + + if not fs: + return None, f"File system with file_system_id={file_system_id} does not exist" + + return fs.get("status"), fs.get("statusMessage") + + +def get_mount_target_status(file_system_id: str): + # TODO: list mount targets and get their statuses, and unify into a single + # usable status (e.g. "ready" only once all expected mount targets # are in an available state). # TODO: What if new AZs are added to the node after initial bucket setup? Filesystem may need new mount targets in these AZs too. # This should not be the responsibility of this function, but where to put it? + return "Not ready" @@ -181,27 +211,25 @@ def _get_available_az_to_subnet(discovery_tag: str) -> dict[str, str]: return {subnet["AvailabilityZoneId"]: subnet["SubnetId"] for subnet in subnets} -def _get_eks_security_group() -> tuple[str, str]: +def _get_eks_security_groups() -> tuple[str, str]: """ Return (sg_id, sg_name) for the EKS security group that Karpenter attaches to an EKS worker node in this cluster. """ - eks_sg_name = f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg" - security_group = ec2_client.describe_security_groups( - Filters=[ - { - "Name": "group-name", - "Values": [eks_sg_name], - } - ] - )["SecurityGroups"][0] - if not security_group: - raise f"Missing eks security group {eks_sg_name}" - return security_group["GroupId"], security_group["GroupName"] + # TODO: Determine if these need to be configurable. + eks_sg_names = [ + f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg", + f"{config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg", + ] + security_groups = ec2_client.describe_security_groups( + Filters=[{"Name": "group-name", "Values": eks_sg_names}] + )["SecurityGroups"] + return [(group["GroupId"], group["GroupName"]) for group in security_groups] # TODO: Investigate if this can be moved to server startup logic or elsewhere? -# Since the `create` part is only needed once per cluster. +# Since the `create` part is only needed once per cluster, no need to run for every bucket. +# This takes approximately 2 seconds to run everytime it is invoked. def _get_or_create_security_groups(vpc_id: str) -> str: """ Ensure the mount target security group exists and has the correct bidirectional @@ -215,7 +243,7 @@ def _get_or_create_security_groups(vpc_id: str) -> str: Returns: The mount target security group ID. """ - compute_security_group_id, compute_security_group_name = _get_eks_security_group() + compute_security_groups = _get_eks_security_groups() mount_target_sg_name = "gen3wf-s3files-mount-target-sg" existing = ec2_client.describe_security_groups( @@ -249,64 +277,69 @@ def _get_or_create_security_groups(vpc_id: str) -> str: "IpProtocol": "tcp", "FromPort": NFS_PORT, "ToPort": NFS_PORT, - "UserIdGroupPairs": [{"GroupId": compute_security_group_id}], + "UserIdGroupPairs": [ + {"GroupId": compute_security_group_id} + for compute_security_group_id, _ in compute_security_groups + ], } ], ) logger.info( - "Authorized inbound TCP/%s on %s (%s) from %s (%s)", + "Authorized inbound TCP/%s on %s (%s) from %s", NFS_PORT, mount_target_sg_name, mount_target_sg_id, - compute_security_group_name, - compute_security_group_id, + compute_security_groups, ) except ClientError as exc: if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": logger.info( - "Inbound TCP/%s on %s (%s) from %s (%s) already exists; skipping.", + "Inbound TCP/%s on %s (%s) from %s already exists; skipping.", NFS_PORT, mount_target_sg_name, mount_target_sg_id, - compute_security_group_name, - compute_security_group_id, + compute_security_groups, ) else: raise # Outbound: each compute SG allows NFS to the mount target SG, idempotently. - try: - ec2_client.authorize_security_group_egress( - GroupId=compute_security_group_id, - IpPermissions=[ - { - "IpProtocol": "tcp", - "FromPort": NFS_PORT, - "ToPort": NFS_PORT, - "UserIdGroupPairs": [{"GroupId": mount_target_sg_id}], - } - ], - ) - logger.info( - "Authorized outbound TCP/%s on %s (%s) to %s (%s)", - NFS_PORT, - compute_security_group_id, - compute_security_group_name, - mount_target_sg_id, - mount_target_sg_name, - ) - except ClientError as exc: - if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": + for ( + compute_security_group_id, + compute_security_group_name, + ) in compute_security_groups: + try: + ec2_client.authorize_security_group_egress( + GroupId=compute_security_group_id, + IpPermissions=[ + { + "IpProtocol": "tcp", + "FromPort": NFS_PORT, + "ToPort": NFS_PORT, + "UserIdGroupPairs": [{"GroupId": mount_target_sg_id}], + } + ], + ) logger.info( - "Outbound TCP/%s on %s (%s) to %s (%s) already exists; skipping.", + "Authorized outbound TCP/%s on %s (%s) to %s (%s)", NFS_PORT, compute_security_group_id, compute_security_group_name, mount_target_sg_id, mount_target_sg_name, ) - else: - raise + except ClientError as exc: + if exc.response["Error"]["Code"] == "InvalidPermission.Duplicate": + logger.info( + "Outbound TCP/%s on %s (%s) to %s (%s) already exists; skipping.", + NFS_PORT, + compute_security_group_id, + compute_security_group_name, + mount_target_sg_id, + mount_target_sg_name, + ) + else: + raise return mount_target_sg_id @@ -334,6 +367,24 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: return "arn:aws:iam::707767160287:role/Gen3WorkflowS3FilesPOC" +# --------- +# Wait for resource creation +# S3Files client does not have any built-in waiters as of yet. +# Writing custom waiters for our use case. +# ---------- + + +def wait_for_file_system_ready(fs_id: str): + fs_status, reason = get_filesystem_status(fs_id) + logger.debug(f"Waiting for Filesystem: `{fs_id}`to be ready.") + while fs_status in ["creating", "updating"]: + time.sleep(2) + fs_status, reason = get_filesystem_status(fs_id) + if fs_status != "available": + raise Exception(f"Failed to create file system.\nReason: {reason}") + logger.debug(f"Filesystem: `{fs_id}` ready.") + + # --------------------------------------------------------------------------- # # Orchestration # --------------------------------------------------------------------------- # @@ -355,6 +406,7 @@ def setup_s3_filesystem(bucket_name: str) -> str: ) file_system_id = _create_s3_files_system(bucket_name, role_arn=role_arn) + wait_for_file_system_ready(file_system_id) available_az_to_subnet_mapping = _get_available_az_to_subnet( discovery_tag=config["EKS_CLUSTER_NAME"] @@ -377,9 +429,7 @@ def setup_s3_filesystem(bucket_name: str) -> str: # target security group in the bucket's region. mount_target_vpc_id = _get_vpc_id() - mount_target_sg_id = _get_or_create_security_groups( - bucket_name=bucket_name, vpc_id=mount_target_vpc_id - ) + mount_target_sg_id = _get_or_create_security_groups(vpc_id=mount_target_vpc_id) # Create new m.t.s of this file system ID for each missing az from the az_to_subnet_dict az_with_mount_targets = {mt["availabilityZoneId"] for mt in mount_targets} From e172079e65f2c072db488eaee6bc80b7a147a13f Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 10:38:29 -0500 Subject: [PATCH 08/33] Update import path --- gen3workflow/aws/s3_files.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index b001a6a..2933a5e 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -3,7 +3,7 @@ from gen3workflow import logger from gen3workflow.config import config -from aws_utils import s3files_client, eks_client, ec2_client +from gen3workflow.aws_utils import s3files_client, eks_client, ec2_client # NFS port used for all communication between EKS pods and S3 Files mount targets. NFS_PORT = 2049 From 73ad87c065d82df8e234a1e14ade55c91f4a718d Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 11:54:32 -0500 Subject: [PATCH 09/33] Move boto3 clients to a different file to avoid circular imports --- gen3workflow/aws/clients.py | 28 ++++++++++++++++++++++++++++ gen3workflow/aws/s3_files.py | 2 +- gen3workflow/aws_utils.py | 33 +++++++-------------------------- 3 files changed, 36 insertions(+), 27 deletions(-) create mode 100644 gen3workflow/aws/clients.py diff --git a/gen3workflow/aws/clients.py b/gen3workflow/aws/clients.py new file mode 100644 index 0000000..1d14f5e --- /dev/null +++ b/gen3workflow/aws/clients.py @@ -0,0 +1,28 @@ +import boto3 +from gen3workflow.config import config + + +def get_boto3_client(service_name: str, **kwargs): + """ + Create a boto3 client for the specified AWS service, + using credentials from the config if provided, + otherwise using IRSA as a fallback in the credential provider chain. + """ + if service_name == "s3": + if config["S3_UPSTREAM_ENDPOINT"]: + kwargs["endpoint_url"] = config["S3_UPSTREAM_ENDPOINT"] + if config["S3_ENDPOINTS_AWS_ACCESS_KEY_ID"]: + kwargs["aws_access_key_id"] = config["S3_ENDPOINTS_AWS_ACCESS_KEY_ID"] + kwargs["aws_secret_access_key"] = config[ + "S3_ENDPOINTS_AWS_SECRET_ACCESS_KEY" + ] + return boto3.client(service_name, **kwargs) + + +iam_client = get_boto3_client("iam") +s3_client = get_boto3_client("s3", region_name=config["USER_BUCKETS_REGION"]) +kms_client = get_boto3_client("kms", region_name=config["USER_BUCKETS_REGION"]) +sts_client = get_boto3_client("sts") +eks_client = get_boto3_client("eks", region_name=config["EKS_CLUSTER_REGION"]) +s3files_client = get_boto3_client("s3files", region_name=config["USER_BUCKETS_REGION"]) +ec2_client = get_boto3_client("ec2", region_name=config["USER_BUCKETS_REGION"]) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 2933a5e..31dd23f 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -3,7 +3,7 @@ from gen3workflow import logger from gen3workflow.config import config -from gen3workflow.aws_utils import s3files_client, eks_client, ec2_client +from gen3workflow.aws.clients import s3files_client, eks_client, ec2_client # NFS port used for all communication between EKS pods and S3 Files mount targets. NFS_PORT = 2049 diff --git a/gen3workflow/aws_utils.py b/gen3workflow/aws_utils.py index 64a669a..cf14416 100644 --- a/gen3workflow/aws_utils.py +++ b/gen3workflow/aws_utils.py @@ -12,6 +12,13 @@ from gen3workflow import logger from gen3workflow.aws.s3_files import get_s3_files_system, setup_s3_filesystem from gen3workflow.config import config +from gen3workflow.aws.clients import ( + eks_client, + sts_client, + iam_client, + kms_client, + s3_client, +) USER_BUCKET_CACHE = SimpleCache(default_timeout=config["USER_BUCKET_CACHE_SECONDS"]) @@ -24,32 +31,6 @@ def dict_to_sorted_json_str(obj: dict) -> str: return json.dumps(obj, sort_keys=True, separators=(",", ":")) -def get_boto3_client(service_name: str, **kwargs): - """ - Create a boto3 client for the specified AWS service, - using credentials from the config if provided, - otherwise using IRSA as a fallback in the credential provider chain. - """ - if service_name == "s3": - if config["S3_UPSTREAM_ENDPOINT"]: - kwargs["endpoint_url"] = config["S3_UPSTREAM_ENDPOINT"] - if config["S3_ENDPOINTS_AWS_ACCESS_KEY_ID"]: - kwargs["aws_access_key_id"] = config["S3_ENDPOINTS_AWS_ACCESS_KEY_ID"] - kwargs["aws_secret_access_key"] = config[ - "S3_ENDPOINTS_AWS_SECRET_ACCESS_KEY" - ] - return boto3.client(service_name, **kwargs) - - -iam_client = get_boto3_client("iam") -s3_client = get_boto3_client("s3", region_name=config["USER_BUCKETS_REGION"]) -kms_client = get_boto3_client("kms", region_name=config["USER_BUCKETS_REGION"]) -sts_client = get_boto3_client("sts") -eks_client = get_boto3_client("eks", region_name=config["EKS_CLUSTER_REGION"]) -s3files_client = get_boto3_client("s3files", region_name=config["USER_BUCKETS_REGION"]) -ec2_client = get_boto3_client("ec2", region_name=config["USER_BUCKETS_REGION"]) - - def get_safe_name_from_hostname( user_id: Union[str, None], reserved_length: int = 0 ) -> str: From 1ac0aa41df503f4748c35025e3fd28b4a41b4f54 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 14:04:53 -0500 Subject: [PATCH 10/33] Update exception type --- gen3workflow/aws/s3_files.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 31dd23f..92fd2ea 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -65,7 +65,7 @@ def get_filesystem_status(file_system_id: str) -> tuple[str | None, str | None]: try: fs = s3files_client.get_file_system(fileSystemId=file_system_id) - except s3files_client.exceptions.FileSystemNotFound: + except s3files_client.exceptions.ResourceNotFoundException: return None, f"File system with file_system_id={file_system_id} does not exist" except ClientError as e: return None, f"Failed to fetch file system {file_system_id}: {e}" From f36379121787ae0a5e1197e11293c927c0c33532 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 15:00:28 -0500 Subject: [PATCH 11/33] Fix get_s3_files_system and add/update TODO comments --- gen3workflow/aws/s3_files.py | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 92fd2ea..e5943fd 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -31,11 +31,11 @@ def get_s3_files_system(bucket_name: str) -> str | None: try: paginator = s3files_client.get_paginator("list_file_systems") for page in paginator.paginate(): - for fs in page.get("FileSystems", []): - if fs.get("Bucket") == bucket_arn: - fs_id = fs["FileSystemId"] + for fs in page.get("fileSystems", []): + if fs.get("bucket") == bucket_arn: + fs_id = fs["fileSystemId"] logger.debug( - f"Found existing S3 Files file system '{fs_id}' (state: {fs['LifeCycleState']})" + f"Found existing S3 Files file system '{fs_id}' (state: {fs['status']})" ) return fs_id except ClientError as e: @@ -77,12 +77,23 @@ def get_filesystem_status(file_system_id: str) -> tuple[str | None, str | None]: def get_mount_target_status(file_system_id: str): + """ # TODO: list mount targets and get their statuses, and unify into a single # usable status (e.g. "ready" only once all expected mount targets # are in an available state). - # TODO: What if new AZs are added to the node after initial bucket setup? Filesystem may need new mount targets in these AZs too. - # This should not be the responsibility of this function, but where to put it? + # Note: We assume new AZs are not added to the node after initial bucket setup? + # Filesystem may need new mount targets in these AZs if they are ever created. + """ + + return "Not ready" + + +def get_s3files_setup_status(filesystem_id): + """ + #TODO: This orchestrates both filesystem status and mount target statuses + and informs whether or not this storage setup is ready to use or not. + """ return "Not ready" @@ -375,11 +386,21 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: def wait_for_file_system_ready(fs_id: str): + """ + Waits for file system to be ready. + + Raises: Exception if file system could not be created. + """ fs_status, reason = get_filesystem_status(fs_id) logger.debug(f"Waiting for Filesystem: `{fs_id}`to be ready.") while fs_status in ["creating", "updating"]: + # TODO: Make it async time.sleep(2) fs_status, reason = get_filesystem_status(fs_id) + if reason: + logger.debug( + f"Filesystem `{fs_id}` is currently in {fs_status=} with {reason=}" + ) if fs_status != "available": raise Exception(f"Failed to create file system.\nReason: {reason}") logger.debug(f"Filesystem: `{fs_id}` ready.") From 15182e521ef5d25897d622c56924dc9e88a12a00 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 21:27:03 -0500 Subject: [PATCH 12/33] Create dynamic IAM role for S3Files --- gen3workflow/aws/s3_files.py | 143 ++++++++++++++++++++++++++++++++++- 1 file changed, 140 insertions(+), 3 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index e5943fd..e74eb29 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -1,9 +1,16 @@ from botocore.exceptions import ClientError import time +import json from gen3workflow import logger from gen3workflow.config import config -from gen3workflow.aws.clients import s3files_client, eks_client, ec2_client +from gen3workflow.aws.clients import ( + s3files_client, + eks_client, + ec2_client, + iam_client, + sts_client, +) # NFS port used for all communication between EKS pods and S3 Files mount targets. NFS_PORT = 2049 @@ -374,8 +381,138 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: Returns: ARN of the (new or existing) IAM role. """ - # TODO: implementation - return "arn:aws:iam::707767160287:role/Gen3WorkflowS3FilesPOC" + # TODO: Make it such that it is under 64 characters + role_name = f"{bucket_name}-s3files-role" + account_id = sts_client.get_caller_identity()["Account"] + bucket_arn = f"arn:aws:s3:::{bucket_name}" + + try: + role = iam_client.get_role(RoleName=role_name) + role_arn = role["Role"]["Arn"] + logger.info("IAM role '%s' already exists (%s)", role_name, role_arn) + return role_arn + except iam_client.exceptions.NoSuchEntityException: + pass + + trust_policy = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "AllowS3FilesAssumeRole", + "Effect": "Allow", + "Principal": {"Service": "elasticfilesystem.amazonaws.com"}, + "Action": "sts:AssumeRole", + "Condition": { + "StringEquals": {"aws:SourceAccount": account_id}, + "ArnLike": { + "aws:SourceArn": f"arn:aws:s3files:{region}:{account_id}:file-system/*" + }, + }, + } + ], + } + + # NOTE: includes the KMS statement unconditionally. If the kmsEncryptionEnabled is false, + # this statement is simply unused -- harmless + inline_policy = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "S3BucketPermissions", + "Effect": "Allow", + "Action": ["s3:ListBucket", "s3:ListBucketVersions"], + "Resource": bucket_arn, + "Condition": {"StringEquals": {"aws:ResourceAccount": account_id}}, + }, + { + "Sid": "S3ObjectPermissions", + "Effect": "Allow", + "Action": [ + "s3:AbortMultipartUpload", + "s3:DeleteObject*", + "s3:GetObject*", + "s3:List*", + "s3:PutObject*", + ], + "Resource": f"{bucket_arn}/*", + "Condition": {"StringEquals": {"aws:ResourceAccount": account_id}}, + }, + { + "Sid": "UseKmsKeyWithS3Files", + "Effect": "Allow", + "Action": [ + "kms:GenerateDataKey", + "kms:Encrypt", + "kms:Decrypt", + "kms:ReEncryptFrom", + "kms:ReEncryptTo", + ], + "Condition": { + "StringLike": { + "kms:ViaService": f"s3.{region}.amazonaws.com", + "kms:EncryptionContext:aws:s3:arn": [ + bucket_arn, + f"{bucket_arn}/*", + ], + } + }, + "Resource": f"arn:aws:kms:{region}:{account_id}:*", + }, + { + "Sid": "EventBridgeManage", + "Effect": "Allow", + "Action": [ + "events:DeleteRule", + "events:DisableRule", + "events:EnableRule", + "events:PutRule", + "events:PutTargets", + "events:RemoveTargets", + ], + "Condition": { + "StringEquals": { + "events:ManagedBy": "elasticfilesystem.amazonaws.com" + } + }, + "Resource": ["arn:aws:events:*:*:rule/DO-NOT-DELETE-S3-Files*"], + }, + { + "Sid": "EventBridgeRead", + "Effect": "Allow", + "Action": [ + "events:DescribeRule", + "events:ListRuleNamesByTarget", + "events:ListRules", + "events:ListTargetsByRule", + ], + "Resource": ["arn:aws:events:*:*:rule/*"], + }, + ], + } + + try: + response = iam_client.create_role( + RoleName=role_name, + AssumeRolePolicyDocument=json.dumps(trust_policy), + Description=f"Role assumed by S3 Files to sync with bucket '{bucket_name}'", + Tags=[{"Key": "app-name", "Value": "gen3-workflow"}], + ) + role_arn = response["Role"]["Arn"] + + iam_client.put_role_policy( + RoleName=role_name, + PolicyName="S3FilesBucketAccess", + PolicyDocument=json.dumps(inline_policy), + ) + logger.info( + "Created IAM role '%s' (%s) for S3 Files bucket access", role_name, role_arn + ) + return role_arn + except ClientError as e: + logger.error( + f"Failed to create IAM role for bucket '{bucket_name}': {e.response['Error']['Message']}" + ) + raise # --------- From fbc754587a2c90e8d79a95d7b97f588c21524a60 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 22:18:57 -0500 Subject: [PATCH 13/33] Add STORAGE_TYPE config variable, add documentation for s3files --- docs/s3Files.md | 21 +++++++++++++++++++ gen3workflow/aws_utils.py | 10 +++------ gen3workflow/config-default.yaml | 3 +++ gen3workflow/config.py | 1 + gen3workflow/routes/storage.py | 36 +++++++++++++++++++++----------- 5 files changed, 52 insertions(+), 19 deletions(-) create mode 100644 docs/s3Files.md diff --git a/docs/s3Files.md b/docs/s3Files.md new file mode 100644 index 0000000..4998d90 --- /dev/null +++ b/docs/s3Files.md @@ -0,0 +1,21 @@ +# S3 Files Setup + +## Overview +TES worker and executor pods use **S3 Files** (Amazon's EFS-backed service) to interact with a user's S3 bucket. While pod lifecycle is managed by the underlying execution engine (Funnel), Gen3Workflow is responsible for provisioning the supporting AWS resources — file systems, mount targets, IAM roles, and security groups — required for S3 Files to function. + +## Enabling S3 Files +Set the following in your config: + +```yaml +STORAGE_TYPE: S3Files +``` + +For Helm-based deployments, set: + +```yaml +{{ .Values.gen3WorkflowConfig.storageType }}: "S3Files" +``` + +## Prerequisites +* Gen3Workflow must be deployed on an EKS cluster. +* The EKS cluster must support S3 Files, provisioned via the [EFS CSI driver](https://github.com/kubernetes-sigs/aws-efs-csi-driver). diff --git a/gen3workflow/aws_utils.py b/gen3workflow/aws_utils.py index cf14416..6deb0f6 100644 --- a/gen3workflow/aws_utils.py +++ b/gen3workflow/aws_utils.py @@ -454,6 +454,8 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: ChecksumAlgorithm="SHA256", ) + enable_bucket_versioning(user_bucket_name) + kms_key_arn = None if config["KMS_ENCRYPTION_ENABLED"]: kms_key_arn = setup_kms_encryption_on_bucket(user_bucket_name) @@ -462,13 +464,7 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: s3_client.delete_bucket_encryption(Bucket=user_bucket_name) s3_client.delete_bucket_policy(Bucket=user_bucket_name) - enable_bucket_versioning(user_bucket_name) - fs_id = get_s3_files_system(user_bucket_name) - if not fs_id: - # Create S3 Files Filesystem ID if not exists - fs_id = setup_s3_filesystem(user_bucket_name) - - return user_bucket_name, kms_key_arn, fs_id + return user_bucket_name, kms_key_arn async def create_user_bucket(user_id: str) -> Tuple[str, str, str]: diff --git a/gen3workflow/config-default.yaml b/gen3workflow/config-default.yaml index 99b88b4..e3bf970 100644 --- a/gen3workflow/config-default.yaml +++ b/gen3workflow/config-default.yaml @@ -46,6 +46,9 @@ S3_ENDPOINTS_AWS_SECRET_ACCESS_KEY: KMS_ENCRYPTION_ENABLED: true +# Acceptable values are EBS and S3Files +STORAGE_TYPE: EBS + ############# # GA4GH TES # ############# diff --git a/gen3workflow/config.py b/gen3workflow/config.py index 364fb81..6f7c2e6 100644 --- a/gen3workflow/config.py +++ b/gen3workflow/config.py @@ -47,6 +47,7 @@ def validate_top_level_configs(self) -> None: "S3_ENDPOINTS_AWS_ACCESS_KEY_ID": {"type": ["string", "null"]}, "S3_ENDPOINTS_AWS_SECRET_ACCESS_KEY": {"type": ["string", "null"]}, "KMS_ENCRYPTION_ENABLED": {"type": "boolean"}, + "STORAGE_TYPE": {"type": ["string", "null"]}, "TASK_IMAGE_WHITELIST": {"type": "array", "items": {"type": "string"}}, "TES_SERVER_URL": {"type": "string"}, "ENABLE_PROMETHEUS_METRICS": {"type": "boolean"}, diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index 071c2d4..0c44f4c 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -10,7 +10,7 @@ from gen3workflow import aws_utils, logger from gen3workflow.auth import Auth -from gen3workflow.aws.s3_files import get_filesystem_status +from gen3workflow.aws import s3_files from gen3workflow.config import config router = APIRouter(prefix="/storage") @@ -40,26 +40,38 @@ async def storage_setup(request: Request, auth=Depends(Auth)) -> dict: bucket_name, kms_key_arn, fs_id = await aws_utils.create_user_bucket(user_id) bucket_prefix = "ga4gh-tes" bucket_region = config["USER_BUCKETS_REGION"] - filesystem_status = get_filesystem_status(fs_id) - try: - await auth.grant_user_access_to_their_own_data( - username=username, user_id=user_id - ) - except ArboristError as e: - logger.error(e.message) - raise HTTPException(e.code, e.message) - return { + 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 ), - "s3files_filesystem_id": fs_id, - "status": filesystem_status, } + if config["STORAGE_TYPE"] == "S3Files": + + fs_id = s3_files.get_s3_files_system(bucket_name) + if not fs_id: + # Create S3 Files Filesystem ID if not exists + fs_id = s3_files.setup_s3_filesystem(bucket_name) + + filesystem_status = s3_files.get_s3files_setup_status(fs_id) + + storage_info["s3files_filesystem_id"] = fs_id + storage_info["status"] = filesystem_status + + try: + await auth.grant_user_access_to_their_own_data( + username=username, user_id=user_id + ) + except ArboristError as e: + logger.error(e.message) + raise HTTPException(e.code, e.message) + + return storage_info + @router.delete("/user-bucket", status_code=HTTP_202_ACCEPTED) @router.delete("/user-bucket/", status_code=HTTP_202_ACCEPTED, include_in_schema=False) From 80cd42538cd0f60a394269e3b09fe1c5f5becd1e Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 22 Jul 2026 22:43:50 -0500 Subject: [PATCH 14/33] Make S3Files a configurable setup --- docs/s3Files.md | 4 ++-- gen3workflow/aws_utils.py | 2 -- gen3workflow/config-default.yaml | 4 ++-- gen3workflow/config.py | 2 +- gen3workflow/routes/storage.py | 7 +++++-- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/s3Files.md b/docs/s3Files.md index 4998d90..3370d7b 100644 --- a/docs/s3Files.md +++ b/docs/s3Files.md @@ -7,13 +7,13 @@ TES worker and executor pods use **S3 Files** (Amazon's EFS-backed service) to i Set the following in your config: ```yaml -STORAGE_TYPE: S3Files +ENABLE_S3_FILES: true ``` For Helm-based deployments, set: ```yaml -{{ .Values.gen3WorkflowConfig.storageType }}: "S3Files" +{{ .Values.gen3WorkflowConfig.enableS3Files }}: true ``` ## Prerequisites diff --git a/gen3workflow/aws_utils.py b/gen3workflow/aws_utils.py index 6deb0f6..e9c2c28 100644 --- a/gen3workflow/aws_utils.py +++ b/gen3workflow/aws_utils.py @@ -454,8 +454,6 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: ChecksumAlgorithm="SHA256", ) - enable_bucket_versioning(user_bucket_name) - kms_key_arn = None if config["KMS_ENCRYPTION_ENABLED"]: kms_key_arn = setup_kms_encryption_on_bucket(user_bucket_name) diff --git a/gen3workflow/config-default.yaml b/gen3workflow/config-default.yaml index e3bf970..3a5f91e 100644 --- a/gen3workflow/config-default.yaml +++ b/gen3workflow/config-default.yaml @@ -46,8 +46,8 @@ S3_ENDPOINTS_AWS_SECRET_ACCESS_KEY: KMS_ENCRYPTION_ENABLED: true -# Acceptable values are EBS and S3Files -STORAGE_TYPE: EBS +# Set it to true to create S3Files resources (default - false) +ENABLE_S3_FILES: false ############# # GA4GH TES # diff --git a/gen3workflow/config.py b/gen3workflow/config.py index 6f7c2e6..294e451 100644 --- a/gen3workflow/config.py +++ b/gen3workflow/config.py @@ -47,7 +47,7 @@ def validate_top_level_configs(self) -> None: "S3_ENDPOINTS_AWS_ACCESS_KEY_ID": {"type": ["string", "null"]}, "S3_ENDPOINTS_AWS_SECRET_ACCESS_KEY": {"type": ["string", "null"]}, "KMS_ENCRYPTION_ENABLED": {"type": "boolean"}, - "STORAGE_TYPE": {"type": ["string", "null"]}, + "ENABLE_S3_FILES": {"type": "boolean"}, "TASK_IMAGE_WHITELIST": {"type": "array", "items": {"type": "string"}}, "TES_SERVER_URL": {"type": "string"}, "ENABLE_PROMETHEUS_METRICS": {"type": "boolean"}, diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index 0c44f4c..50793ef 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -37,7 +37,7 @@ async def storage_setup(request: Request, auth=Depends(Auth)) -> dict: # 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, fs_id = await aws_utils.create_user_bucket(user_id) + bucket_name, kms_key_arn = await aws_utils.create_user_bucket(user_id) bucket_prefix = "ga4gh-tes" bucket_region = config["USER_BUCKETS_REGION"] @@ -50,7 +50,10 @@ async def storage_setup(request: Request, auth=Depends(Auth)) -> dict: ), } - if config["STORAGE_TYPE"] == "S3Files": + if config["ENABLE_S3_FILES"]: + + # Bucket versioning is necessary for S3Files + aws_utils.enable_bucket_versioning(bucket_name) fs_id = s3_files.get_s3_files_system(bucket_name) if not fs_id: From 2cf8c10c949fb95111a3b9714d173845205bc197 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Thu, 23 Jul 2026 09:40:25 -0500 Subject: [PATCH 15/33] Make mount target provisioning a bgTask + Update docs --- docs/s3Files.md | 4 ++++ gen3workflow/aws/s3_files.py | 22 ++++++++++++++-------- gen3workflow/config-default.yaml | 5 ++--- gen3workflow/routes/storage.py | 7 +++++-- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/docs/s3Files.md b/docs/s3Files.md index 3370d7b..6a71f9a 100644 --- a/docs/s3Files.md +++ b/docs/s3Files.md @@ -19,3 +19,7 @@ For Helm-based deployments, set: ## Prerequisites * Gen3Workflow must be deployed on an EKS cluster. * The EKS cluster must support S3 Files, provisioned via the [EFS CSI driver](https://github.com/kubernetes-sigs/aws-efs-csi-driver). + + +#### NOTE: +* Currently Gen3Workflow with S3Files only supports user bucket and the EKS cluster being present in the same region. Support shall be added in the future. diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index e74eb29..fd65699 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -77,9 +77,6 @@ def get_filesystem_status(file_system_id: str) -> tuple[str | None, str | None]: except ClientError as e: return None, f"Failed to fetch file system {file_system_id}: {e}" - if not fs: - return None, f"File system with file_system_id={file_system_id} does not exist" - return fs.get("status"), fs.get("statusMessage") @@ -101,6 +98,7 @@ def get_s3files_setup_status(filesystem_id): #TODO: This orchestrates both filesystem status and mount target statuses and informs whether or not this storage setup is ready to use or not. """ + fs_status = get_filesystem_status(file_system_id=filesystem_id) return "Not ready" @@ -550,9 +548,7 @@ def wait_for_file_system_ready(fs_id: str): def setup_s3_filesystem(bucket_name: str) -> str: """ - Ensure an S3 Files file system exists for `bucket_name`, with one mount target - per AZ the EKS cluster's nodes can run in, and the security groups needed for - pods to reach those mount targets over NFS. + Ensure an S3 Files file system exists for `bucket_name` Returns: The file system ID. @@ -563,12 +559,22 @@ def setup_s3_filesystem(bucket_name: str) -> str: bucket_name=bucket_name, region=region ) - file_system_id = _create_s3_files_system(bucket_name, role_arn=role_arn) - wait_for_file_system_ready(file_system_id) + return _create_s3_files_system(bucket_name, role_arn=role_arn) + + +def provision_mount_targets(file_system_id: str): + """ + Provisions one mount target per AZ where the EKS cluster's nodes can run in, + and the security groups needed for pods to reach those mount targets over NFS. + """ + # NOTE: To avoid blocking `/storage/setup` call, setting s3 filesystem just returns + # the filesystem id and continue with the rest of the steps asynchronously? available_az_to_subnet_mapping = _get_available_az_to_subnet( discovery_tag=config["EKS_CLUSTER_NAME"] ) + + wait_for_file_system_ready(file_system_id) mount_targets = list_mount_targets_for_file_system(file_system_id) # ASSUMPTION: this implementation currently requires the S3 bucket and the EKS diff --git a/gen3workflow/config-default.yaml b/gen3workflow/config-default.yaml index 49e7d38..fe19d29 100644 --- a/gen3workflow/config-default.yaml +++ b/gen3workflow/config-default.yaml @@ -37,9 +37,6 @@ VALID_AUTHZ_AUDIENCE: # AWS S3 # ########## -# NOTE: Currently Gen3Workflow only supports bucket and the EKS cluster being present in the same region. -# This is due to a limitation in the implementation of underlying S3Files feature. -# Support shall be added in the future. USER_BUCKETS_REGION: us-east-1 # connect to another S3-compatible service than AWS S3 (default: AWS S3) @@ -56,6 +53,8 @@ S3_ENDPOINTS_AWS_SECRET_ACCESS_KEY: KMS_ENCRYPTION_ENABLED: true # Set it to true to create S3Files resources (default - false) +# NOTE: Currently Gen3Workflow with S3Files only supports bucket and the EKS cluster being present in the same region. +# Support shall be added in the future. ENABLE_S3_FILES: false ############# diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index 50793ef..03c4a62 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -1,5 +1,5 @@ from gen3authz.client.arborist.errors import ArboristError -from fastapi import APIRouter, Depends, Request, HTTPException +from fastapi import APIRouter, BackgroundTasks, Depends, Request, HTTPException from starlette.status import ( HTTP_200_OK, HTTP_202_ACCEPTED, @@ -18,7 +18,9 @@ @router.get("/setup", status_code=HTTP_200_OK) @router.get("/setup/", status_code=HTTP_200_OK, include_in_schema=False) -async def storage_setup(request: Request, auth=Depends(Auth)) -> dict: +async def storage_setup( + request: Request, background_tasks: BackgroundTasks, auth=Depends(Auth) +) -> dict: """ Return details about the current user's storage setup. This endpoint also serves as a mandatory "first time setup" for the user's bucket @@ -59,6 +61,7 @@ async def storage_setup(request: Request, auth=Depends(Auth)) -> dict: if not fs_id: # Create S3 Files Filesystem ID if not exists fs_id = s3_files.setup_s3_filesystem(bucket_name) + background_tasks.add_task(s3_files.provision_mount_targets, fs_id) filesystem_status = s3_files.get_s3files_setup_status(fs_id) From fc32d1521cd8ae33e634c9ed929bb1f9b5ff751c Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Thu, 23 Jul 2026 12:05:03 -0500 Subject: [PATCH 16/33] Update comments --- gen3workflow/aws/s3_files.py | 4 +--- gen3workflow/routes/storage.py | 2 ++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index fd65699..bf90a95 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -89,6 +89,7 @@ def get_mount_target_status(file_system_id: str): # Note: We assume new AZs are not added to the node after initial bucket setup? # Filesystem may need new mount targets in these AZs if they are ever created. """ + mount_targets = list_mount_targets_for_file_system(file_system_id) return "Not ready" @@ -567,9 +568,6 @@ def provision_mount_targets(file_system_id: str): Provisions one mount target per AZ where the EKS cluster's nodes can run in, and the security groups needed for pods to reach those mount targets over NFS. """ - # NOTE: To avoid blocking `/storage/setup` call, setting s3 filesystem just returns - # the filesystem id and continue with the rest of the steps asynchronously? - available_az_to_subnet_mapping = _get_available_az_to_subnet( discovery_tag=config["EKS_CLUSTER_NAME"] ) diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index 03c4a62..6e4e812 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -61,6 +61,8 @@ async def storage_setup( if not fs_id: # 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? background_tasks.add_task(s3_files.provision_mount_targets, fs_id) filesystem_status = s3_files.get_s3files_setup_status(fs_id) From bb84de30c85f0e7e1107503f7c8932f14c983ab5 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Thu, 23 Jul 2026 12:07:07 -0500 Subject: [PATCH 17/33] Add missing function call. --- gen3workflow/aws/s3_files.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index bf90a95..c38d56b 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -100,7 +100,7 @@ def get_s3files_setup_status(filesystem_id): and informs whether or not this storage setup is ready to use or not. """ fs_status = get_filesystem_status(file_system_id=filesystem_id) - + mt_status = get_mount_target_status(filesystem_id) return "Not ready" From df8917445dcca16c46d03b36a988dc84dd0b7c95 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Fri, 24 Jul 2026 09:56:44 -0500 Subject: [PATCH 18/33] Add more comments --- gen3workflow/aws/s3_files.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index c38d56b..eba617a 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -101,6 +101,10 @@ def get_s3files_setup_status(filesystem_id): """ fs_status = get_filesystem_status(file_system_id=filesystem_id) mt_status = get_mount_target_status(filesystem_id) + + # For a very brief moment when filesystem is created but mount targets are not created, + # mt_status would result in an empty list, this status endpoint must still return + # NOT ready (or maybe 'provisioning') in that case. return "Not ready" @@ -244,7 +248,7 @@ def _get_eks_security_groups() -> tuple[str, str]: return [(group["GroupId"], group["GroupName"]) for group in security_groups] -# TODO: Investigate if this can be moved to server startup logic or elsewhere? +# TODO: Investigate if this can be moved to server startup logic or elsewhere? Terraform maybe? # Since the `create` part is only needed once per cluster, no need to run for every bucket. # This takes approximately 2 seconds to run everytime it is invoked. def _get_or_create_security_groups(vpc_id: str) -> str: From 00f569f6ee94bccd189883ed703994e763c58af4 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Fri, 24 Jul 2026 17:10:24 -0500 Subject: [PATCH 19/33] Re-organize aws_utils into aws.bucket, aws.aws_utils, aws.clients. (Tests need fixing) --- gen3workflow/aws/aws_utils.py | 67 ++++++++ gen3workflow/{aws_utils.py => aws/bucket.py} | 153 ++++++------------- gen3workflow/routes/ga4gh_tes.py | 6 +- gen3workflow/routes/s3.py | 6 +- gen3workflow/routes/storage.py | 17 ++- tests/conftest.py | 2 +- tests/test_aws_utils.py | 62 ++++---- tests/test_ga4gh_tes.py | 8 +- tests/test_misc.py | 64 ++++---- tests/test_s3_endpoint.py | 2 +- 10 files changed, 204 insertions(+), 183 deletions(-) create mode 100644 gen3workflow/aws/aws_utils.py rename gen3workflow/{aws_utils.py => aws/bucket.py} (82%) diff --git a/gen3workflow/aws/aws_utils.py b/gen3workflow/aws/aws_utils.py new file mode 100644 index 0000000..4993460 --- /dev/null +++ b/gen3workflow/aws/aws_utils.py @@ -0,0 +1,67 @@ +import json +from typing import Union +from gen3workflow.config import config + + +def dict_to_sorted_json_str(obj: dict) -> str: + """ + Reads a Python dict and returns a JSON string with ordered keys + Use case: when comparing JSON objects returned by AWS, comparisons are deterministic and less flaky + """ + return json.dumps(obj, sort_keys=True, separators=(",", ":")) + + +def get_safe_name_from_hostname( + user_id: Union[str, None], reserved_length: int = 0 +) -> str: + """ + Generate a valid and length-safe name (for IAM user, S3 bucket, or IAM role) + derived from the configured hostname and optional user ID. + Rules: + - IAM user names: up to 64 characters. + - S3 bucket / IAM role names: up to 63 characters. + - Only alphanumeric characters and the following are allowed: +=,.@_- + (assumes HOSTNAME and user IDs are already compliant). + Args: + user_id (str | None): The user's unique Gen3 ID. If None, will not be included in the safe name. + reserved_length (int): Number of characters to reserve for prefixes/suffixes. + + Returns: + str: safe name + """ + escaped_hostname = config["HOSTNAME"].replace(".", "-") + safe_name = f"gen3wf-{escaped_hostname}" + max_chars = 63 - reserved_length + if user_id: + max_chars = max_chars - len(f"-{user_id}") + if len(safe_name) > max_chars: + safe_name = safe_name[:max_chars] + if user_id: + safe_name = f"{safe_name}-{user_id}" + return safe_name + + +def get_worker_sa_name(user_id: str) -> str: + """ + Generate the name of the Kubernetes service account used by worker pods for the specified user. + + Args: + user_id (str): The user's unique Gen3 ID + Returns: + str: service account name + """ + safe_name = get_safe_name_from_hostname(user_id, reserved_length=len("-worker-sa")) + return f"{safe_name}-worker-sa" + + +def get_bucket_name_from_user_id(user_id: str) -> str: + """ + Generate the S3 bucket name for the specified user. + + Args: + user_id (str): The user's unique Gen3 ID + Returns: + str: S3 bucket name + """ + # Abstracted for future flexibility — currently same as safe name. + return get_safe_name_from_hostname(user_id) diff --git a/gen3workflow/aws_utils.py b/gen3workflow/aws/bucket.py similarity index 82% rename from gen3workflow/aws_utils.py rename to gen3workflow/aws/bucket.py index e9c2c28..d8db38d 100644 --- a/gen3workflow/aws_utils.py +++ b/gen3workflow/aws/bucket.py @@ -1,90 +1,33 @@ import json import random +from cachelib import SimpleCache from typing import Tuple, Union import asyncio -from cachelib import SimpleCache -import boto3 from botocore.exceptions import ClientError from fastapi import HTTPException from starlette.status import HTTP_400_BAD_REQUEST from gen3workflow import logger -from gen3workflow.aws.s3_files import get_s3_files_system, setup_s3_filesystem -from gen3workflow.config import config -from gen3workflow.aws.clients import ( - eks_client, - sts_client, - iam_client, - kms_client, - s3_client, +from gen3workflow.aws.aws_utils import ( + dict_to_sorted_json_str, + get_bucket_name_from_user_id, + get_safe_name_from_hostname, + get_worker_sa_name, ) +from gen3workflow.config import config -USER_BUCKET_CACHE = SimpleCache(default_timeout=config["USER_BUCKET_CACHE_SECONDS"]) - - -def dict_to_sorted_json_str(obj: dict) -> str: - """ - Reads a Python dict and returns a JSON string with ordered keys - Use case: when comparing JSON objects returned by AWS, comparisons are deterministic and less flaky - """ - return json.dumps(obj, sort_keys=True, separators=(",", ":")) - - -def get_safe_name_from_hostname( - user_id: Union[str, None], reserved_length: int = 0 -) -> str: - """ - Generate a valid and length-safe name (for IAM user, S3 bucket, or IAM role) - derived from the configured hostname and optional user ID. - Rules: - - IAM user names: up to 64 characters. - - S3 bucket / IAM role names: up to 63 characters. - - Only alphanumeric characters and the following are allowed: +=,.@_- - (assumes HOSTNAME and user IDs are already compliant). - Args: - user_id (str | None): The user's unique Gen3 ID. If None, will not be included in the safe name. - reserved_length (int): Number of characters to reserve for prefixes/suffixes. - - Returns: - str: safe name - """ - escaped_hostname = config["HOSTNAME"].replace(".", "-") - safe_name = f"gen3wf-{escaped_hostname}" - max_chars = 63 - reserved_length - if user_id: - max_chars = max_chars - len(f"-{user_id}") - if len(safe_name) > max_chars: - safe_name = safe_name[:max_chars] - if user_id: - safe_name = f"{safe_name}-{user_id}" - return safe_name - - -def get_worker_sa_name(user_id: str) -> str: - """ - Generate the name of the Kubernetes service account used by worker pods for the specified user. - - Args: - user_id (str): The user's unique Gen3 ID - Returns: - str: service account name - """ - safe_name = get_safe_name_from_hostname(user_id, reserved_length=len("-worker-sa")) - return f"{safe_name}-worker-sa" - +from gen3workflow.aws import clients -def get_bucket_name_from_user_id(user_id: str) -> str: - """ - Generate the S3 bucket name for the specified user. +# ( +# eks_client, +# sts_client, +# clients.iam_client, +# clients.kms_client, +# clients.s3_client, +# ) - Args: - user_id (str): The user's unique Gen3 ID - Returns: - str: S3 bucket name - """ - # Abstracted for future flexibility — currently same as safe name. - return get_safe_name_from_hostname(user_id) +USER_BUCKET_CACHE = SimpleCache(default_timeout=config["USER_BUCKET_CACHE_SECONDS"]) def get_existing_kms_key_for_bucket(bucket_name: str) -> Tuple[str, str]: @@ -101,7 +44,7 @@ def get_existing_kms_key_for_bucket(bucket_name: str) -> Tuple[str, str]: """ kms_key_alias = f"alias/{bucket_name}" try: - output = kms_client.describe_key(KeyId=kms_key_alias) + output = clients.kms_client.describe_key(KeyId=kms_key_alias) return kms_key_alias, output["KeyMetadata"]["Arn"] except ClientError as e: if e.response["Error"]["Code"] == "NotFoundException": @@ -128,10 +71,10 @@ def create_iam_role_for_funnel_bucket_access(user_id: str) -> str: ) role_name = f"{safe_name}{role_name_suffix}" bucket_name = get_bucket_name_from_user_id(user_id) - aws_account_id = sts_client.get_caller_identity().get("Account") - oidc_token_url = eks_client.describe_cluster(name=config["EKS_CLUSTER_NAME"])[ - "cluster" - ]["identity"]["oidc"]["issuer"].replace("https://", "") + aws_account_id = clients.sts_client.get_caller_identity().get("Account") + oidc_token_url = clients.eks_client.describe_cluster( + name=config["EKS_CLUSTER_NAME"] + )["cluster"]["identity"]["oidc"]["issuer"].replace("https://", "") assume_role_policy_document = { "Version": "2012-10-17", @@ -158,7 +101,7 @@ def create_iam_role_for_funnel_bucket_access(user_id: str) -> str: } try: - worker_role = iam_client.get_role(RoleName=role_name) + worker_role = clients.iam_client.get_role(RoleName=role_name) logger.info(f"IAM role '{role_name}' already exists") current_policy = dict_to_sorted_json_str( worker_role["Role"]["AssumeRolePolicyDocument"] @@ -167,7 +110,7 @@ def create_iam_role_for_funnel_bucket_access(user_id: str) -> str: if current_policy != updated_policy: logger.debug(f"Updating Assume role Policy changed for '{role_name}'.") - iam_client.update_assume_role_policy( + clients.iam_client.update_assume_role_policy( RoleName=role_name, PolicyDocument=json.dumps(assume_role_policy_document), ) @@ -175,7 +118,7 @@ def create_iam_role_for_funnel_bucket_access(user_id: str) -> str: if e.response["Error"]["Code"] != "NoSuchEntity": raise logger.info(f"Creating IAM role '{role_name}'") - worker_role = iam_client.create_role( + worker_role = clients.iam_client.create_role( RoleName=role_name, AssumeRolePolicyDocument=json.dumps(assume_role_policy_document), Tags=[ @@ -232,7 +175,7 @@ def create_iam_role_for_funnel_bucket_access(user_id: str) -> str: } ) - iam_client.put_role_policy( + clients.iam_client.put_role_policy( RoleName=role_name, PolicyName=policy_name, PolicyDocument=json.dumps(policy_document), @@ -257,7 +200,7 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None: logger.debug(f"Existing KMS key '{kms_key_alias}' - '{kms_key_arn}'") else: # the KMS key doesn't exist: create it - output = kms_client.create_key( + output = clients.kms_client.create_key( Tags=[ { "TagKey": "Name", @@ -268,12 +211,14 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None: kms_key_arn = output["KeyMetadata"]["Arn"] logger.debug(f"Created KMS key '{kms_key_arn}'") - kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) + clients.kms_client.create_alias( + AliasName=kms_key_alias, TargetKeyId=kms_key_arn + ) logger.debug(f"Created KMS key alias '{kms_key_alias}'") logger.debug(f"Setting KMS encryption on bucket '{bucket_name}'") try: - existing_bucket_encryption = s3_client.get_bucket_encryption( + existing_bucket_encryption = clients.s3_client.get_bucket_encryption( Bucket=bucket_name )["ServerSideEncryptionConfiguration"] if len(existing_bucket_encryption["Rules"]) > 0: @@ -296,7 +241,7 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None: ], } if new_bucket_encryption != existing_bucket_encryption: - s3_client.put_bucket_encryption( + clients.s3_client.put_bucket_encryption( Bucket=bucket_name, ServerSideEncryptionConfiguration=new_bucket_encryption, ) @@ -306,7 +251,7 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None: logger.debug("Enforcing KMS encryption through bucket policy") try: existing_bucket_policy = json.loads( - s3_client.get_bucket_policy(Bucket=bucket_name)["Policy"] + clients.s3_client.get_bucket_policy(Bucket=bucket_name)["Policy"] ) except ClientError as e: error_code = e.response["Error"]["Code"] @@ -352,7 +297,7 @@ def setup_kms_encryption_on_bucket(bucket_name: str) -> None: ], } if new_bucket_policy != existing_bucket_policy: - s3_client.put_bucket_policy( + clients.s3_client.put_bucket_policy( Bucket=bucket_name, Policy=json.dumps(new_bucket_policy), ) @@ -370,9 +315,11 @@ def enable_bucket_versioning(bucket_name: str) -> None: bucket_name (str): name of the bucket to enable versioning on """ try: - existing_versioning = s3_client.get_bucket_versioning(Bucket=bucket_name) + existing_versioning = clients.s3_client.get_bucket_versioning( + Bucket=bucket_name + ) if existing_versioning.get("Status") != "Enabled": - s3_client.put_bucket_versioning( + clients.s3_client.put_bucket_versioning( Bucket=bucket_name, VersioningConfiguration={"Status": "Enabled"}, ) @@ -398,7 +345,7 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: """ user_bucket_name = get_bucket_name_from_user_id(user_id) try: - s3_client.head_bucket(Bucket=user_bucket_name) + clients.s3_client.head_bucket(Bucket=user_bucket_name) logger.info(f"Bucket '{user_bucket_name}' already exists for user '{user_id}'") except ClientError as e: error_code = e.response["Error"]["Code"] @@ -413,15 +360,15 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: try: if config["USER_BUCKETS_REGION"] == "us-east-1": # it's the default region and cannot be specified in `LocationConstraint` - s3_client.create_bucket(Bucket=user_bucket_name) + clients.s3_client.create_bucket(Bucket=user_bucket_name) else: - s3_client.create_bucket( + clients.s3_client.create_bucket( Bucket=user_bucket_name, CreateBucketConfiguration={ "LocationConstraint": config["USER_BUCKETS_REGION"] }, ) - except s3_client.exceptions.BucketAlreadyOwnedByYou: + except clients.s3_client.exceptions.BucketAlreadyOwnedByYou: # `An error occurred (BucketAlreadyOwnedByYou) when calling the CreateBucket operation: # Your previous request to create the named bucket succeeded and you already own it.` # This can happen if this function is called multiple times in a row. @@ -429,13 +376,13 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: f"S3 bucket '{user_bucket_name}' already exists (race condition?): proceeding" ) else: - waiter = s3_client.get_waiter("bucket_exists") + waiter = clients.s3_client.get_waiter("bucket_exists") waiter.wait(Bucket=user_bucket_name) logger.info(f"Created S3 bucket '{user_bucket_name}' for user '{user_id}'") expiration_days = config["S3_OBJECTS_EXPIRATION_DAYS"] logger.debug(f"Setting bucket objects expiration to {expiration_days} days") - s3_client.put_bucket_lifecycle_configuration( + clients.s3_client.put_bucket_lifecycle_configuration( Bucket=user_bucket_name, LifecycleConfiguration={ "Rules": [ @@ -459,8 +406,8 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: kms_key_arn = setup_kms_encryption_on_bucket(user_bucket_name) else: logger.warning(f"Disabling KMS encryption on bucket '{user_bucket_name}'") - s3_client.delete_bucket_encryption(Bucket=user_bucket_name) - s3_client.delete_bucket_policy(Bucket=user_bucket_name) + clients.s3_client.delete_bucket_encryption(Bucket=user_bucket_name) + clients.s3_client.delete_bucket_policy(Bucket=user_bucket_name) return user_bucket_name, kms_key_arn @@ -503,7 +450,7 @@ def get_all_bucket_objects(user_bucket_name: str) -> list: """ Get all objects from the specified S3 bucket. """ - response = s3_client.list_objects_v2(Bucket=user_bucket_name) + response = clients.s3_client.list_objects_v2(Bucket=user_bucket_name) object_list = response.get("Contents", []) # list_objects_v2 can utmost return 1000 objects in a single response @@ -517,7 +464,7 @@ def get_all_bucket_objects(user_bucket_name: str) -> list: # This is fine for now because this code is only called during integration tests, with a small # number of files in the bucket. while response.get("IsTruncated"): - response = s3_client.list_objects_v2( + response = clients.s3_client.list_objects_v2( Bucket=user_bucket_name, ContinuationToken=response.get("NextContinuationToken"), ) @@ -551,7 +498,7 @@ def delete_all_bucket_objects(user_id: str, user_bucket_name: str) -> None: # we can remove this batching logic and retrieve objects in batches of 1,000 for deletion. limit = 1000 for offset in range(0, len(keys), limit): - response = s3_client.delete_objects( + response = clients.s3_client.delete_objects( Bucket=user_bucket_name, Delete={"Objects": keys[offset : offset + limit]}, ) @@ -581,7 +528,7 @@ def cleanup_user_bucket(user_id: str, delete_bucket: bool = False) -> Union[str, user_bucket_name = get_bucket_name_from_user_id(user_id) try: - s3_client.head_bucket(Bucket=user_bucket_name) + clients.s3_client.head_bucket(Bucket=user_bucket_name) except ClientError as e: error_code = e.response["Error"]["Code"] if error_code == "404": @@ -595,7 +542,7 @@ def cleanup_user_bucket(user_id: str, delete_bucket: bool = False) -> Union[str, logger.info( f"Initializing delete for bucket '{user_bucket_name}' for user '{user_id}'" ) - s3_client.delete_bucket(Bucket=user_bucket_name) + clients.s3_client.delete_bucket(Bucket=user_bucket_name) return user_bucket_name except Exception as e: diff --git a/gen3workflow/routes/ga4gh_tes.py b/gen3workflow/routes/ga4gh_tes.py index 4fdb905..3c3c4a9 100644 --- a/gen3workflow/routes/ga4gh_tes.py +++ b/gen3workflow/routes/ga4gh_tes.py @@ -21,7 +21,8 @@ from gen3workflow.auth import Auth from gen3workflow.config import config from gen3workflow.routes.utils import make_tes_server_request -from gen3workflow import aws_utils +from gen3workflow.aws import aws_utils +from gen3workflow.aws.bucket import create_iam_role_for_funnel_bucket_access router = APIRouter(prefix="/ga4gh/tes/v1") @@ -150,6 +151,7 @@ async def create_task(request: Request, auth=Depends(Auth)) -> dict: logger.error(f"{err_msg}. Allowed images: {config['TASK_IMAGE_WHITELIST']}") raise HTTPException(HTTP_403_FORBIDDEN, err_msg) + # TODO: For S3Files based deployments, verify if there is atleast one mount target that is available for the filesystem # Add internal tags if "tags" not in body: body["tags"] = {} @@ -168,7 +170,7 @@ async def create_task(request: Request, auth=Depends(Auth)) -> dict: if config["EKS_CLUSTER_NAME"]: body["tags"]["_FUNNEL_WORKER_ROLE_ARN"] = ( - aws_utils.create_iam_role_for_funnel_bucket_access(user_id) + create_iam_role_for_funnel_bucket_access(user_id) ) body["tags"]["_WORKER_SA"] = aws_utils.get_worker_sa_name(user_id) diff --git a/gen3workflow/routes/s3.py b/gen3workflow/routes/s3.py index 9adc56d..6b73a31 100644 --- a/gen3workflow/routes/s3.py +++ b/gen3workflow/routes/s3.py @@ -20,8 +20,10 @@ HTTP_404_NOT_FOUND, ) -from gen3workflow import aws_utils, logger +from gen3workflow import logger from gen3workflow.auth import Auth +from gen3workflow.aws import aws_utils +from gen3workflow.aws.bucket import get_existing_kms_key_for_bucket from gen3workflow.config import config s3_root_router = APIRouter(include_in_schema=False) @@ -357,7 +359,7 @@ async def s3_endpoint(path: str, request: Request): and request.method in ["PUT", "POST"] and "uploadId" not in query_params ): - _, kms_key_arn = aws_utils.get_existing_kms_key_for_bucket(user_bucket) + _, kms_key_arn = get_existing_kms_key_for_bucket(user_bucket) if not kms_key_arn: err_msg = "Bucket misconfigured. Hit the `GET /storage/setup` endpoint and try again." logger.error( diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index 6e4e812..ad366b8 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -8,9 +8,14 @@ HTTP_404_NOT_FOUND, ) -from gen3workflow import aws_utils, logger +from gen3workflow import logger from gen3workflow.auth import Auth -from gen3workflow.aws import s3_files +from gen3workflow.aws import aws_utils, s3_files +from gen3workflow.aws.bucket import ( + cleanup_user_bucket, + create_user_bucket, + enable_bucket_versioning, +) from gen3workflow.config import config router = APIRouter(prefix="/storage") @@ -39,7 +44,7 @@ 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 aws_utils.create_user_bucket(user_id) + bucket_name, kms_key_arn = await create_user_bucket(user_id) bucket_prefix = "ga4gh-tes" bucket_region = config["USER_BUCKETS_REGION"] @@ -55,7 +60,7 @@ async def storage_setup( if config["ENABLE_S3_FILES"]: # Bucket versioning is necessary for S3Files - aws_utils.enable_bucket_versioning(bucket_name) + enable_bucket_versioning(bucket_name) fs_id = s3_files.get_s3_files_system(bucket_name) if not fs_id: @@ -97,7 +102,7 @@ async def delete_user_bucket(request: Request, auth=Depends(Auth)) -> None: "delete", [f"/services/workflow/gen3-workflow/storage/{user_id}"] ) logger.info(f"User '{user_id}' deleting their storage bucket") - deleted_bucket_name = aws_utils.cleanup_user_bucket(user_id, delete_bucket=True) + deleted_bucket_name = cleanup_user_bucket(user_id, delete_bucket=True) if not deleted_bucket_name: raise HTTPException( @@ -133,7 +138,7 @@ async def empty_user_bucket(request: Request, auth=Depends(Auth)) -> None: "delete", [f"/services/workflow/gen3-workflow/storage/{user_id}"] ) logger.info(f"User '{user_id}' emptying their storage bucket") - deleted_bucket_name = aws_utils.cleanup_user_bucket(user_id) + deleted_bucket_name = cleanup_user_bucket(user_id) if not deleted_bucket_name: raise HTTPException( diff --git a/tests/conftest.py b/tests/conftest.py index 7f8ba76..5bbacd5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -29,7 +29,7 @@ config.validate() from gen3workflow.app import get_app -from gen3workflow.aws_utils import USER_BUCKET_CACHE +from gen3workflow.aws.bucket import USER_BUCKET_CACHE TEST_USER_ID = "user-64" NEW_TEST_USER_ID = "user-784" # a new user that does not already exist in arborist diff --git a/tests/test_aws_utils.py b/tests/test_aws_utils.py index 7022e98..851a736 100644 --- a/tests/test_aws_utils.py +++ b/tests/test_aws_utils.py @@ -2,7 +2,7 @@ from unittest.mock import patch from tests.conftest import TEST_USER_ID -from gen3workflow import aws_utils +from gen3workflow.aws import aws_utils, clients, bucket from gen3workflow.config import config from tests.test_misc import mock_aws_services @@ -16,23 +16,23 @@ def test_create_role_for_bucket_access_creates_role_when_missing(mock_aws_servic # Create KMS key to make sure, key exists and is added to the policy kms_key_alias = f"alias/gen3wf-localhost-{TEST_USER_ID}" - output = aws_utils.kms_client.create_key() + output = clients.kms_client.create_key() kms_key_arn = output["KeyMetadata"]["Arn"] - aws_utils.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) + clients.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) # Spy on the method while still letting moto execute it with patch.object( - aws_utils.iam_client, + clients.iam_client, "create_role", - wraps=aws_utils.iam_client.create_role, + wraps=clients.iam_client.create_role, ) as create_role_spy, patch.object( - aws_utils.iam_client, + clients.iam_client, "put_role_policy", - wraps=aws_utils.iam_client.put_role_policy, + wraps=clients.iam_client.put_role_policy, ) as put_policy_spy: # Act - aws_utils.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) + bucket.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) # IAM role doesn't exist by default since the mocks are isolated per tests # Assert create_role was called @@ -54,7 +54,7 @@ def test_create_role_for_bucket_access_creates_role_when_missing(mock_aws_servic ) # Compute OIDC issuer from mocked EKS (non-deterministic, therefore can't be hardcoded) - mock_oidc_token_url = aws_utils.eks_client.describe_cluster( + mock_oidc_token_url = clients.eks_client.describe_cluster( name=config["EKS_CLUSTER_NAME"] )["cluster"]["identity"]["oidc"]["issuer"].replace("https://", "") # Build the same assume role policy doc as the function will build @@ -150,29 +150,29 @@ def test_create_role_for_bucket_access_creates_role_when_missing(mock_aws_servic def test_update_assume_role_policy_called_when_policy_updated(mock_aws_services): """ - Test aws_utils.iam.update_assume_role_policy is called when there is a policy update + Test clients.iam.update_assume_role_policy is called when there is a policy update """ # Force the role to exists AND policy to be different to trigger an update role_name = f"gen3wf-localhost-{TEST_USER_ID}-funnel-role" assume_role_policy_doc = {"Version": "2012-10-17", "Statement": []} - aws_utils.iam_client.create_role( + clients.iam_client.create_role( RoleName=role_name, AssumeRolePolicyDocument=json.dumps(assume_role_policy_doc) ) # Create KMS key to make sure, key exists and is added to the policy kms_key_alias = f"alias/gen3wf-localhost-{TEST_USER_ID}" - output = aws_utils.kms_client.create_key() + output = clients.kms_client.create_key() kms_key_arn = output["KeyMetadata"]["Arn"] - aws_utils.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) + clients.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) with patch.object( - aws_utils.iam_client, + clients.iam_client, "update_assume_role_policy", - wraps=aws_utils.iam_client.update_assume_role_policy, + wraps=clients.iam_client.update_assume_role_policy, ) as update_assume_role_spy: # Act - aws_utils.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) + bucket.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) # Assert it was called assert ( @@ -189,16 +189,16 @@ def test_update_assume_role_policy_called_when_policy_updated(mock_aws_services) def test_does_not_update_assume_role_policy_when_unchanged(mock_aws_services): """ - Test aws_utils.iam.update_assume_role_policy is NOT called when the policy is unchanged + Test clients.iam.update_assume_role_policy is NOT called when the policy is unchanged """ # Create KMS key to make sure, key exists and is added to the policy kms_key_alias = f"alias/gen3wf-localhost-{TEST_USER_ID}" - output = aws_utils.kms_client.create_key() + output = clients.kms_client.create_key() kms_key_arn = output["KeyMetadata"]["Arn"] - aws_utils.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) + clients.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) # Compute OIDC issuer from mocked EKS (non-deterministic, therefore can't be hardcoded) - mock_oidc_token_url = aws_utils.eks_client.describe_cluster( + mock_oidc_token_url = clients.eks_client.describe_cluster( name=config["EKS_CLUSTER_NAME"] )["cluster"]["identity"]["oidc"]["issuer"].replace("https://", "") # Build the same assume role policy doc as the function will build @@ -227,18 +227,18 @@ def test_does_not_update_assume_role_policy_when_unchanged(mock_aws_services): } # Force the "role exists AND policy remains same" branch role_name = f"gen3wf-localhost-{TEST_USER_ID}-funnel-role" - aws_utils.iam_client.create_role( + clients.iam_client.create_role( RoleName=role_name, AssumeRolePolicyDocument=json.dumps(assume_role_policy_doc) ) # Spy on the method while still letting moto execute it with patch.object( - aws_utils.iam_client, + clients.iam_client, "update_assume_role_policy", - wraps=aws_utils.iam_client.update_assume_role_policy, + wraps=clients.iam_client.update_assume_role_policy, ) as update_assume_role_spy: # Act - aws_utils.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) + bucket.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) # Assert it was NOT called assert ( @@ -250,27 +250,27 @@ def test_create_role_for_bucket_access_with_no_kms_enabled( monkeypatch, mock_aws_services ): """ - Test aws_utils.iam.create_role is called when a new iam role is being created + Test clients.iam.create_role is called when a new iam role is being created """ - monkeypatch.setitem(aws_utils.config, "KMS_ENCRYPTION_ENABLED", False) + monkeypatch.setitem(bucket.config, "KMS_ENCRYPTION_ENABLED", False) # Create KMS key to make sure, the policy is not updated when # KMS is diabled, despite a key being present kms_key_alias = f"alias/gen3wf-localhost-{TEST_USER_ID}" - output = aws_utils.kms_client.create_key() + output = clients.kms_client.create_key() kms_key_arn = output["KeyMetadata"]["Arn"] - aws_utils.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) + clients.kms_client.create_alias(AliasName=kms_key_alias, TargetKeyId=kms_key_arn) # Spy on the method while still letting moto execute it with patch.object( - aws_utils.iam_client, + clients.iam_client, "put_role_policy", - wraps=aws_utils.iam_client.put_role_policy, + wraps=clients.iam_client.put_role_policy, ) as put_policy_spy: # Act - aws_utils.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) + bucket.create_iam_role_for_funnel_bucket_access(TEST_USER_ID) expected_policy = { "Version": "2012-10-17", diff --git a/tests/test_ga4gh_tes.py b/tests/test_ga4gh_tes.py index c59695a..ecff64a 100644 --- a/tests/test_ga4gh_tes.py +++ b/tests/test_ga4gh_tes.py @@ -122,7 +122,7 @@ async def test_create_task( be made. """ with patch( - "gen3workflow.aws_utils.get_existing_kms_key_for_bucket", + "gen3workflow.aws.bucket.get_existing_kms_key_for_bucket", lambda _: ("test_kms_key_alias", "*"), ): res = await client.post( @@ -211,7 +211,7 @@ async def test_create_gpu_task( """ tags = {"_GPU": is_gpu_task} if is_gpu_task else {} with patch( - "gen3workflow.aws_utils.get_existing_kms_key_for_bucket", + "gen3workflow.aws.bucket.get_existing_kms_key_for_bucket", lambda _: ("test_kms_key_alias", "*"), ): res = await client.post( @@ -393,7 +393,7 @@ async def test_create_task_with_whitelist_images( Ensure that any image sent to the TES server belongs exclusively to whitelisted repositories specified in the configuration. """ with patch( - "gen3workflow.aws_utils.get_existing_kms_key_for_bucket", + "gen3workflow.aws.bucket.get_existing_kms_key_for_bucket", lambda _: ("test_kms_key_alias", "*"), ): res = await client.post( @@ -439,7 +439,7 @@ async def test_create_task_optimized_node_scheduling( try: with patch( - "gen3workflow.aws_utils.get_existing_kms_key_for_bucket", + "gen3workflow.aws.bucket.get_existing_kms_key_for_bucket", lambda _: ("test_kms_key_alias", "*"), ): res = await client.post( diff --git a/tests/test_misc.py b/tests/test_misc.py index b7f6e82..3d298da 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -12,8 +12,8 @@ NEW_TEST_USER_ID, mock_arborist_request, ) -from gen3workflow import aws_utils -from gen3workflow.aws_utils import get_safe_name_from_hostname +from gen3workflow.aws import aws_utils, bucket, clients +from gen3workflow.aws.aws_utils import get_safe_name_from_hostname from gen3workflow.config import config @@ -33,20 +33,20 @@ def mock_aws_services(): Mock all AWS services """ with mock_aws(): - aws_utils.iam_client = boto3.client("iam") - aws_utils.kms_client = boto3.client( + clients.iam_client = boto3.client("iam") + clients.kms_client = boto3.client( "kms", region_name=config["USER_BUCKETS_REGION"] ) - aws_utils.s3_client = boto3.client("s3") - aws_utils.sts_client = boto3.client("sts") - aws_utils.eks_client = boto3.client( + clients.s3_client = boto3.client("s3") + clients.sts_client = boto3.client("sts") + clients.eks_client = boto3.client( "eks", region_name=os.environ.get("EKS_CLUSTER_REGION", "us-east-1") ) # Setup: Create a mock EKS cluster in the virtual environment cluster_name = "test-cluster" - aws_utils.eks_client.create_cluster( + clients.eks_client.create_cluster( name=cluster_name, roleArn="arn:aws:iam::123456789012:role/mock-eks-role", resourcesVpcConfig={"subnetIds": ["subnet-12345"]}, @@ -107,7 +107,7 @@ async def test_storage_setup( # Bucket must not exist before this test with pytest.raises(ClientError) as e: - aws_utils.s3_client.head_bucket(Bucket=expected_bucket_name) + clients.s3_client.head_bucket(Bucket=expected_bucket_name) assert ( e.value.response.get("ResponseMetadata", {}).get("HTTPStatusCode") == 404 ), f"Bucket exists: {e.value}" @@ -119,7 +119,7 @@ async def test_storage_setup( ) assert res.status_code == 200, res.text - kms_key = aws_utils.kms_client.describe_key(KeyId=f"alias/{expected_bucket_name}") + kms_key = clients.kms_client.describe_key(KeyId=f"alias/{expected_bucket_name}") kms_key_arn = kms_key["KeyMetadata"]["Arn"] storage_info = res.json() @@ -131,11 +131,11 @@ async def test_storage_setup( } # check that the bucket was created after the call to `/storage/setup` - bucket_exists = aws_utils.s3_client.head_bucket(Bucket=expected_bucket_name) + bucket_exists = clients.s3_client.head_bucket(Bucket=expected_bucket_name) assert bucket_exists, "Bucket does not exist" # check that the bucket is setup with KMS encryption - bucket_encryption = aws_utils.s3_client.get_bucket_encryption( + bucket_encryption = clients.s3_client.get_bucket_encryption( Bucket=expected_bucket_name ) assert bucket_encryption.get("ServerSideEncryptionConfiguration") == { @@ -151,7 +151,7 @@ async def test_storage_setup( } # check the bucket policy, which should enforce KMS encryption - bucket_policy = aws_utils.s3_client.get_bucket_policy(Bucket=expected_bucket_name) + bucket_policy = clients.s3_client.get_bucket_policy(Bucket=expected_bucket_name) assert json.loads(bucket_policy.get("Policy", "{}")) == { "Version": "2012-10-17", "Statement": [ @@ -192,7 +192,7 @@ async def test_storage_setup( } # check the bucket's lifecycle configuration - lifecycle_config = aws_utils.s3_client.get_bucket_lifecycle_configuration( + lifecycle_config = clients.s3_client.get_bucket_lifecycle_configuration( Bucket=expected_bucket_name ) assert lifecycle_config.get("Rules") == [ @@ -259,11 +259,9 @@ async def test_bucket_enforces_encryption( storage_info = res.json() with pytest.raises(ClientError, match="Forbidden"): - aws_utils.s3_client.put_object( - Bucket=storage_info["bucket"], Key="test-file.txt" - ) + clients.s3_client.put_object(Bucket=storage_info["bucket"], Key="test-file.txt") - unauthorized_kms_key_arn = aws_utils.kms_client.create_key( + unauthorized_kms_key_arn = clients.kms_client.create_key( Tags=[ { "TagKey": "Name", @@ -272,7 +270,7 @@ async def test_bucket_enforces_encryption( ] )["KeyMetadata"]["Arn"] with pytest.raises(ClientError, match="Forbidden"): - aws_utils.s3_client.put_object( + clients.s3_client.put_object( Bucket=storage_info["bucket"], Key="test-file.txt", ServerSideEncryption="aws:kms", @@ -283,8 +281,8 @@ async def test_bucket_enforces_encryption( # in `moto.mock_aws`. This test works well when ran against the real AWS. # Against the real AWS, the 2 calls above also raise `AccessDenied` instead of `Forbidden`. - # authorized_kms_key_arn = aws_utils.kms_client.describe_key(KeyId=f"alias/{storage_info['bucket']}")["KeyMetadata"]["Arn"] - # aws_utils.s3_client.put_object( + # authorized_kms_key_arn = clients.kms_client.describe_key(KeyId=f"alias/{storage_info['bucket']}")["KeyMetadata"]["Arn"] + # clients.s3_client.put_object( # Bucket=storage_info["bucket"], # Key="test-file.txt", # ServerSideEncryption="aws:kms", @@ -307,7 +305,7 @@ async def test_delete_user_bucket( bucket_name = res.json()["bucket"] # Verify the bucket exists - bucket_exists = aws_utils.s3_client.head_bucket(Bucket=bucket_name) + bucket_exists = clients.s3_client.head_bucket(Bucket=bucket_name) assert bucket_exists, "Bucket does not exist" # Delete the bucket @@ -319,7 +317,7 @@ async def test_delete_user_bucket( # Verify the bucket is deleted with pytest.raises(ClientError) as e: - aws_utils.s3_client.head_bucket(Bucket=bucket_name) + clients.s3_client.head_bucket(Bucket=bucket_name) assert ( e.value.response.get("ResponseMetadata", {}).get("HTTPStatusCode") == 404 ), f"Bucket still exists: {e.value}" @@ -357,18 +355,18 @@ async def test_delete_user_bucket_with_files( # Remove the bucket policy enforcing KMS encryption # Moto has limitations that prevent adding objects to a bucket with KMS encryption enabled. # More details: https://github.com/uc-cdis/gen3-workflow/blob/554fc3eb4c1d333f9ef81c1a5f8e75a6b208cdeb/tests/test_misc.py#L161-L171 - aws_utils.s3_client.delete_bucket_policy(Bucket=bucket_name) + clients.s3_client.delete_bucket_policy(Bucket=bucket_name) # Upload more than 1000 objects to ensure batching is working correctly. Not too many so the # test doesn't take too long to run. object_count = 1050 for i in range(object_count): - aws_utils.s3_client.put_object( + clients.s3_client.put_object( Bucket=bucket_name, Key=f"file_{i}", Body=b"Dummy file contents" ) # Verify all the objects in the bucket are fetched even when bucket has more than 1000 objects - object_list = aws_utils.get_all_bucket_objects(bucket_name) + object_list = bucket.get_all_bucket_objects(bucket_name) assert len(object_list) == object_count # Delete the bucket @@ -379,7 +377,7 @@ async def test_delete_user_bucket_with_files( # Verify the bucket is deleted with pytest.raises(ClientError) as e: - aws_utils.s3_client.head_bucket(Bucket=bucket_name) + clients.s3_client.head_bucket(Bucket=bucket_name) assert ( e.value.response.get("ResponseMetadata", {}).get("HTTPStatusCode") == 404 ), f"Bucket still exists: {e.value}" @@ -392,7 +390,7 @@ async def test_delete_user_bucket_no_token(client, mock_aws_services): """ mock_delete_bucket = MagicMock() # Delete the bucket - with patch("gen3workflow.aws_utils.cleanup_user_bucket", mock_delete_bucket): + with patch("gen3workflow.aws.bucket.cleanup_user_bucket", mock_delete_bucket): res = await client.delete("/storage/user-bucket") assert res.status_code == 401, res.text assert res.json() == {"detail": "Must provide an access token"} @@ -414,7 +412,7 @@ async def test_delete_user_bucket_unauthorized( """ mock_delete_bucket = MagicMock() # Delete the bucket - with patch("gen3workflow.aws_utils.cleanup_user_bucket", mock_delete_bucket): + with patch("gen3workflow.aws.bucket.cleanup_user_bucket", mock_delete_bucket): res = await client.delete( "/storage/user-bucket", headers={"Authorization": f"bearer {TEST_USER_TOKEN}"}, @@ -442,11 +440,11 @@ async def test_delete_user_bucket_objects_with_existing_files( # Remove the bucket policy enforcing KMS encryption # Moto has limitations that prevent adding objects to a bucket with KMS encryption enabled. # More details: https://github.com/uc-cdis/gen3-workflow/blob/554fc3eb4c1d333f9ef81c1a5f8e75a6b208cdeb/tests/test_misc.py#L161-L171 - aws_utils.s3_client.delete_bucket_policy(Bucket=bucket_name) + clients.s3_client.delete_bucket_policy(Bucket=bucket_name) object_count = 10 for i in range(object_count): - aws_utils.s3_client.put_object( + clients.s3_client.put_object( Bucket=bucket_name, Key=f"file_{i}", Body=b"Dummy file contents" ) @@ -458,11 +456,11 @@ async def test_delete_user_bucket_objects_with_existing_files( assert res.status_code == 204, res.text # Verify the bucket still exists - bucket_exists = aws_utils.s3_client.head_bucket(Bucket=bucket_name) + bucket_exists = clients.s3_client.head_bucket(Bucket=bucket_name) assert bucket_exists, f"Bucket '{bucket_name} is expected to exist but not found" # Verify all the objects in the bucket are deleted - object_list = aws_utils.get_all_bucket_objects(bucket_name) + object_list = bucket.get_all_bucket_objects(bucket_name) assert ( len(object_list) == 0 ), f"Expected bucket to have no objects, but found {len(object_list)}.\n{object_list=}" diff --git a/tests/test_s3_endpoint.py b/tests/test_s3_endpoint.py index b801e6a..78d279b 100644 --- a/tests/test_s3_endpoint.py +++ b/tests/test_s3_endpoint.py @@ -375,7 +375,7 @@ def test_s3_upload_file(s3_client, access_token_patcher, multipart): object_key = f"test_s3_upload_file{'_multipart' if multipart else ''}.txt" with patch( - "gen3workflow.aws_utils.get_existing_kms_key_for_bucket", + "gen3workflow.aws.bucket.get_existing_kms_key_for_bucket", lambda _: ("test_kms_key_alias", "test_kms_key_arn"), ): with tempfile.NamedTemporaryFile(delete=True) as file_to_upload: From 3f563130a74c4aa80cd6bd85869a5a25431c1dcc Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Fri, 24 Jul 2026 17:33:37 -0500 Subject: [PATCH 20/33] Fix patching in unit tests. All current tests pass --- tests/test_s3_endpoint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_s3_endpoint.py b/tests/test_s3_endpoint.py index 78d279b..e7c1b92 100644 --- a/tests/test_s3_endpoint.py +++ b/tests/test_s3_endpoint.py @@ -375,7 +375,7 @@ def test_s3_upload_file(s3_client, access_token_patcher, multipart): object_key = f"test_s3_upload_file{'_multipart' if multipart else ''}.txt" with patch( - "gen3workflow.aws.bucket.get_existing_kms_key_for_bucket", + "gen3workflow.routes.s3.get_existing_kms_key_for_bucket", lambda _: ("test_kms_key_alias", "test_kms_key_arn"), ): with tempfile.NamedTemporaryFile(delete=True) as file_to_upload: From 28614f894482b482c460c4efabe621cef91d6ad6 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Fri, 24 Jul 2026 17:37:22 -0500 Subject: [PATCH 21/33] Add more TODOs --- gen3workflow/aws/bucket.py | 1 + 1 file changed, 1 insertion(+) diff --git a/gen3workflow/aws/bucket.py b/gen3workflow/aws/bucket.py index d8db38d..75429f9 100644 --- a/gen3workflow/aws/bucket.py +++ b/gen3workflow/aws/bucket.py @@ -473,6 +473,7 @@ def get_all_bucket_objects(user_bucket_name: str) -> list: return object_list +# TODO: Handle `delete_all_bucket_objects` when bucket versioning is enabled. Needed for CI, when S3Files is enabled. def delete_all_bucket_objects(user_id: str, user_bucket_name: str) -> None: """ Deletes all objects from the specified S3 bucket. From aeb3071e4fa9dc1d1f490ba4f33d91af6e9a4cdc Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Mon, 27 Jul 2026 12:31:26 -0500 Subject: [PATCH 22/33] Add unit tests for S3Files --- gen3workflow/aws/s3_files.py | 44 +- tests/test_misc.py | 26 ++ tests/test_s3_files.py | 823 +++++++++++++++++++++++++++++++++++ 3 files changed, 868 insertions(+), 25 deletions(-) create mode 100644 tests/test_s3_files.py diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index eba617a..0a2ee4a 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -4,13 +4,7 @@ from gen3workflow import logger from gen3workflow.config import config -from gen3workflow.aws.clients import ( - s3files_client, - eks_client, - ec2_client, - iam_client, - sts_client, -) +from gen3workflow.aws import clients # NFS port used for all communication between EKS pods and S3 Files mount targets. NFS_PORT = 2049 @@ -36,7 +30,7 @@ def get_s3_files_system(bucket_name: str) -> str | None: ) try: - paginator = s3files_client.get_paginator("list_file_systems") + paginator = clients.s3files_client.get_paginator("list_file_systems") for page in paginator.paginate(): for fs in page.get("fileSystems", []): if fs.get("bucket") == bucket_arn: @@ -71,8 +65,8 @@ def get_filesystem_status(file_system_id: str) -> tuple[str | None, str | None]: return None, "file_system_id must not be empty" try: - fs = s3files_client.get_file_system(fileSystemId=file_system_id) - except s3files_client.exceptions.ResourceNotFoundException: + fs = clients.s3files_client.get_file_system(fileSystemId=file_system_id) + except clients.s3files_client.exceptions.ResourceNotFoundException: return None, f"File system with file_system_id={file_system_id} does not exist" except ClientError as e: return None, f"Failed to fetch file system {file_system_id}: {e}" @@ -122,7 +116,7 @@ def _create_s3_files_system(bucket_name: str, role_arn: str) -> str: bucket_arn = f"arn:aws:s3:::{bucket_name}" try: - response = s3files_client.create_file_system( + response = clients.s3files_client.create_file_system( bucket=bucket_arn, prefix="funnel-temp-files/", roleArn=role_arn, @@ -157,7 +151,7 @@ def create_mount_target_for_file_system( mount_target_sg_id: security group to attach to the mount target's ENI. """ try: - response = s3files_client.create_mount_target( + response = clients.s3files_client.create_mount_target( fileSystemId=file_system_id, subnetId=subnet_id, securityGroups=[mount_target_sg_id], @@ -196,7 +190,7 @@ def list_mount_targets_for_file_system(file_system_id: str) -> list[dict]: """ mount_targets = [] try: - paginator = s3files_client.get_paginator("list_mount_targets") + paginator = clients.s3files_client.get_paginator("list_mount_targets") for page in paginator.paginate(fileSystemId=file_system_id): mount_targets.extend(page.get("mountTargets", [])) except ClientError as e: @@ -217,7 +211,7 @@ def _get_vpc_id() -> str: Return the VPC ID the EKS cluster's nodes run in. Used to determine which VPC the S3 Files mount targets should be created in. """ - cluster = eks_client.describe_cluster(name=config["EKS_CLUSTER_NAME"]) + cluster = clients.eks_client.describe_cluster(name=config["EKS_CLUSTER_NAME"]) return cluster["cluster"]["resourcesVpcConfig"]["vpcId"] @@ -226,7 +220,7 @@ def _get_available_az_to_subnet(discovery_tag: str) -> dict[str, str]: Get one subnet ID per AZ where EKS pods can be scheduled, so that one S3 Files mount target can be created per AZ. """ - subnets = ec2_client.describe_subnets( + subnets = clients.ec2_client.describe_subnets( Filters=[{"Name": "tag:karpenter.sh/discovery", "Values": [discovery_tag]}], )["Subnets"] return {subnet["AvailabilityZoneId"]: subnet["SubnetId"] for subnet in subnets} @@ -242,7 +236,7 @@ def _get_eks_security_groups() -> tuple[str, str]: f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg", f"{config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg", ] - security_groups = ec2_client.describe_security_groups( + security_groups = clients.ec2_client.describe_security_groups( Filters=[{"Name": "group-name", "Values": eks_sg_names}] )["SecurityGroups"] return [(group["GroupId"], group["GroupName"]) for group in security_groups] @@ -267,7 +261,7 @@ def _get_or_create_security_groups(vpc_id: str) -> str: compute_security_groups = _get_eks_security_groups() mount_target_sg_name = "gen3wf-s3files-mount-target-sg" - existing = ec2_client.describe_security_groups( + existing = clients.ec2_client.describe_security_groups( Filters=[{"Name": "group-name", "Values": [mount_target_sg_name]}] )["SecurityGroups"] @@ -279,7 +273,7 @@ def _get_or_create_security_groups(vpc_id: str) -> str: mount_target_sg_id, ) else: - response = ec2_client.create_security_group( + response = clients.ec2_client.create_security_group( GroupName=mount_target_sg_name, Description="S3 Files mount target SG -- allows inbound NFS (2049) from Funnel compute SGs only.", VpcId=vpc_id, @@ -291,7 +285,7 @@ def _get_or_create_security_groups(vpc_id: str) -> str: # Inbound: mount target SG allows NFS from every compute SG, idempotently. try: - ec2_client.authorize_security_group_ingress( + clients.ec2_client.authorize_security_group_ingress( GroupId=mount_target_sg_id, IpPermissions=[ { @@ -330,7 +324,7 @@ def _get_or_create_security_groups(vpc_id: str) -> str: compute_security_group_name, ) in compute_security_groups: try: - ec2_client.authorize_security_group_egress( + clients.ec2_client.authorize_security_group_egress( GroupId=compute_security_group_id, IpPermissions=[ { @@ -386,15 +380,15 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: """ # TODO: Make it such that it is under 64 characters role_name = f"{bucket_name}-s3files-role" - account_id = sts_client.get_caller_identity()["Account"] + account_id = clients.sts_client.get_caller_identity()["Account"] bucket_arn = f"arn:aws:s3:::{bucket_name}" try: - role = iam_client.get_role(RoleName=role_name) + role = clients.iam_client.get_role(RoleName=role_name) role_arn = role["Role"]["Arn"] logger.info("IAM role '%s' already exists (%s)", role_name, role_arn) return role_arn - except iam_client.exceptions.NoSuchEntityException: + except clients.iam_client.exceptions.NoSuchEntityException: pass trust_policy = { @@ -494,7 +488,7 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: } try: - response = iam_client.create_role( + response = clients.iam_client.create_role( RoleName=role_name, AssumeRolePolicyDocument=json.dumps(trust_policy), Description=f"Role assumed by S3 Files to sync with bucket '{bucket_name}'", @@ -502,7 +496,7 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: ) role_arn = response["Role"]["Arn"] - iam_client.put_role_policy( + clients.iam_client.put_role_policy( RoleName=role_name, PolicyName="S3FilesBucketAccess", PolicyDocument=json.dumps(inline_policy), diff --git a/tests/test_misc.py b/tests/test_misc.py index 3d298da..fb7c055 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -27,6 +27,25 @@ def reset_config_hostname(): config["HOSTNAME"] = original_hostname +class S3FilesResourceNotFoundException(ClientError): + """ + Stand-in for the `ResourceNotFoundException` that the AWS S3 Files boto3 client exposes + as `s3files_client.exceptions.ResourceNotFoundException`. + + It subclasses `ClientError`, same as the real botocore-generated exception, so that a + `S3FilesResourceNotFoundException` is also caught by any broader `except ClientError` + that appears after it. + """ + + def __init__(self, message: str = "File system not found"): + super().__init__( + error_response={ + "Error": {"Code": "ResourceNotFoundException", "Message": message} + }, + operation_name="GetFileSystem", + ) + + @pytest.fixture(scope="function") def mock_aws_services(): """ @@ -42,6 +61,13 @@ def mock_aws_services(): clients.eks_client = boto3.client( "eks", region_name=os.environ.get("EKS_CLUSTER_REGION", "us-east-1") ) + clients.ec2_client = boto3.client( + "ec2", region_name=config["USER_BUCKETS_REGION"] + ) + clients.s3files_client = MagicMock(name="s3files_client") + clients.s3files_client.exceptions.ResourceNotFoundException = ( + S3FilesResourceNotFoundException + ) # Setup: Create a mock EKS cluster in the virtual environment cluster_name = "test-cluster" diff --git a/tests/test_s3_files.py b/tests/test_s3_files.py new file mode 100644 index 0000000..efb4e8c --- /dev/null +++ b/tests/test_s3_files.py @@ -0,0 +1,823 @@ +""" +Unit tests for `gen3workflow.aws.s3_files`. + +S3 Files is a very new AWS service. As of this writing, `moto` has no mock backend +for it, so calls through `s3files_client` are stubbed out by hand with +`unittest.mock` (see the `mock_s3_files_aws_clients` fixture in `conftest.py`) +rather than relying on `moto.mock_aws`. All the *other* AWS services used by this +module (`iam`, `ec2`, `eks`, `sts`) are mocked with `moto`, consistent with the +rest of this test suite (see `tests/test_aws_utils.py`). +""" + +import json +import time +from unittest.mock import MagicMock, patch + +import pytest +from botocore.exceptions import ClientError + +from gen3workflow.aws import clients, s3_files +from gen3workflow.config import config +from tests.test_misc import S3FilesResourceNotFoundException, mock_aws_services + + +def _client_error(code: str, message: str = "error", operation_name: str = "Operation"): + """ + Build a real `botocore.exceptions.ClientError`, the way boto3 would raise one. + """ + return ClientError( + error_response={"Error": {"Code": code, "Message": message}}, + operation_name=operation_name, + ) + + +# --------------------------------------------------------------------------- # +# get_s3_files_system +# --------------------------------------------------------------------------- # + + +def test_get_s3_files_system_found(mock_aws_services): + """ + If a file system already exists for the bucket's ARN, its ID is returned. + """ + bucket_name = "test-bucket" + bucket_arn = f"arn:aws:s3:::{bucket_name}" + paginator = MagicMock() + paginator.paginate.return_value = [ + { + "fileSystems": [ + { + "bucket": "arn:aws:s3:::some-other-bucket", + "fileSystemId": "fs-1", + "status": "available", + }, + {"bucket": bucket_arn, "fileSystemId": "fs-2", "status": "available"}, + ] + } + ] + clients.s3files_client.get_paginator.return_value = paginator + + result = s3_files.get_s3_files_system(bucket_name) + + assert result == "fs-2" + clients.s3files_client.get_paginator.assert_called_once_with("list_file_systems") + + +def test_get_s3_files_system_not_found(mock_aws_services): + """ + If no file system matches the bucket's ARN, `None` is returned. + """ + paginator = MagicMock() + paginator.paginate.return_value = [{"fileSystems": []}] + clients.s3files_client.get_paginator.return_value = paginator + + assert s3_files.get_s3_files_system("test-bucket") is None + + +def test_get_s3_files_system_multiple_pages(mock_aws_services): + """ + The match can be on a later page; pagination is followed to completion. + """ + bucket_name = "test-bucket" + bucket_arn = f"arn:aws:s3:::{bucket_name}" + paginator = MagicMock() + paginator.paginate.return_value = [ + { + "fileSystems": [ + { + "bucket": "arn:aws:s3:::other", + "fileSystemId": "fs-1", + "status": "available", + } + ] + }, + { + "fileSystems": [ + {"bucket": bucket_arn, "fileSystemId": "fs-2", "status": "available"} + ] + }, + ] + clients.s3files_client.get_paginator.return_value = paginator + + assert s3_files.get_s3_files_system(bucket_name) == "fs-2" + + +def test_get_s3_files_system_client_error_reraises(mock_aws_services): + """ + A `ClientError` while listing file systems is logged and re-raised, not swallowed. + """ + clients.s3files_client.get_paginator.side_effect = _client_error( + "InternalError", "boom", "ListFileSystems" + ) + + with pytest.raises(ClientError): + s3_files.get_s3_files_system("test-bucket") + + +# --------------------------------------------------------------------------- # +# get_filesystem_status +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize("empty_id", ["", None]) +def test_get_filesystem_status_empty_id(empty_id, mock_aws_services): + """ + An empty/`None` file_system_id short-circuits without calling AWS. + """ + status, message = s3_files.get_filesystem_status(empty_id) + + assert status is None + assert message == "file_system_id must not be empty" + clients.s3files_client.get_file_system.assert_not_called() + + +def test_get_filesystem_status_success(mock_aws_services): + clients.s3files_client.get_file_system.return_value = { + "status": "available", + "statusMessage": None, + } + + status, message = s3_files.get_filesystem_status("fs-123") + + assert status == "available" + assert message is None + clients.s3files_client.get_file_system.assert_called_once_with( + fileSystemId="fs-123" + ) + + +def test_get_filesystem_status_not_found(mock_aws_services): + """ + A `ResourceNotFoundException` is translated into a `(None, )` tuple + instead of propagating. + """ + clients.s3files_client.get_file_system.side_effect = ( + S3FilesResourceNotFoundException() + ) + + status, message = s3_files.get_filesystem_status("fs-missing") + + assert status is None + assert message == "File system with file_system_id=fs-missing does not exist" + + +def test_get_filesystem_status_generic_client_error(mock_aws_services): + """ + Any other `ClientError` is also translated into a `(None, )` tuple + (unlike most other functions in this module, which re-raise). + """ + clients.s3files_client.get_file_system.side_effect = _client_error( + "InternalError", "server exploded", "GetFileSystem" + ) + + status, message = s3_files.get_filesystem_status("fs-123") + + assert status is None + assert "Failed to fetch file system fs-123" in message + assert "server exploded" in message + + +# --------------------------------------------------------------------------- # +# get_mount_target_status / get_s3files_setup_status +# +# Both functions are currently `# TODO` stubs that unconditionally return +# "Not ready". These tests verify the underlying data is still fetched, +# so that once the TODOs are implemented, these tests will fail and call out +# the exact behavior that needs to be updated. +# --------------------------------------------------------------------------- # + + +def test_get_mount_target_status_calls_list_and_returns_not_ready(): + with patch.object( + s3_files, "list_mount_targets_for_file_system", return_value=[] + ) as list_mts: + result = s3_files.get_mount_target_status("fs-123") + + list_mts.assert_called_once_with("fs-123") + assert result == "Not ready" + + +def test_get_s3files_setup_status_calls_dependencies_and_returns_not_ready(): + with patch.object( + s3_files, "get_filesystem_status", return_value=("available", None) + ) as get_fs_status, patch.object( + s3_files, "get_mount_target_status", return_value="Not ready" + ) as get_mt_status: + result = s3_files.get_s3files_setup_status("fs-123") + + get_fs_status.assert_called_once_with(file_system_id="fs-123") + get_mt_status.assert_called_once_with("fs-123") + assert result == "Not ready" + + +# --------------------------------------------------------------------------- # +# _create_s3_files_system +# --------------------------------------------------------------------------- # + + +def test_create_s3_files_system_success(mock_aws_services): + clients.s3files_client.create_file_system.return_value = {"fileSystemId": "fs-new"} + + result = s3_files._create_s3_files_system( + bucket_name="test-bucket", + role_arn="arn:aws:iam::123456789012:role/s3files-role", + ) + + assert result == "fs-new" + clients.s3files_client.create_file_system.assert_called_once_with( + bucket="arn:aws:s3:::test-bucket", + prefix="funnel-temp-files/", + roleArn="arn:aws:iam::123456789012:role/s3files-role", + tags=[{"key": "app-name", "value": "gen3-workflow"}], + ) + + +def test_create_s3_files_system_client_error_reraises(mock_aws_services): + clients.s3files_client.create_file_system.side_effect = _client_error( + "ValidationException", "bad bucket", "CreateFileSystem" + ) + + with pytest.raises(ClientError): + s3_files._create_s3_files_system( + bucket_name="test-bucket", + role_arn="arn:aws:iam::123456789012:role/s3files-role", + ) + + +# --------------------------------------------------------------------------- # +# create_mount_target_for_file_system +# --------------------------------------------------------------------------- # + + +def test_create_mount_target_for_file_system_success(mock_aws_services): + clients.s3files_client.create_mount_target.return_value = { + "mountTargetId": "fsmt-1", + "status": "creating", + } + + s3_files.create_mount_target_for_file_system( + file_system_id="fs-123", subnet_id="subnet-abc", mount_target_sg_id="sg-abc" + ) + + clients.s3files_client.create_mount_target.assert_called_once_with( + fileSystemId="fs-123", subnetId="subnet-abc", securityGroups=["sg-abc"] + ) + + +def test_create_mount_target_for_file_system_client_error_reraises( + mock_aws_services, +): + clients.s3files_client.create_mount_target.side_effect = _client_error( + "ValidationException", "bad subnet", "CreateMountTarget" + ) + + with pytest.raises(ClientError): + s3_files.create_mount_target_for_file_system( + file_system_id="fs-123", subnet_id="subnet-abc", mount_target_sg_id="sg-abc" + ) + + +# --------------------------------------------------------------------------- # +# list_mount_targets_for_file_system +# --------------------------------------------------------------------------- # + + +def test_list_mount_targets_for_file_system_flattens_pages(mock_aws_services): + paginator = MagicMock() + paginator.paginate.return_value = [ + {"mountTargets": [{"mountTargetId": "fsmt-1"}]}, + {"mountTargets": [{"mountTargetId": "fsmt-2"}, {"mountTargetId": "fsmt-3"}]}, + ] + clients.s3files_client.get_paginator.return_value = paginator + + result = s3_files.list_mount_targets_for_file_system("fs-123") + + assert [mt["mountTargetId"] for mt in result] == ["fsmt-1", "fsmt-2", "fsmt-3"] + clients.s3files_client.get_paginator.assert_called_once_with("list_mount_targets") + paginator.paginate.assert_called_once_with(fileSystemId="fs-123") + + +def test_list_mount_targets_for_file_system_empty(mock_aws_services): + paginator = MagicMock() + paginator.paginate.return_value = [{"mountTargets": []}] + clients.s3files_client.get_paginator.return_value = paginator + + assert s3_files.list_mount_targets_for_file_system("fs-123") == [] + + +def test_list_mount_targets_for_file_system_client_error_reraises( + mock_aws_services, +): + clients.s3files_client.get_paginator.side_effect = _client_error( + "InternalError", "boom", "ListMountTargets" + ) + + with pytest.raises(ClientError): + s3_files.list_mount_targets_for_file_system("fs-123") + + +# --------------------------------------------------------------------------- # +# _get_vpc_id +# +# moto's EKS mock does not populate `resourcesVpcConfig.vpcId` on +# `describe_cluster` (it only echoes back input fields, and `vpcId` is not a +# valid `create_cluster` input parameter -- it's server-generated on real AWS). +# So this is mocked directly rather than through moto. +# --------------------------------------------------------------------------- # + + +def test_get_vpc_id(): + fake_eks_client = MagicMock() + fake_eks_client.describe_cluster.return_value = { + "cluster": {"resourcesVpcConfig": {"vpcId": "vpc-abc123"}} + } + + with patch.object(clients, "eks_client", fake_eks_client): + result = s3_files._get_vpc_id() + + assert result == "vpc-abc123" + fake_eks_client.describe_cluster.assert_called_once_with( + name=config["EKS_CLUSTER_NAME"] + ) + + +# --------------------------------------------------------------------------- # +# _get_available_az_to_subnet +# --------------------------------------------------------------------------- # + + +def test_get_available_az_to_subnet(mock_aws_services): + + # Create dummy VPC through moto with subnets and tags + vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] + + discoverable_subnet = clients.ec2_client.create_subnet( + VpcId=vpc_id, CidrBlock="10.0.1.0/24", AvailabilityZone="us-east-1a" + )["Subnet"] + clients.ec2_client.create_tags( + Resources=[discoverable_subnet["SubnetId"]], + Tags=[{"Key": "karpenter.sh/discovery", "Value": "test-cluster"}], + ) + + # a subnet without the discovery tag should not show up in the result + other_subnet = clients.ec2_client.create_subnet( + VpcId=vpc_id, CidrBlock="10.0.2.0/24", AvailabilityZone="us-east-1b" + )["Subnet"] + + result = s3_files._get_available_az_to_subnet(discovery_tag="test-cluster") + + assert result == { + discoverable_subnet["AvailabilityZoneId"]: discoverable_subnet["SubnetId"] + } + assert other_subnet["SubnetId"] not in result.values() + + +def test_get_available_az_to_subnet_no_matches(mock_aws_services): + result = s3_files._get_available_az_to_subnet(discovery_tag="nonexistent-tag") + assert result == {} + + +# --------------------------------------------------------------------------- # +# _get_eks_security_groups +# --------------------------------------------------------------------------- # + + +def test_get_eks_security_groups(mock_aws_services): + + vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] + + workers_sg = clients.ec2_client.create_security_group( + GroupName="test-cluster_EKS_workers_sg", Description="workers", VpcId=vpc_id + ) + jupyter_sg = clients.ec2_client.create_security_group( + GroupName="test-cluster_EKS_nodepool_jupyter_sg", + Description="jupyter", + VpcId=vpc_id, + ) + # an unrelated security group should not be returned + clients.ec2_client.create_security_group( + GroupName="unrelated-sg", Description="unrelated", VpcId=vpc_id + ) + + result = s3_files._get_eks_security_groups() + + result_ids = {sg_id for sg_id, _ in result} + assert result_ids == {workers_sg["GroupId"], jupyter_sg["GroupId"]} + + +def test_get_eks_security_groups_none_found(mock_aws_services): + assert s3_files._get_eks_security_groups() == [] + + +# --------------------------------------------------------------------------- # +# _get_or_create_security_groups +# --------------------------------------------------------------------------- # + + +def test_get_or_create_security_groups_creates_new_sg(mock_aws_services): + vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] + compute_sg = clients.ec2_client.create_security_group( + GroupName="test-cluster_EKS_workers_sg", Description="workers", VpcId=vpc_id + ) + + mount_target_sg_id = s3_files._get_or_create_security_groups(vpc_id=vpc_id) + + # the new mount target security group exists in this VPC: + described = clients.ec2_client.describe_security_groups( + Filters=[{"Name": "group-name", "Values": ["gen3wf-s3files-mount-target-sg"]}] + )["SecurityGroups"] + assert len(described) == 1 + assert described[0]["GroupId"] == mount_target_sg_id + assert described[0]["VpcId"] == vpc_id + + # inbound NFS from the compute SG is allowed on the mount target SG: + ip_permissions = described[0]["IpPermissions"] + assert any( + perm["FromPort"] == s3_files.NFS_PORT + and perm["ToPort"] == s3_files.NFS_PORT + and perm["IpProtocol"] == "tcp" + and any( + pair["GroupId"] == compute_sg["GroupId"] + for pair in perm["UserIdGroupPairs"] + ) + for perm in ip_permissions + ) + + # outbound NFS to the mount target SG is allowed on the compute SG: + compute_sg_described = clients.ec2_client.describe_security_groups( + GroupIds=[compute_sg["GroupId"]] + )["SecurityGroups"][0] + egress_permissions = compute_sg_described["IpPermissionsEgress"] + assert any( + perm.get("FromPort") == s3_files.NFS_PORT + and perm.get("ToPort") == s3_files.NFS_PORT + and any( + pair["GroupId"] == mount_target_sg_id + for pair in perm.get("UserIdGroupPairs", []) + ) + for perm in egress_permissions + ) + + +def test_get_or_create_security_groups_reuses_existing_sg(mock_aws_services): + """ + If the mount target security group already exists, it is reused (not + recreated), but its ingress/egress rules are still (idempotently) ensured. + """ + vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] + clients.ec2_client.create_security_group( + GroupName="test-cluster_EKS_workers_sg", Description="workers", VpcId=vpc_id + ) + existing_mount_target_sg = clients.ec2_client.create_security_group( + GroupName="gen3wf-s3files-mount-target-sg", Description="existing", VpcId=vpc_id + ) + + with patch.object( + clients.ec2_client, + "create_security_group", + wraps=clients.ec2_client.create_security_group, + ) as create_sg_spy: + result = s3_files._get_or_create_security_groups(vpc_id=vpc_id) + + assert result == existing_mount_target_sg["GroupId"] + create_sg_spy.assert_not_called() + + +def test_get_or_create_security_groups_is_idempotent(mock_aws_services): + """ + Calling this function twice (simulating a second bucket/file-system setup that + reuses the same cluster) must not raise, even though both the ingress rule on + the mount target SG and the egress rules on every compute SG will already + exist on the second call (triggering the `InvalidPermission.Duplicate` code + paths for both). + """ + vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] + clients.ec2_client.create_security_group( + GroupName="test-cluster_EKS_workers_sg", Description="workers", VpcId=vpc_id + ) + + first_result = s3_files._get_or_create_security_groups(vpc_id=vpc_id) + # second call should not raise despite duplicate ingress/egress rules + second_result = s3_files._get_or_create_security_groups(vpc_id=vpc_id) + + assert first_result == second_result + + +def test_get_or_create_security_groups_ingress_reraises_non_duplicate_error( + mock_aws_services, +): + """ + A non-duplicate `ClientError` on the ingress-rule call must propagate. + """ + vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] + clients.ec2_client.create_security_group( + GroupName="test-cluster_EKS_workers_sg", Description="workers", VpcId=vpc_id + ) + + with patch.object( + clients.ec2_client, + "authorize_security_group_ingress", + side_effect=_client_error( + "InvalidGroup.NotFound", "gone", "AuthorizeSecurityGroupIngress" + ), + ): + with pytest.raises(ClientError) as exc_info: + s3_files._get_or_create_security_groups(vpc_id=vpc_id) + + assert exc_info.value.response["Error"]["Code"] == "InvalidGroup.NotFound" + + +def test_get_or_create_security_groups_egress_reraises_non_duplicate_error( + mock_aws_services, +): + """ + A non-duplicate `ClientError` on the egress-rule call must propagate. + """ + + vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] + clients.ec2_client.create_security_group( + GroupName="test-cluster_EKS_workers_sg", Description="workers", VpcId=vpc_id + ) + + with patch.object( + clients.ec2_client, + "authorize_security_group_egress", + side_effect=_client_error( + "InvalidGroup.NotFound", "gone", "AuthorizeSecurityGroupEgress" + ), + ): + with pytest.raises(ClientError) as exc_info: + s3_files._get_or_create_security_groups(vpc_id=vpc_id) + + assert exc_info.value.response["Error"]["Code"] == "InvalidGroup.NotFound" + + +# --------------------------------------------------------------------------- # +# _get_or_create_s3_files_bucket_role +# --------------------------------------------------------------------------- # + + +def test_get_or_create_s3_files_bucket_role_creates_new_role(mock_aws_services): + bucket_name = "test-bucket" + region = "us-east-1" + + with patch.object( + clients.iam_client, + "create_role", + wraps=clients.iam_client.create_role, + ) as create_role_spy, patch.object( + clients.iam_client, + "put_role_policy", + wraps=clients.iam_client.put_role_policy, + ) as put_policy_spy: + role_arn = s3_files._get_or_create_s3_files_bucket_role( + bucket_name=bucket_name, region=region + ) + + assert role_arn.endswith(f"role/{bucket_name}-s3files-role") + create_role_spy.assert_called_once() + _, create_role_kwargs = create_role_spy.call_args + assert create_role_kwargs["RoleName"] == f"{bucket_name}-s3files-role" + + trust_policy = json.loads(create_role_kwargs["AssumeRolePolicyDocument"]) + assert trust_policy["Statement"][0]["Principal"] == { + "Service": "elasticfilesystem.amazonaws.com" + } + assert trust_policy["Statement"][0]["Action"] == "sts:AssumeRole" + + put_policy_spy.assert_called_once() + _, put_policy_kwargs = put_policy_spy.call_args + assert put_policy_kwargs["RoleName"] == f"{bucket_name}-s3files-role" + assert put_policy_kwargs["PolicyName"] == "S3FilesBucketAccess" + + inline_policy = json.loads(put_policy_kwargs["PolicyDocument"]) + sids = {statement["Sid"] for statement in inline_policy["Statement"]} + # the KMS statement is included unconditionally, regardless of whether KMS + # encryption is enabled for this deployment + assert "UseKmsKeyWithS3Files" in sids + assert "S3BucketPermissions" in sids + assert "S3ObjectPermissions" in sids + + +def test_get_or_create_s3_files_bucket_role_returns_existing_role( + mock_aws_services, +): + """ + If the role already exists, it is returned as-is and no new role/policy is + created. + """ + bucket_name = "test-bucket" + role_name = f"{bucket_name}-s3files-role" + existing_role = clients.iam_client.create_role( + RoleName=role_name, AssumeRolePolicyDocument="{}" + ) + expected_arn = existing_role["Role"]["Arn"] + + with patch.object( + clients.iam_client, + "create_role", + wraps=clients.iam_client.create_role, + ) as create_role_spy, patch.object( + clients.iam_client, + "put_role_policy", + wraps=clients.iam_client.put_role_policy, + ) as put_policy_spy: + role_arn = s3_files._get_or_create_s3_files_bucket_role( + bucket_name=bucket_name, region="us-east-1" + ) + + assert role_arn == expected_arn + create_role_spy.assert_not_called() + put_policy_spy.assert_not_called() + + +def test_get_or_create_s3_files_bucket_role_client_error_reraises( + mock_aws_services, +): + with patch.object( + clients.iam_client, + "create_role", + side_effect=_client_error("ServiceFailure", "boom", "CreateRole"), + ): + with pytest.raises(ClientError): + s3_files._get_or_create_s3_files_bucket_role( + bucket_name="test-bucket", region="us-east-1" + ) + + +# --------------------------------------------------------------------------- # +# wait_for_file_system_ready +# --------------------------------------------------------------------------- # + + +def test_wait_for_file_system_ready_already_available(): + with patch.object( + s3_files, "get_filesystem_status", return_value=("available", None) + ), patch.object(time, "sleep") as sleep_mock: + s3_files.wait_for_file_system_ready("fs-123") + + sleep_mock.assert_not_called() + + +def test_wait_for_file_system_ready_polls_until_available(): + statuses = [("creating", None), ("updating", None), ("available", None)] + + with patch.object( + s3_files, "get_filesystem_status", side_effect=statuses + ) as get_status_mock, patch.object(time, "sleep") as sleep_mock: + s3_files.wait_for_file_system_ready("fs-123") + + assert get_status_mock.call_count == 3 + assert sleep_mock.call_count == 2 + + +def test_wait_for_file_system_ready_logs_status_message_while_polling(): + """ + While still `creating`/`updating`, a non-empty status message (`reason`) is + logged on each poll rather than being dropped. + """ + statuses = [ + ("creating", None), # initial fetch, before the loop -- reason not checked + ("creating", "provisioning underlying resources"), # fetched inside the loop + ("available", None), + ] + + with patch.object( + s3_files, "get_filesystem_status", side_effect=statuses + ), patch.object(time, "sleep"), patch.object( + s3_files.logger, "debug" + ) as debug_mock: + s3_files.wait_for_file_system_ready("fs-123") + + assert any( + "provisioning underlying resources" in call_args.args[0] + for call_args in debug_mock.call_args_list + ) + + +def test_wait_for_file_system_ready_raises_on_failure(): + with patch.object( + s3_files, + "get_filesystem_status", + return_value=("error", "something went wrong"), + ), patch.object(time, "sleep"): + with pytest.raises(Exception, match="something went wrong"): + s3_files.wait_for_file_system_ready("fs-123") + + +# --------------------------------------------------------------------------- # +# setup_s3_filesystem (orchestration) +# --------------------------------------------------------------------------- # + + +def test_setup_s3_filesystem_orchestrates_role_and_filesystem_creation(): + with patch.object( + s3_files, + "_get_or_create_s3_files_bucket_role", + return_value="arn:aws:iam::123456789012:role/test-bucket-s3files-role", + ) as get_role_mock, patch.object( + s3_files, "_create_s3_files_system", return_value="fs-new" + ) as create_fs_mock: + result = s3_files.setup_s3_filesystem("test-bucket") + + assert result == "fs-new" + get_role_mock.assert_called_once_with( + bucket_name="test-bucket", region=config["USER_BUCKETS_REGION"] + ) + create_fs_mock.assert_called_once_with( + "test-bucket", + role_arn="arn:aws:iam::123456789012:role/test-bucket-s3files-role", + ) + + +# --------------------------------------------------------------------------- # +# provision_mount_targets (orchestration) +# --------------------------------------------------------------------------- # + + +def test_provision_mount_targets_creates_missing_and_skips_existing(): + """ + One AZ already has a mount target and should be skipped; the other AZ is + missing one and a mount target should be created for it. + """ + az_to_subnet = {"az-1": "subnet-1", "az-2": "subnet-2"} + existing_mount_targets = [{"availabilityZoneId": "az-1", "mountTargetId": "fsmt-1"}] + + with patch.object( + s3_files, "_get_available_az_to_subnet", return_value=az_to_subnet + ), patch.object(s3_files, "wait_for_file_system_ready") as wait_mock, patch.object( + s3_files, + "list_mount_targets_for_file_system", + return_value=existing_mount_targets, + ) as list_mts_mock, patch.object( + s3_files, "_get_vpc_id", return_value="vpc-abc" + ), patch.object( + s3_files, "_get_or_create_security_groups", return_value="sg-abc" + ) as get_sgs_mock, patch.object( + s3_files, "create_mount_target_for_file_system" + ) as create_mt_mock: + result = s3_files.provision_mount_targets("fs-123") + + assert result == "fs-123" + wait_mock.assert_called_once_with("fs-123") + list_mts_mock.assert_called_once_with("fs-123") + get_sgs_mock.assert_called_once_with(vpc_id="vpc-abc") + + # only az-2 (missing a mount target) should trigger a create call + create_mt_mock.assert_called_once_with( + file_system_id="fs-123", subnet_id="subnet-2", mount_target_sg_id="sg-abc" + ) + + +def test_provision_mount_targets_no_existing_mount_targets(): + """ + When no mount targets exist yet, one is created per available AZ/subnet. + """ + az_to_subnet = {"az-1": "subnet-1", "az-2": "subnet-2"} + + with patch.object( + s3_files, "_get_available_az_to_subnet", return_value=az_to_subnet + ), patch.object(s3_files, "wait_for_file_system_ready"), patch.object( + s3_files, "list_mount_targets_for_file_system", return_value=[] + ), patch.object( + s3_files, "_get_vpc_id", return_value="vpc-abc" + ), patch.object( + s3_files, "_get_or_create_security_groups", return_value="sg-abc" + ), patch.object( + s3_files, "create_mount_target_for_file_system" + ) as create_mt_mock: + s3_files.provision_mount_targets("fs-123") + + assert create_mt_mock.call_count == 2 + create_mt_mock.assert_any_call( + file_system_id="fs-123", subnet_id="subnet-1", mount_target_sg_id="sg-abc" + ) + create_mt_mock.assert_any_call( + file_system_id="fs-123", subnet_id="subnet-2", mount_target_sg_id="sg-abc" + ) + + +def test_provision_mount_targets_all_azs_already_covered(): + """ + When every AZ already has a mount target, no new mount targets are created. + """ + az_to_subnet = {"az-1": "subnet-1"} + existing_mount_targets = [{"availabilityZoneId": "az-1", "mountTargetId": "fsmt-1"}] + + with patch.object( + s3_files, "_get_available_az_to_subnet", return_value=az_to_subnet + ), patch.object(s3_files, "wait_for_file_system_ready"), patch.object( + s3_files, + "list_mount_targets_for_file_system", + return_value=existing_mount_targets, + ), patch.object( + s3_files, "_get_vpc_id", return_value="vpc-abc" + ), patch.object( + s3_files, "_get_or_create_security_groups", return_value="sg-abc" + ), patch.object( + s3_files, "create_mount_target_for_file_system" + ) as create_mt_mock: + result = s3_files.provision_mount_targets("fs-123") + + create_mt_mock.assert_not_called() + assert result == "fs-123" From 3dd0630bad02c0c4467240d8e42c2c6a0b0b50d2 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 29 Jul 2026 14:04:27 -0500 Subject: [PATCH 23/33] Add fixes related to review comments --- gen3workflow/aws/bucket.py | 26 ++++---- gen3workflow/aws/s3_files.py | 106 +++++++++++++++++++++++++------ gen3workflow/routes/ga4gh_tes.py | 1 - gen3workflow/routes/storage.py | 12 ++-- tests/test_misc.py | 4 ++ tests/test_s3_files.py | 41 ++++++------ 6 files changed, 131 insertions(+), 59 deletions(-) diff --git a/gen3workflow/aws/bucket.py b/gen3workflow/aws/bucket.py index 75429f9..9d60e0c 100644 --- a/gen3workflow/aws/bucket.py +++ b/gen3workflow/aws/bucket.py @@ -1,12 +1,12 @@ import json import random -from cachelib import SimpleCache from typing import Tuple, Union import asyncio from botocore.exceptions import ClientError from fastapi import HTTPException from starlette.status import HTTP_400_BAD_REQUEST +from cachelib import SimpleCache from gen3workflow import logger from gen3workflow.aws.aws_utils import ( @@ -19,16 +19,12 @@ from gen3workflow.aws import clients -# ( -# eks_client, -# sts_client, -# clients.iam_client, -# clients.kms_client, -# clients.s3_client, -# ) - USER_BUCKET_CACHE = SimpleCache(default_timeout=config["USER_BUCKET_CACHE_SECONDS"]) +# Arbitrarily set expiration of older versions of bucket objects, +# required when bucket versioning is enabled. +NONCURRENT_VERSION_EXPIRATION_DAYS = 3 + def get_existing_kms_key_for_bucket(bucket_name: str) -> Tuple[str, str]: """ @@ -333,7 +329,7 @@ def enable_bucket_versioning(bucket_name: str) -> None: raise -async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: +async def _create_user_bucket(user_id: str) -> Tuple[str, str]: """ Create an S3 bucket for the specified user and return information about the bucket. @@ -341,7 +337,7 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: user_id (str): The user's unique Gen3 ID Returns: - tuple: (bucket name, kms key ARN, S3 files filesystem ID) + tuple: (bucket name, kms key ARN) """ user_bucket_name = get_bucket_name_from_user_id(user_id) try: @@ -381,6 +377,7 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: logger.info(f"Created S3 bucket '{user_bucket_name}' for user '{user_id}'") expiration_days = config["S3_OBJECTS_EXPIRATION_DAYS"] + logger.debug(f"Setting bucket objects expiration to {expiration_days} days") clients.s3_client.put_bucket_lifecycle_configuration( Bucket=user_bucket_name, @@ -389,6 +386,9 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: { "ID": f"ExpireAllAfter{expiration_days}Days", "Expiration": {"Days": expiration_days}, + "NoncurrentVersionExpiration": { + "NoncurrentDays": NONCURRENT_VERSION_EXPIRATION_DAYS + }, "Status": "Enabled", # apply to all objects: "Filter": {"Prefix": ""}, @@ -409,6 +409,10 @@ async def _create_user_bucket(user_id: str) -> Tuple[str, str, str]: clients.s3_client.delete_bucket_encryption(Bucket=user_bucket_name) clients.s3_client.delete_bucket_policy(Bucket=user_bucket_name) + if config["ENABLE_S3_FILES"]: + # Bucket versioning is necessary for S3Files + enable_bucket_versioning(user_bucket_name) + return user_bucket_name, kms_key_arn diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 0a2ee4a..d5e1397 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -1,8 +1,11 @@ +from typing import List + from botocore.exceptions import ClientError import time import json from gen3workflow import logger +from gen3workflow.aws.aws_utils import get_safe_name_from_hostname from gen3workflow.config import config from gen3workflow.aws import clients @@ -48,6 +51,10 @@ def get_s3_files_system(bucket_name: str) -> str | None: return None +class FileSystemNotFoundError(Exception): + """Raised when the file system does not exist.""" + + def get_filesystem_status(file_system_id: str) -> tuple[str | None, str | None]: """ Fetch the status of an S3 Files file system. @@ -59,24 +66,32 @@ def get_filesystem_status(file_system_id: str) -> tuple[str | None, str | None]: A tuple of (status, status_message): - (status, status_message) on success, e.g. ("AVAILABLE", None) - (None, error_message) if the file system doesn't exist or the - lookup fails. + lookup fails. + + Raises: Exception if file system is not available or could not be retrieved. """ if not file_system_id: - return None, "file_system_id must not be empty" + raise ValueError("file_system_id must not be empty") try: fs = clients.s3files_client.get_file_system(fileSystemId=file_system_id) + return fs.get("status"), fs.get("statusMessage") except clients.s3files_client.exceptions.ResourceNotFoundException: - return None, f"File system with file_system_id={file_system_id} does not exist" + logger.debug(f"File system with file_system_id={file_system_id} does not exist") + raise FileSystemNotFoundError( + f"File system with file_system_id={file_system_id} does not exist" + ) except ClientError as e: - return None, f"Failed to fetch file system {file_system_id}: {e}" - - return fs.get("status"), fs.get("statusMessage") + logger.debug( + f"Client error while retrieving file system with file_system_id={file_system_id} : {e}" + ) + raise def get_mount_target_status(file_system_id: str): """ - # TODO: list mount targets and get their statuses, and unify into a single + # TODO: -- https://ctds-planx.atlassian.net/browse/MIDRC-1321 + # list mount targets and get their statuses, and unify into a single # usable status (e.g. "ready" only once all expected mount targets # are in an available state). @@ -90,7 +105,8 @@ def get_mount_target_status(file_system_id: str): def get_s3files_setup_status(filesystem_id): """ - #TODO: This orchestrates both filesystem status and mount target statuses + #TODO: -- https://ctds-planx.atlassian.net/browse/MIDRC-1321 + # This orchestrates both filesystem status and mount target statuses and informs whether or not this storage setup is ready to use or not. """ fs_status = get_filesystem_status(file_system_id=filesystem_id) @@ -118,9 +134,15 @@ def _create_s3_files_system(bucket_name: str, role_arn: str) -> str: try: response = clients.s3files_client.create_file_system( bucket=bucket_arn, + # prefix as configured in the Funnel worker PV prefix="funnel-temp-files/", roleArn=role_arn, - tags=[{"key": "app-name", "value": "gen3-workflow"}], + tags=[ + { + "Key": "Name", + "Value": get_safe_name_from_hostname(user_id=None), + } + ], ) file_system_id = response["fileSystemId"] logger.debug( @@ -226,9 +248,9 @@ def _get_available_az_to_subnet(discovery_tag: str) -> dict[str, str]: return {subnet["AvailabilityZoneId"]: subnet["SubnetId"] for subnet in subnets} -def _get_eks_security_groups() -> tuple[str, str]: +def _get_eks_security_groups() -> List[tuple[str, str]]: """ - Return (sg_id, sg_name) for the EKS security group that Karpenter attaches + Return a list of tuples consisting of (sg_id, sg_name) for the EKS security group that Karpenter attaches to an EKS worker node in this cluster. """ # TODO: Determine if these need to be configurable. @@ -492,10 +514,22 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: RoleName=role_name, AssumeRolePolicyDocument=json.dumps(trust_policy), Description=f"Role assumed by S3 Files to sync with bucket '{bucket_name}'", - Tags=[{"Key": "app-name", "Value": "gen3-workflow"}], + Tags=[ + { + "Key": "Name", + "Value": get_safe_name_from_hostname(user_id=None), + } + ], ) role_arn = response["Role"]["Arn"] + except ClientError as e: + logger.error( + f"Failed to create IAM role for bucket '{bucket_name}': {e.response['Error']['Message']}" + ) + raise + + try: clients.iam_client.put_role_policy( RoleName=role_name, PolicyName="S3FilesBucketAccess", @@ -507,7 +541,7 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: return role_arn except ClientError as e: logger.error( - f"Failed to create IAM role for bucket '{bucket_name}': {e.response['Error']['Message']}" + f"Failed to put IAM policy for role {role_arn} for bucket '{bucket_name}': {e.response['Error']['Message']}" ) raise @@ -519,22 +553,54 @@ def _get_or_create_s3_files_bucket_role(bucket_name: str, region: str) -> str: # ---------- -def wait_for_file_system_ready(fs_id: str): +def wait_for_file_system_ready( + fs_id: str, + timeout_seconds: int = 600, + poll_interval_seconds: int = 2, + max_consecutive_failure_tolerance: int = 3, +): """ Waits for file system to be ready. Raises: Exception if file system could not be created. """ - fs_status, reason = get_filesystem_status(fs_id) - logger.debug(f"Waiting for Filesystem: `{fs_id}`to be ready.") - while fs_status in ["creating", "updating"]: - # TODO: Make it async - time.sleep(2) - fs_status, reason = get_filesystem_status(fs_id) + + start_time = time.monotonic() + consecutive_failures = 0 + + while True: + elapsed = time.monotonic() - start_time + if elapsed > timeout_seconds: + raise TimeoutError( + f"Timed out after {elapsed:.0f}s waiting for filesystem `{fs_id}` " + f"to become ready (last status: {fs_status!r}, reason: {reason!r})." + ) + try: + fs_status, reason = get_filesystem_status(fs_id) + consecutive_failures = 0 + except FileSystemNotFoundError as e: + # Not a transient error so not continuing to poll. + raise + except Exception as e: + consecutive_failures += 1 + logger.debug( + f"Lookup failed for filesystem `{fs_id}` " + f"({consecutive_failures}/{max_consecutive_failure_tolerance}): {e}" + ) + if consecutive_failures >= max_consecutive_failure_tolerance: + raise + + if fs_status not in ["creating", "updating"]: + break + + logger.debug(f"Waiting for Filesystem `{fs_id}`to be ready.") + time.sleep(poll_interval_seconds) + if reason: logger.debug( f"Filesystem `{fs_id}` is currently in {fs_status=} with {reason=}" ) + if fs_status != "available": raise Exception(f"Failed to create file system.\nReason: {reason}") logger.debug(f"Filesystem: `{fs_id}` ready.") diff --git a/gen3workflow/routes/ga4gh_tes.py b/gen3workflow/routes/ga4gh_tes.py index 3c3c4a9..0f783ae 100644 --- a/gen3workflow/routes/ga4gh_tes.py +++ b/gen3workflow/routes/ga4gh_tes.py @@ -151,7 +151,6 @@ async def create_task(request: Request, auth=Depends(Auth)) -> dict: logger.error(f"{err_msg}. Allowed images: {config['TASK_IMAGE_WHITELIST']}") raise HTTPException(HTTP_403_FORBIDDEN, err_msg) - # TODO: For S3Files based deployments, verify if there is atleast one mount target that is available for the filesystem # Add internal tags if "tags" not in body: body["tags"] = {} diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index ad366b8..cbf2f2d 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -58,22 +58,18 @@ async def storage_setup( } if config["ENABLE_S3_FILES"]: - - # Bucket versioning is necessary for S3Files - enable_bucket_versioning(bucket_name) - fs_id = s3_files.get_s3_files_system(bucket_name) if not fs_id: # 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 continue with the rest of the steps asynchronously background_tasks.add_task(s3_files.provision_mount_targets, fs_id) - filesystem_status = s3_files.get_s3files_setup_status(fs_id) - storage_info["s3files_filesystem_id"] = fs_id - storage_info["status"] = filesystem_status + # TODO: Tracked in ticket -- https://ctds-planx.atlassian.net/browse/MIDRC-1321 + # filesystem_status = s3_files.get_s3files_setup_status(fs_id) + # storage_info["status"] = filesystem_status try: await auth.grant_user_access_to_their_own_data( diff --git a/tests/test_misc.py b/tests/test_misc.py index fb7c055..d0ab5a3 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -221,12 +221,16 @@ async def test_storage_setup( lifecycle_config = clients.s3_client.get_bucket_lifecycle_configuration( Bucket=expected_bucket_name ) + assert lifecycle_config.get("Rules") == [ { "Expiration": {"Days": config["S3_OBJECTS_EXPIRATION_DAYS"]}, "ID": f"ExpireAllAfter{config['S3_OBJECTS_EXPIRATION_DAYS']}Days", "Filter": {"Prefix": ""}, "Status": "Enabled", + "NoncurrentVersionExpiration": { + "NoncurrentDays": bucket.NONCURRENT_VERSION_EXPIRATION_DAYS + }, } ] diff --git a/tests/test_s3_files.py b/tests/test_s3_files.py index efb4e8c..36d1f6b 100644 --- a/tests/test_s3_files.py +++ b/tests/test_s3_files.py @@ -124,14 +124,16 @@ def test_get_filesystem_status_empty_id(empty_id, mock_aws_services): """ An empty/`None` file_system_id short-circuits without calling AWS. """ - status, message = s3_files.get_filesystem_status(empty_id) + with pytest.raises(ValueError): + status, message = s3_files.get_filesystem_status(empty_id) - assert status is None - assert message == "file_system_id must not be empty" clients.s3files_client.get_file_system.assert_not_called() def test_get_filesystem_status_success(mock_aws_services): + """ + Happy path for getting file system status + """ clients.s3files_client.get_file_system.return_value = { "status": "available", "statusMessage": None, @@ -148,17 +150,13 @@ def test_get_filesystem_status_success(mock_aws_services): def test_get_filesystem_status_not_found(mock_aws_services): """ - A `ResourceNotFoundException` is translated into a `(None, )` tuple - instead of propagating. + A `ResourceNotFoundException` is translated into a `FileSystemNotFoundError` and propogated. """ clients.s3files_client.get_file_system.side_effect = ( S3FilesResourceNotFoundException() ) - - status, message = s3_files.get_filesystem_status("fs-missing") - - assert status is None - assert message == "File system with file_system_id=fs-missing does not exist" + with pytest.raises(s3_files.FileSystemNotFoundError): + status, message = s3_files.get_filesystem_status("fs-missing") def test_get_filesystem_status_generic_client_error(mock_aws_services): @@ -169,12 +167,8 @@ def test_get_filesystem_status_generic_client_error(mock_aws_services): clients.s3files_client.get_file_system.side_effect = _client_error( "InternalError", "server exploded", "GetFileSystem" ) - - status, message = s3_files.get_filesystem_status("fs-123") - - assert status is None - assert "Failed to fetch file system fs-123" in message - assert "server exploded" in message + with pytest.raises(ClientError): + status, message = s3_files.get_filesystem_status("fs-123") # --------------------------------------------------------------------------- # @@ -187,7 +181,10 @@ def test_get_filesystem_status_generic_client_error(mock_aws_services): # --------------------------------------------------------------------------- # -def test_get_mount_target_status_calls_list_and_returns_not_ready(): +def test_get_mount_target_status(): + """ + Test verifies whether mount target status is being called + """ with patch.object( s3_files, "list_mount_targets_for_file_system", return_value=[] ) as list_mts: @@ -197,7 +194,10 @@ def test_get_mount_target_status_calls_list_and_returns_not_ready(): assert result == "Not ready" -def test_get_s3files_setup_status_calls_dependencies_and_returns_not_ready(): +def test_get_s3files_setup_status(): + """ + Test verifies whether get s3files setup calls required dependent functions + """ with patch.object( s3_files, "get_filesystem_status", return_value=("available", None) ) as get_fs_status, patch.object( @@ -216,6 +216,9 @@ def test_get_s3files_setup_status_calls_dependencies_and_returns_not_ready(): def test_create_s3_files_system_success(mock_aws_services): + """ + Tests S3Files create file system happy path. + """ clients.s3files_client.create_file_system.return_value = {"fileSystemId": "fs-new"} result = s3_files._create_s3_files_system( @@ -228,7 +231,7 @@ def test_create_s3_files_system_success(mock_aws_services): bucket="arn:aws:s3:::test-bucket", prefix="funnel-temp-files/", roleArn="arn:aws:iam::123456789012:role/s3files-role", - tags=[{"key": "app-name", "value": "gen3-workflow"}], + tags=[{"Key": "Name", "Value": "gen3wf-localhost"}], ) From db7fd5e6ec07355edb7d616cfde7701f958856b4 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 29 Jul 2026 14:59:37 -0500 Subject: [PATCH 24/33] Fix lint errors in unit tests --- tests/test_s3_files.py | 58 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/test_s3_files.py b/tests/test_s3_files.py index 36d1f6b..b4dced0 100644 --- a/tests/test_s3_files.py +++ b/tests/test_s3_files.py @@ -9,6 +9,8 @@ rest of this test suite (see `tests/test_aws_utils.py`). """ +# pylint: disable=protected-access + import json import time from unittest.mock import MagicMock, patch @@ -236,6 +238,9 @@ def test_create_s3_files_system_success(mock_aws_services): def test_create_s3_files_system_client_error_reraises(mock_aws_services): + """ + A `ClientError` while creating the file system is re-raised, not swallowed. + """ clients.s3files_client.create_file_system.side_effect = _client_error( "ValidationException", "bad bucket", "CreateFileSystem" ) @@ -253,6 +258,9 @@ def test_create_s3_files_system_client_error_reraises(mock_aws_services): def test_create_mount_target_for_file_system_success(mock_aws_services): + """ + Tests happy path for creating a mount target for a file system. + """ clients.s3files_client.create_mount_target.return_value = { "mountTargetId": "fsmt-1", "status": "creating", @@ -270,6 +278,9 @@ def test_create_mount_target_for_file_system_success(mock_aws_services): def test_create_mount_target_for_file_system_client_error_reraises( mock_aws_services, ): + """ + A `ClientError` while creating a mount target is re-raised, not swallowed. + """ clients.s3files_client.create_mount_target.side_effect = _client_error( "ValidationException", "bad subnet", "CreateMountTarget" ) @@ -286,6 +297,9 @@ def test_create_mount_target_for_file_system_client_error_reraises( def test_list_mount_targets_for_file_system_flattens_pages(mock_aws_services): + """ + Mount targets spread across multiple pages are flattened into a single list. + """ paginator = MagicMock() paginator.paginate.return_value = [ {"mountTargets": [{"mountTargetId": "fsmt-1"}]}, @@ -301,6 +315,9 @@ def test_list_mount_targets_for_file_system_flattens_pages(mock_aws_services): def test_list_mount_targets_for_file_system_empty(mock_aws_services): + """ + An empty page of mount targets results in an empty list. + """ paginator = MagicMock() paginator.paginate.return_value = [{"mountTargets": []}] clients.s3files_client.get_paginator.return_value = paginator @@ -311,6 +328,9 @@ def test_list_mount_targets_for_file_system_empty(mock_aws_services): def test_list_mount_targets_for_file_system_client_error_reraises( mock_aws_services, ): + """ + A `ClientError` while listing mount targets is re-raised, not swallowed. + """ clients.s3files_client.get_paginator.side_effect = _client_error( "InternalError", "boom", "ListMountTargets" ) @@ -330,6 +350,9 @@ def test_list_mount_targets_for_file_system_client_error_reraises( def test_get_vpc_id(): + """ + The VPC ID is read from the EKS cluster's `resourcesVpcConfig`. + """ fake_eks_client = MagicMock() fake_eks_client.describe_cluster.return_value = { "cluster": {"resourcesVpcConfig": {"vpcId": "vpc-abc123"}} @@ -350,6 +373,9 @@ def test_get_vpc_id(): def test_get_available_az_to_subnet(mock_aws_services): + """ + Only subnets tagged for discovery are returned, mapped by availability zone. + """ # Create dummy VPC through moto with subnets and tags vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] @@ -376,6 +402,9 @@ def test_get_available_az_to_subnet(mock_aws_services): def test_get_available_az_to_subnet_no_matches(mock_aws_services): + """ + No subnets tagged for discovery results in an empty mapping. + """ result = s3_files._get_available_az_to_subnet(discovery_tag="nonexistent-tag") assert result == {} @@ -386,6 +415,9 @@ def test_get_available_az_to_subnet_no_matches(mock_aws_services): def test_get_eks_security_groups(mock_aws_services): + """ + Only security groups matching the EKS worker/nodepool naming convention are returned. + """ vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] @@ -409,6 +441,9 @@ def test_get_eks_security_groups(mock_aws_services): def test_get_eks_security_groups_none_found(mock_aws_services): + """ + No matching security groups results in an empty list. + """ assert s3_files._get_eks_security_groups() == [] @@ -418,6 +453,10 @@ def test_get_eks_security_groups_none_found(mock_aws_services): def test_get_or_create_security_groups_creates_new_sg(mock_aws_services): + """ + When no mount target security group exists yet, one is created with the + expected ingress/egress NFS rules. + """ vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] compute_sg = clients.ec2_client.create_security_group( GroupName="test-cluster_EKS_workers_sg", Description="workers", VpcId=vpc_id @@ -561,6 +600,10 @@ def test_get_or_create_security_groups_egress_reraises_non_duplicate_error( def test_get_or_create_s3_files_bucket_role_creates_new_role(mock_aws_services): + """ + When no bucket role exists yet, one is created with the expected trust + policy and inline access policy. + """ bucket_name = "test-bucket" region = "us-east-1" @@ -637,6 +680,9 @@ def test_get_or_create_s3_files_bucket_role_returns_existing_role( def test_get_or_create_s3_files_bucket_role_client_error_reraises( mock_aws_services, ): + """ + A `ClientError` while creating the bucket role is re-raised, not swallowed. + """ with patch.object( clients.iam_client, "create_role", @@ -654,6 +700,9 @@ def test_get_or_create_s3_files_bucket_role_client_error_reraises( def test_wait_for_file_system_ready_already_available(): + """ + If the file system is already available, no polling/sleeping occurs. + """ with patch.object( s3_files, "get_filesystem_status", return_value=("available", None) ), patch.object(time, "sleep") as sleep_mock: @@ -663,6 +712,9 @@ def test_wait_for_file_system_ready_already_available(): def test_wait_for_file_system_ready_polls_until_available(): + """ + The function polls (sleeping between calls) until the file system becomes available. + """ statuses = [("creating", None), ("updating", None), ("available", None)] with patch.object( @@ -699,6 +751,9 @@ def test_wait_for_file_system_ready_logs_status_message_while_polling(): def test_wait_for_file_system_ready_raises_on_failure(): + """ + An `error` status causes the wait to raise, surfacing the failure reason. + """ with patch.object( s3_files, "get_filesystem_status", @@ -714,6 +769,9 @@ def test_wait_for_file_system_ready_raises_on_failure(): def test_setup_s3_filesystem_orchestrates_role_and_filesystem_creation(): + """ + The bucket role is created/fetched first, then used to create the file system. + """ with patch.object( s3_files, "_get_or_create_s3_files_bucket_role", From 44e840634abbc4bcb419dbc59591ef3b0084cd0e Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 29 Jul 2026 17:37:53 -0500 Subject: [PATCH 25/33] Fix more PR suggestions and add more unit tests --- gen3workflow/aws/s3_files.py | 19 ++++----- tests/test_misc.py | 5 +++ tests/test_s3_files.py | 77 ++++++++++++++++++++++++++++++++---- 3 files changed, 84 insertions(+), 17 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index d5e1397..c685d4a 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -567,16 +567,17 @@ def wait_for_file_system_ready( start_time = time.monotonic() consecutive_failures = 0 + fs_status = None + statusMessage = None while True: elapsed = time.monotonic() - start_time if elapsed > timeout_seconds: - raise TimeoutError( - f"Timed out after {elapsed:.0f}s waiting for filesystem `{fs_id}` " - f"to become ready (last status: {fs_status!r}, reason: {reason!r})." - ) + timeout_msg = f"Timed out after {elapsed:.0f}s waiting for filesystem `{fs_id}` to become ready (last status: {fs_status!r}, reason: {statusMessage!r})." + logger.debug(timeout_msg) + raise TimeoutError(timeout_msg) try: - fs_status, reason = get_filesystem_status(fs_id) + fs_status, statusMessage = get_filesystem_status(fs_id) consecutive_failures = 0 except FileSystemNotFoundError as e: # Not a transient error so not continuing to poll. @@ -590,19 +591,19 @@ def wait_for_file_system_ready( if consecutive_failures >= max_consecutive_failure_tolerance: raise - if fs_status not in ["creating", "updating"]: + if fs_status and fs_status not in ["creating", "updating"]: break logger.debug(f"Waiting for Filesystem `{fs_id}`to be ready.") time.sleep(poll_interval_seconds) - if reason: + if statusMessage: logger.debug( - f"Filesystem `{fs_id}` is currently in {fs_status=} with {reason=}" + f"Filesystem `{fs_id}` is currently in {fs_status=} with {statusMessage=}" ) if fs_status != "available": - raise Exception(f"Failed to create file system.\nReason: {reason}") + raise Exception(f"Failed to create file system.\nReason: {statusMessage}") logger.debug(f"Filesystem: `{fs_id}` ready.") diff --git a/tests/test_misc.py b/tests/test_misc.py index d0ab5a3..c9d92ca 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -50,6 +50,11 @@ def __init__(self, message: str = "File system not found"): def mock_aws_services(): """ Mock all AWS services + + S3 Files is a very new AWS service. As of this writing, `moto` has no mock backend + for it, so calls through `s3files_client` are stubbed out by hand with + `unittest.mock` rather than relying on `moto.mock_aws`. + All the *other* AWS services (`iam`, `ec2`, `eks`, `sts`) are mocked with `moto`. """ with mock_aws(): clients.iam_client = boto3.client("iam") diff --git a/tests/test_s3_files.py b/tests/test_s3_files.py index b4dced0..808e418 100644 --- a/tests/test_s3_files.py +++ b/tests/test_s3_files.py @@ -1,12 +1,5 @@ """ Unit tests for `gen3workflow.aws.s3_files`. - -S3 Files is a very new AWS service. As of this writing, `moto` has no mock backend -for it, so calls through `s3files_client` are stubbed out by hand with -`unittest.mock` (see the `mock_s3_files_aws_clients` fixture in `conftest.py`) -rather than relying on `moto.mock_aws`. All the *other* AWS services used by this -module (`iam`, `ec2`, `eks`, `sts`) are mocked with `moto`, consistent with the -rest of this test suite (see `tests/test_aws_utils.py`). """ # pylint: disable=protected-access @@ -152,7 +145,7 @@ def test_get_filesystem_status_success(mock_aws_services): def test_get_filesystem_status_not_found(mock_aws_services): """ - A `ResourceNotFoundException` is translated into a `FileSystemNotFoundError` and propogated. + A `ResourceNotFoundException` is translated into a `FileSystemNotFoundError` and propagated. """ clients.s3files_client.get_file_system.side_effect = ( S3FilesResourceNotFoundException() @@ -763,6 +756,74 @@ def test_wait_for_file_system_ready_raises_on_failure(): s3_files.wait_for_file_system_ready("fs-123") +def test_wait_for_file_system_ready_file_system_not_found_reraises_immediately(): + """ + A `FileSystemNotFoundError` is not treated as transient; it's re-raised on + the very first failure, without retrying or sleeping. + """ + with patch.object( + s3_files, + "get_filesystem_status", + side_effect=s3_files.FileSystemNotFoundError("fs not found"), + ) as get_status_mock, patch.object(time, "sleep") as sleep_mock: + with pytest.raises(s3_files.FileSystemNotFoundError): + s3_files.wait_for_file_system_ready("fs-123") + + get_status_mock.assert_called_once_with("fs-123") + sleep_mock.assert_not_called() + + +def test_wait_for_file_system_ready_recovers_from_transient_failure(): + """ + A transient (non-`FileSystemNotFoundError`) exception below the failure + tolerance is retried rather than raised, and polling continues until the + file system becomes available. + """ + statuses = [Exception("transient blip"), ("available", None)] + + with patch.object( + s3_files, "get_filesystem_status", side_effect=statuses + ) as get_status_mock, patch.object(time, "sleep") as sleep_mock: + s3_files.wait_for_file_system_ready("fs-123") + + assert get_status_mock.call_count == 2 + assert sleep_mock.call_count == 1 + + +def test_wait_for_file_system_ready_raises_after_exceeding_failure_tolerance(): + """ + Once consecutive transient failures reach `max_consecutive_failure_tolerance`, + the underlying exception is raised instead of continuing to retry. + """ + with patch.object( + s3_files, + "get_filesystem_status", + side_effect=Exception("boom"), + ) as get_status_mock, patch.object(time, "sleep") as sleep_mock: + with pytest.raises(Exception, match="boom"): + s3_files.wait_for_file_system_ready( + "fs-123", max_consecutive_failure_tolerance=2 + ) + + assert get_status_mock.call_count == 2 + assert sleep_mock.call_count == 1 + + +def test_wait_for_file_system_ready_times_out(): + """ + If the elapsed time exceeds `timeout_seconds`, a `TimeoutError` is raised + without ever calling `get_filesystem_status`. + """ + with patch.object(time, "monotonic", side_effect=[0, 5]), patch.object( + s3_files, "get_filesystem_status" + ) as get_status_mock, patch.object(time, "sleep") as sleep_mock: + with pytest.raises(TimeoutError, match="Timed out"): + s3_files.wait_for_file_system_ready("fs-123", timeout_seconds=1) + + get_status_mock.assert_not_called() + sleep_mock.assert_not_called() + + # --------------------------------------------------------------------------- # # setup_s3_filesystem (orchestration) # --------------------------------------------------------------------------- # From fed3b31ef40830edcd82dbecba89d2c67d437126 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 29 Jul 2026 17:41:07 -0500 Subject: [PATCH 26/33] Fix lint issues --- gen3workflow/aws/s3_files.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index c685d4a..f0878d3 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -568,16 +568,16 @@ def wait_for_file_system_ready( start_time = time.monotonic() consecutive_failures = 0 fs_status = None - statusMessage = None + status_message = None while True: elapsed = time.monotonic() - start_time if elapsed > timeout_seconds: - timeout_msg = f"Timed out after {elapsed:.0f}s waiting for filesystem `{fs_id}` to become ready (last status: {fs_status!r}, reason: {statusMessage!r})." + timeout_msg = f"Timed out after {elapsed:.0f}s waiting for filesystem `{fs_id}` to become ready (last status: {fs_status!r}, reason: {status_message!r})." logger.debug(timeout_msg) raise TimeoutError(timeout_msg) try: - fs_status, statusMessage = get_filesystem_status(fs_id) + fs_status, status_message = get_filesystem_status(fs_id) consecutive_failures = 0 except FileSystemNotFoundError as e: # Not a transient error so not continuing to poll. @@ -597,13 +597,13 @@ def wait_for_file_system_ready( logger.debug(f"Waiting for Filesystem `{fs_id}`to be ready.") time.sleep(poll_interval_seconds) - if statusMessage: + if status_message: logger.debug( - f"Filesystem `{fs_id}` is currently in {fs_status=} with {statusMessage=}" + f"Filesystem `{fs_id}` is currently in {fs_status=} with {status_message=}" ) if fs_status != "available": - raise Exception(f"Failed to create file system.\nReason: {statusMessage}") + raise Exception(f"Failed to create file system.\nReason: {status_message}") logger.debug(f"Filesystem: `{fs_id}` ready.") From c6d720216a7af6490d0b8b9e32c913bd66b35573 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Wed, 29 Jul 2026 23:59:25 -0500 Subject: [PATCH 27/33] Make security group field configurable and add docs on how to fetch them --- docs/s3Files.md | 26 ++++++++++++++++++++++++++ gen3workflow/aws/s3_files.py | 9 ++++----- gen3workflow/config-default.yaml | 6 ++++++ gen3workflow/config.py | 4 ++++ tests/test_s3_files.py | 10 ++++++++-- 5 files changed, 48 insertions(+), 7 deletions(-) diff --git a/docs/s3Files.md b/docs/s3Files.md index 6a71f9a..8a750dc 100644 --- a/docs/s3Files.md +++ b/docs/s3Files.md @@ -23,3 +23,29 @@ For Helm-based deployments, set: #### NOTE: * Currently Gen3Workflow with S3Files only supports user bucket and the EKS cluster being present in the same region. Support shall be added in the future. + + +### Determining the EKS Security Group Names for S3 Files Setup + +`gen3-workflow` needs the names of the EKS security groups attached to the nodes that run workflow tasks, in order to allow S3 Files mount targets network access. These are currently **not derivable programmatically** and must be looked up manually per cluster, then set as config values. + +**Steps for a Gen3 operator:** + +1. **Identify the node role(s) workflow tasks run on** for this environment. In most clusters this is `role=workflow` (and `role=gpu` if GPU tasks are supported). Check the cluster's `gen3-gitops` `cluster-values.yaml` for how these roles are configured, since role → security-group mapping can differ per environment. + +2. **Find the Karpenter `EC2NodeClass` for each relevant role** in `gen3-helm` under `helm/cluster-level-resources/templates/karpenter-config-resources-.yaml` (e.g. `karpenter-config-resources-workflow.yaml`). Look at the `securityGroupSelectorTerms` block — nodes get whichever security groups carry the matching `karpenter.sh/discovery` tag value (`selectorTag` from Helm values). + +3. **Query AWS for the security groups carrying that discovery tag**, scoped to the cluster's VPC: + ``` + aws ec2 describe-security-groups \ + --filters "Name=tag:karpenter.sh/discovery,Values=" \ + --query "SecurityGroups[].{Name:GroupName,Id:GroupId}" + ``` + +4. **Manually identify the "main" security group** from the results — the one that actually holds the cluster's inbound/outbound networking rules (as opposed to any minimal/placeholder groups also carrying the tag). This step is judgment-based today; usually the one with most inbound and outbound rules. + +5. Repeat for each role that can run workflow tasks. Note: in `devplanetv2`, nodes labeled `role=workflow` carry the same discovery tag as jupyter nodes, so the jupyter security group ends up governing traffic for workflow nodes — check whether your cluster has the same overlap before assuming you only need one SG name. + +6. Set the resulting security group name(s) in the `gen3-workflow` config under the field `EKS_SECURITY_GROUP_NAMES`. + +> **Known limitation:** this is a temporary, manually-maintained workaround. The long-term plan is to move nodepool (and likely security group) creation into `gen3-workflow` itself once per-user nodepools are implemented, at which point this lookup process should be revisited/removed. diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index f0878d3..5fce0b7 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -253,11 +253,10 @@ def _get_eks_security_groups() -> List[tuple[str, str]]: Return a list of tuples consisting of (sg_id, sg_name) for the EKS security group that Karpenter attaches to an EKS worker node in this cluster. """ - # TODO: Determine if these need to be configurable. - eks_sg_names = [ - f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg", - f"{config["EKS_CLUSTER_NAME"]}_EKS_nodepool_jupyter_sg", - ] + eks_sg_names = config.get("EKS_SECURITY_GROUP_NAMES") + if not eks_sg_names: + eks_sg_names = [f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg"] + security_groups = clients.ec2_client.describe_security_groups( Filters=[{"Name": "group-name", "Values": eks_sg_names}] )["SecurityGroups"] diff --git a/gen3workflow/config-default.yaml b/gen3workflow/config-default.yaml index fe19d29..8326c71 100644 --- a/gen3workflow/config-default.yaml +++ b/gen3workflow/config-default.yaml @@ -98,3 +98,9 @@ ENABLE_OPTIMIZED_NODE_SCHEDULING: true EKS_CLUSTER_NAME: "" EKS_CLUSTER_REGION: us-east-1 WORKER_PODS_NAMESPACE: default-namespace + +# Names of the EKS security groups that Karpenter attaches to an EKS worker node in this cluster +# Usually going to be in the lines of f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg" +# Steps to fetch the values are mentioned in the docs/s3Files.md under the section "Determining the EKS Security Group Names for S3 Files Setup" + +EKS_SECURITY_GROUP_NAMES: [] diff --git a/gen3workflow/config.py b/gen3workflow/config.py index abc1d12..61979f6 100644 --- a/gen3workflow/config.py +++ b/gen3workflow/config.py @@ -60,6 +60,10 @@ def validate_top_level_configs(self) -> None: "EKS_CLUSTER_NAME": {"type": "string"}, "EKS_CLUSTER_REGION": {"type": "string"}, "WORKER_PODS_NAMESPACE": {"type": "string"}, + "EKS_SECURITY_GROUP_NAMES": { + "type": "array", + "items": {"type": "string"}, + }, }, } validate(instance=self, schema=schema) diff --git a/tests/test_s3_files.py b/tests/test_s3_files.py index 808e418..6cc952e 100644 --- a/tests/test_s3_files.py +++ b/tests/test_s3_files.py @@ -407,9 +407,9 @@ def test_get_available_az_to_subnet_no_matches(mock_aws_services): # --------------------------------------------------------------------------- # -def test_get_eks_security_groups(mock_aws_services): +def test_get_eks_security_groups(mock_aws_services, monkeypatch): """ - Only security groups matching the EKS worker/nodepool naming convention are returned. + Only security groups matching the EKS_SECURITY_GROUP_NAMES mentioned in the config are returned. """ vpc_id = clients.ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"] @@ -427,6 +427,12 @@ def test_get_eks_security_groups(mock_aws_services): GroupName="unrelated-sg", Description="unrelated", VpcId=vpc_id ) + monkeypatch.setitem( + config, + "EKS_SECURITY_GROUP_NAMES", + ["test-cluster_EKS_workers_sg", "test-cluster_EKS_nodepool_jupyter_sg"], + ) + result = s3_files._get_eks_security_groups() result_ids = {sg_id for sg_id, _ in result} From 1e458032d131516e72aee7c305bd857408e29120 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Thu, 30 Jul 2026 00:01:17 -0500 Subject: [PATCH 28/33] Update the link in config.py --- gen3workflow/config-default.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen3workflow/config-default.yaml b/gen3workflow/config-default.yaml index 8326c71..acda0de 100644 --- a/gen3workflow/config-default.yaml +++ b/gen3workflow/config-default.yaml @@ -101,6 +101,6 @@ WORKER_PODS_NAMESPACE: default-namespace # Names of the EKS security groups that Karpenter attaches to an EKS worker node in this cluster # Usually going to be in the lines of f"{config["EKS_CLUSTER_NAME"]}_EKS_workers_sg" -# Steps to fetch the values are mentioned in the docs/s3Files.md under the section "Determining the EKS Security Group Names for S3 Files Setup" +# Steps to fetch the values are mentioned in the (docs/s3Files.md#determining-the-eks-security-group-names-for-s3-files-setup) EKS_SECURITY_GROUP_NAMES: [] From 6f9c69ed817e79e5e57aefbfd72ff51c3b30b50d Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Thu, 30 Jul 2026 00:34:15 -0500 Subject: [PATCH 29/33] remove TODO comment --- gen3workflow/aws/s3_files.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 5fce0b7..271f300 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -263,9 +263,6 @@ def _get_eks_security_groups() -> List[tuple[str, str]]: return [(group["GroupId"], group["GroupName"]) for group in security_groups] -# TODO: Investigate if this can be moved to server startup logic or elsewhere? Terraform maybe? -# Since the `create` part is only needed once per cluster, no need to run for every bucket. -# This takes approximately 2 seconds to run everytime it is invoked. def _get_or_create_security_groups(vpc_id: str) -> str: """ Ensure the mount target security group exists and has the correct bidirectional From bc4e9b873ec626e2571a7b39814818fbb08d490e Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Thu, 30 Jul 2026 11:07:39 -0500 Subject: [PATCH 30/33] Remove unused imports --- gen3workflow/routes/storage.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gen3workflow/routes/storage.py b/gen3workflow/routes/storage.py index cbf2f2d..f5d4baa 100644 --- a/gen3workflow/routes/storage.py +++ b/gen3workflow/routes/storage.py @@ -10,11 +10,10 @@ from gen3workflow import logger from gen3workflow.auth import Auth -from gen3workflow.aws import aws_utils, s3_files +from gen3workflow.aws import s3_files from gen3workflow.aws.bucket import ( cleanup_user_bucket, create_user_bucket, - enable_bucket_versioning, ) from gen3workflow.config import config From 838564557b3220806f942c2605e058c3f658e340 Mon Sep 17 00:00:00 2001 From: Sai Shanmukha Date: Thu, 30 Jul 2026 13:32:20 -0500 Subject: [PATCH 31/33] S3 files `_create_file_system` hotfix --- gen3workflow/aws/s3_files.py | 4 ++-- tests/test_s3_files.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gen3workflow/aws/s3_files.py b/gen3workflow/aws/s3_files.py index 271f300..35739a3 100644 --- a/gen3workflow/aws/s3_files.py +++ b/gen3workflow/aws/s3_files.py @@ -139,8 +139,8 @@ def _create_s3_files_system(bucket_name: str, role_arn: str) -> str: roleArn=role_arn, tags=[ { - "Key": "Name", - "Value": get_safe_name_from_hostname(user_id=None), + "key": "Name", + "value": get_safe_name_from_hostname(user_id=None), } ], ) diff --git a/tests/test_s3_files.py b/tests/test_s3_files.py index 6cc952e..e4d8aa0 100644 --- a/tests/test_s3_files.py +++ b/tests/test_s3_files.py @@ -226,7 +226,7 @@ def test_create_s3_files_system_success(mock_aws_services): bucket="arn:aws:s3:::test-bucket", prefix="funnel-temp-files/", roleArn="arn:aws:iam::123456789012:role/s3files-role", - tags=[{"Key": "Name", "Value": "gen3wf-localhost"}], + tags=[{"key": "Name", "value": "gen3wf-localhost"}], ) From 26dcbb05dbcd9db8840dabc98fd904c0aad7d056 Mon Sep 17 00:00:00 2001 From: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:31:57 -0500 Subject: [PATCH 32/33] Fix create alias AlreadyExistsException --- gen3workflow/aws/bucket.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gen3workflow/aws/bucket.py b/gen3workflow/aws/bucket.py index 9d60e0c..5a5b9c1 100644 --- a/gen3workflow/aws/bucket.py +++ b/gen3workflow/aws/bucket.py @@ -437,7 +437,8 @@ async def create_user_bucket(user_id: str) -> Tuple[str, str, str]: return bucket_info except ClientError as e: if ( - e.response["Error"]["Code"] != "OperationAborted" + e.response["Error"]["Code"] + not in ["OperationAborted", "AlreadyExistsException"] or attempt == max_tries ): raise From 7f0a7dba44801bcb5f23ce88190ffe7dc433b39a Mon Sep 17 00:00:00 2001 From: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:33:25 -0500 Subject: [PATCH 33/33] Update docstring --- gen3workflow/aws/bucket.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gen3workflow/aws/bucket.py b/gen3workflow/aws/bucket.py index 5a5b9c1..8d7bc9c 100644 --- a/gen3workflow/aws/bucket.py +++ b/gen3workflow/aws/bucket.py @@ -421,8 +421,10 @@ async def create_user_bucket(user_id: str) -> Tuple[str, str, str]: Wrapper for `_create_user_bucket` that handles caching and retries. Gracefully handles race conditions, for example: - `An error occurred (OperationAborted) when calling the PutBucketEncryption operation: - A conflicting conditional operation is currently in progress against this resource.` + - `An error occurred (OperationAborted) when calling the PutBucketEncryption operation: + A conflicting conditional operation is currently in progress against this resource.` + - `An error occurred (AlreadyExistsException) when calling the CreateAlias operation: + An alias with the name XYZ already exists` """ if USER_BUCKET_CACHE.has(user_id): return USER_BUCKET_CACHE.get(user_id)