Support secrets with "local up" by efekarakus · Pull Request #808 · aws/amazon-ecs-cli

efekarakus · 2019-06-24T22:42:41Z

Issue #, if available: #797

Description of changes:

Retrieves the secret ARNs from the Compose file by parsing the service labels.
Queries either SSM or SecretsManager to get the decrypted secrets.
Starts the local task with the secrets as env variables.

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

Contacted our doc writer
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-06-25T01:01:21Z

Manual Tests

Given

My remote task definition is defined as:

{
    "taskDefinition": {
        "taskDefinitionArn": "arn:aws:ecs:us-east-1:111111111:task-definition/TaskForECSLocalWithSecrets:10",
        "containerDefinitions": [
            {
                "name": "nginx",
                "image": "nginx",
                "cpu": 0,
                "portMappings": [],
                "essential": true,
                "environment": [],
                "mountPoints": [],
                "volumesFrom": [],
                "secrets": [
                    {
                        "name": "DEFAULT_DB_PASSWORD",
                        "valueFrom": "TEST_DB_PASSWORD"
                    },
                    {
                        "name": "IAD_DB_PASSWORD",
                        "valueFrom": "arn:aws:ssm:us-east-1:111111111:parameter/TEST_DB_PASSWORD"
                    },
                    {
                        "name": "TestSecret1",
                        "valueFrom": "arn:aws:secretsmanager:us-east-1:111111111:secret:alpha/efe/local-j0gCbT"
                    },
                    {
                        "name": "DUB_DB_PASSWORD",
                        "valueFrom": "arn:aws:ssm:eu-west-1:111111111:parameter/TEST_DUB_DB_PASSWORD"
                    },
                    {
                        "name": "PDX_DB_PASSWORD",
                        "valueFrom": "arn:aws:ssm:us-west-2:111111111:parameter/TEST_PDX_DB_PASSWORD"
                    }
                ]
            }
        ],
        "family": "TaskForECSLocalWithSecrets",
        "executionRoleArn": "arn:aws:iam::111111111:role/ecsTaskExecutionRole",
        "networkMode": "awsvpc",
        "revision": 10,
        "volumes": [],
        "status": "ACTIVE",
        "requiresAttributes": [
            {
                "name": "ecs.capability.secrets.asm.environment-variables"
            },
            {
                "name": "com.amazonaws.ecs.capability.docker-remote-api.1.18"
            },
            {
                "name": "ecs.capability.task-eni"
            },
            {
                "name": "ecs.capability.secrets.ssm.environment-variables"
            }
        ],
        "placementConstraints": [],
        "compatibilities": [
            "EC2",
            "FARGATE"
        ],
        "requiresCompatibilities": [
            "EC2"
        ],
        "cpu": "256",
        "memory": "512"
    }
}

When

Ran the following command:

$ ecs-cli local up -t arn:aws:ecs:us-east-1:685593908319:task-definition/TaskForECSLocalWithSecrets:10
docker-compose.local.yml file already exists. Do you want to write over this file? [y/N]
y
Successfully wrote docker-compose.local.yml
INFO[0001] The network ecs-local-network already exists 
INFO[0001] The amazon-ecs-local-container-endpoints container already exists with ID 05e41907637fe79a8ad7a0d4fea4b862c3b37aea801afdbc59822337e76d8722 
INFO[0001] Started container with ID 05e41907637fe79a8ad7a0d4fea4b862c3b37aea801afdbc59822337e76d8722 
Recreating local-cmds_nginx_1 ... done

Then

✅ The environment variables passed to the container are correct:

PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=daecdc705a5d
DEFAULT_DB_PASSWORD=1234
DUB_DB_PASSWORD=DUB1234
IAD_DB_PASSWORD=1234
PDX_DB_PASSWORD=PDX1234
TestSecret1=DevX
NGINX_VERSION=1.17.0
NJS_VERSION=0.3.2
PKG_RELEASE=1~stretch
HOME=/root

iamhopaul123 · 2019-06-24T23:48:53Z

+	secretARN := aws.StringValue(cs.secret.ValueFrom)
+	parsedARN, err := arn.Parse(secretARN)
+	if err != nil {
+		if strings.Contains(err.Error(), "arn: invalid prefix") {


I am a little bit confused about this: arn.Parse errors "arn: invalid prefix" when the secretARN does not begin with "arn:". So why does ServiceName return to "ssm" if it errors out as "arn: invalid prefix"? It could be an invalid secretARN.

Good question! I tried to explain it in the comment inside this if-block.The ECS Task Definition also accepts a value that's not an ARN which is just the parameter name in SSM (see https://docs.aws.amazon.com/AmazonECS/latest/developerguide/specifying-sensitive-data.html#secrets-logconfig). So this function will also default to SSM.

If it's just an invalid ARN then we will try to look it up in SSM and the request will fail, so the user will receive an error while trying to start the container with local up. Does that help?

I see. So SSM is the default one you set. Then, that will make sense!

iamhopaul123 · 2019-06-25T21:12:28Z

 func upCommand() cli.Command {
 	return cli.Command{
 		Name:   "up",
 		Usage:  "Create a Compose file from an ECS task definition and run it.",


Is that kind of too much overlap compared to createCommand? Except for running it.

Yes but that's expected. We split create and up because it's possible that a user wants to modify the Compose file after running create and then they can just run up.

I just created a backlog task to change the behavior of local create slightly: #809

SoManyHs · 2019-06-25T20:59:23Z

-// structure to a docker compose schema, which will be written to a
-// docker-compose.local.yml file.
-
+// Package converter converts an ecs.TaskDefinition or a yaml file to a docker compose schema.


Comment here a tad confusing, since the task definition file can also be JSON.

👍 agreed, changed to // Package converter translates entities to a docker compose schema and vice versa.

SoManyHs · 2019-06-25T21:01:53Z

 // ConvertToDockerCompose creates the payload from an ECS Task Definition to be written as a docker compose file
 func ConvertToDockerCompose(taskDefinition *ecs.TaskDefinition) ([]byte, error) {
-	services := []composeV3.ServiceConfig{}
+	var services []composeV3.ServiceConfig


Is instantiating this with a var (e.g. nil slice) because we want the JSON marshalling to return nil versus an empty slice (in line 68)? (See: https://golang.org/pkg/encoding/json/#Marshal)

I didn't think that much about it :P, it's just because that's the recommended way of initializing empty slices: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

Sure. Ultimately, we should have a check that there is at least one service defined anyway; whether its pre-marshalled form is nil or empty shouldn't matter that much i don't think.

SoManyHs · 2019-06-25T21:31:47Z

+
+type secretsManagerDecrypter struct {
+	client *secretsmanager.SecretsManager
+}


hmmmm what do you think about having these client objects live somewhere else? I feel like the _app.go files should only contain "controller" logic rather than "model" logic (if we're thinking about this from a MVC sort of perspective). It feels sort of weird to have these and their DecryptMethod functions defined here.

Also, the fact that this is duplicated maybe hints at having a secretsDecrypter interface that can be implemented by either SSM or SecretsManager.

hmmmm what do you think about having these client objects live somewhere else?

That makes sense, I moved it to a package under local/secrets/clients.

Also, the fact that this is duplicated maybe hints at having a secretsDecrypter interface that can be implemented by either SSM or SecretsManager.

That's actually exactly what happens now. These two structs implement the SecretDecrypter interface defined here: https://github.com/aws/amazon-ecs-cli/pull/808/files#diff-798f5fba8ffe42058a2ac18e2adb3d7cR32

Ah, got it. Any reason not to put the interface in the same place as the implementation? (non-blocking)

Just to follow the guidelines:

Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values. The implementing package should return concrete (usually pointer or struct) types: that way, new methods can be added to implementations without requiring extensive refactoring.

https://github.com/golang/go/wiki/CodeReviewComments#interfaces

SoManyHs · 2019-06-25T21:35:27Z

+
+		decrypted := ""
+		err = nil
+		if service == secretsmanager.ServiceName {


Nit: You could use a switch case statement here instead.

This test is broken until we implement the full cycle with local create -> local up -> local down. Removing it for now.

SoManyHs

🎉

efekarakus requested review from a team, SoManyHs and iamhopaul123 June 24, 2019 22:42

efekarakus changed the title ~~Local/up with secrets~~ Support secrets with "local up" Jun 24, 2019

efekarakus force-pushed the local/up-with-secrets branch from e7426f5 to 2e80b3d Compare June 25, 2019 04:27

iamhopaul123 reviewed Jun 25, 2019

View reviewed changes

SoManyHs reviewed Jun 25, 2019

View reviewed changes

efekarakus added 7 commits June 25, 2019 15:30

Replace --arn flag with --task-def in "local create"

f83da8b

Rename directory to localproject to be consistent with the package name

ce21f71

Support "local up" to start a Compose project with secrets

732bb35

Move container secrets to its own package

89ac7d4

Enable cross-region parameter value retrieval

64628c3

Remove "run a single local ECS task" e2e test case

dfddf36

This test is broken until we implement the full cycle with local create -> local up -> local down. Removing it for now.

Move SSM and SecretManager Decrypter structs to their own package

9dfd058

efekarakus force-pushed the local/up-with-secrets branch from 2e80b3d to 9dfd058 Compare June 26, 2019 04:05

SoManyHs approved these changes Jun 26, 2019

View reviewed changes

efekarakus merged commit 9dfd058 into aws:ecs-local Jun 26, 2019

efekarakus deleted the local/up-with-secrets branch June 26, 2019 16:39

SoManyHs mentioned this pull request Jul 2, 2019

Read arn region #844

Merged

7 tasks

keen99 mentioned this pull request Aug 7, 2024

local up with secretsmanager secrets doesnt support all valid arn formats #1169

Open

Conversation

efekarakus commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efekarakus commented Jun 25, 2019

Manual Tests

Given

When

Then

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SoManyHs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

efekarakus commented Jun 24, 2019 •

edited

Loading

iamhopaul123 Jun 25, 2019 •

edited

Loading