From 7189837cde1230ba54c06fe29eadc66667763fd7 Mon Sep 17 00:00:00 2001
From: Abel Luck <abel@guardianproject.info>
Date: Tue, 14 Feb 2023 15:47:16 +0000
Subject: [PATCH] fix: Deprecate `deployment_principal_arns` (#257)

This patch refactors the passing of deployment principals, such that it
uses static/known map keys. This allows this module to be applied in the
same terraform run that the deployment principal (e.g., an iam user) is
applied.

Hashicorp recommends storing only known values in map keys, and leaving
all dynamic/unknown values in the map values
([source0](https://developer.hashicorp.com/terraform/language/meta-arguments/for_each#limitations-on-values-used-in-for_each),
[source1](https://github.com/hashicorp/terraform/issues/30838#issuecomment-1096848884)).
---
 deprecated.tf |  2 ++
 main.tf       |  6 +++---
 variables.tf  | 13 ++++++++++---
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/deprecated.tf b/deprecated.tf
index 81dc6e85..9d4719a9 100644
--- a/deprecated.tf
+++ b/deprecated.tf
@@ -7,6 +7,8 @@ locals {
   cloudfront_access_log_include_cookies = var.log_include_cookies == null ? var.cloudfront_access_log_include_cookies : var.log_include_cookies
   cloudfront_access_log_prefix          = var.log_prefix == null ? var.cloudfront_access_log_prefix : var.log_prefix
 
+  deployment_principals = var.deployment_principal_arns == null ? var.deployment_principals : { for arn, path_prefix in var.deployment_principal_arns : arn => { "arn" : arn, "path_prefix" : path_prefix } }
+
   # New variables, but declare them here for consistency
   cloudfront_access_log_create_bucket = var.cloudfront_access_log_create_bucket
 }
diff --git a/main.tf b/main.tf
index 7ba9c1e3..76615a11 100644
--- a/main.tf
+++ b/main.tf
@@ -183,19 +183,19 @@ data "aws_iam_policy_document" "s3_website_origin" {
 }
 
 data "aws_iam_policy_document" "deployment" {
-  for_each = local.enabled ? var.deployment_principal_arns : {}
+  for_each = local.enabled ? local.deployment_principals : {}
 
   statement {
     actions = var.deployment_actions
 
     resources = distinct(flatten([
       [local.origin_bucket.arn],
-      formatlist("${local.origin_bucket.arn}/%s*", each.value),
+      formatlist("${local.origin_bucket.arn}/%s*", each.value.path_prefix),
     ]))
 
     principals {
       type        = "AWS"
-      identifiers = [each.key]
+      identifiers = [each.value.arn]
     }
   }
 }
diff --git a/variables.tf b/variables.tf
index 256a445b..0bbe4dc9 100644
--- a/variables.tf
+++ b/variables.tf
@@ -480,8 +480,8 @@ variable "versioning_enabled" {
   description = "When set to 'true' the s3 origin bucket will have versioning enabled"
 }
 
-variable "deployment_principal_arns" {
-  type        = map(list(string))
+variable "deployment_principals" {
+  type        = map(object({ path_prefix = string, arn = string }))
   default     = {}
   description = <<-EOT
     (Optional) Map of IAM Principal ARNs to lists of S3 path prefixes to grant `deployment_actions` permissions.
@@ -633,6 +633,13 @@ variable "origin_groups" {
 
 # Variables below here are DEPRECATED and should not be used anymore
 
+variable "deployment_principal_arns" {
+  type        = map(list(string))
+  default     = null
+  description = "DEPRECATED. Use `deployment_principals` instead."
+}
+
+
 variable "access_log_bucket_name" {
   type        = string
   default     = null
@@ -679,4 +686,4 @@ variable "http_version" {
   type        = string
   default     = "http2"
   description = "The maximum HTTP version to support on the distribution. Allowed values are http1.1, http2, http2and3 and http3"
-}
\ No newline at end of file
+}