Setup ecs local network #773

efekarakus · 2019-05-15T22:10:53Z

Description of changes: Add a dummy ecs-cli local up command that creates the ECS local network and starts the Local endpoints container. The network setup is run on every local up, however if the network or container already exist then we don't do anything.

Enter [N/A] in the box, if an item is not applicable to your change.

Testing

Unit tests passed
Integration tests passed
Unit tests added for new functionality
Listed manual checks and their outputs in the comments (example)
Link to issue or PR for the integration tests: Integration tests for ecs-cli local #772

Documentation

[N/A] Contacted our doc writer
[N/A] Updated our README

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

efekarakus · 2019-05-15T22:26:40Z

Integration Tests

Running integration tests...
go test -timeout 60m -tags integ -v ./ecs-cli/integ/e2e/...
=== RUN   TestCreateClusterWithEC2Task
=== PAUSE TestCreateClusterWithEC2Task
=== RUN   TestCreateClusterWithFargateService
=== PAUSE TestCreateClusterWithFargateService
=== CONT  TestCreateClusterWithEC2Task
=== CONT  TestCreateClusterWithFargateService
--- PASS: TestCreateClusterWithFargateService (318.08s)
    configure.go:42: Created config local-integ-test-fargate-tutorial-config
    up.go:62: Created cluster local-integ-test-fargate-tutorial-cluster in stack amazon-ecs-cli-setup-local-integ-test-fargate-tutorial-cluster
    fargate_service_test.go:84: Created /var/folders/13/y9bcvw7557d5bvlvrj8jz0k04gs5dl/T/docker-compose-635069048.yml successfully
    fargate_service_test.go:115: Created /var/folders/13/y9bcvw7557d5bvlvrj8jz0k04gs5dl/T/ecs-params-293767543.yml successfully
    compose.go:101: Created service with name e2e-fargate-test-service
    compose.go:112: Project e2e-fargate-test-service has 1 running containers
    compose.go:172: Scaled the service e2e-fargate-test-service to 2 tasks
    compose.go:112: Project e2e-fargate-test-service has 2 running containers
    compose.go:231: Deleted service e2e-fargate-test-service
    cfn.go:50: Success: stack amazon-ecs-cli-setup-local-integ-test-fargate-tutorial-cluster does not exist
    down.go:49: Deleted stack amazon-ecs-cli-setup-local-integ-test-fargate-tutorial-cluster
--- PASS: TestCreateClusterWithEC2Task (478.95s)
    configure.go:53: Created config local-integ-test-ec2-tutorial-config
    up.go:62: Created cluster local-integ-test-ec2-tutorial-cluster in stack amazon-ecs-cli-setup-local-integ-test-ec2-tutorial-cluster
    ecs.go:43: Number of container instances mismatch, wanted = 2, got = 0
    runner.go:89: Current timestamp=2019-05-15 15:05:10.214367 -0700 PDT m=+187.205115573, sleeping for 15s
    ecs.go:43: Number of container instances mismatch, wanted = 2, got = 0
    runner.go:89: Current timestamp=2019-05-15 15:05:25.53114 -0700 PDT m=+202.521403217, sleeping for 15s
    ecs.go:43: Number of container instances mismatch, wanted = 2, got = 1
    runner.go:89: Current timestamp=2019-05-15 15:05:40.848788 -0700 PDT m=+217.838564105, sleeping for 15s
    ecs.go:43: Number of container instances mismatch, wanted = 2, got = 1
    runner.go:89: Current timestamp=2019-05-15 15:05:56.141675 -0700 PDT m=+233.130963113, sleeping for 15s
    ecs.go:51: Cluster local-integ-test-ec2-tutorial-cluster has 2 instances
    ec2_task_test.go:96: Created /var/folders/13/y9bcvw7557d5bvlvrj8jz0k04gs5dl/T/docker-compose-952169861.yml successfully
    compose.go:70: Created containers for e2e-ec2-tutorial-taskdef
    ecs.go:75: Cluster local-integ-test-ec2-tutorial-cluster has 1 tasks
    ps.go:36: Project e2e-ec2-tutorial-taskdef has 2 running containers
    compose.go:140: Scaled the task e2e-ec2-tutorial-taskdef to 2
    ecs.go:75: Cluster local-integ-test-ec2-tutorial-cluster has 2 tasks
    ps.go:36: Project e2e-ec2-tutorial-taskdef has 4 running containers
    compose.go:199: Deleted task e2e-ec2-tutorial-taskdef
    ecs.go:75: Cluster local-integ-test-ec2-tutorial-cluster has 0 tasks
    cfn.go:50: Success: stack amazon-ecs-cli-setup-local-integ-test-ec2-tutorial-cluster does not exist
    down.go:49: Deleted stack amazon-ecs-cli-setup-local-integ-test-ec2-tutorial-cluster
PASS
ok  	github.com/aws/amazon-ecs-cli/ecs-cli/integ/e2e	479.036s

Manual Tests

Create a new network and local endpoints container

> dev-ecs-cli local up 
INFO[0015] Created network ecs-local-network with ID 526236d1340e8c511cfb09d64f0694eeab170a50d2d4bc73e07c6da179e0b510 
INFO[0015] Created the amazon-ecs-local-container-endpoints container with ID 665ad83bc34e25b72181c8e00d7f652f442596478ea143a4e0ff9ce0cfce9b1b 
INFO[0016] Started container with ID 665ad83bc34e25b72181c8e00d7f652f442596478ea143a4e0ff9ce0cfce9b1b

Already existing network but new local endpoints container

> dev-ecs-cli local up
INFO[0000] The network ecs-local-network already exists 
INFO[0000] Created the amazon-ecs-local-container-endpoints container with ID 9746be61aaa8e77aa259ab75e2f0edfb1b5f6f92d353631eff098eca0b8cd59e 
INFO[0000] Started container with ID 9746be61aaa8e77aa259ab75e2f0edfb1b5f6f92d353631eff098eca0b8cd59e

Already existing network and already existing local endpoints container

> dev-ecs-cli local up
INFO[0000] The network ecs-local-network already exists 
INFO[0000] The amazon-ecs-local-container-endpoints container already exists with ID 9746be61aaa8e77aa259ab75e2f0edfb1b5f6f92d353631eff098eca0b8cd59e 
INFO[0000] Started container with ID 9746be61aaa8e77aa259ab75e2f0edfb1b5f6f92d353631eff098eca0b8cd59e

Validate that local endpoints container is setup properly by requesting for metadata

>  docker exec 4b7d060ae51e curl 169.254.170.2/v3 | jq 
{
"DockerId": "4b7d060ae51e9f893902485d2229e62d84be682f4209b41736d82a7cbfe7053d",
"Name": "ecs-local-other_app_1",
"DockerName": "ecs-local-other_app_1",
"Image": "ecs-local-other_app",
"ImageID": "sha256:0ac30848f74a68b442cee205df0b20e8a038bb061776a1786363510dbc2c6f85",
"Labels": {
  "com.docker.compose.config-hash": "1af7b8f3b7f0467dfd40b0565f8717c59132cb0cba91c444590e1e57a324c3b4",
  "com.docker.compose.container-number": "1",
  "com.docker.compose.oneoff": "False",
  "com.docker.compose.project": "ecs-local-other",
  "com.docker.compose.service": "app",
  "com.docker.compose.version": "1.23.2",
  "taskDefinitionArn": "arn:aws:ecs:us-east-1:685593908319:task-definition/efe-test:1"
},
"DesiredStatus": "RUNNING",
"KnownStatus": "RUNNING",
"Limits": {},
"CreatedAt": "2019-05-15T22:24:29Z",
"StartedAt": "2019-05-15T22:24:29Z",
"Type": "NORMAL",
"Networks": [
  {
    "NetworkMode": "ecs-local-network",
    "IPv4Addresses": [
      "169.254.170.3"
    ]
  }
]
}

ecs-cli/modules/cli/local/network/network.go

PettitWesley · 2019-05-16T21:01:37Z

ecs-cli/modules/cli/local/network/network.go

+			Image: localEndpointsImageName,
+			Env: []string{
+				"AWS_PROFILE=default",
+				"HOME=/home",


Just curious, in the future, how will we make these configurable?

This need not be done in this PR, just noting it's something to think about.

As discussed offline, $HOME doesn't need to be configurable

Great point! Updated also our milestones to also refer to this comment.

PettitWesley · 2019-05-16T21:06:17Z

ecs-cli/modules/cli/local/network/network.go

+		localEndpointsContainerName,
+	)
+	if err != nil {
+		if strings.Contains(err.Error(), "Conflict") {


Let me make sure I understand this correctly. So what happens in the future when someone updates the Docker SDK and Docker changes their error message to not include the string "Conflict"?

The CLI command will just fail and then print the Docker error message right?
So as long as Docker keeps its error messages descriptive, the user will know what happened.

This is really annoying, I didn't want to use string matching in the error but there is no other alternative because of how Docker handles errors from the daemon (see https://github.com/moby/moby/blob/28d7dba41d0c0d9c7f0dafcc79d3c59f2b3f5dc3/client/request.go#L236)

So we don't have any way of validating if the error is a conflict, like IsConflict, anymore :(

As you said, if they remove "Conflict" from the error message then we will crash and log the new error. Hopefully, our integ tests can catch these sort of issues before a customer contacts us.

if they remove "Conflict" from the error message then we will crash and log the new error

Cool. I just wanted to verify that. While this is not great code, I understand that there is no other choice for now, so I'm fine with it.

PettitWesley · 2019-05-16T22:18:43Z

ecs-cli/modules/cli/local/network/network.go

+	if err != nil {
+		logrus.Fatalf("Failed to list containers with name %s due to %v", localEndpointsContainerName, err)
+	}
+	return resp[0].ID


I think you should add a check that the response is non-nil and contains at least one element.

Also Nit: For cases like this (where you're using a list API but you expect one result), I like to explicitly return an error if the response has more than one element. Just in case something goes really weird. May be I'm just paranoid though.

Done!

Added a check for len(resp) != 1, I don't think it could ever contain more than 1 element since we can't create two containers with the same name. But maybe we can run into a weird scenario where the container is removed in between ContainerCreate and ContainerList which results in an empty list.

PettitWesley · 2019-05-16T22:20:29Z

ecs-cli/modules/cli/local/network/network.go

@@ -0,0 +1,217 @@
+// Copyright 2015-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.


Nit: I wonder if the name of the file should be something different, since both network and container creation happen here.

I tried splitting it up into network.go, container.go and setup.go but ended up not liking it as much because all these functions were scattered around and we lost locality. I'm down to just rename this file though or package 👍. Let me know if you can have any suggestions for a better name.

On second thought, I renamed the file setup.go since the only public function in the file is Setup(). The next task is to implement teardown.go with the Teardown() function. What do you think?

Sounds good 👍

allisaurus · 2019-05-16T23:43:09Z

ecs-cli/modules/cli/local/network/network.go

+
+type idempotentContainerStarter interface {
+	ContainerList(ctx context.Context, options types.ContainerListOptions) ([]types.Container, error)
+	ContainerCreate(ctx context.Context, config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, containerName string) (container.ContainerCreateCreatedBody, error)


This params list is super long. What do you think about breaking it into multiple lines for readability?

allisaurus · 2019-05-16T23:47:01Z

ecs-cli/modules/cli/local/network/network.go

+)
+
+// Configuration for the Local Endpoints container.
+const (


are/will we consume amazon/amazon-ecs-local-container-endpoints package as a dependency? I feel like these constants would exist in that package and we should reference them from there if possible.

Unfortunately, these constants are not defined in amazon/amazon-ecs-local-container-endpoints. That container can be run with another IP address but it would not be very useful since the agent listens on 169.254.170.2 to answer requests on task metadata.

I think changing these constants is very unlikely since that would break the experience for current sidecars. Also, our current binary is already at 42MB and bringing the agent (pretty big dependency) would increase that further :/

allisaurus · 2019-05-16T23:53:11Z

ecs-cli/modules/cli/local/network/network.go

+// already exists or the container is already running then the operation is a no-op.
+//
+// If there is any unexpected errors, we exit the program with a fatal log.
+func Setup(docker IdempotentLocalNetworkStarter) {


nit: can we rename the "docker" param(s) to be more specific, like "dockerConfiguration" or "dockerOps"? To differentiate from calling the docker daemon or API directly.

Done! Renamed to dockerClient.

SoManyHs

Small nits but generally LGTM (modulo rebasing on scaffolding stuff pushed earlier)!

SoManyHs · 2019-05-17T17:07:00Z

ecs-cli/modules/cli/local/network/network.go

+
+	"github.com/docker/docker/api/types/network"
+
+	"github.com/docker/docker/api/types"


Nit: Don't need so many newlines between the dependencies; imports should be grouped by package name (See https://github.com/golang/go/wiki/CodeReviewComments#imports)

sigghhhhh I finally got this issue fixed, my IDE had a weird setting where it used gofmt for imports but goimports for the rest of the code 😐

ecs-cli/modules/cli/local/network/network.go

SoManyHs · 2019-05-17T18:12:28Z

ecs-cli/modules/cli/local/network/network.go

+	setupLocalEndpointsContainer(docker)
+}
+
+// setupLocalNetwork creates the local network if it doesn't already exist.


Point of discussion -- I tend to not be in favor of code comments if the functionality is obvious through good naming (which is the case here :) ). Also, I believe that the linter doesn't enforce code comments unless the function is exported?

Yeah, I agree that comments should describe things not obvious from the code :) I removed these private function comments except for createLocalEndpointsContainer since they don't add much value as you said.

The problem is I feel like even public functions suffer the same issue when they're obvious. Comments seem to violate DRY :/ Example from the std library https://golang.org/pkg/strings/#Contains

I honestly find writing good comments to be super difficult and it makes so much difference.

efekarakus · 2019-05-20T16:37:34Z

I'm going to go ahead and merge this change to keep us on schedule as I believe the feedback was addressed. Please, feel free to leave a comment if something stands out! I'll address it in a separate commit part of my next PR.

efekarakus requested review from allisaurus, SoManyHs, PettitWesley and piradeepk May 15, 2019 22:10

efekarakus force-pushed the setup-ecs-local-network branch 2 times, most recently from c7c4620 to 1137871 Compare May 15, 2019 22:41

PettitWesley reviewed May 16, 2019

View reviewed changes

ecs-cli/modules/cli/local/network/network.go Outdated Show resolved Hide resolved

PettitWesley reviewed May 16, 2019

View reviewed changes

allisaurus reviewed May 16, 2019

View reviewed changes

Add docker/client as a dependency

2b24dc7

SoManyHs approved these changes May 17, 2019

View reviewed changes

efekarakus force-pushed the setup-ecs-local-network branch from 1137871 to eb657c0 Compare May 17, 2019 21:40

Setup ECS local network and endpoints container on local up

8ef2ceb

efekarakus force-pushed the setup-ecs-local-network branch from bffc9ed to 8ef2ceb Compare May 20, 2019 16:21

efekarakus merged commit 8ef2ceb into aws:ecs-local May 20, 2019

efekarakus deleted the setup-ecs-local-network branch May 20, 2019 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup ecs local network #773

Setup ecs local network #773

efekarakus commented May 15, 2019

efekarakus commented May 15, 2019

PettitWesley May 16, 2019

PettitWesley May 17, 2019

PettitWesley May 17, 2019

efekarakus May 17, 2019

PettitWesley May 16, 2019

efekarakus May 17, 2019

PettitWesley May 20, 2019

PettitWesley May 16, 2019

efekarakus May 17, 2019

PettitWesley May 16, 2019

efekarakus May 17, 2019

efekarakus May 18, 2019

PettitWesley May 20, 2019

allisaurus May 16, 2019

efekarakus May 17, 2019

allisaurus May 16, 2019

efekarakus May 17, 2019

allisaurus May 16, 2019

efekarakus May 17, 2019

SoManyHs left a comment •

edited

Loading

SoManyHs May 17, 2019

efekarakus May 17, 2019

SoManyHs May 17, 2019

efekarakus May 17, 2019

efekarakus May 17, 2019

efekarakus commented May 20, 2019

		@@ -0,0 +1,217 @@
		// Copyright 2015-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.


		"github.com/docker/docker/api/types/network"

		"github.com/docker/docker/api/types"

Setup ecs local network #773

Setup ecs local network #773

Conversation

efekarakus commented May 15, 2019

efekarakus commented May 15, 2019

Integration Tests

Manual Tests

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

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

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

Choose a reason for hiding this comment

SoManyHs left a comment • edited Loading

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

efekarakus commented May 20, 2019

SoManyHs left a comment •

edited

Loading