diff --git a/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperation.java b/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperation.java index b8807730391..8e196a87234 100644 --- a/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperation.java +++ b/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperation.java @@ -142,26 +142,36 @@ protected RegisterTaskDefinitionRequest makeTaskDefinitionRequest(String ecsServ containerEnvironment.add(new KeyValuePair().withName("CLOUD_STACK").withValue(description.getStack())); containerEnvironment.add(new KeyValuePair().withName("CLOUD_DETAIL").withValue(description.getFreeFormDetails())); - PortMapping portMapping = new PortMapping() - .withProtocol(description.getPortProtocol() != null ? description.getPortProtocol() : "tcp"); - - if (AWSVPC_NETWORK_MODE.equals(description.getNetworkMode())) { - portMapping - .withHostPort(description.getContainerPort()) - .withContainerPort(description.getContainerPort()); - } else { - portMapping - .withHostPort(0) - .withContainerPort(description.getContainerPort()); - } + ContainerDefinition containerDefinition = new ContainerDefinition() + .withName(EcsServerGroupNameResolver.getEcsContainerName(newServerGroupName)) + .withEnvironment(containerEnvironment) + .withCpu(description.getComputeUnits()) + .withMemoryReservation(description.getReservedMemory()) + .withImage(description.getDockerImageAddress()); Collection portMappings = new LinkedList<>(); - portMappings.add(portMapping); + + if (description.getContainerPort() != null) { + PortMapping portMapping = new PortMapping() + .withProtocol(description.getPortProtocol() != null ? description.getPortProtocol() : "tcp"); + + if (AWSVPC_NETWORK_MODE.equals(description.getNetworkMode())) { + portMapping + .withHostPort(description.getContainerPort()) + .withContainerPort(description.getContainerPort()); + } else { + portMapping + .withHostPort(0) + .withContainerPort(description.getContainerPort()); + } + + portMappings.add(portMapping); + } if (description.getServiceDiscoveryAssociations() != null) { for (CreateServerGroupDescription.ServiceDiscoveryAssociation config : description.getServiceDiscoveryAssociations()) { if (config.getContainerPort() != null && config.getContainerPort() != 0 && config.getContainerPort() != description.getContainerPort()) { - portMapping = new PortMapping().withProtocol("tcp"); + PortMapping portMapping = new PortMapping().withProtocol("tcp"); if (AWSVPC_NETWORK_MODE.equals(description.getNetworkMode())) { portMapping .withHostPort(config.getContainerPort()) @@ -176,13 +186,7 @@ protected RegisterTaskDefinitionRequest makeTaskDefinitionRequest(String ecsServ } } - ContainerDefinition containerDefinition = new ContainerDefinition() - .withName(EcsServerGroupNameResolver.getEcsContainerName(newServerGroupName)) - .withEnvironment(containerEnvironment) - .withPortMappings(portMappings) - .withCpu(description.getComputeUnits()) - .withMemoryReservation(description.getReservedMemory()) - .withImage(description.getDockerImageAddress()); + containerDefinition.setPortMappings(portMappings); if (!NO_IMAGE_CREDENTIALS.equals(description.getDockerImageCredentialsSecret()) && description.getDockerImageCredentialsSecret() != null) { @@ -246,8 +250,7 @@ protected RegisterTaskDefinitionRequest makeTaskDefinitionRequest(String ecsServ private Service createService(AmazonECS ecs, TaskDefinition taskDefinition, String ecsServiceRole, String newServerGroupName, Service sourceService) { - Collection loadBalancers = new LinkedList<>(); - loadBalancers.add(retrieveLoadBalancer(EcsServerGroupNameResolver.getEcsContainerName(newServerGroupName))); + Collection loadBalancers = retrieveLoadBalancers(EcsServerGroupNameResolver.getEcsContainerName(newServerGroupName)); Integer desiredCount = description.getCapacity().getDesired(); if (sourceService != null && @@ -480,12 +483,13 @@ private DeploymentResult makeDeploymentResult(Service service) { return result; } - private LoadBalancer retrieveLoadBalancer(String containerName) { - LoadBalancer loadBalancer = new LoadBalancer(); - loadBalancer.setContainerName(containerName); - loadBalancer.setContainerPort(description.getContainerPort()); + private Collection retrieveLoadBalancers(String containerName) { + Collection loadBalancers = new LinkedList<>(); + if (description.getTargetGroup() != null && !description.getTargetGroup().isEmpty()) { + LoadBalancer loadBalancer = new LoadBalancer(); + loadBalancer.setContainerName(containerName); + loadBalancer.setContainerPort(description.getContainerPort()); - if (description.getTargetGroup() != null) { AmazonElasticLoadBalancing loadBalancingV2 = getAmazonElasticLoadBalancingClient(); DescribeTargetGroupsRequest request = new DescribeTargetGroupsRequest().withNames(description.getTargetGroup()); @@ -499,8 +503,9 @@ private LoadBalancer retrieveLoadBalancer(String containerName) { throw new IllegalArgumentException("There is no target group with the name " + description.getTargetGroup() + "."); } + loadBalancers.add(loadBalancer); } - return loadBalancer; + return loadBalancers; } private AWSApplicationAutoScaling getSourceAmazonApplicationAutoScalingClient() { diff --git a/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServerGroupDescriptionValidator.java b/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServerGroupDescriptionValidator.java index e3b127330ca..429e6601fcc 100644 --- a/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServerGroupDescriptionValidator.java +++ b/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServerGroupDescriptionValidator.java @@ -114,7 +114,7 @@ public void validate(List priorDescriptions, Object description, Errors errors) if (createServerGroupDescription.getContainerPort() < 0 || createServerGroupDescription.getContainerPort() > 65535) { rejectValue(errors, "containerPort", "invalid"); } - } else { + } else if (createServerGroupDescription.getTargetGroup() != null && !createServerGroupDescription.getTargetGroup().isEmpty()) { rejectValue(errors, "containerPort", "not.nullable"); } diff --git a/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationService.java b/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationService.java index 785bb89ae26..e2145e78d41 100644 --- a/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationService.java +++ b/clouddriver-ecs/src/main/java/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationService.java @@ -70,35 +70,58 @@ public List> getHealthStatus(String taskId, String serviceNa String healthKey = Keys.getTaskHealthKey(accountName, region, taskId); TaskHealth taskHealth = taskHealthCacheClient.get(healthKey); - if (service == null || taskHealth == null) { - List> healthMetrics = new ArrayList<>(); + String taskKey = Keys.getTaskKey(accountName, region, taskId); + Task task = taskCacheClient.get(taskKey); + + List> healthMetrics = new ArrayList<>(); + // Load balancer-based health + if (service == null || taskHealth == null) { Map loadBalancerHealth = new HashMap<>(); loadBalancerHealth.put("instanceId", taskId); loadBalancerHealth.put("state", "Unknown"); loadBalancerHealth.put("type", "loadBalancer"); healthMetrics.add(loadBalancerHealth); - return healthMetrics; + } else { + List loadBalancers = service.getLoadBalancers(); + //There should only be 1 based on AWS documentation. + if (loadBalancers.size() == 1) { + Map loadBalancerHealth = new HashMap<>(); + loadBalancerHealth.put("instanceId", taskId); + loadBalancerHealth.put("state", taskHealth.getState()); + loadBalancerHealth.put("type", taskHealth.getType()); + + healthMetrics.add(loadBalancerHealth); + } else if (loadBalancers.size() >= 2) { + throw new IllegalArgumentException("Cannot have more than 1 load balancer while checking ECS health."); + } } - List loadBalancers = service.getLoadBalancers(); - //There should only be 1 based on AWS documentation. - if (loadBalancers.size() == 1) { + // Task-based health + if (task != null) { + Map taskPlatformHealth = new HashMap<>(); + taskPlatformHealth.put("instanceId", taskId); + taskPlatformHealth.put("type", "ecs"); + taskPlatformHealth.put("healthClass", "platform"); + taskPlatformHealth.put("state", toPlatformHealthState(task.getLastStatus())); + healthMetrics.add(taskPlatformHealth); + } - List> healthMetrics = new ArrayList<>(); - Map loadBalancerHealth = new HashMap<>(); - loadBalancerHealth.put("instanceId", taskId); - loadBalancerHealth.put("state", taskHealth.getState()); - loadBalancerHealth.put("type", taskHealth.getType()); + return healthMetrics; + } - healthMetrics.add(loadBalancerHealth); - return healthMetrics; - } else if (loadBalancers.size() >= 2) { - throw new IllegalArgumentException("Cannot have more than 1 load balancer while checking ECS health."); + private String toPlatformHealthState(String ecsTaskStatus) { + switch (ecsTaskStatus) { + case "PROVISIONING": + case "PENDING": + case "ACTIVATING": + return "Starting"; + case "RUNNING": + return "Unknown"; + default: + return "Down"; } - return null; - } public String getClusterArn(String accountName, String region, String taskId) { diff --git a/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperationSpec.groovy b/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperationSpec.groovy index 471aede546e..805b6dc849e 100644 --- a/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperationSpec.groovy +++ b/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/ops/CreateServerGroupAtomicOperationSpec.groovy @@ -364,6 +364,19 @@ class CreateServerGroupAtomicOperationSpec extends CommonAtomicOperation { request.getContainerDefinitions().get(0).getLogConfiguration().getOptions() == logOptions } + def 'should allow no port mappings'() { + given: + def description = Mock(CreateServerGroupDescription) + description.getContainerPort() >> null + def operation = new CreateServerGroupAtomicOperation(description) + + when: + def request = operation.makeTaskDefinitionRequest('arn:aws:iam::test:test-role', 'mygreatapp-stack1-details2-v0011') + + then: + request.getContainerDefinitions().get(0).getPortMappings().isEmpty() + } + def 'should allow using secret credentials for the docker image'() { given: def description = Mock(CreateServerGroupDescription) diff --git a/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServergroupDescriptionValidatorSpec.groovy b/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServergroupDescriptionValidatorSpec.groovy index 01f17f368d2..4f3dc373838 100644 --- a/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServergroupDescriptionValidatorSpec.groovy +++ b/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/deploy/validators/EcsCreateServergroupDescriptionValidatorSpec.groovy @@ -105,6 +105,19 @@ class EcsCreateServergroupDescriptionValidatorSpec extends AbstractValidatorSpec 0 * errors.rejectValue(_, _) } + void 'should pass without load balancer'() { + given: + def description = getDescription() + description.containerPort = null + description.targetGroup = null + def errors = Mock(Errors) + + when: + validator.validate([], description, errors) + + then: + 0 * errors.rejectValue(_, _) + } @Override AbstractECSDescription getNulledDescription() { diff --git a/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationServiceSpec.groovy b/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationServiceSpec.groovy index 6066c1e813f..15e2fc1c887 100644 --- a/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationServiceSpec.groovy +++ b/clouddriver-ecs/src/test/groovy/com/netflix/spinnaker/clouddriver/ecs/services/ContainerInformationServiceSpec.groovy @@ -66,12 +66,19 @@ class ContainerInformationServiceSpec extends Specification { serviceCacheClient.get(_) >> cachedService taskHealthCacheClient.get(_) >> cachedTaskHealth + taskCacheClient.get(_) >> new Task(lastStatus: 'RUNNING') def expectedHealthStatus = [ [ instanceId: taskId, state : state, type : type + ], + [ + instanceId: taskId, + state : 'Unknown', + type :'ecs', + healthClass: 'platform' ] ] @@ -142,6 +149,42 @@ class ContainerInformationServiceSpec extends Specification { retrievedHealthStatus == expectedHealthStatus } + def 'should return correct health status based on last task status'() { + setup: + def taskId = 'task-id' + def serviceName = 'test-service-name' + taskCacheClient.get(_) >> new Task(lastStatus: lastStatus) + def expectedHealthStatus = [ + [ + instanceId: taskId, + state : 'Unknown', + type : 'loadBalancer' + ], + [ + instanceId: taskId, + state : resultStatus, + type : 'ecs', + healthClass: 'platform' + ] + ] + def retrievedHealthStatus = service.getHealthStatus(taskId, serviceName, 'test-account', 'us-west-1') + + expect: + retrievedHealthStatus == expectedHealthStatus + + where: + lastStatus | resultStatus + 'PROVISIONING' | 'Starting' + 'PENDING' | 'Starting' + 'ACTIVATING' | 'Starting' + 'RUNNING' | 'Unknown' + 'DEACTIVATING' | 'Down' + 'STOPPING' | 'Down' + 'DEPROVISIONING' | 'Down' + 'STOPPED' | 'Down' + } + + def 'should throw an exception when the service has multiple loadbalancers'() { given: def cachedService = new Service(