Print whole container_definitions in aws_ecs_task_definition instead of checksum in the plan #1195

s-maj · 2017-07-20T15:44:39Z

Currently, when aws_ecs_task_definition is updated, people can see only container_definitions checksum in the plan. There is no visibility what exactly is being pushed to ECS and I think this quite confusing. In other resources, like aws_iam_policy, old version of policy and new version policy is visible in the plan. Also, this change would allow to utilize terraform-landspace to produce a pretty plan for ECS tasks (screenshot).

Tests:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsTaskDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsTaskDefinition -timeout 120m
=== RUN   TestAccAWSEcsTaskDefinition_basic
--- PASS: TestAccAWSEcsTaskDefinition_basic (19.93s)
=== RUN   TestAccAWSEcsTaskDefinition_withScratchVolume
--- PASS: TestAccAWSEcsTaskDefinition_withScratchVolume (12.85s)
=== RUN   TestAccAWSEcsTaskDefinition_withEcsService
--- PASS: TestAccAWSEcsTaskDefinition_withEcsService (138.14s)
=== RUN   TestAccAWSEcsTaskDefinition_withTaskRoleArn
--- PASS: TestAccAWSEcsTaskDefinition_withTaskRoleArn (19.50s)
=== RUN   TestAccAWSEcsTaskDefinition_withNetworkMode
--- PASS: TestAccAWSEcsTaskDefinition_withNetworkMode (21.53s)
=== RUN   TestAccAWSEcsTaskDefinition_constraint
--- PASS: TestAccAWSEcsTaskDefinition_constraint (10.78s)
=== RUN   TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource
--- PASS: TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource (19.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	242.187s

astawiarski · 2017-07-20T15:49:17Z

This would be an incredible improvement - it would solve our current visibility issues during the plan stage.

s-maj · 2017-08-16T10:13:16Z

Hi @radeksimko! Is there a chance to merge this soon? I took a look at the latest Hashicorp blog post (https://www.hashicorp.com/blog/continuous-deployment-with-nomad-and-terraform/) and even Nomad provider uses full jobspec instead of hash :)

Ninir · 2017-08-17T21:12:26Z

Hey folks,

In my opinion, this is a great addition. The thing that bothers me is that people will see a diff when migrating from the hash-ed version to this plain-text one.
I would like to keep it silent, using the DiffSuppressFunc, thus being able to detect whether the value is encrypted or not...

What do you think @radeksimko, is it worth it?

radeksimko · 2017-08-21T12:20:22Z

Hi @s-maj
thanks for the contribution.
The idea looks reasonable, I can't remember why did I use the hash back then in the initial implementation 🤔

The thing that bothers me is that people will see a diff when migrating from the hash-ed version to this plain-text one.

Yep, that's exactly my concern, too.

We can't merge this as is otherwise we'd upset many users that have existing task definitions in their state and schema change like this would cause recreation of the whole TD because hash != content from diff perspective. Just try creating TD with current stable version of Terraform/AWS provider, then run plan with compiled provider from your branch and you'll see what I mean.

The best (and idiomatic) way to address this is to add a state migration.

Take a look at existing state migrations and tests if you need any inspiration and/or let me know if anything's unclear in regards to migrations.

Here's full diff (hopefully) explaining what it takes to do the whole migration: e253f88

s-maj · 2017-08-21T15:01:33Z

Hi @radeksimko

Yep, totally.

About schema migration, from what I understand, task definition "blob" is being reduced to sha1 hash and this is stored in state file as an attribute of the resource. By doing migration I can only operate on the hash which might quite hard to reverse :)

I guess I need to replace old value in this field with new value from the input, but I don't if this is possible or if it's a good approach.

radeksimko · 2017-08-21T15:43:41Z

I guess I need to replace old value in this field with new value from the input, but I don't if this is possible or if it's a good approach.

There isn't any way to get access to the config, which is probably by design.

I think the best way forward is to pull down the current body from the ECS API. The migration function interface has meta which contains the ECS connection, so you should be able to do that.

It may not always be accurate, because state may be drifted from the reality, but Terraform should refresh the state automatically as part of plan or apply, unless the user passed -refresh=false.

s-maj · 2017-08-27T13:05:22Z

Alright, I guess it should look like this:

conn := meta.(*AWSClient).ecsconn

out, err := conn.DescribeTaskDefinition(&ecs.DescribeTaskDefinitionInput{
	TaskDefinition: aws.String(is.Attributes["arn"]),
})

if err != nil {
	return is, nil
}

taskDefinition := out.TaskDefinition

strB, _ := json.Marshal(taskDefinition.ContainerDefinitions)
is.Attributes["container_definitions"] = string(strB)

but I am getting tons of nulls or empty lists (fields not present in Terraform) in the output:

[{\"Command\":null,\"Cpu\":10,\"DisableNetworking\":null,\"DnsSearchDomains\":null,\"DnsServers\":null,\"DockerLabels\":null,\"DockerSecurityOptions\":null,\"EntryPoint\":null,\"Environment\":[],\"Essential\":true,\"ExtraHosts\":null,\"Hostname\":null,\"Image\":\"service-first\",\"Links\":null,\"LogConfiguration\":null,\"Memory\":512,\"MemoryReservation\":null,\"MountPoints\":[],\"Name\":\"first\",\"PortMappings\":[{\"ContainerPort\":80,\"HostPort\":80,\"Protocol\":\"tcp\"}],\"Privileged\":null,\"ReadonlyRootFilesystem\":null,\"Ulimits\":null,\"User\":null,\"VolumesFrom\":[],\"WorkingDirectory\":null},{\"Command\":null,\"Cpu\":10,\"DisableNetworking\":null,\"DnsSearchDomains\":null,\"DnsServers\":null,\"DockerLabels\":null,\"DockerSecurityOptions\":null,\"EntryPoint\":null,\"Environment\":[],\"Essential\":true,\"ExtraHosts\":null,\"Hostname\":null,\"Image\":\"service-second\",\"Links\":null,\"LogConfiguration\":null,\"Memory\":256,\"MemoryReservation\":null,\"MountPoints\":[],\"Name\":\"second\",\"PortMappings\":[{\"ContainerPort\":443,\"HostPort\":443,\"Protocol\":\"tcp\"}],\"Privileged\":null,\"ReadonlyRootFilesystem\":null,\"Ulimits\":null,\"User\":null,\"VolumesFrom\":[],\"WorkingDirectory\":null}]" => "[\n  {\n    \"name\": \"first\",\n    \"image\": \"service-first\",\n    \"cpu\": 10,\n    \"memory\": 512,\n    \"essential\": true,\n    \"portMappings\": [\n      {\n        \"containerPort\": 80,\n        \"hostPort\": 80\n      }\n    ]\n  },\n  {\n    \"name\": \"second\",\n    \"image\": \"service-second\",\n    \"cpu\": 10,\n    \"memory\": 256,\n    \"essential\": true,\n    \"portMappings\": [\n      {\n        \"containerPort\": 443,\n        \"hostPort\": 443\n      }\n    ]\n  }\n]\n" (forces new resource)

which is more or less exptected since json:",omitempty" is not set in ContainerDefinition. There is a ticket about this topic (aws/aws-sdk-go#271) but it's not going to happen any time soon.

I tried to work on string output from SDK but even AWS API responds with empty fields:

2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws: 2017/08/27 13:48:25 [DEBUG] Received task definition {
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:   TaskDefinition: {
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:     ContainerDefinitions: [{
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Cpu: 10,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Environment: [],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Essential: true,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Image: "service-first",
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Memory: 512,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         MountPoints: [],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Name: "first",
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         PortMappings: [{
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:             ContainerPort: 80,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:             HostPort: 80,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:             Protocol: "tcp"
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:           }],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         VolumesFrom: []
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:       },{
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Cpu: 10,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Environment: [],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Essential: true,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Image: "service-second",
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Memory: 256,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         MountPoints: [],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         Name: "second",
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         PortMappings: [{
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:             ContainerPort: 443,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:             HostPort: 443,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:             Protocol: "tcp"
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:           }],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:         VolumesFrom: []
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:       }],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:     Family: "service2",
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:     PlacementConstraints: [],
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:     Revision: 1,
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:     Status: "ACTIVE",
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:     TaskDefinitionArn: "arn:aws:ecs:eu-west-2:XXXXXXXXXXXX:task-definition/service2:1",
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:     Volumes: []
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws:   }
2017/08/27 13:48:25 [DEBUG] plugin: terraform-provider-aws: }

where orginal task definion is:

[
  {
    "name": "first",
    "image": "service-first",
    "cpu": 10,
    "memory": 512,
    "essential": true,
    "portMappings": [
      {
        "containerPort": 80,
        "hostPort": 80
      }
    ]
  },
  {
    "name": "second",
    "image": "service-second",
    "cpu": 10,
    "memory": 256,
    "essential": true,
    "portMappings": [
      {
        "containerPort": 443,
        "hostPort": 443
      }
    ]
  }
]

resource "aws_ecs_task_definition" "service" {
  family                = "service2"
  container_definitions = "${file("service.json")}"
}

@radeksimko could you please advise how to approach this?

radeksimko · 2017-08-29T05:58:44Z

I see - that's annoying - it might have been the reason why I originally decided to use hash instead 😄

However we now have some ways to deal with equivalent JSON structures which aren't exactly same in string-based match. Take a look at https://github.com/terraform-providers/terraform-provider-aws/blob/fd6ad1d8c976ee779ff9a543d1ad128166959d01/aws/resource_aws_cloudwatch_dashboard.go#L40

which should prevent spurious diffs from appearing. I hope the problem is generic enough we can use the existing generic JSON helper. Eventually we'd need to create a custom one, similar to the one we did for IAM policies https://github.com/terraform-providers/terraform-provider-aws/blob/fd6ad1d8c976ee779ff9a543d1ad128166959d01/aws/resource_aws_iam_policy.go#L42

which is work worth its own package/repository.

s-maj · 2017-09-20T12:10:37Z

Existing diff suppression functions won't help in this case and fixing this is a bit beyond my skill set :) But it seems NLB will be part of 1.0.0 which will break backward compatibility for ECS services (at least web based containers) so it might good moment to add this PR to this mix (maybe along with #1476)

radeksimko · 2017-11-08T16:03:19Z

@s-maj I just pushed a few enhancements to your branch:

state migration, so it's not a breaking change anymore
diff suppression function, to avoid showing no-op differences between API representation and config, like empty array and no array
fixed flattening of definitions, so that we can correctly show genuine differences during plan

Can you take a look at those changes and try them out to see if it's working for you as expected in your context?

btw. the reason you were getting the odd diff with null values is because the SDK struct does not use standard json tags which means that standard (un)marshaler doesn't (un)marshal the data correctly. I had to use the custom marshaler from SDK's package.

catsby

👍 from me. Will defer for another 👍 from @s-maj as Radek has asked them to review as well

radeksimko · 2017-11-09T15:43:48Z

FYI: I will merge this on Monday, 13th if there's no further feedback.

s-maj · 2017-11-09T15:45:08Z

I'll take a look over weekend. Thanks!

…

On Thu, Nov 9, 2017 at 3:43 PM, Radek Simko ***@***.***> wrote: FYI: I will merge this on Monday, 13th if there's no further feedback. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOhaWsz6Dz6Y4a2rk7bL72UlqUdDqkLVks5s0x26gaJpZM4OeTS7> .

s-maj · 2017-11-11T18:25:16Z

@radeksimko All grand, thanks a million!

achille-roussel · 2017-11-17T00:00:24Z

Hey guys, we're seeing terraform recreate the task definitions on every plan and all that changed is the version of the AWS provider on our end.

Is anybody else experiencing this issue?

bturbes · 2017-11-17T02:31:31Z

We are also seeing this same behavior.

ziggythehamster · 2017-11-17T02:55:12Z

Also seeing it here.

This code needs to be amended so that empty arrays coming back from AWS are treated the same as the JSON missing the empty array. Also, the environment array can come back in any order from AWS even if your task definition has them in a different order.

nathanielks · 2017-11-17T03:42:37Z

likewise!

ckyoog · 2017-12-12T19:54:40Z

Hi @radeksimko This change is good, but I prefer the chksum way. In my case, my container definition contains some credential information, I don't want them to be printed out on screen when doing plan. Could you please at least offer an option to make it still print chksum number instead of whole JSON text?

tomelliff · 2017-12-12T19:58:16Z

I wouldn't recommend putting secrets in the task definition. I presume you're injecting them as environment variables? You might want to consider fetching them from SSM parameter store or something like Vault at runtime in an entrypoint script.

ckyoog · 2017-12-13T03:21:31Z

@tomelliff, yes, you guess right. I know it is not a good idea to use environment variables for such thing. But anyway, I think it still might be useful to print and store chksum in some situations (?). I will appreciate it very much if you can think about it. :)

achille-roussel · 2017-12-13T05:51:19Z

This change also has a sad side effect of making the plan output so large that its truncated in the terraform enterprise web view, making the plan impossible to review.

It’s kind of a TFE issue but making it somehow an opt-in feature would be useful in that case.

ghost · 2020-04-08T17:45:01Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

instead of calculating checksum, print whole container definition

a771e52

radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 11, 2017

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 21, 2017

radeksimko mentioned this pull request Sep 26, 2017

Add support for batch_job_definition and batch_job_queue #1710

Merged

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 16, 2017

radeksimko modified the milestones: v1.2.0, v1.3.0 Oct 26, 2017

radeksimko added 3 commits November 8, 2017 15:34

r/ecs_task_definition: Add state migration

6ea8fa5

r/ecs_task_definition: Implement DiffSuppressFunc

bc8279a

r/ecs_task_definition: Fix flattening of container definitions

9b4f1fd

radeksimko requested a review from catsby November 8, 2017 16:05

catsby approved these changes Nov 8, 2017

View reviewed changes

radeksimko merged commit 6452ef1 into hashicorp:master Nov 12, 2017

ziggythehamster mentioned this pull request Nov 17, 2017

aws_ecs_task_definition: Regression from issue #1195 causing an endless cycle of diffs #2336

Closed

catsby mentioned this pull request Dec 11, 2017

Whitespace changes in heredoc force new resources #59

Closed

s-maj deleted the ecs_task_definition_json branch January 15, 2018 15:10

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print whole container_definitions in aws_ecs_task_definition instead of checksum in the plan #1195

Print whole container_definitions in aws_ecs_task_definition instead of checksum in the plan #1195

s-maj commented Jul 20, 2017

astawiarski commented Jul 20, 2017

s-maj commented Aug 16, 2017

Ninir commented Aug 17, 2017

radeksimko commented Aug 21, 2017

s-maj commented Aug 21, 2017

radeksimko commented Aug 21, 2017 •

edited

Loading

s-maj commented Aug 27, 2017 •

edited

Loading

radeksimko commented Aug 29, 2017

s-maj commented Sep 20, 2017

radeksimko commented Nov 8, 2017

catsby left a comment

radeksimko commented Nov 9, 2017

s-maj commented Nov 9, 2017 via email

s-maj commented Nov 11, 2017

achille-roussel commented Nov 17, 2017

bturbes commented Nov 17, 2017

ziggythehamster commented Nov 17, 2017

nathanielks commented Nov 17, 2017

ckyoog commented Dec 12, 2017

tomelliff commented Dec 12, 2017

ckyoog commented Dec 13, 2017

achille-roussel commented Dec 13, 2017

ghost commented Apr 8, 2020

Print whole container_definitions in aws_ecs_task_definition instead of checksum in the plan #1195

Print whole container_definitions in aws_ecs_task_definition instead of checksum in the plan #1195

Conversation

s-maj commented Jul 20, 2017

astawiarski commented Jul 20, 2017

s-maj commented Aug 16, 2017

Ninir commented Aug 17, 2017

radeksimko commented Aug 21, 2017

s-maj commented Aug 21, 2017

radeksimko commented Aug 21, 2017 • edited Loading

s-maj commented Aug 27, 2017 • edited Loading

radeksimko commented Aug 29, 2017

s-maj commented Sep 20, 2017

radeksimko commented Nov 8, 2017

catsby left a comment

Choose a reason for hiding this comment

radeksimko commented Nov 9, 2017

s-maj commented Nov 9, 2017 via email

s-maj commented Nov 11, 2017

achille-roussel commented Nov 17, 2017

bturbes commented Nov 17, 2017

ziggythehamster commented Nov 17, 2017

nathanielks commented Nov 17, 2017

ckyoog commented Dec 12, 2017

tomelliff commented Dec 12, 2017

ckyoog commented Dec 13, 2017

achille-roussel commented Dec 13, 2017

ghost commented Apr 8, 2020

radeksimko commented Aug 21, 2017 •

edited

Loading

s-maj commented Aug 27, 2017 •

edited

Loading