Add iam_lambda_role parameter under cluster section in the config file #2304

hanwen-pcluste · 2020-12-14T14:50:11Z

Add iam_lambda_role parameter to the config file. If specified, this role will be attached to all Lambda function resources created by CloudFormation templates.
If both ec2_iam_role and iam_lambda_role are provided, and the scheduler is sge, torque, or slurm, there will be no role created by pcluster commands. Note that if awsbatch is the scheduler, there will be role created during pcluster create.
Integration tests: Extract some functions (role creation, policy creation) from storage.kms_key_factory to conftest. The code in kms_key_factory is kept untouched to limit the scale of this commit.

Signed-off-by: Hanwen hanwenli@amazon.com

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This change is verified by the following tests:

test-suites:
  cfn-init:
    test_cfn_init.py::test_replace_compute_on_failure:
      dimensions:
        - regions: ["af-south-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["ubuntu1804"]
          schedulers: ["slurm"]
  cli_commands:
    test_cli_commands.py::test_sit_cli_commands:
      dimensions:
        - regions: ["us-west-2"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux"]
          schedulers: ["sge"]
  cloudwatch_logging:
    test_cloudwatch_logging.py::test_cloudwatch_logging:
      dimensions:
        - regions: ["eu-west-1"]
          instances: {{ common.INSTANCES_DEFAULT_ARM }}
          oss: ["ubuntu1804"]
          schedulers: ["slurm"]
  configure:
    test_pcluster_configure.py::test_pcluster_configure:
      dimensions:
        - regions: ["us-east-1"]
          instances: {{ common.INSTANCES_DEFAULT_ARM }}
          oss: ["ubuntu1804"]
          schedulers: ["slurm"]
    test_pcluster_configure.py::test_region_without_t2micro:
      dimensions:
        - regions: ["eu-north-1"] # must be regions that do not have t2.micro
          oss: ["alinux"]
          schedulers: ["slurm"]
  dashboard:
    test_dashboard.py::test_dashboard:
      dimensions:
        - regions: ["ap-northeast-2"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["ubuntu1604"]
          schedulers: ["slurm"]
  dns:
    test_dns.py::test_hit_no_cluster_dns_mpi:
      dimensions:
        - regions: ["eu-west-2"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux"]
          schedulers: ["slurm"]
  iam:
    test_iam.py::test_iam_roles:
      dimensions:
        - regions: ["us-east-2"] # me-south-1
          schedulers: ["awsbatch", "slurm", "sge"]
          oss: ["alinux"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
  tags:
    test_tag_propagation.py::test_tag_propagation:
      dimensions:
        - regions: ["ap-southeast-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux2"]
          schedulers: ["slurm", "torque", "awsbatch"]
  update:
    test_update.py::test_update_awsbatch:
      dimensions:
        - regions: ["eu-south-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux2"]
          schedulers: ["awsbatch"]
    test_update.py::test_update_hit:
      dimensions:
        - regions: ["eu-west-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux"]
          schedulers: ["slurm"]
    test_update.py::test_update_sit:
      dimensions:
        - regions: ["eu-west-2"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["centos7"]
          schedulers: ["sge"]
  multiple_nics:
    test_multiple_nics.py::test_multiple_nics:
      dimensions:
        - regions: ["us-east-1"]
          instances: ["p4d.24xlarge"]
          oss: ["alinux2", "ubuntu1604"]
          schedulers: ["slurm"]
  resource_bucket:
    test_resource_bucket.py::test_resource_bucket:
      dimensions:
        - regions: ["ap-southeast-1"]
          instances: {{ common.INSTANCES_DEFAULT_X86 }}
          oss: ["alinux2"]
          schedulers: ["slurm", "awsbatch"]

codecov · 2020-12-14T14:53:27Z

Codecov Report

Merging #2304 (e11716d) into develop (b2bca77) will increase coverage by 0.01%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           develop    #2304      +/-   ##
===========================================
+ Coverage    61.61%   61.63%   +0.01%     
===========================================
  Files           40       40              
  Lines         6146     6151       +5     
===========================================
+ Hits          3787     3791       +4     
- Misses        2359     2360       +1

Impacted Files	Coverage Δ
cli/src/pcluster/config/mappings.py	`100.00% <ø> (ø)`
cli/src/pcluster/config/cfn_param_types.py	`92.97% <80.00%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da8ef65...1546d8f. Read the comment docs.

hanwen-pcluste · 2020-12-14T16:50:45Z

tests/integration-tests/conftest.py

+    return boto3.client("iam", region_name=region).create_policy(
+        PolicyName=iam_policy_name, PolicyDocument=parallel_cluster_instance_policy
+    )["Policy"]["Arn"]


Tried using troposphere to create the policy. But it is hard to covert the policy json to the required awacs.aws.PolicyDocument object. In other words, troposphere does not accept json as the PolicyDocument.
Of course, we can use static CloudForamtion templates

CHANGELOG.md

cli/src/pcluster/config/cfn_param_types.py

cli/src/pcluster/config/mappings.py

tests/integration-tests/tests/iam/test_iam.py

enrico-usai · 2020-12-15T07:50:45Z

CHANGELOG.md

  failures due to CloudFormation throttling.
 - Add support for io2 EBS volume type.
+- Add `iam_lambda_role` parameter under `cluster` section to enable the possibility to specify an existing IAM role to 
+  be used with AWS Lambda functions that implement CustomResources in CloudFormation. 


...to be used with AWS Lambda functions that implement CustomResources in CloudFormation. --> ...to be used by AWS Lambda Functions part of the CloudFormation template. or something like this? up to you.

cli/src/pcluster/config/cfn_param_types.py

enrico-usai · 2020-12-15T08:02:44Z

tests/integration-tests/conftest.py

+        )
+
+
+def _create_iam_policies(iam_policy_name, region, policy_filename):


minor: I'd rename it to _render_iam_policies, The "render" word highlights the fact we're using jinja.

tests/integration-tests/conftest.py

cli/src/pcluster/config/mappings.py

tests/integration-tests/tests/iam/test_iam.py

tests/integration-tests/tests/iam/test_iam/test_iam_roles/HIT.update.ini

enrico-usai · 2020-12-15T10:06:53Z

tests/integration-tests/tests/iam_policies/test_iam_policies.py

@@ -1,54 +0,0 @@
-# Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.


It would be good to see this file as modified file instead of removed.

I tried refactor it again. But it seems github only recognize move without change of the content

tests/integration-tests/tests/iam/test_iam/test_iam_roles/SIT.update.ini

1. Add `iam_lambda_role` parameter to the config file. If specified, this role will be attached to all Lambda function resources created by CloudFormation Templates. 2. If both `ec2_iam_role` and `iam_lambda_role` are provided, and the scheduler is `sge`, `torque`, or `slurm`, there will be no created by `pcluster` commands. Note that if `awsbatch` is the scheduler, there will be role created during `pcluster create`. 3. Integration tests: Extract some functions (role creation, policy creation) from `storage.kms_key_factory` to `conftest`. The code in `kms_key_factory` is kept untouched to limit the scale of this commit. Signed-off-by: Hanwen <hanwenli@amazon.com>

…tion This PR will complete the leftover from aws#2304. In that PR, we duplicated code for IAM policies, IAM roles creation. We didn't clean up the duplication to limit the scale of that PR. Moreover, this PR will scope the fixtures for IAM policies, IAM roles, KMS keys to session, allowing reusing of the resources across tests. Note that the resources are not reused across parallel test runs. Signed-off-by: Hanwen <hanwenli@amazon.com>

hanwen-pcluste force-pushed the lambda_function_role branch from 494b7b4 to 1b47fa4 Compare December 14, 2020 14:59

hanwen-pcluste commented Dec 14, 2020

View reviewed changes

demartinofra reviewed Dec 14, 2020

View reviewed changes

hanwen-pcluste force-pushed the lambda_function_role branch 2 times, most recently from c641e68 to e11716d Compare December 15, 2020 02:49

enrico-usai reviewed Dec 15, 2020

View reviewed changes

hanwen-pcluste force-pushed the lambda_function_role branch 2 times, most recently from 6091c3f to 9ec981a Compare December 15, 2020 14:17

hanwen-pcluste marked this pull request as draft December 15, 2020 14:33

hanwen-pcluste force-pushed the lambda_function_role branch 7 times, most recently from 6db2e6a to 3a45e25 Compare December 15, 2020 21:16

hanwen-pcluste marked this pull request as ready for review December 15, 2020 21:17

hanwen-pcluste marked this pull request as draft December 15, 2020 21:17

hanwen-pcluste marked this pull request as ready for review December 15, 2020 21:17

hanwen-pcluste marked this pull request as draft December 15, 2020 21:17

hanwen-pcluste marked this pull request as ready for review December 15, 2020 21:17

hanwen-pcluste force-pushed the lambda_function_role branch 2 times, most recently from 2ef605b to d50dfc8 Compare December 15, 2020 21:32

hanwen-pcluste force-pushed the lambda_function_role branch from d50dfc8 to 1546d8f Compare December 15, 2020 21:36

tilne approved these changes Dec 15, 2020

View reviewed changes

hanwen-pcluste merged commit 3621fb8 into aws:develop Dec 16, 2020

hanwen-pcluste mentioned this pull request Dec 21, 2020

integ-tests: clean up code for IAM policies, IAM roles, KMS keys creation #2325

Closed

		)


		def _create_iam_policies(iam_policy_name, region, policy_filename):

		@@ -1,54 +0,0 @@
		# Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Add iam_lambda_role parameter under cluster section in the config file #2304

Add iam_lambda_role parameter under cluster section in the config file #2304

Uh oh!

Conversation

hanwen-pcluste commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hanwen-pcluste Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enrico-usai Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enrico-usai Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enrico-usai Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

hanwen-pcluste Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hanwen-pcluste commented Dec 14, 2020 •

edited

Loading

codecov bot commented Dec 14, 2020 •

edited

Loading