Skip to content
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

VpcNetwork should throw a descriptive error when selecting 0 subnets #2011

Closed
MichaelHindley opened this issue Mar 14, 2019 · 2 comments · Fixed by #2025 · May be fixed by MechanicalRock/account-reaper#6
Closed

VpcNetwork should throw a descriptive error when selecting 0 subnets #2011

MichaelHindley opened this issue Mar 14, 2019 · 2 comments · Fixed by #2025 · May be fixed by MechanicalRock/account-reaper#6
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud feature-request A feature should be added or improved.

Comments

@MichaelHindley
Copy link

Trying to set up a ecs cluster with asg in a existing non-cdk managed vpc.
The externalVpc is used without issue when working with Lambdas, but with ASG it fails due to AvailabilityZones being empty, but they do exist and they do work with at least one other resource.

const externalVpc = ec2.VpcNetwork.import(this, 'exampleVPC', {
  availabilityZones: [cdk.Fn.importValue('az1'), cdk.Fn.importValue('az1')],
  publicSubnetIds: [cdk.Fn.importValue('publicsubnet1'), cdk.Fn.importValue('publicsubnet2')],
  vpcId: cdk.Fn.importValue('vpcId'),
});

// Cluster config
const cluster = new ecs.Cluster(this, 'cluster-test', {
  clusterName: 'cluster-test',
  vpc: externalVpc,
});

cluster.addCapacity('asg-test', {
  instanceType: new ec2.InstanceType('t3.small'),
  taskDrainTimeSeconds: 0,
});

results in

 CREATE_FAILED        | AWS::AutoScaling::AutoScalingGroup

 Property AvailabilityZones cannot be empty.
        new AutoScalingGroup (code\aws\node_modules\@aws-cdk\aws-autoscaling\lib\auto-scaling-group.ts:297:29)
        \_ Cluster.addCapacity (code\aws\node_modules\@aws-cdk\aws-ecs\lib\cluster.ts:78:30)
        \_ new ContainerRegistryStack (ode\operator-interface\aws\lib\container-repository-stack\ecr.ts:32:13)
        \_ Object.<anonymous> (code\operator-interface\aws\bin\aws.ts:12:1)
        \_ Module._compile (internal/modules/cjs/loader.js:689:30)
        \_ Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        \_ Module.load (internal/modules/cjs/loader.js:599:32)
        \_ tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        \_ Function.Module._load (internal/modules/cjs/loader.js:530:3)
        \_ Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        \_ startup (internal/bootstrap/node.js:283:19)
        \_ bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)

The actual error is that vpcPlacement: { subnetsToUse: SubnetType.Public } is missing in the config to config.addCapacity.

vpcPlacement is an optional parameter, but without it you get an error in this case.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 15, 2019

Yes, VpcNetwork should throw a descriptive error if it cannot find any subnets of the indicated type.

@rix0rrr rix0rrr added feature-request A feature should be added or improved. @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud labels Mar 15, 2019
@rix0rrr rix0rrr changed the title Missleading error in ecs.cluster.addCapacity when omitting optional parameter vpcPlacement VpcNetwork should throw a descriptive error when selecting 0 subnets Mar 15, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 15, 2019

Note to future implementors: not in all cases; sometimes consumers will do analysis on the list of subnets returned, and it's okay for it to be empty.

Honestly, it's probably better to add an API subnetIds(vpcPlacement): string[] which has the default validating behavior. This will satisfy the 9 out of 10 cases where constructs just want to take a list of subnets and stick them into a CFN resource, leaving constructs that have fallbacks to use the other function.

rix0rrr added a commit that referenced this issue Mar 15, 2019
Selecting 0 subnets is invalid for most CFN resources. We move the check
for this upstream to the VPC construct, which can throw a nicely
descriptive error message if it happens to not contain any subnets that
match the type we're looking for.

The alternative is that the error happens during CloudFormation
deployment, but then it's not very clear anymore what the cause for
the error was.

Fixes #2011.
rix0rrr added a commit that referenced this issue Mar 21, 2019
Selecting 0 subnets is invalid for most CFN resources. We move the check
for this upstream to the VPC construct, which can throw a nicely
descriptive error message if it happens to not contain any subnets that
match the type we're looking for.

The alternative is that the error happens during CloudFormation
deployment, but then it's not very clear anymore what the cause for
the error was.

Fixes #2011.

BREAKING CHANGE: `vpcPlacement` has been renamed to `vpcSubnets`
on all objects, `subnetsToUse` has been renamed to `subnetType`.
`natGatewayPlacement` has been renamed to `natGatewaySubnets`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud feature-request A feature should be added or improved.
Projects
None yet
2 participants