-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Serverless canary creates IAM role resource if Role property is not provided + Policies property added #2107
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Changes from 1 commit
33f5838
0898cf7
7efa492
fa6fbac
f1e522c
cf97819
f1d3397
6bc192f
e1664f6
e1d8537
7719e86
2e1b336
cf328cc
1b771c1
acf386a
6c2d87f
faae49c
fdd09a3
4680636
81feecd
7f380fe
c76d39e
4786b5f
a0f3b5e
cc9035c
fab31c8
2ab5ac9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
document = { | ||
"PolicyName": logical_id + "CanaryPolicy", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we split this into 3 methods, one for each statement? |
||
"PolicyDocument": { | ||
"Statement": [ | ||
{ | ||
"Effect": "Allow", | ||
"Action": ["s3:PutObject", "s3:GetBucketLocation"], | ||
"Resource": [{"Fn::Join": ["", ["arn:aws:s3:::", result_bucket, "/*"]]}], | ||
}, | ||
{ | ||
"Effect": "Allow", | ||
"Action": ["logs:CreateLogStream", "logs:PutLogEvents", "logs:CreateLogGroup"], | ||
"Resource": [ | ||
{ | ||
"Fn::Join": [ | ||
"", | ||
[ | ||
{ | ||
"Fn::Sub": "arn:${AWS::Partition}:logs:${AWS::Region}:${" | ||
"AWS::AccountId}:log-group:/${AWS::Partition}/lambda/cwsyn-" | ||
}, | ||
canary_name, | ||
"-*", | ||
], | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we don't put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
], | ||
}, | ||
{"Effect": "Allow", "Action": ["s3:ListAllMyBuckets"], "Resource": ["*"]}, | ||
{ | ||
"Effect": "Allow", | ||
"Action": "cloudwatch:PutMetricData", | ||
"Resource": "*", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to be more specific about the resource? (or the condition below is enough) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the condition should be enough, its the same one used by cloudwatch dashboard when you generate a role policy through there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
"Condition": {"StringEquals": {"cloudwatch:namespace": "CloudWatchSynthetics"}}, | ||
}, | ||
] | ||
}, | ||
} | ||
return document |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -834,6 +834,7 @@ class SamCanary(SamResourceMacro): | |
"SuccessRetentionPeriod": PropertyType(False, is_type(int)), | ||
"VpcConfig": PropertyType(False, is_type(dict)), | ||
"Environment": PropertyType(False, dict_of(is_str(), is_type(dict))), | ||
"Policies": PropertyType(False, one_of(is_str(), is_type(dict), list_of(one_of(is_str(), is_type(dict))))), | ||
} | ||
|
||
def to_cloudformation(self, **kwargs): | ||
|
@@ -845,18 +846,76 @@ def to_cloudformation(self, **kwargs): | |
:rtype: list | ||
""" | ||
resources = [] | ||
managed_policy_map = kwargs.get("managed_policy_map", {}) | ||
synthetics_canary = self._construct_synthetics_canary() | ||
resources.append(synthetics_canary) | ||
|
||
# A S3 Bucket resource will be added to the transformed template if the user doesn't provide an artifact | ||
# bucket to store canary results | ||
if synthetics_canary.ArtifactS3Location is None: | ||
artifact_bucket_name = "" | ||
if self.ArtifactS3Location is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this change from synthetics_canary.ArtifactS3Location to self.ArtifactS3Location? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
s3bucket = self._construct_artifact_bucket() | ||
resources.append(s3bucket) | ||
synthetics_canary.ArtifactS3Location = {"Fn::Join": ["", ["s3://", {"Ref": s3bucket.logical_id}]]} | ||
artifact_bucket_name = {"Ref": s3bucket.logical_id} | ||
|
||
if self.Role is None: | ||
role = self._construct_role(artifact_bucket_name, synthetics_canary.Name, managed_policy_map) | ||
resources.append(role) | ||
synthetics_canary.ExecutionRoleArn = role.get_runtime_attr("arn") | ||
|
||
return resources | ||
|
||
def _construct_role(self, artifact_bucket_name, canary_name, managed_policy_map): | ||
"""Constructs a Role based on this SAM Canaries's Policies and ArtifactS3Location property. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could use more comments here to explain what is happening here (what polices will be added in what scenario) |
||
:returns: the generated IAM Role | ||
:rtype: model.iam.IAMRole | ||
""" | ||
|
||
assume_role_policy_document = IAMRolePolicies.lambda_assume_role_policy() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this feels like user have no way to override this. In SAM Function we have a property For more context if user want to have more protection by adding condition for account ID check they can do that without |
||
role_attributes = self.get_passthrough_resource_attributes() | ||
|
||
# add AWS managed policies if user has enabled VpcConfig or Tracing | ||
managed_policy_arns = [] | ||
if self.VpcConfig: | ||
managed_policy_arns.append( | ||
ArnGenerator.generate_aws_managed_policy_arn("service-role/AWSLambdaVPCAccessExecutionRole") | ||
) | ||
if self.Tracing: | ||
managed_policy_name = get_xray_managed_policy_name() | ||
managed_policy_arns.append(ArnGenerator.generate_aws_managed_policy_arn(managed_policy_name)) | ||
|
||
# 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the |
||
policy_template_processor=None, | ||
) | ||
|
||
# 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We didn't use |
||
policy_documents.append( | ||
IAMRolePolicies.execute_canary_policy( | ||
logical_id=self.logical_id, result_bucket=artifact_bucket_name, canary_name=canary_name | ||
) | ||
) | ||
|
||
execution_role = construct_role_for_resource( | ||
resource_logical_id=self.logical_id, | ||
attributes=role_attributes, | ||
managed_policy_map=managed_policy_map, | ||
assume_role_policy_document=assume_role_policy_document, | ||
resource_policies=function_policies, | ||
managed_policy_arns=managed_policy_arns, | ||
policy_documents=policy_documents, | ||
permissions_boundary=None, | ||
tags=self._construct_tag_list(self.Tags), | ||
) | ||
return execution_role | ||
|
||
def _construct_artifact_bucket(self): | ||
"""Constructs a S3Bucket resource to store canary artifacts. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some documentation on what this method do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we can make the method naming consistent with others above:
<service>-<action>-policy
.