Add ability to subscribe to DDoS detected alarms #386
Conversation
… provision and update requests
…ce does not specify alarm notification email
…for CDN instances
…en going from CDN to CDN dedicated WAF plan
…notification_email
| return service_instance.instance_type in [ | ||
| ServiceInstanceTypes.CDN.value, | ||
| ServiceInstanceTypes.CDN_DEDICATED_WAF.value, | ||
| ] |
There was a problem hiding this comment.
maybe late to be checking this, but why aren't we just using isinstance(service_intance, CDNServiceInstance)
There was a problem hiding this comment.
Absolutely no reason. Is there an inherent benefit to one approach vs the other?
There was a problem hiding this comment.
isinstance is a builtin, so I think it's a bit more idiomatic, and probably faster (although speed isn't really critical here - nobody would notice if this took seconds to perform, except during testing)
| def is_cdn_dedicated_waf_instance(service_instance) -> bool: | ||
| return ( | ||
| service_instance.instance_type == ServiceInstanceTypes.CDN_DEDICATED_WAF.value | ||
| ) |
There was a problem hiding this comment.
same question here: why not just isinstance(service_instance, DedicatedCDNServiceInstance)
| operation = db.session.get(Operation, operation_id) | ||
| service_instance = operation.service_instance | ||
|
|
||
| operation.step_description = "Creating SNS notification topic" | ||
| flag_modified(operation, "step_description") | ||
| db.session.add(operation) | ||
| db.session.commit() |
There was a problem hiding this comment.
(not for this PR)
this pattern is repeated in almost every task, should we refactor it out?
Related to https://github.com/cloud-gov/private/issues/1097
Changes proposed in this pull request:
alarm_notification_emailthat customers must specify for CDN w/dedicated WAF instances to receive notifications from Cloudwatch alarmsThings to check
INFOand debugging statements are written withlog.debugor similar, then they won't be written to the otput, which can prevent unintentional leaks of sensitive data.Security considerations
No direct security considerations for the broker, but these changes will facilitate better security monitoring for customers