Conversation
phobologic
left a comment
phobologic
left a comment
There was a problem hiding this comment.
One small comment/question - not sure the AllowedCIDRs is necessary, and I get the feeling it'll open the blueprint up to feature bloat around security rules. Otherwise, this looks awesome. Let me know what you think about removing the AllowedCIDR stuff, @danielkza. Thanks!
| ) | ||
| self.security_group = Ref(sg) | ||
|
|
||
| if variables["AllowedCIDRs"]: |
There was a problem hiding this comment.
So we tend to do this in a separate stack @ Remind - we found that we wanted to update security rules more often than databases themselves, so it felt safer. It also meant we could use the full security group rule stack, which can build just about any sort of rule (we tend to use SecurityGroups as the source of the rules, not CIDR). I think this also gets trickier when you start working with Aurora, right?
There was a problem hiding this comment.
You have a good point, this isn't really that flexible. Would you find the TroposphereType to be a good middle ground, as you mentioned in the other issue? I find having seperate stacks for the SGs a bit cumbersome and usually avoid it.
There was a problem hiding this comment.
Actually - there's an issue with that, in that the rules wouldn't have a SecurityGroup attribute, which is required for the SecurityGroupIngress type. That would normally be set to the SG that is created here. Though didn't you just add a feature to TroposphereType that allows for modifying the resource after the fact, not requiring immediate validation? If so, then that'd work with this. Otherwise you could do what we did in the security_rules.py blueprint and just accept a dict, then use from_dict after you've added the SecurityGroup attribute.
2 participants
This should fix a couple of bugs I found, and make using the template a bit easier by setting up security group ingress rules based on CIDRs.