Local create, Part the First #785

SoManyHs · 2019-06-10T04:02:35Z

Issue #, if available:

Description of changes:

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:

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.

$ dep ensure -update github.com/docker/go-units

TODO: fix terrible hack on convertLinuxParameters

These fields have the `yaml:",omitempty"` specification, so they will not be written into the docker compose yaml as long as the values are empty. For many of the ServiceConfig fields, an empty data structure is the same as nil as far as the Marshaller is concerned, so explicitly returning nil on an empty input is unnecessary.

TODO access context to create SSM/SM clients

- Create LocalProject interface, which holds the cli.Context - Acounts for -o flag for specifying custom docker compose file to write to

SoManyHs · 2019-06-10T04:37:40Z

For the following task-definition.json:

{
   "containerDefinitions": [
      {
         "image": "nginx",
         "name": "web"
      },
      {
         "command": [
            "/bin/sh -c \"echo '<html> <head> <title>Amazon ECS Sample App</title> <style>body {margin-top: 40px; background-color: #333;} </style> </head><body> <div style=color:white;text-align:center> <h1>Amazon ECS Sample App</h1> <h2>Congratulations!</h2> <p>Your application is now running on a container in Amazon ECS.</p> </div></body></html>' >  /usr/local/apache2/htdocs/index.html && httpd-foreground\""
         ],
         "entryPoint": [
            "sh",
            "-c"
         ],
         "essential": true,
         "image": "httpd:2.4",
         "logConfiguration": {
            "logDriver": "awslogs",
            "options": {
               "awslogs-group" : "/ecs/fargate-task-definition",
               "awslogs-region": "us-east-1",
               "awslogs-stream-prefix": "ecs"
            }
         },
	 "secrets": [{
		 "name": "MY_SECRET",
		 "valueFrom": "arn:aws:secretsmanager:us-west-2:xxxxxxxxxxx51:secret:MY_SECRET"
	 }],
         "name": "sample-fargate-app",
         "portMappings": [
            {
               "containerPort": 80,
               "hostPort": 80,
               "protocol": "tcp"
            }
         ]
      }
   ],
   "cpu": "256",
   "executionRoleArn": "arn:aws:iam::012345678910:role/ecsTaskExecutionRole",
   "family": "fargate-task-definition",
   "memory": "512",
   "networkMode": "awsvpc",
   "requiresCompatibilities": [
       "FARGATE"
    ]
}

Running ecs-cli local create (looks for task-definition.json in current dir, writes to docker-compose.local.yml)

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ ecs-cli local create
Successfully wrote ./docker-compose.local.yml

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ cat docker-compose.local.yml 
version: "3.0"
services:
  sample-fargate-app:
    command:
    - '/bin/sh -c "echo ''<html> <head> <title>Amazon ECS Sample App</title> <style>body
      {margin-top: 40px; background-color: #333;} </style> </head><body> <div style=color:white;text-align:center>
      <h1>Amazon ECS Sample App</h1> <h2>Congratulations!</h2> <p>Your application
      is now running on a container in Amazon ECS.</p> </div></body></html>'' >  /usr/local/apache2/htdocs/index.html
      && httpd-foreground"'
    entrypoint:
    - sh
    - -c
    environment:
      MY_SECRET: secretsmanager
    image: httpd:2.4
    logging:
      driver: awslogs
      options:
        awslogs-group: /ecs/fargate-task-definition
        awslogs-region: us-east-1
        awslogs-stream-prefix: ecs
    ports:
    - target: 80
      published: 80
      protocol: tcp
  web:
    image: nginx

If local create is re-run:

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ ecs-cli local create
./docker-compose.local.yml file already exists. Do you want to write over this file? [y/N]
n
FATA[0002] Error with local create: Aborted writing compose file. To retry, rename or move ./docker-compose.local.yml 

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ ecs-cli local create
./docker-compose.local.yml file already exists. Do you want to write over this file? [y/N]
y
Successfully wrote ./docker-compose.local.yml

With output flag:

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ ecs-cli local create -o foo.yml
Successfully wrote foo.yml

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ cat foo.yml 
version: "3.0"
services:
  sample-fargate-app:
    command:
    - '/bin/sh -c "echo ''<html> <head> <title>Amazon ECS Sample App</title> <style>body
      {margin-top: 40px; background-color: #333;} </style> </head><body> <div style=color:white;text-align:center>
      <h1>Amazon ECS Sample App</h1> <h2>Congratulations!</h2> <p>Your application
      is now running on a container in Amazon ECS.</p> </div></body></html>'' >  /usr/local/apache2/htdocs/index.html
      && httpd-foreground"'
    entrypoint:
    - sh
    - -c
    environment:
      MY_SECRET: secretsmanager
    image: httpd:2.4
    logging:
      driver: awslogs
      options:
        awslogs-group: /ecs/fargate-task-definition
        awslogs-region: us-east-1
        awslogs-stream-prefix: ecs
    ports:
    - target: 80
      published: 80
      protocol: tcp
  web:
    image: nginx

SoManyHs · 2019-06-10T04:42:50Z

Using --arn flag (to pull from registered task definition):

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ aws ecs describe-task-definition --task-definition test_fields
{
    "taskDefinition": {
        "status": "ACTIVE",
        "family": "test_fields",
        "placementConstraints": [],
        "requiresAttributes": [
            {
                "name": "com.amazonaws.ecs.capability.docker-remote-api.1.17"
            }
        ],
        "compatibilities": [
            "EC2"
        ],
        "volumes": [],
        "requiresCompatibilities": [
            "EC2"
        ],
        "taskDefinitionArn": "arn:aws:ecs:us-west-2:xxxxxxxxxxx51:task-definition/test_fields:71",
        "containerDefinitions": [
            {
                "dnsSearchDomains": [],
                "environment": [],
                "readonlyRootFilesystem": false,
                "pseudoTerminal": true,
                "name": "web",
                "links": [],
                "mountPoints": [],
                "image": "amazon/amazon-ecs-sample",
                "linuxParameters": {
                    "devices": [],
                    "capabilities": {}
                },
                "essential": true,
                "portMappings": [
                    {
                        "protocol": "tcp",
                        "containerPort": 80,
                        "hostPort": 80
                    }
                ],
                "dnsServers": [],
                "dockerSecurityOptions": [],
                "entryPoint": [],
                "privileged": false,
                "memory": 512,
                "command": [],
                "extraHosts": [],
                "cpu": 0,
                "volumesFrom": []
            }
        ],
        "revision": 71
    }
}

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ ecs-cli local create -a test_fields
./docker-compose.local.yml file already exists. Do you want to write over this file? [y/N]
y
Successfully wrote ./docker-compose.local.yml

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ cat docker-compose.local.yml
version: "3.0"
services:
  web:
    image: amazon/amazon-ecs-sample
    ports:
    - target: 80
      published: 80
      protocol: tcp
    tty: true

SoManyHs · 2019-06-10T04:49:49Z

Using --file flag for task definition:

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ ecs-cli local create --file foo.json 
./docker-compose.local.yml file already exists. Do you want to write over this file? [y/N]
y
Successfully wrote ./docker-compose.local.yml

~/go/src/github.com/aws/amazon-ecs-cli(local-create)$ cat docker-compose.local.yml 
version: "3.0"
services:
  sample-fargate-app:
    command:
    - '/bin/sh -c "echo ''<html> <head> <title>Amazon ECS Sample App</title> <style>body
      {margin-top: 40px; background-color: #333;} </style> </head><body> <div style=color:white;text-align:center>
      <h1>Amazon ECS Sample App</h1> <h2>Congratulations!</h2> <p>Your application
      is now running on a container in Amazon ECS.</p> </div></body></html>'' >  /usr/local/apache2/htdocs/index.html
      && httpd-foreground"'
    entrypoint:
    - sh
    - -c
    environment:
      MY_SECRET: secretsmanager
    image: httpd:2.4
    logging:
      driver: awslogs
      options:
        awslogs-group: /ecs/fargate-task-definition
        awslogs-region: us-east-1
        awslogs-stream-prefix: ecs
    ports:
    - target: 80
      published: 80
      protocol: tcp
  web:
    image: nginx

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

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

efekarakus · 2019-06-10T20:36:36Z

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

+func (c *secretsManagerClient) GetSecretValue(secretName string) (string, error) {
+	input := &secretsmanager.GetSecretValueInput{
+		SecretId:     aws.String(secretName),
+		VersionStage: aws.String("AWSCURRENT"), // VersionStage defaults to AWSCURRENT if unspecified


nit: Can we add a comment linking to https://docs.aws.amazon.com/secretsmanager/latest/apireference/API_GetSecretValue.html#API_GetSecretValue_RequestSyntax

Anything in particular that's confusing about this? We don't do this for other wrapped API methods on clients, but happy to make this clearer somehow.

No, nothing is confusing. I just had to look it up online to see if they had an explanation for "AWSCURRENT". I like providing a //See <link>, but it's a nitpick.

Given that it's the default value, I'm guessing we can probably remove this entirely? I lifted a lot of this code from the Secrets Manager console but i'm wondering now if it's even necessary.

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

efekarakus · 2019-06-10T20:41:29Z

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

+
+	client := secretsmanager.NewSecretsManagerClient(commandConfig)
+
+	secret, err := client.GetSecretValue(secretName)


dumb question: Is it okay for us to retrieve the value of their secret and output it in a file? Is there some sort of security concern here?

The call is tied to their config, so nobody would be able to do this unless they have the customer's creds. No different from how you can display a secret value in the Secrets Manager console. But maybe ping @allisaurus on this?

I share @efekarakus 's concerns re: outputting the secret contents to the file - I don't think the comparison to the console holds b/c now we are creating something that's persisting on the file system. Is there a way we can set the value as an env var and reference that var in the generated compose file?

Writing to env vars and writing to a file aren't really that different from a security perspective -- you could argue writing to an environment variable where other processes might read from it might be worse. One thing that might mitigate this a bit is to change the file permissions to 400 or 600, so only the current user has read/write permissions and no one else does.

In any case, I don't believe there is significant risk, but we could add a log message that says that we'll be pulling and writing the decrypting a secret to the file just to make this more obvious.

That makes sense! We'll consult with others offline to discuss what would be the right approach to move forward.

One of the other recommendations suggested by @hencrice is to pass the secrets as environment variables to the docker-compose up command with local up. So something like NAME=secret docker-compose -f docker-compose.local.yml up -d. We'll check if this would be a good approach.

In the mean time to unblock you, what do you think about keeping the client.GetSecretValue as is because it's guaranteed to be used but deleting this function?

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

Refactored Write to separate IO functionality from main caller, for easier testing

Since this file could potentialy include decrypted secrets, we don't want this to be readable by other users. TODO: allow file permissions to be configurable?

efekarakus

Mostly looks good to me, just two questions and the rest are conflicts that can emerge with @iamhopaul123's changes.

efekarakus · 2019-06-14T19:36:32Z

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

+	arn := p.context.String(flags.TaskDefinitionArnFlag)
+	filename := p.context.String(flags.TaskDefinitionFileFlag)
+
+	if arn != "" && filename != "" {


FYI: @iamhopaul123 is also making modifications for ECS local :) he needed this same logic in local ps and local down so it was moved to a function here: https://github.com/aws/amazon-ecs-cli/pull/789/files#diff-d09e4b573b1ca106569f2a8dfcad6cd0R50

efekarakus · 2019-06-14T19:39:15Z

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

+	}
+
+	if taskDefinition == nil {
+		return fmt.Errorf("Could not detect valid Task Definition")


nit: The recommendation for errors is to start with lowercase: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

efekarakus · 2019-06-14T19:40:33Z

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

+	return config.NewCommandConfig(context, rdwr)
+}
+
+// FIXME: NOTE this will actually read from either ARN or Task Definition family name


@iamhopaul123 added a new flag for this here: https://github.com/aws/amazon-ecs-cli/pull/789/files#diff-9c07acb4867a9b5d46a004942a378e76R153

efekarakus · 2019-06-14T19:50:38Z

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

+
+	client := secretsmanager.NewSecretsManagerClient(commandConfig)
+
+	secret, err := client.GetSecretValue(secretName)


That makes sense! We'll consult with others offline to discuss what would be the right approach to move forward.

One of the other recommendations suggested by @hencrice is to pass the secrets as environment variables to the docker-compose up command with local up. So something like NAME=secret docker-compose -f docker-compose.local.yml up -d. We'll check if this would be a good approach.

In the mean time to unblock you, what do you think about keeping the client.GetSecretValue as is because it's guaranteed to be used but deleting this function?

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

efekarakus · 2019-06-14T19:52:43Z

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

+const (
+	LocalOutDefaultFileName = "./docker-compose.local.yml"
+	LocalOutFileMode        = os.FileMode(0644) // Owner=read/write, Other=readonly
+	LocalInFileName         = "./task-definition.json"


Was there a reason why we didn't address this comment? :p

efekarakus

Talked offline, we will address this comment as follow-up: #785 (comment)

SoManyHs added 25 commits May 29, 2019 15:41

Write to docker-compose.local.yml with confirmation

c9e5909

Read task definition

feaa70b

Refactor control flow

604c4ac

Fix license, add package comment

6bd8470

Add assertion for TTY in convert task definition

e44779a

WIP convert task def

e4350dc

Update docker/go-units

83b374d

$ dep ensure -update github.com/docker/go-units

Add convertToTmpfs

8b592f8

TODO: fix terrible hack on convertLinuxParameters

Add convertUlimits

4a31e38

Add convertLinuxParameters func

462e3f0

Add convertDevices

b86003e

Add ConvertShmSize

97dbea3

Add convertEnvironment, capAdd, capDrop

6c4a229

Convert extrahosts

88f54bb

Convert HealthCheck

8ba0ae0

Convert labels

337bd2b

Convert Logging

c0ca8a0

Convert volumes

c3fdc46

Convert ports

3a2f29b

Convert StdinOpen

40de9e7

Convert Sysctls

3939b11

WIP add secrets

fe83f86

TODO access context to create SSM/SM clients

Refactor to encapsulate read/write/conversion logic into interface

c8a8dfb

- Create LocalProject interface, which holds the cli.Context - Acounts for -o flag for specifying custom docker compose file to write to

go fmt

84b079c

SoManyHs requested a review from efekarakus June 10, 2019 04:02

SoManyHs added 2 commits June 9, 2019 21:22

WIP Get Secret Value from secret manager

9bb2696

Ran make generate

5aa6f62

SoManyHs force-pushed the local-create branch from 9c77f19 to 5aa6f62 Compare June 10, 2019 04:23

Fix bug in ReadTaskDefinition

b695e4f

Fix bug in convert shmsize

52b4b0d

efekarakus requested a review from a team June 10, 2019 19:49

efekarakus reviewed Jun 10, 2019

View reviewed changes

SoManyHs requested a review from allisaurus June 10, 2019 21:17

SoManyHs added 3 commits June 10, 2019 18:10

Add public docstrings, cleanup formatting

d734cc9

Unit test localproject.Write

3a73a58

Refactored Write to separate IO functionality from main caller, for easier testing

Change file permissions on local compose output

4825679

Since this file could potentialy include decrypted secrets, we don't want this to be readable by other users. TODO: allow file permissions to be configurable?

efekarakus reviewed Jun 14, 2019

View reviewed changes

SoManyHs mentioned this pull request Jun 18, 2019

Add task-def and file flags to local down/ps #789

Merged

4 tasks

Merge branch 'ecs-local' into local-create

86efe88

SoManyHs changed the title ~~Local create~~ Local create, Part the First Jun 18, 2019

efekarakus approved these changes Jun 18, 2019

View reviewed changes

SoManyHs merged commit 86efe88 into aws:ecs-local Jun 18, 2019

efekarakus mentioned this pull request Jun 18, 2019

Use filepath.Join for default ECS local I/O files #796

Closed

hencrice mentioned this pull request Jun 19, 2019

Update output file name #798

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local create, Part the First #785

Local create, Part the First #785

SoManyHs commented Jun 10, 2019 •

edited by iamhopaul123

Loading

SoManyHs commented Jun 10, 2019

SoManyHs commented Jun 10, 2019 •

edited

Loading

SoManyHs commented Jun 10, 2019

efekarakus Jun 10, 2019

SoManyHs Jun 10, 2019

efekarakus Jun 10, 2019

SoManyHs Jun 11, 2019

efekarakus Jun 10, 2019

SoManyHs Jun 10, 2019

allisaurus Jun 11, 2019 •

edited

Loading

SoManyHs Jun 13, 2019

efekarakus Jun 14, 2019

efekarakus left a comment

efekarakus Jun 14, 2019

efekarakus Jun 14, 2019

efekarakus Jun 14, 2019 •

edited

Loading

efekarakus Jun 14, 2019

efekarakus Jun 14, 2019

efekarakus left a comment


		client := secretsmanager.NewSecretsManagerClient(commandConfig)

		secret, err := client.GetSecretValue(secretName)

Local create, Part the First #785

Local create, Part the First #785

Conversation

SoManyHs commented Jun 10, 2019 • edited by iamhopaul123 Loading

SoManyHs commented Jun 10, 2019

SoManyHs commented Jun 10, 2019 • edited Loading

SoManyHs commented Jun 10, 2019

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

allisaurus Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus left a comment

Choose a reason for hiding this comment

SoManyHs commented Jun 10, 2019 •

edited by iamhopaul123

Loading

SoManyHs commented Jun 10, 2019 •

edited

Loading

allisaurus Jun 11, 2019 •

edited

Loading

efekarakus Jun 14, 2019 •

edited

Loading