Declaring 2 aws_ecr_repository_policy for a single repository overwrites the 1st one

[lra]

Terraform Version

Terraform v0.11.3
+ provider.aws v1.10.0

Affected Resource(s)

• aws_ecr_repository_policy

Terraform Configuration Files

resource "aws_ecr_repository" "some_repo" {
  name = "some-repo"
}

resource "aws_ecr_repository_policy" "some_repo_policy_1" {
  repository = "${aws_ecr_repository.some_repo.name}"
  policy     = "${data.aws_iam_policy_document.policy_1.json}"
}

resource "aws_ecr_repository_policy" "some_repo_policy_2" {
  repository = "${aws_ecr_repository.some_repo.name}"
  policy     = "${data.aws_iam_policy_document.policy_2.json}"
}

Expected Behavior

As only one policy is supported for one ECR repo, it should at least error out if we try to declare two.

Actual Behavior

It uploads the 1st one, and uploads the 2nd one, overwriting the 1st one.

It leads to a dangerous behavior as the 1st policy is never really applied, while terraform states that everything is fine.

If AWS is to blame, and its API should error out, I can submit the bug there.

[loivis]

It's related to aws api behavior. For reposiroty policy, there are only set and delete actions. Action set doesn't care if the policy already exists or not. If I didn't missing anything, it's tricky to identity error here.
https://docs.aws.amazon.com/AmazonECR/latest/APIReference/API_SetRepositoryPolicy.html

[lra]

A simple fix would be for TF to forbid more than one aws_ecr_repository_policy for a given repository.

When you declare more than one, only the last one is actually set.

[bflad]

Hi @lra! 👋 Certainly a great request here.

As @loivis mentioned, this particular case is a little more nuanced because the API doesn't support knowing whether you're overwriting an existing policy; it'll allow you submit over it as you wish repeatedly. The only solution within the resource that comes to my mind would be implementing a RepositoryPolicyNotFoundException check before attempting to SetRepositoryPolicy when in the "creation" portion of the Terraform resource. That check and the SetRepositoryPolicy call would require a wrapping mutex to prevent race conditions. It could only provide an apply-time error for the misconfiguration. I would personally be a little hesitant to introduce the additional complexity in the resource, but if there is a great demand for it, maybe its worth it.

A simple fix would be for TF to forbid more than one aws_ecr_repository_policy for a given repository.

Caveat: I'm not a full-time core developer so there might be plenty of gaps in my knowledge, pitfalls to any approach like this, or it might have already been discussed/roadmapped/dropped. I tried searching for an open issue on the matter and couldn't find one.

Here is where it gets more fun, although the fix is not exactly simple off the top of my head. 😅 This support would need to be added upstream in Terraform core as its the place that manages the resource state and graph. Terraform core might be able to provide a schema configuration option where resources can define what schema attribute(s) would make itself redundant (e.g. on aws_ecr_repository_policy the value of the repository attribute). After plan time, Terraform core could then cross check all resources in the state using that configuration if provided.

There are some great benefits with this approach:

• Plan time validation
• Seemingly small amount of details required in each resource to implement

There are some thornier issues with this approach though:

• Being able to flag the redundancy as global vs regional -- although theoretically this could be implemented easily if the resource redundancy configuration was a function instead of a simple attribute list
• It would not work across multiple Terraform states
• It does not help in the case of overwriting an existing resource on creation

So all that said, its probably worth at least creating a ticket upstream in Terraform to see where it lands.

What do you think about both sides of this?

[lra]

I don't think the 1st solution would make any sense, if I declare one policy in terraform, I would want to overwrite whatever policy is set on AWS, or not set at all. I would not want my apply to be blocked by any existing policy. This would be annoying and counter intuitive ;)

So I think the 2nd is the solution to dig.

Trying to find a simpler way to implement this: What about removing the aws_ecr_repository_policy resource type, and adding an optional policy attribute to the aws_ecr_repository resource type? This would enforce the fact that you can only have one policy per resource.

[bflad]

We would like to address the duplicate Terraform resource problem, more generically for all Terraform resources that could be potentially duplicated by certain criteria (such as per-region and per-name). The enhancement that would be available to all Terraform resources, which we could then implement the Terraform AWS Provider, can be tracked upstream in the Terraform Plugin SDK: hashicorp/terraform-plugin-sdk#224.

[bflad]

Hi folks 👋 Thank you for submitting this and this is an excellent use case of somewhere that Terraform and the Terraform AWS Provider could be much more helpful since in many cases they have enough information to return an error upfront during planning instead of unexpected behavior during apply.

I believe this falls under the provider-wide enhancement proposal of #14394, so by adding this link here it will add a reference to that issue so we can include it as a use case when thinking about the implementation details. Since this is likely something we will want more broadly across many resources, I'm going to close this particular issue to consolidate discussions, efforts, and prioritization on the topic while the reference would serve as the cue to make this specific resource one of the initial implementations. I would suggest those 👍 upvoting and subscribing here to do so on #14394 so we can appropriately gauge interest. Please feel free to provide feedback there.

Thanks again!

[XiamiYoung]

aws_ecr_repository_policy should take array as parameter, but as a workaround, I managed to make it work using template:

data "template_file" "ecr_policy_template" {
template = file("${path.module}/policy.json")
vars = {
your_param = var.your_param
}
}

So you'll be able to create a json with multiple statements.

[ghost]

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!