Serverless canary creates IAM role resource if Role property is not provided + Policies property added #2107

aaythapa · 2021-07-29T21:53:03Z

Issue #, if available:

Description of changes:

Implementing the making of AWS::IAM::Role resource if user doesn't provide the Role property in AWS::Serverless::Canary.
Added the a Policies property to Serverless::Canary so that user can add policies to the AWS::IAM::Role resource. Similar to the Policies property in AWS::Serverless::Function (https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-resource-function.html#sam-function-policies)

Description of how you validated changes:

Unit tests to make sure an IAM::Role resource was made whenever the Role property wasn't filled and the right roles were attached to the policy.
Unit tests to make sure whenever Policies was defined it was appended to the IAM::Role resource
Integration tests to make sure Serverless::Canary and IAM::Role deployed correctly.

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

…ode review changes

… made name lower case to follow synthetics canary requirements

Co-authored-by: Chris Rehn <crehn@outlook.com>

…anary

…S3Location property

…rovided + Policies property added

codecov-commenter · 2021-07-29T22:03:08Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/sam-canary@f05f778). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##             feat/sam-canary    #2107   +/-   ##
==================================================
  Coverage                   ?   94.01%           
==================================================
  Files                      ?       93           
  Lines                      ?     6348           
  Branches                   ?     1278           
==================================================
  Hits                       ?     5968           
  Misses                     ?      175           
  Partials                   ?      205

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 f05f778...2ab5ac9. Read the comment docs.

…anary

c2tarun

PR description says that unit tests were added but I didn't find them. Did you forgot to check them in?

c2tarun · 2021-07-30T14:45:30Z

samtranslator/model/iam.py

@@ -129,3 +129,45 @@ def lambda_invoke_function_role_policy(cls, function_arn, logical_id):
            },
        }
        return document
+
+    @classmethod
+    def execute_canary_policy(cls, logical_id, result_bucket, canary_name):


Some documentation on what this method do.

Also we can make the method naming consistent with others above: <service>-<action>-policy.

c2tarun · 2021-07-30T14:50:52Z

samtranslator/model/sam_resources.py

-        if synthetics_canary.ArtifactS3Location is None:
+
+        artifact_bucket_name = ""
+        if self.ArtifactS3Location is None:


Why did this change from synthetics_canary.ArtifactS3Location to self.ArtifactS3Location?

to keep it consistent with the other times that I check whether or not a property is None. Like for example later in the code is check for self.Tracing and self.VpcConfig

c2tarun · 2021-07-30T15:56:51Z

samtranslator/model/sam_resources.py

+        :rtype: model.iam.IAMRole
+        """
+
+        assume_role_policy_document = IAMRolePolicies.lambda_assume_role_policy()


So this feels like user have no way to override this. In SAM Function we have a property AssumeRolePolicyDocument which allows user to override this. Can we do similar to that?

For more context if user want to have more protection by adding condition for account ID check they can do that without AssumeRolePolicyDocument

c2tarun · 2021-07-30T15:59:10Z

samtranslator/model/sam_resources.py

+        # if user has defined Policies property, those policies will be appended to this role
+        function_policies = ResourcePolicies(
+            {"Policies": self.Policies},
+            # No support for policy templates in the "core"


What does this mean?

the No support for policy templates in the "core"? Not too sure, it was included in the other places where ResourcePolicies is used so I included it here just in case

c2tarun · 2021-07-30T16:03:15Z

samtranslator/model/iam.py

+    @classmethod
+    def execute_canary_policy(cls, logical_id, result_bucket, canary_name):
+        document = {
+            "PolicyName": logical_id + "CanaryPolicy",


Can we split this into 3 methods, one for each statement?

…unit tests

aahung · 2021-07-30T17:45:23Z

samtranslator/model/iam.py

@@ -129,3 +129,45 @@ def lambda_invoke_function_role_policy(cls, function_arn, logical_id):
            },
        }
        return document
+
+    @classmethod
+    def execute_canary_policy(cls, logical_id, result_bucket, canary_name):


Also we can make the method naming consistent with others above: <service>-<action>-policy.

aahung · 2021-07-30T17:54:29Z

samtranslator/model/iam.py

+                                "Fn::Join": [
+                                    "",
+                                    [
+                                        {
+                                            "Fn::Sub": "arn:${AWS::Partition}:logs:${AWS::Region}:${"
+                                            "AWS::AccountId}:log-group:/${AWS::Partition}/lambda/cwsyn-"
+                                        },
+                                        canary_name,
+                                        "-*",
+                                    ],
+                                ]


Is there a reason we don't put canary_name into the Fn::Sub string? (removing Join)

did some more testing on the logs and turns out the canary name is not even needed. Will take it out and take out the Fn::Join as well

aahung · 2021-07-30T17:58:02Z

samtranslator/model/sam_resources.py

+        """Constructs a Role based on this SAM Canaries's Policies and ArtifactS3Location property.
+


could use more comments here to explain what is happening here (what polices will be added in what scenario)

aahung · 2021-07-30T18:02:11Z

samtranslator/model/sam_resources.py

+        # The policy to execute the canary is only added to the role if the user hasn't defined ArtifactS3Location
+        # this emulates CloudWatch Synthetics Canary dashboard's behavior
+        policy_documents = []
+        if self.ArtifactS3Location is None:


nit: We didn't use if self.Tracing is not None above, to keep consistent, can we use if not self.ArtifactS3Location: here?

aahung · 2021-07-30T18:03:07Z

samtranslator/model/iam.py

+                    {
+                        "Effect": "Allow",
+                        "Action": "cloudwatch:PutMetricData",
+                        "Resource": "*",


Is there a way to be more specific about the resource? (or the condition below is enough)

the condition should be enough, its the same one used by cloudwatch dashboard when you generate a role policy through there

I don't think that is Sam meant about Resources. To answer Sam's question we allow Canary Lambda to putmetricdata so that the integration to publishing metrics is not very brittle. Another approach could be that metric data is created and only permission to those will be granted. But every time we require to publish new metric we have to update it. Plus I couldn't find any way to create metric data using CFN, so it might have to be done by hand. This is similar to LambdaBasicExecution permission for logs.

aahung

LGTM, thanks for addressing the comments

aahung · 2021-08-03T20:54:56Z

samtranslator/model/sam_resources.py

        role_attributes = self.get_passthrough_resource_attributes()
+        if self.AssumeRolePolicyDocument is not None:


nit: Use if self.AssumeRolePolicyDocument:

aahung · 2021-08-03T20:55:16Z

tests/translator/output/aws-cn/canary_with_assume_role_policy.json

+      }
+    }
+  }
+}


nit: Add an extra line at the end of the file

c2tarun

LGTM!

c2tarun · 2021-08-05T16:37:12Z

samtranslator/model/iam.py

+                    {
+                        "Effect": "Allow",
+                        "Action": "cloudwatch:PutMetricData",
+                        "Resource": "*",


I don't think that is Sam meant about Resources. To answer Sam's question we allow Canary Lambda to putmetricdata so that the integration to publishing metrics is not very brittle. Another approach could be that metric data is created and only permission to those will be granted. But every time we require to publish new metric we have to update it. Plus I couldn't find any way to create metric data using CFN, so it might have to be done by hand. This is similar to LambdaBasicExecution permission for logs.

aaythapa and others added 18 commits June 28, 2021 17:03

initial barebones serverless canary implementation and testing

33f5838

initial barebones serverless canary implementation and testing with c…

0898cf7

…ode review changes

fixed some property type errors

7efa492

rearranged _construct_code_dict method + added comments for clarity +…

fa6fbac

… made name lower case to follow synthetics canary requirements

Update samtranslator/model/cloudwatch.py

f1e522c

Co-authored-by: Chris Rehn <crehn@outlook.com>

made _extract_not_none_properties static + added more documentation

cf97819

Merge remote-tracking branch 'origin/feat/sam-canary' into feat/sam-c…

f1d3397

…anary

made _extract_not_none_properties static + added more documentation

6bc192f

serverless canary now makes s3 bucket for user if they don't Artifact…

e1664f6

…S3Location property

Merge branch 'feat/sam-canary' into feat/sam-canary

e1d8537

make black redo

7719e86

changed canary name + removed some files

2e1b336

makes sure artifacts3location format is correct + added tests

cf328cc

pr revision v1 - added constants

1b771c1

pr revision v2 - took out s3 uri validation since cfn handles it

acf386a

pr revision v3 - updated documentation for canary name

6c2d87f

pr revision v4 - fixed documentation mistake

faae49c

Serverless canary creates IAM role resource if Role property is not p…

fdd09a3

…rovided + Policies property added

aaythapa changed the title ~~Feat/sam canary~~ Serverless canary creates IAM role resource if Role property is not provided + Policies property added Jul 29, 2021

Merge branch 'feat/sam-canary' into feat/sam-canary

4680636

aaythapa added 2 commits July 29, 2021 18:10

Integration tests for IAM::Role resource and Policies property

81feecd

Merge remote-tracking branch 'origin/feat/sam-canary' into feat/sam-c…

7f380fe

…anary

c2tarun suggested changes Jul 30, 2021

View reviewed changes

Unit tests for IAM::Role resource and Policies property + additional …

c76d39e

…unit tests

aahung reviewed Jul 30, 2021

View reviewed changes

added AssumeRolePolicy property + some documentation

4786b5f

sriram-mv mentioned this pull request Jul 30, 2021

Support for AWS::Synthetics::Canary deploy #1934

Closed

aaythapa added 2 commits August 3, 2021 16:28

split policies to for canary into multiple methods

a0f3b5e

changed "Tracing" property to "ActiveTracing"

cc9035c

aahung approved these changes Aug 3, 2021

View reviewed changes

aaythapa added 2 commits August 3, 2021 17:21

pr revision

fab31c8

added Tags to the AWS::S3::Bucket resource made by SAM

2ab5ac9

c2tarun approved these changes Aug 5, 2021

View reviewed changes

c2tarun merged commit 2af925f into aws:feat/sam-canary Aug 5, 2021

		"""Constructs a Role based on this SAM Canaries's Policies and ArtifactS3Location property.

		role_attributes = self.get_passthrough_resource_attributes()
		if self.AssumeRolePolicyDocument is not None:

Serverless canary creates IAM role resource if Role property is not provided + Policies property added #2107

Serverless canary creates IAM role resource if Role property is not provided + Policies property added #2107

Uh oh!

Conversation

aaythapa commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

c2tarun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aahung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c2tarun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaythapa commented Jul 29, 2021 •

edited

Loading

codecov-commenter commented Jul 29, 2021 •

edited

Loading