6100: Add default security group for ecs tasks and services#6492
6100: Add default security group for ecs tasks and services#6492
Conversation
ca61688
added
the
needs-reviewer
|
|
||
| // Security Groups | ||
| CdkDefinedInstanceProperty ECS_SECURITY_GROUP = Index.propertyBuilder("sleeper.ecs.security.group") | ||
| .description("The security group to be used for ECS tasks and services.") |
There was a problem hiding this comment.
We can be clear that this is the security group ID, as opposed to e.g. the ARN. Maybe the property name should have .id at the end as well?
| .newInstancesProtectedFromScaleIn(false) | ||
| .updatePolicy(UpdatePolicy.rollingUpdate()) | ||
| .securityGroup(SecurityGroup.fromSecurityGroupId(policiesStack, instanceId, | ||
| instanceProperties.get(ECS_SECURITY_GROUP))) |
There was a problem hiding this comment.
It looks like this is defining a new security group construct, adding it to the managed policies stack, and setting a construct ID of the Sleeper instance ID.
Can we just call a getter to return the security group from the policies stack?
| private void createEcsSecurityGroup(Construct scope, InstanceProperties instanceProperties) { | ||
| SecurityGroup securityGroup = SecurityGroup.Builder.create(scope, "ECS") | ||
| .vpc(Vpc.fromLookup(scope, "vpc", VpcLookupOptions.builder().vpcId(instanceProperties.get(VPC_ID)).build())) | ||
| .description("ECS security group") |
There was a problem hiding this comment.
Can we explain roughly what this is for in the description? It'll be used for ECS tasks and services that don't need to serve incoming requests.
| .newInstancesProtectedFromScaleIn(false) | ||
| .updatePolicy(UpdatePolicy.rollingUpdate()) | ||
| .securityGroup(SecurityGroup.fromSecurityGroupId(policiesStack, instanceId, | ||
| instanceProperties.get(ECS_SECURITY_GROUP))) |
There was a problem hiding this comment.
The documentation of this method says "launchTemplate and mixedInstancesPolicy must not be specified when this property is specified". It looks like it wants you to put it on the launch template.
We don't have a system test to catch this right now as none of them use the ECS-based committer. Please can you test it by configuring your instance to use the committer explicitly?
There was a problem hiding this comment.
Good spot, I've moved it higher up into the launch template itself and will test it with a deployment
patchwork01
removed
the
needs-reviewer
ca61688
added
the
needs-reviewer
|
Ready for rereview |
java/deployment/cdk/src/main/java/sleeper/cdk/stack/core/EcsClusterTasksStack.java
Outdated
Show resolved
Hide resolved
patchwork01
removed
the
needs-reviewer
2 participants
Make sure you have checked all steps below.
Issue
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
Tests
Documentation
separate issue for that below.