Skip to content

Commit 8261b62

Browse files
adanajaadanaja
committed
Merge branch 'morosi-fix' into 'master'
Make Bucket.resources not mutate the notifications targets See merge request it/e3-aws!92
2 parents 337a740 + 0d4417e commit 8261b62

File tree

8 files changed

+319
-78
lines changed

8 files changed

+319
-78
lines changed

‎src/e3/aws/troposphere/cloudfront/__init__.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -306,15 +306,17 @@ def resources(self, stack: Stack) -> list[AWSObject]:
306306
# Add bucket policy granting read access to te cloudfront distribution
307307
self.add_oai_access_to_bucket()
308308

309+
# Add a lambda invalidating cloudfront cache when bucket objects are modified
310+
invalidation_resources = self.add_cache_invalidation(stack)
311+
309312
result = [
310313
*(self.bucket.resources(stack) if self._create_bucket else []),
311314
self.cache_policy,
312315
self.distribution,
313316
self.origin_access_identity,
314317
]
315318

316-
# Add a lambda invalidating cloudfront cache when bucket objects are modified
317-
result.extend(self.add_cache_invalidation(stack))
319+
result.extend(invalidation_resources)
318320

319321
# Add route53 records if needed
320322
if self.r53_route_from:

‎src/e3/aws/troposphere/s3/bucket.py‎

Lines changed: 64 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ def __init__(
8888
] = []
8989
self.topic_configurations: list[tuple[dict[str, str], Topic | None, str]] = []
9090
self.queue_configurations: list[tuple[dict[str, str], Queue | None, str]] = []
91+
self.notification_resources: list[AWSObject] = []
9192
self.depends_on: list[str] = []
9293
self.bucket_kwargs = bucket_kwargs
9394

@@ -134,12 +135,38 @@ def add_notification_configuration(
134135
if isinstance(target, Topic):
135136
params["Topic"] = target.arn
136137
self.topic_configurations.append((params, target, permission_suffix))
138+
139+
# Add policy allowing to publish to topic
140+
topic_policy_name = target.add_allow_service_to_publish_statement(
141+
applicant=f"{name_to_id(self.name)}",
142+
service="s3",
143+
condition={"ArnLike": {"aws:SourceArn": self.arn}},
144+
)
145+
self.depends_on.append(topic_policy_name)
137146
elif isinstance(target, Function):
138147
params["Function"] = target.arn
139148
self.lambda_configurations.append((params, target, permission_suffix))
149+
150+
# Add Permission invoke for lambda
151+
self.notification_resources.append(
152+
target.invoke_permission(
153+
name_suffix=permission_suffix,
154+
service="s3",
155+
source_arn=self.arn,
156+
source_account=AccountId,
157+
)
158+
)
140159
elif isinstance(target, Queue):
141160
params["Queue"] = target.arn
142161
self.queue_configurations.append((params, target, permission_suffix))
162+
163+
# Add policy allowing to publish to queue
164+
queue_policy_name = target.add_allow_service_to_write_statement(
165+
applicant=f"{name_to_id(self.name)}",
166+
service="s3",
167+
condition={"ArnLike": {"aws:SourceArn": self.arn}},
168+
)
169+
self.depends_on.append(queue_policy_name)
143170
elif ":sns:" in target:
144171
params["Topic"] = target
145172
self.topic_configurations.append((params, None, permission_suffix))
@@ -150,81 +177,9 @@ def add_notification_configuration(
150177
params["Queue"] = target
151178
self.queue_configurations.append((params, None, permission_suffix))
152179

153-
@property
154-
def notification_setup(
155-
self,
156-
) -> tuple[s3.NotificationConfiguration, list[AWSObject]]:
157-
"""Return notification configuration and associated resources."""
158-
notification_resources = []
159-
notification_config = None
160-
params = {}
161-
if self.lambda_configurations:
162-
params.update(
163-
{
164-
"LambdaConfigurations": [
165-
s3.LambdaConfigurations(**lambda_params)
166-
for lambda_params, _, _ in self.lambda_configurations
167-
]
168-
}
169-
)
170-
# Add Permission invoke for lambdas
171-
for _, function, suffix in self.lambda_configurations:
172-
if function:
173-
notification_resources.append(
174-
function.invoke_permission(
175-
name_suffix=suffix,
176-
service="s3",
177-
source_arn=self.arn,
178-
source_account=AccountId,
179-
)
180-
)
181-
if self.topic_configurations:
182-
params.update(
183-
{
184-
"TopicConfigurations": [
185-
s3.TopicConfigurations(**topic_params)
186-
for topic_params, _, _ in self.topic_configurations
187-
]
188-
}
189-
)
190-
# Add policy allowing to publish to topics
191-
for _, topic, _ in self.topic_configurations:
192-
if topic:
193-
topic_policy_name = topic.add_allow_service_to_publish_statement(
194-
applicant=f"{name_to_id(self.name)}",
195-
service="s3",
196-
condition={"ArnLike": {"aws:SourceArn": self.arn}},
197-
)
198-
self.depends_on.append(topic_policy_name)
199-
if self.queue_configurations:
200-
params.update(
201-
{
202-
"QueueConfigurations": [
203-
s3.QueueConfigurations(**queue_params)
204-
for queue_params, _, _ in self.queue_configurations
205-
]
206-
}
207-
)
208-
for _, queue, _ in self.queue_configurations:
209-
if queue:
210-
queue_policy_name = queue.add_allow_service_to_write_statement(
211-
applicant=f"{name_to_id(self.name)}",
212-
service="s3",
213-
condition={"ArnLike": {"aws:SourceArn": self.arn}},
214-
)
215-
self.depends_on.append(queue_policy_name)
216-
217-
if params:
218-
notification_config = s3.NotificationConfiguration(
219-
name_to_id(self.name + "NotifConfig"), **params
220-
)
221-
222-
return notification_config, notification_resources
223-
224180
def resources(self, stack: Stack) -> list[AWSObject]:
225181
"""Construct and return a s3.Bucket and its associated s3.BucketPolicy."""
226182
# Handle versioning configuration
227-
optional_resources = []
228183
versioning_status = "Suspended"
229184
if self.enable_versioning:
230185
versioning_status = "Enabled"
@@ -256,8 +211,35 @@ def resources(self, stack: Stack) -> list[AWSObject]:
256211
name_to_id(self.name) + "LifeCycleConfig", Rules=self.lifecycle_rules
257212
)
258213

259-
notification_config, notification_resources = self.notification_setup
260-
optional_resources.extend(notification_resources)
214+
# Build the parameters of NotificationConfiguration, keeping only the
215+
# parameters with non empty lists
216+
notification_params: dict[str, Any] = {
217+
k: v
218+
for k, v in [
219+
(
220+
"LambdaConfigurations",
221+
[
222+
s3.LambdaConfigurations(**params)
223+
for params, _, _ in self.lambda_configurations
224+
],
225+
),
226+
(
227+
"TopicConfigurations",
228+
[
229+
s3.TopicConfigurations(**params)
230+
for params, _, _ in self.topic_configurations
231+
],
232+
),
233+
(
234+
"QueueConfigurations",
235+
[
236+
s3.QueueConfigurations(**params)
237+
for params, _, _ in self.queue_configurations
238+
],
239+
),
240+
]
241+
if v
242+
}
261243

262244
attr = {"DeletionPolicy": "Retain"}
263245
for key, val in {
@@ -268,7 +250,13 @@ def resources(self, stack: Stack) -> list[AWSObject]:
268250
Status=versioning_status
269251
),
270252
"LifecycleConfiguration": lifecycle_config,
271-
"NotificationConfiguration": notification_config,
253+
"NotificationConfiguration": (
254+
s3.NotificationConfiguration(
255+
name_to_id(self.name + "NotifConfig"), **notification_params
256+
)
257+
if notification_params
258+
else None
259+
),
272260
"DependsOn": self.depends_on,
273261
}.items():
274262
if val:
@@ -282,7 +270,7 @@ def resources(self, stack: Stack) -> list[AWSObject]:
282270
Bucket=self.ref,
283271
PolicyDocument=self.policy_document.as_dict,
284272
),
285-
*optional_resources,
273+
*self.notification_resources,
286274
]
287275

288276
@property

‎tests/tests_e3_aws/troposphere/s3websitedistribution.json‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,21 @@
11
{
22
"HostBucket": {
33
"DeletionPolicy": "Retain",
4+
"DependsOn": [
5+
"TestS3wDistInvalidationTopicPolicy"
6+
],
47
"Properties": {
58
"BucketName": "host-bucket",
9+
"NotificationConfiguration": {
10+
"TopicConfigurations": [
11+
{
12+
"Event": "s3:ObjectCreated:*",
13+
"Topic": {
14+
"Ref": "TestS3wDistInvalidationTopic"
15+
}
16+
}
17+
]
18+
},
619
"BucketEncryption": {
720
"ServerSideEncryptionConfiguration": [
821
{
@@ -307,6 +320,37 @@
307320
},
308321
"Type": "AWS::SNS::Topic"
309322
},
323+
"TestS3wDistInvalidationTopicPolicy": {
324+
"Properties": {
325+
"PolicyDocument": {
326+
"Statement": [
327+
{
328+
"Action": "sns:Publish",
329+
"Condition": {
330+
"ArnLike": {
331+
"aws:SourceArn": "arn:aws:s3:::host-bucket"
332+
}
333+
},
334+
"Effect": "Allow",
335+
"Principal": {
336+
"Service": "s3.amazonaws.com"
337+
},
338+
"Resource": {
339+
"Ref": "TestS3wDistInvalidationTopic"
340+
},
341+
"Sid": "HostBucketPubAccess"
342+
}
343+
],
344+
"Version": "2012-10-17"
345+
},
346+
"Topics": [
347+
{
348+
"Ref": "TestS3wDistInvalidationTopic"
349+
}
350+
]
351+
},
352+
"Type": "AWS::SNS::TopicPolicy"
353+
},
310354
"TestS3wDistCacheInvalidationLambdaSub": {
311355
"Properties": {
312356
"Endpoint": {

‎tests/tests_e3_aws/troposphere/s3websitedistribution_bucket.json‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,37 @@
307307
},
308308
"Type": "AWS::SNS::Topic"
309309
},
310+
"TestS3wDistInvalidationTopicPolicy": {
311+
"Properties": {
312+
"PolicyDocument": {
313+
"Statement": [
314+
{
315+
"Action": "sns:Publish",
316+
"Condition": {
317+
"ArnLike": {
318+
"aws:SourceArn": "arn:aws:s3:::host-bucket"
319+
}
320+
},
321+
"Effect": "Allow",
322+
"Principal": {
323+
"Service": "s3.amazonaws.com"
324+
},
325+
"Resource": {
326+
"Ref": "TestS3wDistInvalidationTopic"
327+
},
328+
"Sid": "HostBucketPubAccess"
329+
}
330+
],
331+
"Version": "2012-10-17"
332+
},
333+
"Topics": [
334+
{
335+
"Ref": "TestS3wDistInvalidationTopic"
336+
}
337+
]
338+
},
339+
"Type": "AWS::SNS::TopicPolicy"
340+
},
310341
"TestS3wDistCacheInvalidationLambdaSub": {
311342
"Properties": {
312343
"Endpoint": {

‎tests/tests_e3_aws/troposphere/s3websitedistribution_iam_path.json‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,21 @@
11
{
22
"HostBucket": {
33
"DeletionPolicy": "Retain",
4+
"DependsOn": [
5+
"TestS3wDistInvalidationTopicPolicy"
6+
],
47
"Properties": {
58
"BucketName": "host-bucket",
9+
"NotificationConfiguration": {
10+
"TopicConfigurations": [
11+
{
12+
"Event": "s3:ObjectCreated:*",
13+
"Topic": {
14+
"Ref": "TestS3wDistInvalidationTopic"
15+
}
16+
}
17+
]
18+
},
619
"BucketEncryption": {
720
"ServerSideEncryptionConfiguration": [
821
{
@@ -307,6 +320,37 @@
307320
},
308321
"Type": "AWS::SNS::Topic"
309322
},
323+
"TestS3wDistInvalidationTopicPolicy": {
324+
"Properties": {
325+
"PolicyDocument": {
326+
"Statement": [
327+
{
328+
"Action": "sns:Publish",
329+
"Condition": {
330+
"ArnLike": {
331+
"aws:SourceArn": "arn:aws:s3:::host-bucket"
332+
}
333+
},
334+
"Effect": "Allow",
335+
"Principal": {
336+
"Service": "s3.amazonaws.com"
337+
},
338+
"Resource": {
339+
"Ref": "TestS3wDistInvalidationTopic"
340+
},
341+
"Sid": "HostBucketPubAccess"
342+
}
343+
],
344+
"Version": "2012-10-17"
345+
},
346+
"Topics": [
347+
{
348+
"Ref": "TestS3wDistInvalidationTopic"
349+
}
350+
]
351+
},
352+
"Type": "AWS::SNS::TopicPolicy"
353+
},
310354
"TestS3wDistCacheInvalidationLambdaSub": {
311355
"Properties": {
312356
"Endpoint": {

0 commit comments

Comments
 (0)