From be9d3be7d7d2805231b7a4fd47db2d3f25131351 Mon Sep 17 00:00:00 2001 From: Manuel Luypaert Date: Thu, 9 May 2024 10:49:25 +0100 Subject: [PATCH 1/4] Moved Nextflow bucket permissions into managed policy. This managed policy can then be attached to gh-actions role for execution of aws-run integration testing. --- pipeline/aws_infra/cdk_classes/aws_batch.py | 19 ++++++++++++------- .../tests/unit/test_cdk_infra_stack.py | 8 ++++++++ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/pipeline/aws_infra/cdk_classes/aws_batch.py b/pipeline/aws_infra/cdk_classes/aws_batch.py index 30555adb..1623bc70 100644 --- a/pipeline/aws_infra/cdk_classes/aws_batch.py +++ b/pipeline/aws_infra/cdk_classes/aws_batch.py @@ -17,6 +17,7 @@ class PaviExecutionEnvironment: compute_environment: aws_batch.ManagedEc2EcsComputeEnvironment job_queue: aws_batch.JobQueue nf_workdir_bucket: s3.Bucket | s3.IBucket + nf_bucket_access_policy: iam.ManagedPolicy def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Optional[str]) -> None: """ @@ -70,19 +71,23 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option ) ] ) - cdk_tags.of(s3_workdir_bucket_policy_doc).add("Product", "PAVI") - cdk_tags.of(s3_workdir_bucket_policy_doc).add("Managed_by", "PAVI") + s3_workdir_bucket_policy = iam.ManagedPolicy(scope,'pavi-s3-nextflow-bucket-policy', + managed_policy_name='agr-pavi-pipeline-nf-bucket-access', + description='Grant required access to PAVI pipeline nextflow bucket to run nextflow pipelines using it.', + document=s3_workdir_bucket_policy_doc) + cdk_tags.of(s3_workdir_bucket_policy).add("Product", "PAVI") # type: ignore + cdk_tags.of(s3_workdir_bucket_policy).add("Managed_by", "PAVI") # type: ignore + + self.nf_bucket_access_policy = s3_workdir_bucket_policy instance_role = iam.Role(scope, 'pavi-pipeline-compute-environment-instance-role', description='Role granting permissions for Nextflow ECS execution', assumed_by=iam.ServicePrincipal('ec2.amazonaws.com'), # type: ignore managed_policies=[ iam.ManagedPolicy.from_aws_managed_policy_name("service-role/AmazonEC2ContainerServiceforEC2Role"), - iam.ManagedPolicy.from_managed_policy_name(scope, "iam-ecr-read-policy", "ReadOnlyAccessECR") - ], - inline_policies={ - 's3-workdir-policy': s3_workdir_bucket_policy_doc - }) + iam.ManagedPolicy.from_managed_policy_name(scope, "iam-ecr-read-policy", "ReadOnlyAccessECR"), + self.nf_bucket_access_policy + ]) cdk_tags.of(instance_role).add("Product", "PAVI") cdk_tags.of(instance_role).add("Managed_by", "PAVI") diff --git a/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py b/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py index 214414f7..6e1f5e88 100644 --- a/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py +++ b/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py @@ -56,6 +56,14 @@ def test_pipeline_nf_s3_bucket() -> None: } }) +# The S3 bucket policy is assigned to the GH-actions role for PAVI, in addition to PAVI-managed resources. +# Changing the name of this policy might require the gh-actions role to be updated to include the new role. +def test_pipeline_nf_s3_bucket_policy() -> None: + template.has_resource(type=ResourceType.of('AWS::IAM::ManagedPolicy').compliance_resource_type, props={ + "Properties": { + "ManagedPolicyName": "agr-pavi-pipeline-nf-bucket-access" + } + }) # Below tests check for resource that must be available in the stack, # but which can be replaced/renamed without manual interventions other than From dc9fe5b5b2a99b9cb25654cdcafa1a50adb86f96 Mon Sep 17 00:00:00 2001 From: Manuel Luypaert Date: Thu, 9 May 2024 10:51:38 +0100 Subject: [PATCH 2/4] Refactored NF output bucket variable --- pipeline/aws_infra/cdk_classes/aws_batch.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pipeline/aws_infra/cdk_classes/aws_batch.py b/pipeline/aws_infra/cdk_classes/aws_batch.py index 1623bc70..f1dc2596 100644 --- a/pipeline/aws_infra/cdk_classes/aws_batch.py +++ b/pipeline/aws_infra/cdk_classes/aws_batch.py @@ -16,7 +16,7 @@ class PaviExecutionEnvironment: compute_environment: aws_batch.ManagedEc2EcsComputeEnvironment job_queue: aws_batch.JobQueue - nf_workdir_bucket: s3.Bucket | s3.IBucket + nf_output_bucket: s3.Bucket | s3.IBucket nf_bucket_access_policy: iam.ManagedPolicy def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Optional[str]) -> None: @@ -37,7 +37,7 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option bucket_name = 'agr-pavi-pipeline-nextflow' if env_suffix: bucket_name += f'-{env_suffix}' - self.nf_workdir_bucket = s3.Bucket( + self.nf_output_bucket = s3.Bucket( scope=scope, id='pavi-pipeline-nf-workdir-bucket', bucket_name=bucket_name, access_control=s3.BucketAccessControl.PRIVATE, @@ -47,10 +47,10 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option removal_policy=RemovalPolicy.RETAIN, versioned=False ) - cdk_tags.of(self.nf_workdir_bucket).add("Product", "PAVI") - cdk_tags.of(self.nf_workdir_bucket).add("Managed_by", "PAVI") + cdk_tags.of(self.nf_output_bucket).add("Product", "PAVI") # type: ignore + cdk_tags.of(self.nf_output_bucket).add("Managed_by", "PAVI") # type: ignore else: - self.nf_workdir_bucket = s3.Bucket.from_bucket_name( + self.nf_output_bucket = s3.Bucket.from_bucket_name( scope=scope, id='pavi-pipeline-nf-workdir-bucket', bucket_name=shared_work_dir_bucket) @@ -61,13 +61,13 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option sid="S3BucketWriteAll", effect=iam.Effect.ALLOW, actions=['s3:Put*'], - resources=[self.nf_workdir_bucket.bucket_arn + '/*'] + resources=[self.nf_output_bucket.bucket_arn + '/*'] ), iam.PolicyStatement( sid="S3BucketReadAll", effect=iam.Effect.ALLOW, actions=['s3:ListBucket*', 's3:Get*'], - resources=[self.nf_workdir_bucket.bucket_arn, self.nf_workdir_bucket.bucket_arn + '/*'] + resources=[self.nf_output_bucket.bucket_arn, self.nf_output_bucket.bucket_arn + '/*'] ) ] ) From 877a300942af7ddb4410c74780b7631705cb3827 Mon Sep 17 00:00:00 2001 From: Manuel Luypaert Date: Thu, 9 May 2024 10:52:14 +0100 Subject: [PATCH 3/4] Ignore more Resource vs IResource CDK typing mismatches --- pipeline/aws_infra/cdk_classes/aws_batch.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pipeline/aws_infra/cdk_classes/aws_batch.py b/pipeline/aws_infra/cdk_classes/aws_batch.py index f1dc2596..77780d1a 100644 --- a/pipeline/aws_infra/cdk_classes/aws_batch.py +++ b/pipeline/aws_infra/cdk_classes/aws_batch.py @@ -88,8 +88,8 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option iam.ManagedPolicy.from_managed_policy_name(scope, "iam-ecr-read-policy", "ReadOnlyAccessECR"), self.nf_bucket_access_policy ]) - cdk_tags.of(instance_role).add("Product", "PAVI") - cdk_tags.of(instance_role).add("Managed_by", "PAVI") + cdk_tags.of(instance_role).add("Product", "PAVI") # type: ignore + cdk_tags.of(instance_role).add("Managed_by", "PAVI") # type: ignore ce_name = 'pavi_pipeline_ecs' if env_suffix: @@ -105,8 +105,8 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option self.compute_environment.apply_removal_policy(RemovalPolicy.DESTROY) - cdk_tags.of(self.compute_environment).add("Product", "PAVI") - cdk_tags.of(self.compute_environment).add("Managed_by", "PAVI") + cdk_tags.of(self.compute_environment).add("Product", "PAVI") # type: ignore + cdk_tags.of(self.compute_environment).add("Managed_by", "PAVI") # type: ignore self.compute_environment.tags.set_tag('Name', 'PAVI pipeline execution worker', priority=None, apply_to_launched_instances=True) @@ -121,5 +121,5 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option self.job_queue.add_compute_environment(self.compute_environment, order=1) # type: ignore - cdk_tags.of(self.job_queue).add("Product", "PAVI") - cdk_tags.of(self.job_queue).add("Managed_by", "PAVI") + cdk_tags.of(self.job_queue).add("Product", "PAVI") # type: ignore + cdk_tags.of(self.job_queue).add("Managed_by", "PAVI") # type: ignore From 46d03d9498e71898c356140d444c67e4ec34df4f Mon Sep 17 00:00:00 2001 From: Manuel Luypaert Date: Thu, 9 May 2024 10:55:39 +0100 Subject: [PATCH 4/4] Python style fixes --- pipeline/aws_infra/cdk_classes/aws_batch.py | 6 +++--- pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pipeline/aws_infra/cdk_classes/aws_batch.py b/pipeline/aws_infra/cdk_classes/aws_batch.py index 77780d1a..6b8e1247 100644 --- a/pipeline/aws_infra/cdk_classes/aws_batch.py +++ b/pipeline/aws_infra/cdk_classes/aws_batch.py @@ -71,12 +71,12 @@ def __init__(self, scope: Stack, env_suffix: str, shared_work_dir_bucket: Option ) ] ) - s3_workdir_bucket_policy = iam.ManagedPolicy(scope,'pavi-s3-nextflow-bucket-policy', + s3_workdir_bucket_policy = iam.ManagedPolicy(scope, 'pavi-s3-nextflow-bucket-policy', managed_policy_name='agr-pavi-pipeline-nf-bucket-access', description='Grant required access to PAVI pipeline nextflow bucket to run nextflow pipelines using it.', document=s3_workdir_bucket_policy_doc) - cdk_tags.of(s3_workdir_bucket_policy).add("Product", "PAVI") # type: ignore - cdk_tags.of(s3_workdir_bucket_policy).add("Managed_by", "PAVI") # type: ignore + cdk_tags.of(s3_workdir_bucket_policy).add("Product", "PAVI") # type: ignore + cdk_tags.of(s3_workdir_bucket_policy).add("Managed_by", "PAVI") # type: ignore self.nf_bucket_access_policy = s3_workdir_bucket_policy diff --git a/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py b/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py index 6e1f5e88..4e574fcd 100644 --- a/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py +++ b/pipeline/aws_infra/tests/unit/test_cdk_infra_stack.py @@ -56,6 +56,7 @@ def test_pipeline_nf_s3_bucket() -> None: } }) + # The S3 bucket policy is assigned to the GH-actions role for PAVI, in addition to PAVI-managed resources. # Changing the name of this policy might require the gh-actions role to be updated to include the new role. def test_pipeline_nf_s3_bucket_policy() -> None: @@ -65,6 +66,7 @@ def test_pipeline_nf_s3_bucket_policy() -> None: } }) + # Below tests check for resource that must be available in the stack, # but which can be replaced/renamed without manual interventions other than # updating all references to those resources to use the new name (check all code).