aws_ecs_container_definition

[tburow]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

currently for ECS container defintions - you have to either do an inline json doc - or run a template for more complex containers. this is not only clunky - but creates infinite change issues when theres a delta between the deployed container def and the local json. often this is due to things like

- cpu              = 0 -> null
- mountPoints      = [] -> null

while most of the time this ranks as a nuisance it has unessisary impact on deployed services/resources when theres really no change.

This feature request - to try to elimiate some of this adverse delta handling is to push the container defintion into a real resource much like was done with IAM. Using aws_iam_policy_document as an example - it would be extremely usefull to create a new resource that you can build the container json in the same manner via HCL syntax and pass that to the task definition as a resource, something to the effect of aws_ecs_container_definition

New or Affected Resource(s)

• aws_ecs_container_definition

Potential Terraform Configuration

References

• IAM example

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document

• Cloudformation ref to Container definitions

• https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-containerdefinitions.html

[DavidJFelix]

I've been wondering about this for awhile and I was just recently bothered enough to come look at the code, so here are some notes I have so far.

The data type aws_iam_policy_document type has a few key files in code to look at:

• The data source - data_source_aws_iam_policy_document.go
• The data source test - data_source_aws_iam_policy_document_test.go
• The IAM Policy model - iam_policy_model.go

It's important to note that the API for IAM does not actually feature a type for this policy document, it's stringified in the response of IAM's GetPolicyVersion API call, which can be seen in the aws-sdk-go documentation

When we think about developing a solution for ECS, we should contrast that ECS does provide a type for the container definition as a part of the task definition request/response, but terraform doesn't allow those fields to be represented in the aws_ecs_task_definition type because it uses a shallow definition for tasks. You can see the types here:

• Container Definition
• Register (Create) Task Definition
• Describe Task Definition

So, rather than replicate the use of a data type that drops JSON into a field on the aws_ecs_task_definition resource, perhaps we should actually expand the schema on the task definition resource. Here are some relevant files:

• The resource - resource_aws_ecs_task_definition.go
• The resource tests - resource_aws_ecs_task_definition_test.go
• Helper functions for JSON expansion - structure.go
• Helper equivalency for container definitions - ecs_task_definition_equivalency.go
• Helper equivalency for container definitions tests - ecs_task_definition_equivalency_test.go

Summary:

I think a change to the aws_ecs_task_definition may be more correct than creating an additional resource/data type. I'll continue to post here if I have developments on this effort or any new information.

[DavidJFelix]

Linking this issue as it seems like it might be something to consider when working on a solution.

[github-actions[bot]]

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

[WhyNotHugo]

.

[bryantbiggs]

not stale

[tburow]

Def not stale - This is a feature request discussed with Hashi support team. Would appreciate this being prioritized sometime soon.

[bryantbiggs]

I will see if I can find any willing contributors to help support

[tburow]

I will see if I can find any willing contributors to help support

Thanks Bryant - Id work on it but I'm all thumbs in this code, def outside my wheel house

[bolshakoff]

I am all thumbs in the whole codebase, but would love to try. 😄 🙊

[bolshakoff]

I attacked it from a wider perspective, and here is my conclusion:

1. Having container_definitions as a part of task_definition sounds like the best, the most natural solution, especially since it is also a part of TaskDefinition in AWS API. So, I agree with @DavidJFelix here.
2. It would also make it more natural on the implementation side, because AWS SDK accepts container definition as a ContainerDefinition struct. So, we'll no longer have to unmarshal and marshal JSON.
3. The next benefit is that it will be more consistent with other providers, for example, GCP which has containers as a part of its CloudRun resource.
4. Finally, having container_definitions integrated with HCL would be great, because it would allow, for example, ignoring image and thus facilitate an even more seamless support for external deployments than the recently merged track_latest attribute.

There are several problems though:

1. Backwards compatibility. Can we accept both JSON and a strongly-typed block as an input? If not, that would be a breaking change; do we plan to have some sort of v2 version (like Cloud Run, for example)?..
2. Any other potential blockers?

[bolshakoff]

So, for the backwards compatibility, I'm going in the following direction:

• Keep container_definitions as JSON.
• Add container_definitions_structured.

Thoughts?

[tburow]

off the cuff I think your on the right track. It’s early and I havnt had my coffee yet lol.

my only concern “consistency with other existing resource flows”

morning rambling…. It’s been a min or two since I wrote the original request. I don’t have a Strong opinion, my original post was related AWS cli handling not SDK. When you list or submit a container definition via the cli, (usually) you just json, much like IAM.

Context: Tho the container def. is technically listed inside the task definition, it is a major component that is treated as it’s own contained piece. It can range from simple to super complex.

I would recommend a separate data resource to define the container that outputs the json (or multiple return formats) keeping consistency with other existing resource flows such as the IAM example. JSON regardless of sdk things, is useful for other complex integrations. While yes there could be better ways, having something that follows the same overall approaches in terraform and doesn’t create a one off would be a better terraform user experience. How that’s done on the backend is mostly obscured from me.

[bolshakoff]

I wouldn't treat CLI as an important example, as it is just another interface to API, tailored to a command line. (If anything, it might actually be considered as an anti-example, because here, in Terraform, we are dealing with 2D text files, not with a 1D command line.)

And while I recognize the IAM policy document example, I still don't feel the “consistency with other existing resource flows” vibe, because, from a quick research, it seems that such approach is used only for policy documents.

However, from the users' demand perspective, implementing a container definition document indeed seems like a much better investment than extending task definition. Especially considering that it doesn't seem to have backwards compatibility problems or similar blockers. So, I'm parking the implementation of task definition extension and starting to explore the document implementation a bit deeper.

(Originally, didn't favor it too much, because for me, the biggest pain point in this area was the inability to ignore container image, but now, thanks to track_latest attribute, it is fixed; and I can soon see myself dealing with monitoring containers and benefiting from the ability to refactor them via a document...)

[bolshakoff]

Now the question is, how to name it? Here's an interesting comment by @bflad: https://github.com/hashicorp/terraform-provider-aws/pull/5862#issuecomment-425446332 According to it, we might need to introduce the following name: aws_ecs_task_definition_container_definition.

[bolshakoff]

Or aws_ecs_container_definition_document?

EDIT: of course not; "document" prefix is IAM-specific.

[bolshakoff]

Actually, no; I don't like the idea of introducing the data source for container definition. Not only because it has the naming conflict problem, but mainly because it feels a bit too complex, and not aligned with normal expectations from the task_definition resource (imho).

At the same time, the good, ironic news is that there is actually no naming conflict for container_definition structured block. Because if it's going to be a HCL list, the syntax would be:

resource "aws_ecs_task_definition" "good_large" {
  container_definition {
    name = "car"
    # ...
  }

  container_definition {
    name = "sidecar"
    # ...
  }

  # ...
}

So, I'd rather implement that.

@tburow - sorry, I know you are more inclined towards the "IAM document" approach, but the ambition, the vibe behind this issue is similar to that of implementing a structured block, so I keep the discussion here, in one place...

Anyway, @bryantbiggs - what do you think? Should I proceed with implementing the above example?

[bryantbiggs]

The containerDefinition has a clear type which means it should be added per the normal HCL schema route:

type ContainerDefinition struct {
  ...
}

However, I don't know if its possible to patch this in to the current implementation without introducing breaking changes. Today the input is just a json blob, tomorrow its an HCL schema

If you look at the IAM policy, its input is just simply a string which is why the document resource was created. I don't think the document type route is appropriate for this scenario:

PolicyDocument *string

[bolshakoff]

OK, I'll dive deeper into the implementation of HCL block and see how painful the interplay with JSON will be.