Add aws_ecs_task_definition.proxy_configuration #8780

SebastianC · 2019-05-26T00:38:13Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

adds proxy_configuration to aws_ecs_task_definition for AppMesh

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Example usage:

resource "aws_ecs_task_definition" "myTask" {
  container_definitions = "${file("./containerDefinitions.json")}"
  family = "nginx"
  requires_compatibilities = ["FARGATE"]
  execution_role_arn = "${aws_iam_role.ecs_task_role.arn}"
  task_role_arn = "${aws_iam_role.ecs_task_exec_role.arn}"
  network_mode = "awsvpc"
  cpu = "256"
  memory = "512"
  proxy_configuration = {
    type = "APPMESH"
    container_name = "nginx"
    properties = [
      {
        name = "IgnoredUID"
        value = "1337"
      },
      {
        name = "AppPorts"
        value = "8080"
      },
      {
        name = "ProxyIngressPort"
        value = "15000"
      },
      {
        name = "ProxyEgressPort"
        value = "15001"
      },
      {
        name = "EgressIgnoredIPs"
        value = "169.254.170.2,169.254.169.254"
      }
    ]
  }
}

I would like some guidance on passing the acceptance tests. when running the acceptance test I get the following error : testing.go:568: Step 0 error: config is invalid: Unsupported argument: An argument named "proxy_configuration" is not expected here. Did you mean to define a block of type "proxy_configuration"? and I am unable to see how to resolve this issue.

I have confirmed that the proxy configuration applies properly when using terraform plan followed by terraform apply if I build and then switch out aws providers.

Here is the full output of the acceptance test:

go test ./aws -v -parallel 1 -run=TestAccAWSEcsTaskDefinition_ProxyConfiguration -timeout 120m
=== RUN   TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== PAUSE TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== CONT  TestAccAWSEcsTaskDefinition_ProxyConfiguration
--- FAIL: TestAccAWSEcsTaskDefinition_ProxyConfiguration (1.16s)
    testing.go:568: Step 0 error: config is invalid: Unsupported argument: An argument named "proxy_configuration" is not expected here. Did you mean to define a block of type "proxy_configuration"?
FAIL
FAIL    github.com/terraform-providers/terraform-provider-aws/aws       5.338s

aws/resource_aws_ecs_task_definition.go

…idelines. resolves #8253

bflad

Hi @SebastianC 👋 Thanks for contributing this! You're off to a good start. Please see the below for some initial feedback and let us know if you have any questions or do not have time to implement the items.

aws/resource_aws_ecs_task_definition_test.go

aws/resource_aws_ecs_task_definition.go

website/docs/r/ecs_task_definition.html.markdown

SebastianC · 2019-06-04T15:02:09Z

@bflad thanks for the feedback. I hope to get to implementing these changes this week, and will let you know if I have any issues.

SebastianC · 2019-06-13T00:31:29Z

All changes from comments above has been made. Results from unit testing:

$ go test ./aws -v -parallel 1 -run=TestAccAWSEcsTaskDefinition_* -timeout 120m
=== RUN   TestAccAWSEcsTaskDefinition_basic
=== PAUSE TestAccAWSEcsTaskDefinition_basic
=== RUN   TestAccAWSEcsTaskDefinition_withScratchVolume
=== PAUSE TestAccAWSEcsTaskDefinition_withScratchVolume
=== RUN   TestAccAWSEcsTaskDefinition_withDockerVolume
=== PAUSE TestAccAWSEcsTaskDefinition_withDockerVolume
=== RUN   TestAccAWSEcsTaskDefinition_withDockerVolumeMinimalConfig
=== PAUSE TestAccAWSEcsTaskDefinition_withDockerVolumeMinimalConfig
=== RUN   TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume
=== PAUSE TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume
=== RUN   TestAccAWSEcsTaskDefinition_withEcsService
=== PAUSE TestAccAWSEcsTaskDefinition_withEcsService
=== RUN   TestAccAWSEcsTaskDefinition_withTaskRoleArn
=== PAUSE TestAccAWSEcsTaskDefinition_withTaskRoleArn
=== RUN   TestAccAWSEcsTaskDefinition_withNetworkMode
=== PAUSE TestAccAWSEcsTaskDefinition_withNetworkMode
=== RUN   TestAccAWSEcsTaskDefinition_withIPCMode
=== PAUSE TestAccAWSEcsTaskDefinition_withIPCMode
=== RUN   TestAccAWSEcsTaskDefinition_withPidMode
=== PAUSE TestAccAWSEcsTaskDefinition_withPidMode
=== RUN   TestAccAWSEcsTaskDefinition_constraint
=== PAUSE TestAccAWSEcsTaskDefinition_constraint
=== RUN   TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource
=== PAUSE TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource
=== RUN   TestAccAWSEcsTaskDefinition_arrays
=== PAUSE TestAccAWSEcsTaskDefinition_arrays
=== RUN   TestAccAWSEcsTaskDefinition_Fargate
=== PAUSE TestAccAWSEcsTaskDefinition_Fargate
=== RUN   TestAccAWSEcsTaskDefinition_ExecutionRole
=== PAUSE TestAccAWSEcsTaskDefinition_ExecutionRole
=== RUN   TestAccAWSEcsTaskDefinition_Inactive
=== PAUSE TestAccAWSEcsTaskDefinition_Inactive
=== RUN   TestAccAWSEcsTaskDefinition_Tags
=== PAUSE TestAccAWSEcsTaskDefinition_Tags
=== RUN   TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== PAUSE TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== CONT  TestAccAWSEcsTaskDefinition_basic
--- PASS: TestAccAWSEcsTaskDefinition_basic (15.69s)
=== CONT  TestAccAWSEcsTaskDefinition_constraint
--- PASS: TestAccAWSEcsTaskDefinition_constraint (7.87s)
=== CONT  TestAccAWSEcsTaskDefinition_ProxyConfiguration
--- PASS: TestAccAWSEcsTaskDefinition_ProxyConfiguration (7.88s)
=== CONT  TestAccAWSEcsTaskDefinition_Tags
--- PASS: TestAccAWSEcsTaskDefinition_Tags (19.45s)
=== CONT  TestAccAWSEcsTaskDefinition_Inactive
--- PASS: TestAccAWSEcsTaskDefinition_Inactive (13.56s)
=== CONT  TestAccAWSEcsTaskDefinition_ExecutionRole
--- PASS: TestAccAWSEcsTaskDefinition_ExecutionRole (12.62s)
=== CONT  TestAccAWSEcsTaskDefinition_Fargate
--- PASS: TestAccAWSEcsTaskDefinition_Fargate (11.46s)
=== CONT  TestAccAWSEcsTaskDefinition_arrays
--- PASS: TestAccAWSEcsTaskDefinition_arrays (7.99s)
=== CONT  TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource
--- PASS: TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource (14.38s)
=== CONT  TestAccAWSEcsTaskDefinition_withEcsService
--- PASS: TestAccAWSEcsTaskDefinition_withEcsService (31.74s)
=== CONT  TestAccAWSEcsTaskDefinition_withPidMode
--- PASS: TestAccAWSEcsTaskDefinition_withPidMode (11.11s)
=== CONT  TestAccAWSEcsTaskDefinition_withIPCMode
--- PASS: TestAccAWSEcsTaskDefinition_withIPCMode (11.33s)
=== CONT  TestAccAWSEcsTaskDefinition_withNetworkMode
--- PASS: TestAccAWSEcsTaskDefinition_withNetworkMode (11.22s)
=== CONT  TestAccAWSEcsTaskDefinition_withTaskRoleArn
--- PASS: TestAccAWSEcsTaskDefinition_withTaskRoleArn (11.57s)
=== CONT  TestAccAWSEcsTaskDefinition_withDockerVolumeMinimalConfig
--- PASS: TestAccAWSEcsTaskDefinition_withDockerVolumeMinimalConfig (7.87s)
=== CONT  TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume
--- FAIL: TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume (5.38s)
    testing.go:568: Step 0 error: Check failed: Check 6/6 error: aws_ecs_task_definition.sleep: Attribute 'volume.584193650.docker_volume_configuration.0.driver' expected "local", got ""
=== CONT  TestAccAWSEcsTaskDefinition_withDockerVolume
--- PASS: TestAccAWSEcsTaskDefinition_withDockerVolume (7.82s)
=== CONT  TestAccAWSEcsTaskDefinition_withScratchVolume
--- PASS: TestAccAWSEcsTaskDefinition_withScratchVolume (7.80s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-aws/aws       222.825s

Note for this PR:

=== RUN   TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== PAUSE TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== CONT  TestAccAWSEcsTaskDefinition_ProxyConfiguration
--- PASS: TestAccAWSEcsTaskDefinition_ProxyConfiguration (7.88s)

Also note the failure on: TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume.

=== RUN   TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume
=== PAUSE TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume
=== CONT  TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume
--- FAIL: TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume (5.63s)
    testing.go:568: Step 0 error: Check failed: Check 6/6 error: aws_ecs_task_definition.sleep: Attribute 'volume.584193650.docker_volume_configuration.0.driver' expected "local", got ""

When I run this against the master I forked from (7acc9ba) I get the same failure.

I am unable to validate against the current version of master because I'm running into errors in the Bazaar dependency management tool:

$ go build
go: labix.org/v2/[email protected]: bzr branch --use-existing-dir https://launchpad.net/mgo/v2 . in {deleted}\go\pkg\mod\cache\vcs\ca61c737a32b1e09a0919e15375f9c2b6aa09860cc097f1333b3c3d29e040ea8: exit status 4:
        bzr: ERROR: httplib.IncompleteRead: IncompleteRead(34 bytes read)

SebastianC · 2019-06-13T00:32:15Z

I apologize for the delay. I believe this is ready for you to review again. @bflad

Please see my notes regarding "d.Set() during the Read operation":

i believe this is resolved via 098e3a6 but i'm unable to see how to validate it.

while debugging i added some log.PrintLns and saw the logs appear as i expected them to if i generated a crash.log, but i'm not familiar enough with how this is used internally to perform better validation.

bflad

Hi @SebastianC 👋 Thanks for the updates here. I've provided some followup review items. Hopefully after these we can get this in. Please reach out if you have any questions or do not have time to implement them.

aws/resource_aws_ecs_task_definition.go

aws/resource_aws_ecs_task_definition_test.go

website/docs/r/ecs_task_definition.html.markdown

rubycut · 2019-06-19T11:15:49Z

@bflad What is ETA on this pull request?

bflad · 2019-06-19T19:09:08Z

@rubycut this is awaiting the community contributor to address the feedback recently provided 2 days ago (#8780 (review)). If there is no response in two weeks, a maintainer will finish it for merge.

SebastianC · 2019-06-19T22:40:19Z

This is ready to review again. @bflad

To validate the unit test, I: implemented the changes you mentioned, commented out the "set" operation, ran the unit test, saw the failure you said would happen. un-commented the "set" operation, re-ran the unit tests, and saw the test pass... I think we're good to go there.

$ go test ./aws -v -parallel 1 -run=TestAccAWSEcsTaskDefinition_ProxyConfiguration -timeout 120m
=== RUN   TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== PAUSE TestAccAWSEcsTaskDefinition_ProxyConfiguration
=== CONT  TestAccAWSEcsTaskDefinition_ProxyConfiguration
--- PASS: TestAccAWSEcsTaskDefinition_ProxyConfiguration (16.60s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       (cached)

CC: @rubycut

bflad

Looks good, @SebastianC, thanks so much! 🚀

Output from acceptance testing:

--- PASS: TestAccAWSEcsTaskDefinition_withDockerVolume (13.85s)
--- PASS: TestAccAWSEcsTaskDefinition_constraint (14.51s)
--- PASS: TestAccAWSEcsTaskDefinition_withScratchVolume (14.81s)
--- PASS: TestAccAWSEcsTaskDefinition_arrays (15.41s)
--- PASS: TestAccAWSEcsTaskDefinition_withDockerVolumeMinimalConfig (15.57s)
--- PASS: TestAccAWSEcsTaskDefinition_withNetworkMode (16.79s)
--- PASS: TestAccAWSEcsTaskDefinition_withIPCMode (17.92s)
--- PASS: TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource (19.93s)
--- PASS: TestAccAWSEcsTaskDefinition_basic (20.29s)
--- PASS: TestAccAWSEcsTaskDefinition_ExecutionRole (23.65s)
--- PASS: TestAccAWSEcsTaskDefinition_Tags (24.04s)
--- PASS: TestAccAWSEcsTaskDefinition_Inactive (25.94s)
--- PASS: TestAccAWSEcsTaskDefinition_Fargate (26.12s)
--- PASS: TestAccAWSEcsTaskDefinition_ProxyConfiguration (28.28s)
--- PASS: TestAccAWSEcsTaskDefinition_withTaskRoleArn (28.67s)
--- PASS: TestAccAWSEcsTaskDefinition_withPidMode (28.96s)
--- PASS: TestAccAWSEcsTaskDefinition_withEcsService (86.30s)

bflad · 2019-06-20T22:12:09Z

This has been released in version 2.16.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2019-11-03T15:20:43Z

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!

adding proxy configuration to ecs_task_definition. resolves #8253

3ebdec4

gofmt. resolves #8253

94032a4

jstewmon reviewed May 31, 2019

View reviewed changes

aws/resource_aws_ecs_task_definition.go Outdated Show resolved Hide resolved

changing use of schema.TypeSet to schema.TypeList per contribution gu…

26e6797

…idelines. resolves #8253

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 4, 2019

bflad reviewed Jun 4, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 4, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 4, 2019

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 4, 2019

SebastianC added 3 commits June 12, 2019 16:31

first pass of code review items #8253

d005634

converting sdk ProxyConfiguration to tf configuration

098e3a6

go fmt

67ccad6

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 13, 2019

bflad reviewed Jun 18, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 18, 2019

bflad self-assigned this Jun 18, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 19, 2019

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 19, 2019

updates from code review

fdaadee

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 19, 2019

bflad self-requested a review June 20, 2019 01:12

bflad added this to the v2.16.0 milestone Jun 20, 2019

bflad approved these changes Jun 20, 2019

View reviewed changes

bflad merged commit 2e5830d into hashicorp:master Jun 20, 2019

bflad added a commit that referenced this pull request Jun 20, 2019

Update CHANGELOG for #8780

8717c2b

ghost locked and limited conversation to collaborators Nov 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add aws_ecs_task_definition.proxy_configuration #8780

Add aws_ecs_task_definition.proxy_configuration #8780

SebastianC commented May 26, 2019

bflad left a comment

SebastianC commented Jun 4, 2019

SebastianC commented Jun 13, 2019

SebastianC commented Jun 13, 2019

bflad left a comment

rubycut commented Jun 19, 2019

bflad commented Jun 19, 2019

SebastianC commented Jun 19, 2019

bflad left a comment

bflad commented Jun 20, 2019

ghost commented Nov 3, 2019

Add aws_ecs_task_definition.proxy_configuration #8780

Add aws_ecs_task_definition.proxy_configuration #8780

Conversation

SebastianC commented May 26, 2019

Community Note

bflad left a comment

Choose a reason for hiding this comment

SebastianC commented Jun 4, 2019

SebastianC commented Jun 13, 2019

SebastianC commented Jun 13, 2019

bflad left a comment

Choose a reason for hiding this comment

rubycut commented Jun 19, 2019

bflad commented Jun 19, 2019

SebastianC commented Jun 19, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jun 20, 2019

ghost commented Nov 3, 2019