AWS ELB-ALB support #211

vcastellm · 2017-02-23T14:45:52Z

We want to be able to create a new service and assign it an Application Load Balancer.

Add 5 new params that allows to specify the Load Balancer details in order for the service to be created with an associated ELB/ALB.

Specify the Target Group ARN, Load Balancer Name, Container Name, Container Port and Role to allow the service to register new instances in the ELB/ALB.

--target-group-arn: The full Amazon Resource Name (ARN) of the Elastic Load Balancing target group associated with a service.
--load-balancer-name: The name of the classic load balancer.
--container-name: The name of the container (as it appears in a container definition) to associate with the load balancer.
--container-port: The port on the container to associate with the load balancer. This port must correspond to a containerPort in the service's task definition. Your container instances must allow ingress traffic on the hostPort of the port mapping.
--role: The name or full Amazon Resource Name (ARN) of the IAM role that allows Amazon ECS to make calls to your load balancer on your behalf. This parameter is required if you are using a load balancer with your service. If you specify the role parameter, you must also specify a load balancer object with the loadBalancers parameter.

If --target-group-arn or --load-balancer-name is not passed then the current behaviour is maintained and it doesn't try to associate an ELB with the service.

Note: If both target-goup-arn and load-balancer-name present, target-group-arn param will take precedence.

When creating a new ECS service, allow the user to specify an ELB target group ARN, container name and port that it will associate with the newly created ECS service.

yinshiua · 2017-03-02T19:24:51Z

ecs-cli/modules/compose/ecs/service.go

+	}
+
+	s.loadBalancer = &ecs.LoadBalancer{
+		TargetGroupArn: aws.String(targetGroupArn),


Can we also support Classic Load Balancer (ELB) as well? You will need to add LoadBalancerName as a parameter.
http://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_LoadBalancer.html

yinshiua · 2017-03-02T19:29:09Z

ecs-cli/modules/aws/clients/ecs/client.go

+func (client *ecsClient) CreateService(serviceName, taskDefName string, loadBalancer *ecs.LoadBalancer, role string, deploymentConfig *ecs.DeploymentConfiguration) error {
+	var err error
+
+	if *loadBalancer.TargetGroupArn != "" {


I recommend initializing the common inputs first, and if TargetGroupARN or LoadBalancerName is specified, add the fields that are needed for creating ELB/ALB.

You'll need to check if loadBalancer is nil before dereferencing it.

As per loadContext method https://github.com/aws/amazon-ecs-cli/pull/211/files#diff-97b4106c54aad3d402f437209e9fc4e3R78 loadBalancer could have empty values but it'll never be nil.

samuelkarp · 2017-03-03T00:29:41Z

ecs-cli/modules/aws/clients/ecs/client.go

+func (client *ecsClient) CreateService(serviceName, taskDefName string, loadBalancer *ecs.LoadBalancer, role string, deploymentConfig *ecs.DeploymentConfiguration) error {
+	var err error
+
+	if *loadBalancer.TargetGroupArn != "" {


You'll need to check if loadBalancer is nil before dereferencing it.

samuelkarp · 2017-03-03T00:30:11Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

@@ -136,3 +136,36 @@ func deploymentConfigFlags(specifyDefaults bool) []cli.Flag {
 		},
 	}
 }
+
+func loadBalancerFlags(specifyDefaults bool) []cli.Flag {


@nrdlngr Can you review this text?

Just FYI, the text is copy pasted from the param description in the AWS API docs. But please, review.

CreateService use a common CreateServiceInput object for both cases (ELB/no ELB) set it first, then set the ELB params if present.

New param --load-balancer-name allow to set a classic load balancer name for the service. Note that in case of both target-goup-arn and load-balancer-name present, target-group-arn param will take precedence.

yinshiua · 2017-03-08T22:07:50Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

+	loadBalancerNameUsageString := "[Optional] The name of the load balancer."
+	roleUsageString := "[Optional] The name or full Amazon Resource Name (ARN) of the IAM role that allows Amazon ECS to make calls to your load balancer on your behalf. This parameter is required if you are using a load balancer with your service. If you specify the role parameter, you must also specify a target-group-arn, container-name and container-port."
+
+	if specifyDefaults {


When will this be false?

Also, I don't think we need to specify the defaults as none. "Optional" already indicates that it will be empty.

You're right, it should never be called with false. Removed

yinshiua · 2017-03-08T22:08:10Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

+		containerNameUsageString += " Defaults to none"
+		containerPortUsageString += " Defaults to none"
+		loadBalancerNameUsageString += " Defaults to none"
+		roleUsageString += " Defaults to ecsServiceRole"


Does it actually defaults to ecsServiceRole?

Also, to specify defaults use Value in the cli.StringFlag

I took license here and used the role name used in AWS docs http://docs.aws.amazon.com/AmazonECS/latest/developerguide/service_IAM_role.html on creating an IAM role for ECS services.

@Victorcoder I'm seeing 2 issues here.

You are specifying in the command help text that the role defaults to ecsServiceRole, but in your actual implementation of createService, you're not setting the default to ecsServiceRole

By setting the default role, we are assuming everyone has created the role ecsServiceRole which is not always the case, we should be creating the role for them if the role does not exist.

I would leave out the defaults in the command help text for now, and add it back once we have the implementations. Hope that makes sense!

yinshiua · 2017-03-10T21:55:12Z

ecs-cli/modules/compose/ecs/flags.go

+	ContainerNameFlag                       = "container-name"
+	ContainerPortFlag                       = "container-port"
+	LoadBalancerNameFlag                    = "load-balancer-name"
+	RoleDefaultValue                        = "ecsServiceRole"


Can you also remove the RoleDefaultValue? This is something we need to revisit

yinshiua · 2017-03-10T22:32:19Z

ecs-cli/modules/compose/ecs/service.go

+		ContainerPort: containerPort,
+	}
+
+	if targetGroupArn != "" {


It would be nice to educate the customers that loadBalancerName and targetGroupArn by throwing an error when both are specified instead of picking one for them.

What I did in with the other params is let the AWS API validate the user input, the validation errors are quite meaninfull in most cases.

If we remove the targetGoupArn/loadBalancerName fallback here, we get a good server side message educating the user:

FATA[0000] InvalidParameterException: loadBalancerName and targetGroupArn cannot both be specified. You must specify either a load BalancerName or a targetGroupArn.

So removing the check here

yinshiua · 2017-03-10T22:40:15Z

ecs-cli/modules/aws/clients/ecs/client.go

-	if err != nil {
+	}
+
+	if *loadBalancer.TargetGroupArn != "" || *loadBalancer.LoadBalancerName != "" {


I understand that for the current use case, the loadBalancer will always be set and will never be nil, but this CreateServce method can be reused in the future, hence I would still recommend adding a nil pointer check here.

Can you change *loadBalancer.TargetGroupArn to use aws.StringValue (aws.StringValue(loadBalancer.TargetGroupArn) and aws.StringValue(loadBalancer.LoadBalancerName))

yinshiua · 2017-03-10T22:41:24Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

@@ -136,3 +136,35 @@ func deploymentConfigFlags(specifyDefaults bool) []cli.Flag {
 		},
 	}
 }
+
+func loadBalancerFlags() []cli.Flag {
+	targetGroupArnUsageString := "[Optional] The full Amazon Resource Name (ARN) of the Elastic Load Balancing target group associated with a service."


I am working on reviewing the text with others, and will update you once I have the final version.

Would you mind changing this to:

"[Optional] Specifies the full Amazon Resource Name (ARN) of a previously configured Elastic Load Balancing target group to associate with your service."

We let AWS API to inform the user of the error.

nrdlngr

Just a few doc suggestions to keep things consistent with our existing help text.

I modified a few of the help strings to indicate that they were required for both classic and application load balancers, and that the load balancer resources must exist before the command is run.

nrdlngr · 2017-03-13T22:11:00Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

+	containerNameUsageString := "[Optional] Mandatory if target-group-arn is specified. The container name (as it appears in a container definition)."
+	containerPortUsageString := "[Optional] Mandatory if target-group-arn is specified. The container port to access from the load balancer."
+	loadBalancerNameUsageString := "[Optional] The name of the load balancer."
+	roleUsageString := fmt.Sprintf("[Optional] The name or full Amazon Resource Name (ARN) of the IAM role that allows Amazon ECS to make calls to your load balancer on your behalf. This parameter is required if you are using a load balancer with your service. If you specify the role parameter, you must also specify a target-group-arn, container-name and container-port.")


Would you mind changing this to:

"[Optional] Specifies the name or full Amazon Resource Name (ARN) of the IAM role that allows Amazon ECS to make calls to your load balancer or target group on your behalf. This parameter is required if you are using a load balancer or target group with your service. If you specify the role parameter, you must also specify a load balancer name or target group ARN, along with a container name and container port."

nrdlngr · 2017-03-13T22:11:18Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

@@ -136,3 +136,35 @@ func deploymentConfigFlags(specifyDefaults bool) []cli.Flag {
 		},
 	}
 }
+
+func loadBalancerFlags() []cli.Flag {
+	targetGroupArnUsageString := "[Optional] The full Amazon Resource Name (ARN) of the Elastic Load Balancing target group associated with a service."


Would you mind changing this to:

"[Optional] Specifies the full Amazon Resource Name (ARN) of a previously configured Elastic Load Balancing target group to associate with your service."

nrdlngr · 2017-03-13T22:11:22Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

+
+func loadBalancerFlags() []cli.Flag {
+	targetGroupArnUsageString := "[Optional] The full Amazon Resource Name (ARN) of the Elastic Load Balancing target group associated with a service."
+	containerNameUsageString := "[Optional] Mandatory if target-group-arn is specified. The container name (as it appears in a container definition)."


Would you mind changing this to:

"[Optional] Specifies the container name (as it appears in a container definition). This parameter is required if a load balancer or target group is specified."

nrdlngr · 2017-03-13T22:11:26Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

+func loadBalancerFlags() []cli.Flag {
+	targetGroupArnUsageString := "[Optional] The full Amazon Resource Name (ARN) of the Elastic Load Balancing target group associated with a service."
+	containerNameUsageString := "[Optional] Mandatory if target-group-arn is specified. The container name (as it appears in a container definition)."
+	containerPortUsageString := "[Optional] Mandatory if target-group-arn is specified. The container port to access from the load balancer."


Would you mind changing this to:

"[Optional] Specifies the port on the container to associate with the load balancer. This port must correspond to a containerPort in the service's task definition. This parameter is required if a load balancer or target group is specified."

nrdlngr · 2017-03-13T22:11:28Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

+	targetGroupArnUsageString := "[Optional] The full Amazon Resource Name (ARN) of the Elastic Load Balancing target group associated with a service."
+	containerNameUsageString := "[Optional] Mandatory if target-group-arn is specified. The container name (as it appears in a container definition)."
+	containerPortUsageString := "[Optional] Mandatory if target-group-arn is specified. The container port to access from the load balancer."
+	loadBalancerNameUsageString := "[Optional] The name of the load balancer."


Would you mind changing this to:

"[Optional] Specifies the name of a previously configured Elastic Load Balancing load balancer to associate with your service."

yinshiua · 2017-03-17T02:23:46Z

@Victorcoder Can you please make text changes base on @nrdlngr's suggestions? Thank you!

vcastellm · 2017-03-20T11:56:34Z

Changes applied @nrdlngr

yinshiua · 2017-03-20T18:01:17Z

ecs-cli/modules/compose/cli/ecs/app/service_command.go

+	containerNameUsageString := "[Optional] Specifies the container name (as it appears in a container definition). This parameter is required if a load balancer or target group is specified."
+	containerPortUsageString := "[Optional] Specifies the port on the container to associate with the load balancer. This port must correspond to a containerPort in the service's task definition. This parameter is required if a load balancer or target group is specified."
+	loadBalancerNameUsageString := "[Optional] Specifies the name of a previously configured Elastic Load Balancing load balancer to associate with your service."
+	roleUsageString := fmt.Sprintf("[Optional] Specifies the name or full Amazon Resource Name (ARN) of the IAM role that allows Amazon ECS to make calls to your load balancer or target group on your behalf. This parameter is required if you are using a load balancer or target group with your service. If you specify the role parameter, you must also specify a load balancer name or target group ARN, along with a container name and container port.")


Thanks for updating the text. Just one minor comment

printf is not longer needed here.

Text updated

yinshiua · 2017-03-22T23:41:18Z

@Victorcoder Are you fine with your commit being merged in under the Apache 2.0 license?
Thanks!

vcastellm · 2017-03-23T09:04:16Z

@yinshiua Thanks to you, I'm ok, go ahead.

yinshiua · 2017-03-23T21:37:21Z

Thanks for your contribution.

I am giving a pass on this PR, but will have minor followup changes on bug fixing.

markriggins · 2017-03-24T12:56:19Z

Shouldn't this be able to read from a JSON or YAML file, so the service can be created, dropped, recreated without having to reproduce command line options?

What about other service parameters such as maximumPercent and minimumHealthyPercent?

vcastellm · 2017-03-24T14:30:12Z

@markriggins That could be a good improvement but those parameters wouldn't be compose compatible so it will force to have another file to create the service.

In any case, that would be the goal of another PR.

markriggins · 2017-03-24T16:44:56Z

Yeah, we wound up adding a *service-name--env.json *file to our services, and then wrapping ecs-cli with a script to pick up these extra service settings. But we had no choice, since we could not add new keywords to the yaml. ecs-cli could add some custom sections, no? Mark Riggins CLOUD COMPUTING ARCHITECT 336.312.7125 www.SocialCode.com <http://socialcode.com/>

…

On Fri, Mar 24, 2017 at 10:30 AM, Victor Castell ***@***.***> wrote: @markriggins <https://github.com/markriggins> That could be a good improvement but those parameters wouldn't be compose compatible so it will force to have another file to create the service. In any case, that would be the goal of another PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#211 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNfuj8EIUWc06CI9CQJTocHO3XDCg6oks5ro9N2gaJpZM4MKDRN> .

serragnoli · 2017-04-17T19:51:01Z

Would it be possible for someone to give an example on how to use this new feature please?

yinshiua · 2017-04-17T20:53:36Z

@markriggins This would be a good enhancement to read all these fields from yaml or JSON. (ex.ecs-cli compose --f docker-compose.yml service up --f ecs-service-config.yml)

@serragnoli An example for ALB would be:

ecs-cli compose service up --target-group-arn arn:aws:elasticloadbalancing:us-west-2:<**aws-account-id**>:targetgroup/ecs-cli/090481523c5c99ea
--container-name wordpress --container-port 80 --role ecsServiceRole

markriggins · 2017-04-18T17:17:55Z

@serragnoli <https://github.com/serragnoli> -- this feature is currently unavailable via ecs-cli, so we had to use the `aws ecs` command instead after reading these settings from a json file and merging them with the existing settings. The important line from the script reads something like this. aws ecs update-service --service ecscompose-service-$serviceName --cli-input-json "$service_settings" Mark Riggins CLOUD COMPUTING ARCHITECT 336.312.7125 www.SocialCode.com <http://socialcode.com/>

…

On Mon, Apr 17, 2017 at 3:51 PM, Fabio Serragnoli ***@***.***> wrote: Would it be possible for someone to give an example on how to use this new feature please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#211 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNfutIIDqAEejrQVkng5OF4kaCBvd5uks5rw8KngaJpZM4MKDRN> .

vchan2002 · 2017-04-19T00:09:45Z

@yinshiua Thank you for the example....

Just to clarify, when using the --target-group-arn setting, I would assume that the target group is already existing as a part of an ALB, and thus there's no need to specify the ALB, right?

Thus, when using the --load-balancer-name setting instead, it would assume that it is using a classic load balancer, not an ALB, since, as I'd imagine, just putting an ALB would be insufficient in stating where it needs to go?

I'll still trying to navigate my way thru in terms of understanding ECS, in terms of tasks, services, etc... so any help would be greatly appreciated!

yinshiua · 2017-04-21T00:57:18Z

@vchan2002
Yes, current feature only allows you to use an existing ALB or ELB.
To specify an ALB, you use --target-group-arn
To specify an ELB, you use --load-balancer-name
Once the service is created with an ALB or ELB, the name/arn is immutable to the service. You will not be able to switch to use a different ALB or ELB.

The following documentations may be helpful:
http://docs.aws.amazon.com/AmazonECS/latest/developerguide/service-load-balancing.html
http://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_LoadBalancer.html

vcastellm force-pushed the allow-alb-setup branch from 533731a to 4e4421f Compare February 28, 2017 10:34

Allow ELB ALB assignation

6011ce7

When creating a new ECS service, allow the user to specify an ELB target group ARN, container name and port that it will associate with the newly created ECS service.

vcastellm force-pushed the allow-alb-setup branch from 4e4421f to 6011ce7 Compare February 28, 2017 11:04

yinshiua reviewed Mar 2, 2017

View reviewed changes

samuelkarp reviewed Mar 3, 2017

View reviewed changes

Victor Castell added 2 commits March 3, 2017 16:40

Refactor to set common attrs

9d5e7d1

CreateService use a common CreateServiceInput object for both cases (ELB/no ELB) set it first, then set the ELB params if present.

Allow the possibility to specify ELB

1ad46c7

New param --load-balancer-name allow to set a classic load balancer name for the service. Note that in case of both target-goup-arn and load-balancer-name present, target-group-arn param will take precedence.

yinshiua reviewed Mar 8, 2017

View reviewed changes

Victor Castell added 3 commits March 9, 2017 18:07

Use a constant for role default value

f4afbe8

Always show default values in load balancer flags help.

80e9b97

Remove "Defaults to none" text

2ad2d3b

yinshiua reviewed Mar 10, 2017

View reviewed changes

Victor Castell added 4 commits March 13, 2017 09:49

No default value for Role

0a6ef9d

Better use aws.StringValue helper for checking

f89d342

nil pointer check for loadBalancer

0a76e25

Don't assume a preferred load balancer type

3dc50b3

We let AWS API to inform the user of the error.

nrdlngr previously requested changes Mar 13, 2017

View reviewed changes

Correct params usage text

32bb258

yinshiua reviewed Mar 20, 2017

View reviewed changes

yinshiua mentioned this pull request Mar 20, 2017

ECS-CLI Amazon ELB Support #81

Closed

Remove printf not needed here.

92ce387

yinshiua merged commit df62308 into aws:master Mar 23, 2017

vcastellm mentioned this pull request Mar 24, 2017

Create services with a load balancer #21

Closed

AWS ELB-ALB support #211

AWS ELB-ALB support #211

Conversation

vcastellm commented Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcastellm Mar 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yinshiua Mar 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcastellm Mar 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrdlngr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yinshiua commented Mar 17, 2017

vcastellm commented Mar 20, 2017

Choose a reason for hiding this comment

yinshiua commented Mar 22, 2017

vcastellm commented Mar 23, 2017

yinshiua commented Mar 23, 2017

markriggins commented Mar 24, 2017

vcastellm commented Mar 24, 2017

markriggins commented Mar 24, 2017 via email

serragnoli commented Apr 17, 2017

yinshiua commented Apr 17, 2017

markriggins commented Apr 18, 2017 via email

vchan2002 commented Apr 19, 2017

yinshiua commented Apr 21, 2017

vcastellm commented Feb 23, 2017 •

edited

vcastellm Mar 6, 2017 •

edited

yinshiua Mar 10, 2017 •

edited

vcastellm Mar 13, 2017 •

edited