Skip to content
This repository was archived by the owner on Nov 27, 2023. It is now read-only.

Commit eda269b

Browse files
mat007mat007
authored
Merge pull request #2117 from lucassscaravelli/fix/panic-ec2-sourceless-volumes
fix: panic with ec2 source less volumes
2 parents cf3c840 + 0c3e6de commit eda269b

File tree

4 files changed

+70
-35
lines changed

4 files changed

+70
-35
lines changed

‎ecs/autoscaling_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ services:
3232
x-aws-autoscaling:
3333
cpu: 75
3434
max: 10
35-
`, useDefaultVPC)
35+
`, nil, useDefaultVPC)
3636
target := template.Resources["FooScalableTarget"].(*autoscaling.ScalableTarget)
3737
assert.Check(t, target != nil) //nolint:staticcheck
3838
assert.Check(t, target.MaxCapacity == 10) //nolint:staticcheck

‎ecs/cloudformation.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,10 @@ func (b *ecsAPIService) convert(ctx context.Context, project *types.Project) (*c
173173

174174
func (b *ecsAPIService) createService(project *types.Project, service types.ServiceConfig, template *cloudformation.Template, resources awsResources) error {
175175
taskExecutionRole := b.createTaskExecutionRole(project, service, template)
176-
taskRole := b.createTaskRole(project, service, template, resources)
176+
taskRole, err := b.createTaskRole(project, service, template, resources)
177+
if err != nil {
178+
return err
179+
}
177180

178181
definition, err := b.createTaskDefinition(project, service, resources)
179182
if err != nil {
@@ -452,7 +455,7 @@ func (b *ecsAPIService) createTaskExecutionRole(project *types.Project, service
452455
return taskExecutionRole
453456
}
454457

455-
func (b *ecsAPIService) createTaskRole(project *types.Project, service types.ServiceConfig, template *cloudformation.Template, resources awsResources) string {
458+
func (b *ecsAPIService) createTaskRole(project *types.Project, service types.ServiceConfig, template *cloudformation.Template, resources awsResources) (string, error) {
456459
taskRole := fmt.Sprintf("%sTaskRole", normalizeResourceName(service.Name))
457460
rolePolicies := []iam.Role_Policy{}
458461
if roles, ok := service.Extensions[extensionRole]; ok {
@@ -462,6 +465,13 @@ func (b *ecsAPIService) createTaskRole(project *types.Project, service types.Ser
462465
})
463466
}
464467
for _, vol := range service.Volumes {
468+
if vol.Source == "" {
469+
return "", fmt.Errorf(
470+
"service %s has an invalid volume %s: ECS does not support sourceless volumes",
471+
service.Name,
472+
vol.Target,
473+
)
474+
}
465475
rolePolicies = append(rolePolicies, iam.Role_Policy{
466476
PolicyName: fmt.Sprintf("%s%sVolumeMountPolicy", normalizeResourceName(service.Name), normalizeResourceName(vol.Source)),
467477
PolicyDocument: volumeMountPolicyDocument(vol.Source, resources.filesystems[vol.Source].ARN()),
@@ -474,15 +484,15 @@ func (b *ecsAPIService) createTaskRole(project *types.Project, service types.Ser
474484
}
475485
}
476486
if len(rolePolicies) == 0 && len(managedPolicies) == 0 {
477-
return ""
487+
return "", nil
478488
}
479489
template.Resources[taskRole] = &iam.Role{
480490
AssumeRolePolicyDocument: ecsTaskAssumeRolePolicyDocument,
481491
Policies: rolePolicies,
482492
ManagedPolicyArns: managedPolicies,
483493
Tags: serviceTags(project, service),
484494
}
485-
return taskRole
495+
return taskRole, nil
486496
}
487497

488498
func (b *ecsAPIService) createCloudMap(project *types.Project, template *cloudformation.Template, vpc string) {

‎ecs/cloudformation_test.go‎

Lines changed: 54 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package ecs
1818

1919
import (
2020
"context"
21+
"errors"
2122
"fmt"
2223
"io/ioutil"
2324
"reflect"
@@ -42,7 +43,7 @@ import (
4243
func TestSimpleConvert(t *testing.T) {
4344
bytes, err := ioutil.ReadFile("testdata/input/simple-single-service.yaml")
4445
assert.NilError(t, err)
45-
template := convertYaml(t, string(bytes), useDefaultVPC)
46+
template := convertYaml(t, string(bytes), nil, useDefaultVPC)
4647
resultAsJSON, err := marshall(template, "yaml")
4748
assert.NilError(t, err)
4849
result := fmt.Sprintf("%s\n", string(resultAsJSON))
@@ -60,7 +61,7 @@ services:
6061
awslogs-datetime-pattern: "FOO"
6162
6263
x-aws-logs_retention: 10
63-
`, useDefaultVPC)
64+
`, nil, useDefaultVPC)
6465
def := template.Resources["FooTaskDefinition"].(*ecs.TaskDefinition)
6566
logging := getMainContainer(def, t).LogConfiguration
6667
if logging != nil {
@@ -80,7 +81,7 @@ services:
8081
image: hello_world
8182
env_file:
8283
- testdata/input/envfile
83-
`, useDefaultVPC)
84+
`, nil, useDefaultVPC)
8485
def := template.Resources["FooTaskDefinition"].(*ecs.TaskDefinition)
8586
env := getMainContainer(def, t).Environment
8687
var found bool
@@ -102,7 +103,7 @@ services:
102103
- testdata/input/envfile
103104
environment:
104105
- "FOO=ZOT"
105-
`, useDefaultVPC)
106+
`, nil, useDefaultVPC)
106107
def := template.Resources["FooTaskDefinition"].(*ecs.TaskDefinition)
107108
env := getMainContainer(def, t).Environment
108109
var found bool
@@ -124,7 +125,7 @@ services:
124125
replicas: 4
125126
update_config:
126127
parallelism: 2
127-
`, useDefaultVPC)
128+
`, nil, useDefaultVPC)
128129
service := template.Resources["FooService"].(*ecs.Service)
129130
assert.Check(t, service.DeploymentConfiguration.MaximumPercent == 150)
130131
assert.Check(t, service.DeploymentConfiguration.MinimumHealthyPercent == 50)
@@ -139,7 +140,7 @@ services:
139140
update_config:
140141
x-aws-min_percent: 25
141142
x-aws-max_percent: 125
142-
`, useDefaultVPC)
143+
`, nil, useDefaultVPC)
143144
service := template.Resources["FooService"].(*ecs.Service)
144145
assert.Check(t, service.DeploymentConfiguration.MaximumPercent == 125)
145146
assert.Check(t, service.DeploymentConfiguration.MinimumHealthyPercent == 25)
@@ -151,7 +152,7 @@ services:
151152
foo:
152153
image: hello_world
153154
x-aws-pull_credentials: "secret"
154-
`, useDefaultVPC)
155+
`, nil, useDefaultVPC)
155156
x := template.Resources["FooTaskExecutionRole"]
156157
assert.Check(t, x != nil)
157158
role := *(x.(*iam.Role))
@@ -179,7 +180,7 @@ networks:
179180
name: public
180181
back-tier:
181182
internal: true
182-
`, useDefaultVPC)
183+
`, nil, useDefaultVPC)
183184
assert.Check(t, template.Resources["FronttierNetwork"] != nil)
184185
assert.Check(t, template.Resources["BacktierNetwork"] != nil)
185186
assert.Check(t, template.Resources["BacktierNetworkIngress"] != nil)
@@ -207,7 +208,7 @@ func TestLoadBalancerTypeApplication(t *testing.T) {
207208
`,
208209
}
209210
for _, y := range cases {
210-
template := convertYaml(t, y, useDefaultVPC)
211+
template := convertYaml(t, y, nil, useDefaultVPC)
211212
lb := template.Resources["LoadBalancer"]
212213
assert.Check(t, lb != nil)
213214
loadBalancer := *lb.(*elasticloadbalancingv2.LoadBalancer)
@@ -224,7 +225,7 @@ services:
224225
image: nginx
225226
foo:
226227
image: bar
227-
`, useDefaultVPC)
228+
`, nil, useDefaultVPC)
228229
for _, r := range template.Resources {
229230
assert.Check(t, r.AWSCloudFormationType() != "AWS::ElasticLoadBalancingV2::TargetGroup")
230231
assert.Check(t, r.AWSCloudFormationType() != "AWS::ElasticLoadBalancingV2::Listener")
@@ -239,7 +240,7 @@ services:
239240
image: nginx
240241
deploy:
241242
replicas: 10
242-
`, useDefaultVPC)
243+
`, nil, useDefaultVPC)
243244
s := template.Resources["TestService"]
244245
assert.Check(t, s != nil)
245246
service := *s.(*ecs.Service)
@@ -251,7 +252,7 @@ func TestTaskSizeConvert(t *testing.T) {
251252
services:
252253
test:
253254
image: nginx
254-
`, useDefaultVPC)
255+
`, nil, useDefaultVPC)
255256
def := template.Resources["TestTaskDefinition"].(*ecs.TaskDefinition)
256257
assert.Equal(t, def.Cpu, "256")
257258
assert.Equal(t, def.Memory, "512")
@@ -265,7 +266,7 @@ services:
265266
limits:
266267
cpus: '0.5'
267268
memory: 2048M
268-
`, useDefaultVPC)
269+
`, nil, useDefaultVPC)
269270
def = template.Resources["TestTaskDefinition"].(*ecs.TaskDefinition)
270271
assert.Equal(t, def.Cpu, "512")
271272
assert.Equal(t, def.Memory, "2048")
@@ -279,7 +280,7 @@ services:
279280
limits:
280281
cpus: '4'
281282
memory: 8192M
282-
`, useDefaultVPC)
283+
`, nil, useDefaultVPC)
283284
def = template.Resources["TestTaskDefinition"].(*ecs.TaskDefinition)
284285
assert.Equal(t, def.Cpu, "4096")
285286
assert.Equal(t, def.Memory, "8192")
@@ -298,7 +299,7 @@ services:
298299
- discrete_resource_spec:
299300
kind: gpus
300301
value: 2
301-
`, useDefaultVPC, useGPU)
302+
`, nil, useDefaultVPC, useGPU)
302303
def = template.Resources["TestTaskDefinition"].(*ecs.TaskDefinition)
303304
assert.Equal(t, def.Cpu, "4000")
304305
assert.Equal(t, def.Memory, "792")
@@ -314,7 +315,7 @@ services:
314315
- discrete_resource_spec:
315316
kind: gpus
316317
value: 2
317-
`, useDefaultVPC, useGPU)
318+
`, nil, useDefaultVPC, useGPU)
318319
def = template.Resources["TestTaskDefinition"].(*ecs.TaskDefinition)
319320
assert.Equal(t, def.Cpu, "")
320321
assert.Equal(t, def.Memory, "")
@@ -329,7 +330,7 @@ services:
329330
devices:
330331
- capabilities: [gpu]
331332
count: 2
332-
`, useDefaultVPC, useGPU)
333+
`, nil, useDefaultVPC, useGPU)
333334
def = template.Resources["TestTaskDefinition"].(*ecs.TaskDefinition)
334335
assert.Equal(t, def.Cpu, "")
335336
assert.Equal(t, def.Memory, "")
@@ -343,7 +344,7 @@ services:
343344
ports:
344345
- 80:80
345346
- 88:88
346-
`, useDefaultVPC)
347+
`, nil, useDefaultVPC)
347348
lb := template.Resources["LoadBalancer"]
348349
assert.Check(t, lb != nil)
349350
loadBalancer := *lb.(*elasticloadbalancingv2.LoadBalancer)
@@ -359,7 +360,7 @@ networks:
359360
default:
360361
external: true
361362
name: sg-123abc
362-
`, useDefaultVPC, func(m *MockAPIMockRecorder) {
363+
`, nil, useDefaultVPC, func(m *MockAPIMockRecorder) {
363364
m.SecurityGroupExists(gomock.Any(), "sg-123abc").Return(true, nil)
364365
})
365366
assert.Check(t, template.Resources["DefaultNetwork"] == nil)
@@ -378,7 +379,7 @@ volumes:
378379
db-data:
379380
external: true
380381
name: fs-123abc
381-
`, useDefaultVPC, func(m *MockAPIMockRecorder) {
382+
`, nil, useDefaultVPC, func(m *MockAPIMockRecorder) {
382383
m.ResolveFileSystem(gomock.Any(), "fs-123abc").Return(existingAWSResource{id: "fs-123abc"}, nil)
383384
})
384385
s := template.Resources["DbdataNFSMountTargetOnSubnet1"].(*efs.MountTarget)
@@ -403,7 +404,7 @@ volumes:
403404
performance_mode: maxIO
404405
throughput_mode: provisioned
405406
provisioned_throughput: 1024
406-
`, useDefaultVPC, func(m *MockAPIMockRecorder) {
407+
`, nil, useDefaultVPC, func(m *MockAPIMockRecorder) {
407408
m.ListFileSystems(gomock.Any(), map[string]string{
408409
api.ProjectLabel: t.Name(),
409410
api.VolumeLabel: "db-data",
@@ -423,6 +424,25 @@ volumes:
423424
assert.Equal(t, s.FileSystemId, cloudformation.Ref(n)) //nolint:staticcheck
424425
}
425426

427+
func TestCreateSourcelessVolume(t *testing.T) {
428+
convertYaml(t, `
429+
services:
430+
test:
431+
image: nginx
432+
volumes:
433+
- db-data
434+
volumes:
435+
db-data:
436+
`,
437+
errors.New("service test has an invalid volume db-data: ECS does not support sourceless volumes"),
438+
useDefaultVPC, func(m *MockAPIMockRecorder) {
439+
m.ListFileSystems(gomock.Any(), map[string]string{
440+
api.ProjectLabel: t.Name(),
441+
api.VolumeLabel: "db-data",
442+
}).Return(nil, nil)
443+
})
444+
}
445+
426446
func TestCreateAccessPoint(t *testing.T) {
427447
template := convertYaml(t, `
428448
services:
@@ -433,7 +453,7 @@ volumes:
433453
driver_opts:
434454
uid: 1002
435455
gid: 1002
436-
`, useDefaultVPC, func(m *MockAPIMockRecorder) {
456+
`, nil, useDefaultVPC, func(m *MockAPIMockRecorder) {
437457
m.ListFileSystems(gomock.Any(), gomock.Any()).Return(nil, nil)
438458
})
439459
a := template.Resources["DbdataAccessPoint"].(*efs.AccessPoint)
@@ -449,7 +469,7 @@ services:
449469
image: nginx
450470
volumes:
451471
db-data: {}
452-
`, useDefaultVPC, func(m *MockAPIMockRecorder) {
472+
`, nil, useDefaultVPC, func(m *MockAPIMockRecorder) {
453473
m.ListFileSystems(gomock.Any(), map[string]string{
454474
api.ProjectLabel: t.Name(),
455475
api.VolumeLabel: "db-data",
@@ -480,7 +500,7 @@ services:
480500
init: true
481501
user: "user"
482502
working_dir: "working_dir"
483-
`, useDefaultVPC)
503+
`, nil, useDefaultVPC)
484504
def := template.Resources["TestTaskDefinition"].(*ecs.TaskDefinition)
485505
container := getMainContainer(def, t)
486506
assert.Equal(t, container.Image, "image")
@@ -511,7 +531,7 @@ services:
511531
ports:
512532
- 80:80
513533
- 88:88
514-
`, useDefaultVPC)
534+
`, nil, useDefaultVPC)
515535
for _, r := range template.Resources {
516536
tags := reflect.Indirect(reflect.ValueOf(r)).FieldByName("Tags")
517537
if !tags.IsValid() {
@@ -533,7 +553,7 @@ x-aws-cluster: "arn:aws:ecs:region:account:cluster/name"
533553
services:
534554
test:
535555
image: nginx
536-
`, useDefaultVPC, func(m *MockAPIMockRecorder) {
556+
`, nil, useDefaultVPC, func(m *MockAPIMockRecorder) {
537557
m.ResolveCluster(gomock.Any(), "arn:aws:ecs:region:account:cluster/name").Return(existingAWSResource{
538558
arn: "arn:aws:ecs:region:account:cluster/name",
539559
id: "name",
@@ -548,7 +568,7 @@ x-aws-vpc: "arn:aws:ec2:us-west-1:EXAMPLE:vpc/vpc-1234acbd"
548568
services:
549569
test:
550570
image: nginx
551-
`, func(m *MockAPIMockRecorder) {
571+
`, nil, func(m *MockAPIMockRecorder) {
552572
m.CheckVPC(gomock.Any(), "vpc-1234acbd").Return(nil)
553573
m.GetSubNets(gomock.Any(), "vpc-1234acbd").Return([]awsResource{
554574
existingAWSResource{id: "subnet1"},
@@ -559,7 +579,7 @@ services:
559579
})
560580
}
561581

562-
func convertYaml(t *testing.T, yaml string, fn ...func(m *MockAPIMockRecorder)) *cloudformation.Template {
582+
func convertYaml(t *testing.T, yaml string, assertErr error, fn ...func(m *MockAPIMockRecorder)) *cloudformation.Template {
563583
project := loadConfig(t, yaml)
564584
ctrl := gomock.NewController(t)
565585
defer ctrl.Finish()
@@ -573,7 +593,12 @@ func convertYaml(t *testing.T, yaml string, fn ...func(m *MockAPIMockRecorder))
573593
aws: m,
574594
}
575595
template, err := backend.convert(context.TODO(), project)
576-
assert.NilError(t, err)
596+
if assertErr == nil {
597+
assert.NilError(t, err)
598+
} else {
599+
assert.Error(t, err, assertErr.Error())
600+
}
601+
577602
return template
578603
}
579604

‎ecs/ec2_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ services:
4242
- discrete_resource_spec:
4343
kind: gpus
4444
value: 1
45-
`, useDefaultVPC)
45+
`, nil, useDefaultVPC)
4646
lc := template.Resources["LaunchConfiguration"].(*autoscaling.LaunchConfiguration)
4747
assert.Check(t, lc.ImageId == "ami123456789")
4848
assert.Check(t, lc.InstanceType == "t0.femto")

0 commit comments

Comments
 (0)