Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for secrets #61

Merged
merged 10 commits into from Jun 13, 2019

Conversation

@mhvelplund
Copy link
Collaborator

mhvelplund commented Apr 11, 2019

The module now supports an extra parameter container_secrets, similar to
container_envvars.

Values in this map will be treated as SSM keys and the actual value
injected into the environment of the container will be pulled from SSM.
To work, the SSM keys must also be added to ssm_paths.

Example:

container_secrets = {
  DB_USER = "arn:aws:ssm:eu-west-1:123456789012:parameter/myapp/dev/db.user"
  DB_PASSWORD = "/myapp/dev/db.password"
}
ssm_paths         = ["myapp/dev"]

Note that non-ARN key references begin with a slash, but the SSM path doesn't.

The example above injects the (decrypted) contents of
/myapp/dev/db.password into the env. variable DB_PASSWORD.

Mads Hvelplund
The module now supports an extra parameter `container_secrets`, similar to
`container_envvars`.

Values in this map will be treated as SSM keys and the actual value
injected into the environment of the container will be pulled from SSM.
To work, the SSM keys must also be added to `ssm_paths`.

Example:

```
container_secrets = {
  DB_PASSWORD = "myapp/dev/db.password"
}c
ssm_paths         = ["myapp/dev"]
```

The example above injects the (decrypted) contents of
`/myapp/dev/db.password` into the env. variable `DB_PASSWORD`.
Copy link
Collaborator

maartenvanderhoef left a comment

Cool addition! Current downside is that there is no drift detection for secrets, meaning if you add a secret it will not recognize it as such. You could add a block here and use a container_label to store a md5 sum of the secrets used for later comparison:

and here

lookup(local.docker_labels[0],"_airship_dockerlabel_hash","") != var.live_aws_ecs_task_definition_docker_label_hash ||

To be able to get drift detection.

main.tf Outdated Show resolved Hide resolved
@mhvelplund

This comment has been minimized.

Copy link
Collaborator Author

mhvelplund commented Apr 26, 2019

I got around to look at the drift detection you requested in the PR above. Looking at the code, I'm uncertain about how to proceed.

In the lookup lambda, i can see where you get the environment vars, but it doesn't look like javascript's describetaskdefintion returns the secrets as part of the container ...

I use ecs-deploy on my CI server to update the container image when the code changes. The script fetches the running taskdef and replace the image before uploading a new task def.

However, if I run terraform to update something else, it detects a "change" and wants to downgrade to the last task defintion created with Terraform.

I'm not sure if this is related to missing drift detection on secrets, or if it's because ecs-deploy removes something that Airship uses to detect the newest image.

@mhvelplund

This comment has been minimized.

Copy link
Collaborator Author

mhvelplund commented May 27, 2019

Hmm. Travis built the code (https://travis-ci.org/blinkist/terraform-aws-airship-ecs-service/builds/537776545) but the check result hasn't been fed back. Not sure why.

Anyway, I think I fixed the drift detectionas you requested. I had to finish the docker label support to do so, so I don't think anyone has tried to use that before :)

What do you think about the PR now?

@Jamie-BitFlight Jamie-BitFlight self-assigned this Jun 11, 2019
Copy link
Collaborator

Jamie-BitFlight left a comment

Thank you.

variables.tf Outdated Show resolved Hide resolved
Mads Hvelplund added 3 commits Jun 13, 2019
As part of that, I removed the security group rules, since they only
replicated the default behaviour of the default SG, but added some
brittleness in the step that discovers the NLB's ip addresses.
from_port = "32768"
to_port = "65535"
protocol = "tcp"
cidr_blocks = ["${formatlist("%s/32",sort(distinct(compact(concat(list(""),data.aws_network_interface.nlb.private_ips)))))}"]

This comment has been minimized.

Copy link
@mhvelplund

mhvelplund Jun 13, 2019

Author Collaborator

I pondered how to do something similar, since getting the NLB's IP is difficult. However, this only seems to work on the first run of the example, and fail on subsequent runs since there are now two interfaces in the subnet.

AFAICT it isn't needed either, since we are running in the default VPC that already has a fitting SG.

This comment has been minimized.

Copy link
@Jamie-BitFlight

Jamie-BitFlight Jun 13, 2019

Collaborator

In subsequent runs what error does it fail with? It should handle having more network interfaces.

This comment has been minimized.

Copy link
@mhvelplund

mhvelplund Jun 13, 2019

Author Collaborator

I'm not absolutely sure, but the issue only happened when I added a new service that was dependent on the same NLB. When data.aws_network_interface.nlb.private_ips is resolved in the original example folder there is only the one interface belonging to the NLB, but after running, the first service creates a new interface, causing the data element to fail resolution because there are now two interfaces and it only allows one. It might be timing specific, and it might be something resolveable by making the filter tighter, but ultimately it didn't look like it was needed since the default SG only allows traffic from itself, and it would be enough to ad the rule that allows access from the developers own IP. Am I missing something?

@mhvelplund

This comment has been minimized.

Copy link
Collaborator Author

mhvelplund commented Jun 13, 2019

@maartenvanderhoef @Jamie-BitFlight
Ok, so I added an example of how to use the new secrets to the example folder. Along the way I added the "sleep.cmd" file that is needed to deploy Airship's ecs-cluster module from Windows. I debated with myself if it was passive agressive, but my coworker convinced me that it might make you remove the "sleep 30" hack from the other module ;)

@Jamie-BitFlight Jamie-BitFlight dismissed maartenvanderhoef’s stale review Jun 13, 2019

These changes have been made, and the drift detection resolved. But I can't see how to approve it otherwise.

@Jamie-BitFlight Jamie-BitFlight merged commit aafb884 into blinkist:master Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.