-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(deadline): add security group configuration for Repository and RenderQueue #319
feat(deadline): add security group configuration for Repository and RenderQueue #319
Conversation
73c3c83
to
eb2878f
Compare
3425e70
to
e67d618
Compare
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.
Change looks good. Just a couple small remarks
/** | ||
* Options to add additional security groups to the `RenderQueue`. | ||
*/ | ||
readonly securityGroups?: RenderQueueSecurityGroupsOptions; |
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.
How about more informative variable name?
Like securityGroupsOptions
.
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.
Updated
/** | ||
* The `AutoScalingGroup` security groups. | ||
*/ | ||
readonly autoScalingGroup?: ISecurityGroup[]; |
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.
-
Maybe name these
backend
andfrontend
instead?frontend
-> Security group for the load balancer.backend
-> Security group for everything behind the load balancer. It's probable that we add the Deadline WebService to this same construct, and it makes some sense for it to have the same SG as the RCS. -
The normal pattern in CDK constructs is to accept a single
ISecurityGroup
as a construct property, rather than an array, and then expose anaddSecurityGroups()
method. What do you think about following that pattern here?
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.
Sounds good, updated
e67d618
to
4fd6663
Compare
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.
This is great work, Jericho. Thanks for the contribution!
b399e39
to
c557523
Compare
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.
Looks good for me.
Fixes #307
Changes
RenderQueue
: AddaddSecurityGroups
method that adds security groups to theAutoScalingGroup
,LoadBalancer
, or both.Repository
: AddaddSecurityGroup
method that adds security groups to theDatabaseConnection
.DatabaseConnection
CfnDBCluster
.MongoDbInstance
: Added anaddSecurityGroup
method to add security groups to theAutoScalingGroup
of theStaticPrivateIpServer
.Testing
SecurityGroup
inStorageTier
and added it to theRepository
construct in theServiceTier
.SecurityGroup
inServiceTier
and added it to theRenderQueue
construct in theServiceTier
.Repository
(DocDB): Added to the DocDB ClusterRepository
(MongoDB): Added to the AutoScalingGroupRenderQueue
: Added to the AutoScalingGroup and LoadBalanceraddSecurityGroup(s)
methods to add security groupsaddSecurityGroup(s)
methods to add security groupsBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license