Skip to content

Commit

Permalink
Fix YAML parse warning on networks field
Browse files Browse the repository at this point in the history 
Previously, the check for the "networks" field was based on the
NetworkConfigs attribute on the project to be empty. Libcompose changed
the behavior of the Parse method to always create a "default" network
config (See: docker/libcompose@18dad7cb).

This caused a lot of spurious warnings for customers when they would try
to run a task even if the networks field was not specified in their
docker-compose files.

Fixes #406 and closes #237
  • Loading branch information
@SoManyHs
SoManyHs committed Jan 25, 2018
1 parent 40c6571 commit b223148
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 24 deletions.
28 changes: 26 additions & 2 deletions ecs-cli/modules/utils/compose/convert_task_definition.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,10 @@ func logUnsupportedConfigFields(project *project.Project) {
if project.VolumeConfigs != nil && len(project.VolumeConfigs) > 0 {
log.WithFields(log.Fields{"option name": "volumes"}).Warn("Skipping unsupported YAML option...")
}
if project.NetworkConfigs != nil && len(project.NetworkConfigs) > 0 {
// ecsProject#parseCompose, which calls the underlying libcompose.Project#Parse(),
// always populates the project.NetworkConfig with one entry ("default").
// See: https://github.com/docker/libcompose/blob/master/project/project.go#L277
if project.NetworkConfigs != nil && len(project.NetworkConfigs) > 1 {
log.WithFields(log.Fields{"option name": "networks"}).Warn("Skipping unsupported YAML option...")
}
}
Expand All @@ -174,9 +177,16 @@ func logUnsupportedServiceConfigFields(serviceName string, config *config.Servic
}
}

if tagName == "networks" && !validNetworksForService(config) {
log.WithFields(log.Fields{
"option name": tagName,
"service name": serviceName,
}).Warn("Skipping unsupported YAML option for service...")
}

zeroValue := isZero(field)
// if value is present for the field that is not in supportedYamlTags map, log a warning
if !zeroValue && !supportedComposeYamlOptionsMap[tagName] {
if tagName != "networks" && !zeroValue && !supportedComposeYamlOptionsMap[tagName] {
log.WithFields(log.Fields{
"option name": tagName,
"service name": serviceName,
Expand All @@ -185,6 +195,20 @@ func logUnsupportedServiceConfigFields(serviceName string, config *config.Servic
}
}

func validNetworksForService(config *config.ServiceConfig) bool {
if config.Networks == nil {
return false
}
if config.Networks.Networks == nil {
return false
}
if len(config.Networks.Networks) != 1 {
return false
}

return true
}

// isZero checks if the value is nil or empty or zero
func isZero(v reflect.Value) bool {
switch v.Kind() {
Expand Down
79 changes: 57 additions & 22 deletions ecs-cli/modules/utils/compose/convert_task_definition_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ const (
hostPath = "./cache"
)

var defaultNetwork = &yaml.Network{
Name: "default",
RealName: "project_default",
}

func TestConvertToTaskDefinition(t *testing.T) {
name := "mysql"
cpu := int64(131072) // 128 * 1024
Expand All @@ -52,7 +57,6 @@ func TestConvertToTaskDefinition(t *testing.T) {
user := "user"
workingDir := "/var"
taskRoleArn := "arn:aws:iam::123456789012:role/my_role"

serviceConfig := &config.ServiceConfig{
CPUShares: yaml.StringorInt(cpu),
Command: []string{command},
Expand All @@ -61,6 +65,7 @@ func TestConvertToTaskDefinition(t *testing.T) {
Links: links,
MemLimit: yaml.MemStringorInt(int64(1048576) * memory), //1 MiB = 1048576B
MemReservation: yaml.MemStringorInt(int64(524288) * memory),
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
Privileged: privileged,
ReadOnly: readOnly,
SecurityOpt: securityOpts,
Expand Down Expand Up @@ -125,7 +130,7 @@ func TestConvertToTaskDefinition(t *testing.T) {
}

func TestConvertToTaskDefinitionLaunchTypeEmpty(t *testing.T) {
serviceConfig := &config.ServiceConfig{}
serviceConfig := serviceConfigWithDefaultNetworks()

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
if len(taskDefinition.RequiresCompatibilities) > 0 {
Expand All @@ -134,7 +139,7 @@ func TestConvertToTaskDefinitionLaunchTypeEmpty(t *testing.T) {
}

func TestConvertToTaskDefinitionLaunchTypeEC2(t *testing.T) {
serviceConfig := &config.ServiceConfig{}
serviceConfig := serviceConfigWithDefaultNetworks()

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "EC2")
if len(taskDefinition.RequiresCompatibilities) != 1 {
Expand All @@ -144,7 +149,7 @@ func TestConvertToTaskDefinitionLaunchTypeEC2(t *testing.T) {
}

func TestConvertToTaskDefinitionLaunchTypeFargate(t *testing.T) {
serviceConfig := &config.ServiceConfig{}
serviceConfig := serviceConfigWithDefaultNetworks()

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "FARGATE")
if len(taskDefinition.RequiresCompatibilities) != 1 {
Expand Down Expand Up @@ -176,7 +181,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

if assert.NoError(t, err) {
assert.Equal(t, "host", aws.StringValue(taskDefinition.NetworkMode), "Expected network mode to match")
Expand Down Expand Up @@ -219,7 +224,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

containerDefs := taskDefinition.ContainerDefinitions
mysql := findContainerByName("mysql", containerDefs)
Expand Down Expand Up @@ -259,7 +264,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

containerDefs := taskDefinition.ContainerDefinitions
mysql := findContainerByName("mysql", containerDefs)
Expand Down Expand Up @@ -297,7 +302,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

containerDefs := taskDefinition.ContainerDefinitions
mysql := findContainerByName("mysql", containerDefs)
Expand Down Expand Up @@ -335,7 +340,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

containerDefs := taskDefinition.ContainerDefinitions
mysql := findContainerByName("mysql", containerDefs)
Expand Down Expand Up @@ -372,7 +377,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

containerDefs := taskDefinition.ContainerDefinitions
mysql := findContainerByName("mysql", containerDefs)
Expand Down Expand Up @@ -410,7 +415,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

_, err = convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
_, err = convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

// At least one container must be marked essential
assert.Error(t, err)
Expand Down Expand Up @@ -442,7 +447,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

_, err = convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
_, err = convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

// At least one container must be marked essential
assert.Error(t, err)
Expand Down Expand Up @@ -473,7 +478,7 @@ task_definition:

taskRoleArn := "arn:aws:iam::123456789012:role/tweedledum"

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, taskRoleArn, ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), taskRoleArn, ecsParams)

if assert.NoError(t, err) {
assert.Equal(t, "host", aws.StringValue(taskDefinition.NetworkMode), "Expected network mode to match")
Expand Down Expand Up @@ -505,7 +510,7 @@ task_definition:
ecsParams, err := ReadECSParams(ecsParamsFileName)
assert.NoError(t, err, "Could not read ECS Params file")

taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, &config.ServiceConfig{}, "", ecsParams)
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, []string{"mysql", "wordpress"}, serviceConfigWithDefaultNetworks(), "", ecsParams)

if assert.NoError(t, err) {
assert.Equal(t, "200", aws.StringValue(taskDefinition.Cpu), "Expected CPU to match")
Expand All @@ -517,7 +522,10 @@ task_definition:
func TestConvertToTaskDefinitionWithDnsSearch(t *testing.T) {
dnsSearchDomains := []string{"search.example.com"}

serviceConfig := &config.ServiceConfig{DNSSearch: dnsSearchDomains}
serviceConfig := &config.ServiceConfig{
DNSSearch: dnsSearchDomains,
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
containerDef := *taskDefinition.ContainerDefinitions[0]
Expand All @@ -530,7 +538,10 @@ func TestConvertToTaskDefinitionWithDnsSearch(t *testing.T) {
func TestConvertToTaskDefinitionWithDnsServers(t *testing.T) {
dnsServer := "1.2.3.4"

serviceConfig := &config.ServiceConfig{DNS: []string{dnsServer}}
serviceConfig := &config.ServiceConfig{
DNS: []string{dnsServer},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
containerDef := *taskDefinition.ContainerDefinitions[0]
Expand All @@ -545,7 +556,10 @@ func TestConvertToTaskDefinitionWithDockerLabels(t *testing.T) {
"com.foo.label2": "value",
}

serviceConfig := &config.ServiceConfig{Labels: dockerLabels}
serviceConfig := &config.ServiceConfig{
Labels: dockerLabels,
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
containerDef := *taskDefinition.ContainerDefinitions[0]
Expand All @@ -560,6 +574,7 @@ func TestConvertToTaskDefinitionWithEnv(t *testing.T) {
env := envKey + "=" + envValue
serviceConfig := &config.ServiceConfig{
Environment: []string{env},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
Expand All @@ -579,6 +594,7 @@ func TestConvertToTaskDefinitionWithEnvFromShell(t *testing.T) {

serviceConfig := &config.ServiceConfig{
Environment: []string{envKey1, envKey2 + "="},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

os.Setenv(envKey1, envValue1)
Expand Down Expand Up @@ -606,7 +622,10 @@ func TestConvertToTaskDefinitionWithEnvFromShell(t *testing.T) {
}

func TestConvertToTaskDefinitionWithPortMappings(t *testing.T) {
serviceConfig := &config.ServiceConfig{Ports: []string{portMapping}}
serviceConfig := &config.ServiceConfig{
Ports: []string{portMapping},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
containerDef := *taskDefinition.ContainerDefinitions[0]
Expand All @@ -630,7 +649,10 @@ func TestConvertToTaskDefinitionWithVolumesFrom(t *testing.T) {
}

func setupAndTestVolumesFrom(t *testing.T, volume, sourceContainer string, readOnly bool) {
serviceConfig := &config.ServiceConfig{VolumesFrom: []string{volume}}
serviceConfig := &config.ServiceConfig{
VolumesFrom: []string{volume},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}
taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
containerDef := *taskDefinition.ContainerDefinitions[0]
verifyVolumeFrom(t, containerDef.VolumesFrom[0], sourceContainer, readOnly)
Expand All @@ -641,15 +663,18 @@ func TestConvertToTaskDefinitionWithExtraHosts(t *testing.T) {
ipAddress := "127.10.10.10"

extraHost := hostname + ":" + ipAddress
serviceConfig := &config.ServiceConfig{ExtraHosts: []string{extraHost}}
serviceConfig := &config.ServiceConfig{
ExtraHosts: []string{extraHost},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
containerDef := *taskDefinition.ContainerDefinitions[0]
verifyExtraHost(t, containerDef.ExtraHosts[0], hostname, ipAddress)
}

func TestConvertToTaskDefinitionWithLogConfiguration(t *testing.T) {
taskDefinition := convertToTaskDefinitionInTest(t, "name", &config.ServiceConfig{}, "", "")
taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfigWithDefaultNetworks(), "", "")
containerDef := *taskDefinition.ContainerDefinitions[0]

if containerDef.LogConfiguration != nil {
Expand All @@ -666,6 +691,7 @@ func TestConvertToTaskDefinitionWithLogConfiguration(t *testing.T) {
Driver: logDriver,
Options: logOpts,
},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition = convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
Expand All @@ -683,7 +709,8 @@ func TestConvertToTaskDefinitionWithUlimits(t *testing.T) {
typeName := "nofile"
basicType := yaml.NewUlimit(typeName, softLimit, softLimit) // "nofile=1024"
serviceConfig := &config.ServiceConfig{
Ulimits: yaml.Ulimits{Elements: []yaml.Ulimit{basicType}},
Ulimits: yaml.Ulimits{Elements: []yaml.Ulimit{basicType}},
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
Expand All @@ -698,6 +725,7 @@ func TestConvertToTaskDefinitionWithVolumes(t *testing.T) {
serviceConfig := &config.ServiceConfig{
Volumes: &yaml.Volumes{Volumes: []*yaml.Volume{&volume}},
VolumesFrom: volumesFrom,
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

taskDefinition := convertToTaskDefinitionInTest(t, "name", serviceConfig, "", "")
Expand Down Expand Up @@ -938,6 +966,12 @@ func convertToTaskDefinitionInTest(t *testing.T, name string, serviceConfig *con
return taskDefinition
}

func serviceConfigWithDefaultNetworks() *config.ServiceConfig {
return &config.ServiceConfig{
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}
}

func convertToTaskDefWithEcsParamsInTest(t *testing.T, names []string, serviceConfig *config.ServiceConfig, taskRoleArn string, ecsParams *ECSParams) (*ecs.TaskDefinition, error) {
serviceConfigs := config.NewServiceConfigs()
for _, name := range names {
Expand Down Expand Up @@ -1062,6 +1096,7 @@ func TestMemReservationHigherThanMemLimit(t *testing.T) {
ReadOnly: readOnly,
User: user,
WorkingDir: workingDir,
Networks: &yaml.Networks{Networks: []*yaml.Network{defaultNetwork}},
}

serviceConfigs := config.NewServiceConfigs()
Expand Down

0 comments on commit b223148

Please sign in to comment.